-
Notifications
You must be signed in to change notification settings - Fork 170
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
feat: allow pod in place update to cowork with mutating admission webhook #7869
base: main
Are you sure you want to change the base?
feat: allow pod in place update to cowork with mutating admission webhook #7869
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7869 +/- ##
==========================================
- Coverage 63.39% 63.33% -0.06%
==========================================
Files 339 339
Lines 41424 41438 +14
==========================================
- Hits 26259 26245 -14
- Misses 12834 12870 +36
+ Partials 2331 2323 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -261,7 +261,10 @@ func equalBasicInPlaceFields(old, new *corev1.Pod) bool { | |||
|
|||
func equalResourcesInPlaceFields(old, new *corev1.Pod) bool { |
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.
old -> current
new -> expected
is better
aeca824
to
dcd4689
Compare
@@ -259,7 +259,10 @@ func equalBasicInPlaceFields(old, new *corev1.Pod) bool { | |||
|
|||
func equalResourcesInPlaceFields(old, new *corev1.Pod) bool { | |||
if len(old.Spec.Containers) != len(new.Spec.Containers) { |
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 simply remove this if-statement to support the case.
The following for-loop will make sure that all containers in the new
pod are present in the old
pod and have the same resources, so if a new container is added to the new pod, the function returns false, which causes a pod recreation. But if the old pod contains unknown containers (injected by external webhooks), the function will still return true, which causes an in-place update.
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.
great suggestion!
dcd4689
to
14c097b
Compare
@@ -258,9 +258,6 @@ func equalBasicInPlaceFields(old, new *corev1.Pod) bool { | |||
} | |||
|
|||
func equalResourcesInPlaceFields(old, new *corev1.Pod) bool { |
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.
func equalResourcesInPlaceFields(old, new *corev1.Pod) bool { | |
// equalResourcesInPlaceFields checks if the desired values of pod resources are equal to their current actual values. If they are equal, it returns true. Containers in 'old' that are not recognized (they may have been injected by external mutating admission webhooks) will not participate in the comparison. | |
func equalResourcesInPlaceFields(old, new *corev1.Pod) bool { |
@@ -198,6 +198,28 @@ var _ = Describe("instance util test", func() { | |||
policy, err = getPodUpdatePolicy(its, pod5) | |||
Expect(err).Should(BeNil()) | |||
Expect(policy).Should(Equal(NoOpsPolicy)) | |||
|
|||
By("build a pod without revision updated, with IgnorePodVerticalScaling disabled") |
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.
By("build a pod without revision updated, with IgnorePodVerticalScaling disabled") | |
By("simulating a webhook to add an unknown container to the pod, with IgnorePodVerticalScaling disabled, it should use the in-place update policy") |
No description provided.