Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add containerd support to PrepareNode script #5071

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

NamanAg30
Copy link
Contributor

@NamanAg30 NamanAg30 commented Jun 1, 2023

This patch gives the user the option of using containerd runtime while using the PrepareNode.ps1 for configuring k8s on windows node.
for #4952

@NamanAg30 NamanAg30 requested review from rajnkamr and XinShuYang June 1, 2023 10:59
@NamanAg30 NamanAg30 marked this pull request as ready for review June 1, 2023 11:14
@NamanAg30 NamanAg30 force-pushed the preparenode branch 2 times, most recently from 1e2de40 to e5b9708 Compare June 1, 2023 11:47
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
XinShuYang
XinShuYang previously approved these changes Jun 7, 2023
Copy link
Contributor

@XinShuYang XinShuYang left a comment

Choose a reason for hiding this comment

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

LGTM, @wenyingd do you have more comments?

@rajnkamr rajnkamr added this to the Antrea v1.13 release milestone Jun 19, 2023
@rajnkamr rajnkamr added the action/release-note Indicates a PR that should be included in release notes. label Jun 19, 2023
@NamanAg30 NamanAg30 force-pushed the preparenode branch 2 times, most recently from 1f2addb to 25c1e6a Compare June 28, 2023 08:30
@NamanAg30 NamanAg30 force-pushed the preparenode branch 2 times, most recently from 716bde2 to e0863dc Compare July 5, 2023 10:44
@rajnkamr rajnkamr added the area/OS/windows Issues or PRs related to the Windows operating system. label Jul 11, 2023

$cmd = "C:\k\kubelet.exe $global:KubeletArgs --cert-dir=$env:SYSTEMDRIVE\var\lib\kubelet\pki --config=/var/lib/kubelet/config.yaml --bootstrap-kubeconfig=/etc/kubernetes/bootstrap-kubelet.conf --kubeconfig=/etc/kubernetes/kubelet.conf --hostname-override=$(hostname) --pod-infra-container-image=`"mcr.microsoft.com/oss/kubernetes/pause:1.3.0`" --enable-debugging-handlers --cgroups-per-qos=false --enforce-node-allocatable=`"`" --network-plugin=cni --resolv-conf=`"`" --log-dir=/var/log/kubelet --logtostderr=false --image-pull-progress-deadline=20m --node-ip=$env:NODE_IP"
$StartKubeletFileContent += [Environment]::NewLine + '$global:KubeletArgs += "--cert-dir=$env:SYSTEMDRIVE\var\lib\kubelet\pki --config=/var/lib/kubelet/config.yaml --bootstrap-kubeconfig=/etc/kubernetes/bootstrap-kubelet.conf --kubeconfig=/etc/kubernetes/kubelet.conf --hostname-override=$(hostname) --pod-infra-container-image=`"mcr.microsoft.com/oss/kubernetes/pause:1.4.1`" --enable-debugging-handlers --cgroups-per-qos=false --enforce-node-allocatable=`"`" --resolv-conf=`"`" --node-ip=$env:NODE_IP"'
Copy link
Contributor

Choose a reason for hiding this comment

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

Question1: I think this kubelet command is based on version 1.27, do you verify if it can work with the previous K8s versions?

Quesion2: "--network-plugin=cni" is needed when using docker if my memory is correct, but I didn't see it in the changed code. Did you confirm it can work with docker or we do not support docker?

Besdies, I remember containerd has a config of "--container-runtime-endpoint ", does it require now?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://kubernetes.io/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins/#installation
Prior to Kubernetes 1.24, the CNI plugins could also be managed by the kubelet using the cni-bin-dir and network-plugin command-line parameters. These command-line parameters were removed in Kubernetes 1.24, with management of the CNI no longer in scope for kubelet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wenyingd --container-runtime-endpoint flag for containerd is automatically added to kubeadm-flags.env file on running kubeadm join command , which is being used in $global:KubeletArgs in the Start-Kubelet script

@XinShuYang
Copy link
Contributor

Please also resolve conflicts.

@NamanAg30 NamanAg30 force-pushed the preparenode branch 2 times, most recently from 2c26fa0 to 98e8ba3 Compare July 21, 2023 07:36
@NamanAg30 NamanAg30 requested a review from wenyingd July 21, 2023 11:16
@NamanAg30 NamanAg30 force-pushed the preparenode branch 3 times, most recently from 5a6ad93 to 013e356 Compare July 21, 2023 13:20
docs/windows.md Outdated Show resolved Hide resolved
hack/windows/Prepare-Node.ps1 Outdated Show resolved Hide resolved

if ($netId.Length -lt 1) {
docker network create -d nat host
}' + [Environment]::NewLine
Copy link
Member

Choose a reason for hiding this comment

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

but does it mean?

Copy link
Contributor Author

@NamanAg30 NamanAg30 Jul 21, 2023

Choose a reason for hiding this comment

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

Sorry , i did not understand this comment.
Are you asking about [Environment]::NewLine? (used to give new line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From line 112 to 116 , (from '$netId ) ... it is one string being added to StartKubeletFileContent

Copy link
Member

Choose a reason for hiding this comment

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

Got it, I misunderstood it.

hack/windows/Prepare-Node.ps1 Show resolved Hide resolved
@NamanAg30 NamanAg30 force-pushed the preparenode branch 2 times, most recently from 72636c9 to cff49ab Compare July 21, 2023 20:42
wenyingd
wenyingd previously approved these changes Jul 24, 2023
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

XinShuYang
XinShuYang previously approved these changes Jul 24, 2023
Copy link
Contributor

@XinShuYang XinShuYang left a comment

Choose a reason for hiding this comment

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

LGTM

@NamanAg30 NamanAg30 requested a review from tnqn July 24, 2023 05:52
tnqn
tnqn previously approved these changes Jul 25, 2023
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM


if ($netId.Length -lt 1) {
docker network create -d nat host
}' + [Environment]::NewLine
Copy link
Member

Choose a reason for hiding this comment

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

Got it, I misunderstood it.

@tnqn
Copy link
Member

tnqn commented Jul 25, 2023

/skip-all

Signed-off-by: Naman Agarwal <naman.agarwal75@gmail.com>
@NamanAg30 NamanAg30 dismissed stale reviews from tnqn, XinShuYang, and wenyingd via f574613 July 25, 2023 11:39
@NamanAg30
Copy link
Contributor Author

@tnqn , ready for merge now

@tnqn
Copy link
Member

tnqn commented Jul 25, 2023

/skip-all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/OS/windows Issues or PRs related to the Windows operating system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants