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
kubeadm: handle dup unix:// prefix in node annotation #110656
Conversation
/cc @neolit123 |
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 think we can just remove all prefixes with strings.Replace and then re-add it once
/triage accepted |
6febe31
to
a74d7bc
Compare
newSocket := strings.ReplaceAll(nro.CRISocket, kubeadmapiv1.DefaultContainerRuntimeURLScheme+"://", "") | ||
newSocket = kubeadmapiv1.DefaultContainerRuntimeURLScheme + "://" + newSocket |
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.
one potential problem here would be if the user provided socket path was not unix://
(linux) or npipe
(win).
for example, user value:
URL://URL://path
would become:
DEFAULTURL://path
but i don't think we support anything other than unix://
(linux) or npipe
(win), so it should be fine.
dupURLScheme := strings.HasPrefix(cfg.NodeRegistration.CRISocket, kubeadmapiv1.DefaultContainerRuntimeURLScheme+"://"+kubeadmapiv1.DefaultContainerRuntimeURLScheme+"://") | ||
if dupURLScheme { | ||
socket := strings.ReplaceAll(cfg.NodeRegistration.CRISocket, kubeadmapiv1.DefaultContainerRuntimeURLScheme+"://", "") | ||
cfg.NodeRegistration.CRISocket = kubeadmapiv1.DefaultContainerRuntimeURLScheme + "://" + socket |
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.
the logic in runKubeletConfigPhase()
for upgrade node
will write the fixed cri socket to remove dups.
but during apply
are we going to fix it as well?
this cfg
here might be only for local use and not updated in the cluster CM, but i could be wrong.
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 my test, this will be called during kubeadm config image pull
. To fix it during kubeadm config image pull
would be werid, but it should be fixed.
- The
upgrade node
will be fixed by therunKubeletConfigPhase()
. upgrade apply
will callPerformPostUpgradeTasks
that will callAnnotateCRISocket
to overwrite the annotation.
Thats why this can fix both.
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 my test, this will be called during
kubeadm config image pull
. To fix it duringkubeadm config image pull
would be werid, but it should be fixed.
this
here means the common enforceRequirements
.
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 my test, this will be called during kubeadm config image pull
hm, we should not mutate the CRI socket during actions that are not upgrade.
basically if the socket is corrupted (too many prefixes) upgrade should fix it early in the upgrade phase (enforceRequirements is a good location it seems)
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.
BTW
- directly run
kubeadm config image pull
can success with dupunix://
prefix annotation on the node. - but
kubeadm upgrade apply v1.xx
will fail to runkubeadm config image pull
-
- with this fix, we will update the annotation in
enforceRequirements
. This can be fixed.
- with this fix, we will update the annotation in
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.
Ok. I have not tested this, but please send additional changes if needed.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, pacoxu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
xref #107295 & kubernetes/kubeadm#2426
Special notes for your reviewer:
Does this PR introduce a user-facing change?