Skip to content

Conversation

@Huimintai
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@Huimintai Huimintai requested a review from choujimmy as a code owner August 10, 2021 07:06
return err
}
option := &image.Option{Version: c.Spec.Version, RegistryDomain: p.config.Registry.Domain}
err = image.PullKubernetesImages(c, machineSSH, option)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not prepull all k8s images

done
}

function download::critools() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this func

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove it.

download::docker
download::containerd
download::nerdctl
download::critools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this func

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove it.

env = append(env, fmt.Sprintf("DOCKER_VERSIONS=%s", strings.Join(spec.DockerVersions, " ")))
env = append(env, fmt.Sprintf("CONTAINERD_VERSIONS=%s", strings.Join(spec.ContainerdVersions, " ")))
env = append(env, fmt.Sprintf("CRITOOLS_VERSIONS=%s", strings.Join(spec.CriToolsVersions, " ")))
env = append(env, fmt.Sprintf("NERDCTL_VERSIONS=%s", strings.Join(spec.NerdctlVersions, " ")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove it.

Copy link
Contributor

@huxiaoliang huxiaoliang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need address comments

return err
}
cmdString := fmt.Sprintf("docker pull %s", images.Get().TKEGateway.FullName())
cmdString := fmt.Sprintf("nerdctl --insecure-registry --namespace k8s.io pull %s", images.Get().TKEGateway.FullName())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why --insecre

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need pull without certs.

return err
}
} else {
cmd = fmt.Sprintf("nerdctl --insecure-registry --namespace k8s.io pull %s", image)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add containerd proxy case.

- name: kubelet-volume
mountPath: /app/provider/baremetal/conf/kubelet/
- name: kubeadm-volume
- name: kubeadm-config-volume
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use this kubead pathc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo on next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to kubeadm-volume

)

func Install(s ssh.Interface, version string) error {
func Install(s ssh.Interface, version string, option *Option) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version in option

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the version into option.

volumeMounts:
- mountPath: /app/provider/baremetal
name: provider-volume
- mountPath: /app/provider/baremetal/conf/containerd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need new path

- name: containerd-volume
emptyDir: {}
- name: kubeadm-config-volume
emptyDir: {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete cm path

@@ -0,0 +1,4 @@
runtime-endpoint: unix:///run/containerd/containerd.sock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need this yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove it.

if err != nil {
return errors.Wrap(err, machine.IP)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need try contained works well with gpu

items = append(items, components.Pause.FullName())

return items
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consolidate ListKubernetesImageFullNamesWithVersion and ListKubernetesNodeImageFullNamesWithVersion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consolidate done.

Copy link
Contributor

@huxiaoliang huxiaoliang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't merge, the comments doesn't get fully addressed

Conflicts:
	api/platform/v1/generated.pb.go
@leoryu leoryu merged commit cb97532 into tkestack:master Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants