-
Notifications
You must be signed in to change notification settings - Fork 359
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
To support new changes for Windows HostProcess Pod for K8S v1.28 and containerd 1.7 #5528
Conversation
0b65bb8
to
f6434f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-windows-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
build/yamls/windows/containerd/conf/Install-WindowsCNI-Containerd.ps1
Outdated
Show resolved
Hide resolved
c60c29c
to
25a3d6d
Compare
/test-windows-all |
LGTM, did all CI tests pass on the 1.28 testbed? @NamanAg30 |
f3b4a3a
to
eab7706
Compare
f3574f5
to
61e84b7
Compare
build/yamls/windows/containerd/conf/Install-WindowsCNI-Containerd.ps1
Outdated
Show resolved
Hide resolved
28b9ff1
to
fda949e
Compare
fda949e
to
bff5ce2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We can optimize the usage of antctl from Node ssh to Pod operations in a separate change.
cp $mountPath/var/run/secrets/kubernetes.io/serviceaccount/ca.crt C:/var/run/secrets/kubernetes.io/serviceaccount | ||
cp $mountPath/var/run/secrets/kubernetes.io/serviceaccount/token C:/var/run/secrets/kubernetes.io/serviceaccount | ||
|
||
# From containerd version 1.7 onwards, the servcieaccount directory, the ca.cert and token files will automatically be created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the new code use a different indent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks.
|
||
# From containerd version 1.7 onwards, the servcieaccount directory, the ca.cert and token files will automatically be created. | ||
$serviceAccountPath = "C:\var\run\secrets\kubernetes.io\serviceaccount" | ||
if (-Not $(Test-Path $serviceAccountPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is better, but will the original code fail if the path exists?
Does the PR fix anything or just improve code? This is important to understand what's the compatibility of previous Antrea releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will fail in containerd 1.7. If the path exists, related files can not be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how does it fix it if the PR just skips creating the path when it exists. The related files still cannot be modified, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically there are three cases:
- If paths exist and files are unchanged, the agent skips copying files.
- If paths don't exist, the agent copies files to the destination.
- If paths exist and files have been modified, there is no way to modify the file, and the pod is expected to crash. The user should clean up the related paths by themselves.
Previously, the agent always copied related files, regardless of their existence, which could lead to agent crashes due to authorization issues. This PR fixes the case 1.
After discussing with Wenying, now I know that these 3 cases only can happen with containerd 1.6. For contianerd 1.7 since the destination and source path is the same path so we can just skip coping files, the code change in this PR is only to make it compatible for containerd1.6. @tnqn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this link (https://kubernetes.io/docs/tasks/configure-pod-container/create-hostprocess-pod/), with containerd versions <=1.6, the ca file and token of the service account only exists inside containers, but antrea-agent needs files under path "c:\var\run\secrets\kubernetes.io\serviceaccount" to create kubeClient, so we should copy the files to the target path. However, since containerd 1.7, containerd would automatically place the files under path "c:\var\run\secrets\kubernetes.io\serviceaccount" ( it is supposed to be 3 links to the same file), so we don't perform "copy" actions.
So the change here is to be consistent of the previous versions.
93c05d0
to
e9981e9
Compare
/test-windows-containerd-conformance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, but the commit title and body aren't following the convention. I will edit it when merging it, please format it correctly for future PRs.
/test-windows-containerd-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-windows-containerd-e2e |
…ins.sh w.r.t new k8s and containerd version Upgrade StartKubelet script and ci test script for k8s 1.28 version and antrea-windows-containerd.yaml for containerd 1.7 version. Signed-off-by: Naman Agarwal <naman.agarwal75@gmail.com> Signed-off-by: Shuyang Xin <gavinx@vmware.com>
e9981e9
to
47630ea
Compare
This change is only for checksum update and code rebase. |
/test-windows-containerd-e2e |
/test-windows-containerd-e2e |
/skip-all |
1 similar comment
/skip-all |
The patch is required to handle following cases:-
1.To handle WindowsHostProcessContainer for k8s 1.28 and and make required adjustment in antrea-windows-containerd.yaml for containerd 1.7.
2.To change commands to clean containerd environment on jumper node from ci/jenkins.sh (since on new tested we only have docker runtime)