-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
Fix description of Ports in PodSpec #110564
Conversation
Welcome @j4m3s-s! |
95e88c1
to
8991f49
Compare
Hi @j4m3s-s. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@cyclinder The PR we talked about the other day. |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/sig api-machinery |
/retest |
/remove-sig api-machinery |
they were dancing with this problem in the referred issue |
/remove-sig api-machinery |
// This array might be out-of-date because it merges on containerPort and not (containerPort, protocol). | ||
// This information should not be used to get accurate ports exposed by pod. | ||
// See https://github.com/kubernetes/kubernetes/issues/103544 for more information. |
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.
That doesn't seem like the right doc fix. I would say "don't use strategic merge patch with this field" and explain why.
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 my current contribution makes it clearer that this information should not be taken as truth. Do you want me to add a part indicating "don't use strategic merge patch with this field" or replace my comment by yours ?
I think mine makes it clearer for those not necessarily familiar with merge strategies in k8s. WDYT ?
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.
Other than a strategic merge patch or purely client-side confusion, is there any other way for this field to become “out of date”?
If the only API means to get it wrong are strategic merge patch, we can say so.
💭 We could also, in the API, send a Warning: response any time someone does a strategic merge patch that updates this field (and client-go etc then presents the warning to its caller).
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 my current contribution makes it clearer that this information should not be taken as truth.
But it is the truth. If you have a Service that is delivering to port "foo
" on a given pod, and someone uses strategic merge patch and accidentally removes that entry from Ports, then the service will stop delivering traffic to that pod. The problem is not that Ports contains incorrect information; it's that the pod has been redefined, and the new definition isn't what you had wanted.
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.
Sorry, I should have been clearer in my wording. Indeed you are right, it is the truth.
I don't think there's any other way of the fields Ports
to be out-of-date.
I agree we should indicate the problem is using strategic merge patch in this case, but I would prefer if we also make it clear for people that are not familiar with it (as was I when I encountered this bug). Is it okay to link k8s docs in those comments ?
I would say something along the lines of
Use of strategic merge patch with this field can lead to errors due to information seemingly being the same but actually being different. This happens when applying diffing with arrays with same port number since the diffing takes only port into account and not the tuple (port, protocol).
See <link to the issue>
See <link to strategic merge patch like https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch>.
Sending a warning sounds like a great idea ! Do you mind giving me some pointers on how to do this ? Should I add this to this PR or should I make another one ?
I'm also open to suggestions on wording since I'm not a native english speaker.
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 other case I know of is:
https://github.com/kubernetes/api/blob/v0.24.2/core/v1/types.go#L4986
But that's a field that end users don't really modify anyway (since kubelet would overwrite their modifications) so the docs don't need to worry as much about being understandable to end users...
Use of strategic merge patch with this field can lead to errors due to information seemingly being the same but actually being different. This happens when applying diffing with arrays with same port number since the diffing takes only port into account and not the tuple (port, protocol).
I'm not sure it's worth explaining in that much detail since it doesn't especially help the user to solve their problem. I'd just note that trying to modify it with strategic merge patch may corrupt the data, but that you can still use other kinds of patch if you need to. And link to this PR or to #108255 and then people who want to know more can follow the link.
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.
Sending a warning sounds like a great idea ! Do you mind giving me some pointers on how to do this ? Should I add this to this PR or should I make another one ?
I opened #110627 about that, but it's likely to be complicated. I don't know exactly how the apiserver warning mechanism works...
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 followed the issue as well. I'm not an expert in client-go / API server but I'm a bit worried on how the client will get an error properly.
Otherwise, thanks for the feedback, will update the PR.
a62d064
to
c35e4df
Compare
/triage accepted |
c35e4df
to
e37bb65
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.
Thanks!
/lgtm
/approve
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
3 similar comments
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
e37bb65
to
9da646d
Compare
Sorry, I took a bit of time to fix the last issue on my PR, this should be mergeable now. I think all the tests that aren't passing aren't my fault (please correct me if my analysis is wrong ofc :). ). |
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.
Thanks!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: j4m3s-s, thockin 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 |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #108255
Special notes for your reviewer:
First time contributor here so, if anything's missing / incorrect please enlighten me.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: