-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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: mirroring of last-applied-configuration annotation in EndpointSlices #102731
fix: mirroring of last-applied-configuration annotation in EndpointSlices #102731
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @sharmarajdaksh! |
Hi @sharmarajdaksh. 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. |
/ok-to-test |
@aojea perhaps time to update owners for endpointslice? https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/endpointslice/OWNERS |
@aojea I'm sorry I didn't know for sure where to ping/mention this PR! |
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 for your work on this @sharmarajdaksh!
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "test-ep-1", | ||
Annotations: map[string]string{ | ||
corev1.LastAppliedConfigAnnotation: "{\"apiVersion\":\"v1\",\"kind\":\"Endpoints\",\"metadata\":{\"annotations\":{},\"name\":\"ep-test\",\"namespace\":\"default\"},\"subsets\":[{\"addresses\":[{\"ip\":\"193.99.144.80\"}],\"notReadyAddresses\":[{\"ip\":\"195.54.164.39\"}],\"ports\":[{\"name\":\"http\",\"port\":80,\"protocol\":\"TCP\"},{\"name\":\"https\",\"port\":443,\"protocol\":\"TCP\"}]}]}", |
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'm not sure this is the primary issue you want to be testing here. If I'm understanding this correctly, it ensures that an EndpointSlice that has this annotation will have it removed after a mirroring sync has occurred. That's helpful for cleanup, but what I think we actually need to test is that an Endpoints resource with this annotation is not translated to a mirrored EndpointSlice resource.
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.
agreed
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, let's test:
- endpoint with corev1.LastAppliedConfigAnnotation: "_data_here" is notmirrored
- endpoint with corev1.LastAppliedConfigAnnotation: empty is notmirrored
- endpoint with 2 annotations: corev1.LastAppliedConfigAnnotation: "_data_here" and foo:bar only mirrors foo bar
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.
Sounds good
@@ -837,6 +947,8 @@ func TestReconcile(t *testing.T) { | |||
expectMetrics(t, *tc.expectedMetrics) | |||
} | |||
|
|||
expectAnnotations(t, endpoints, tc.existingEndpointSlices) |
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.
This is not actually what you want here. You're comparing the two test inputs (Endpoints and EndpointSlices that exist before controller syncs), not the actual output (fetched below).
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.
Actually, I think you could just expand expectEndpointSlices
to always expect this annotation to be unset. This would be similar to how it always expects the service name label to be set: https://github.com/kubernetes/kubernetes/blob/576457ac2508d2e06782805f9b3cb27dbbcf7324/pkg/controller/endpointslicemirroring/reconciler_test.go#L988-L991.
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 see my mistake. Thank you for the pointers. I'll update my PR
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.
@robscott
If i understand you correctly, all that should be needed at the line you specified inside reconciler_test.go
is:
if epSlice.Annotations[corev1.LastAppliedConfigAnnotation] != "" {
t.Errorf("Expected LastAppliedConfig Annotation to be unser, got %s", epSlice.Annotations[corev1.LastAppliedConfigAnnotation])
}
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.
Yep, exactly something like that. We'd also need some way for a test case to actually set that annotation on the original Endpoints and EndpointSlice resources as well. You've already got the EndpointSlice use case covered, but it will be a bit more difficult to do the same for the Endpoints in the test case since we just have a subsets
object instead of the full resource. Maybe just add a new endpointsAnnotations
struct to the testCase struct?
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, a map[string]string should work for that, I believe?
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.
Yep, that sounds ideal.
/lgtm
defer approval to RobScott |
Added the release note |
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.
/remove-sig cluster-lifecycle
/remove-area kubeadm
/remove-sig node instrumentaton |
@ehashman: Those labels are not set on the issue: In response to this:
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. |
/remove-sig instrumentation |
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 for your work on this @sharmarajdaksh! Just a couple tiny nits, otherwise LGTM.
}, { | ||
testName: "Annotation mirroring when LastAppliedConfigAnnotation is set", | ||
epAnnotations: map[string]string{ | ||
corev1.LastAppliedConfigAnnotation: "{\"apiVersion\":\"v1\",\"kind\":\"Endpoints\",\"metadata\":{\"annotations\":{},\"name\":\"ep-test\",\"namespace\":\"default\"},\"subsets\":[{\"addresses\":[{\"ip\":\"193.99.144.80\"}],\"notReadyAddresses\":[{\"ip\":\"195.54.164.39\"}],\"ports\":[{\"name\":\"http\",\"port\":80,\"protocol\":\"TCP\"},{\"name\":\"https\",\"port\":443,\"protocol\":\"TCP\"}]}]}", |
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.
This is a tiny nit, but could this annotation be a bit shorter? I don't think it needs to be very long, and a shorter one would make editing this test slightly easier in the future.
}, { | ||
testName: "Annotation mirroring when multiple annotations are set", | ||
epAnnotations: map[string]string{ | ||
corev1.LastAppliedConfigAnnotation: "{\"apiVersion\":\"v1\",\"kind\":\"Endpoints\",\"metadata\":{\"annotations\":{},\"name\":\"ep-test\",\"namespace\":\"default\"},\"subsets\":[{\"addresses\":[{\"ip\":\"193.99.144.80\"}],\"notReadyAddresses\":[{\"ip\":\"195.54.164.39\"}],\"ports\":[{\"name\":\"http\",\"port\":80,\"protocol\":\"TCP\"},{\"name\":\"https\",\"port\":443,\"protocol\":\"TCP\"}]}]}", |
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.
Same comment about annotation length.
Hostname: "pod-1", | ||
}}, | ||
}}, | ||
existingEndpointSlices: []*discovery.EndpointSlice{}, |
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.
Can you add a test case to ensure that an existing slice with this annotation has it cleared out? I think you may have had that test case covered earlier in this PR.
Sure no problem, I'll sort the issues you mentioned |
b8b1767
to
3feb01a
Compare
3feb01a
to
4b4e435
Compare
@robscott lmk if it looks good now! |
expectedClientActions: 1, | ||
expectedMetrics: &expectedMetrics{addedPerSync: 1, numCreated: 1, desiredEndpoints: 1, desiredSlices: 1, actualSlices: 1}, | ||
}, { | ||
testName: "Annotation mirroring and updation when the last-applied-configuration annotation is present on an existing endpoint slice", |
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.
Test name is confusing to me, maybe something like this:
testName: "Annotation mirroring and updation when the last-applied-configuration annotation is present on an existing endpoint slice", | |
testName: "Annotation mirroring should remove last-applied-configuration annotation from existing endpoint slice", |
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.
Hmm, actually now that you mention it I think the test names for all the tests I added could be better
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
Handles incorrect mirroring of endpoint annotations to created endpoint slices, specifically the last-applied-config. Also updates tests and adds test cases for the same
4b4e435
to
211485c
Compare
/retest |
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott, sharmarajdaksh 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:
Fixes the Issue reported in #102684
Which issue(s) this PR fixes:
Fixes #102684
Does this PR introduce a user-facing change?