Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] fix probe-level termination grace period bug and add test cases #827

Closed
wants to merge 1 commit into from
Closed

[WIP] fix probe-level termination grace period bug and add test cases #827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/api/pod/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ func dropDisabledFields(
})
}

if !utilfeature.DefaultFeatureGate.Enabled(features.ProbeTerminationGracePeriod) && !probeGracePeriodInUse(oldPodSpec) {
if !utilfeature.DefaultFeatureGate.Enabled(features.ProbeTerminationGracePeriod) {
// Set pod-level terminationGracePeriodSeconds to nil if the feature is disabled and it is not used
VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool {
if c.LivenessProbe != nil {
Expand Down
79 changes: 42 additions & 37 deletions pkg/api/pod/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,50 +1189,55 @@ func TestDropProbeGracePeriod(t *testing.T) {
},
}

enabled := true
for _, oldPodInfo := range podInfo {
for _, newPodInfo := range podInfo {
oldPodHasGracePeriod, oldPod := oldPodInfo.hasGracePeriod, oldPodInfo.pod()
newPodHasGracePeriod, newPod := newPodInfo.hasGracePeriod, newPodInfo.pod()
if newPod == nil {
continue
}

t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
//enabled := true
for _, enabled := range []bool{true, false} {
for _, oldPodInfo := range podInfo {
for _, newPodInfo := range podInfo {
oldPod := oldPodInfo.pod()
newPodHasGracePeriod, newPod := newPodInfo.hasGracePeriod, newPodInfo.pod()

var oldPodSpec *api.PodSpec
if oldPod != nil {
oldPodSpec = &oldPod.Spec
if newPod == nil {
continue
}
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)

// old pod should never be changed
if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod()))
}
t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {

switch {
case enabled || oldPodHasGracePeriod:
// new pod should not be changed if the feature is enabled, or if the old pod had subpaths
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
}
case newPodHasGracePeriod:
// new pod should be changed
if reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod was not changed")
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProbeTerminationGracePeriod, enabled)()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch.


var oldPodSpec *api.PodSpec
if oldPod != nil {
oldPodSpec = &oldPod.Spec
}
// new pod should not have subpaths
if !reflect.DeepEqual(newPod, podWithoutProbeGracePeriod()) {
t.Errorf("new pod had probe-level terminationGracePeriod: %v", diff.ObjectReflectDiff(newPod, podWithoutProbeGracePeriod()))
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)

// old pod should never be changed
if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod()))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the diff function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I left this part unchanged. It was there already: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/pod/util_test.go#L1208

}
default:
// new pod should not need to be changed
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))

switch {
case enabled:
// new pod should not be changed if the feature is enabled
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod()))
}

case newPodHasGracePeriod:
// new pod should not have grace period if the feature gate is disabled
t.Errorf("new pod should not have grace period if the feature is disabled")

case reflect.DeepEqual(newPod, newPodInfo.pod()):
Copy link

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 makes sense as a case.

Let's write down our behaviour matrix and then figure out the test cases.

  • Feature flag is on:
    • Pod has grace period: unchanged
    • Pod does not have grace period: unchanged
  • Feature flag is off:
    • Pod has grace period: should be dropped
    • Pod does not have grace period: unchanged

Thus, I'd expect our cases to be:

  1. if enabled || !oldHasGracePeriod: should be unchanged
  2. Now we know it's disabled. if oldHasGracePeriod: should be dropped
  3. We must also check, if none of the above are triggered, if !enabled && newHasGracePeriod, as we'd always want to error in that case, since the field should have been blanked out.

Copy link
Author

@raisaat raisaat Jun 24, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think https://github.com/raisaat/kubernetes/blob/feature-probe-termination-grace-period-issue2238/pkg/api/pod/util_test.go#L1225 takes care of case 3, since we know that if case 1 fails, !enabled is true so !enabled && newPodHasGracePeriod can be written as just newPodHasGracePeriod.

I was thinking case 3 takes care of case 2, since case 3 makes sure that the new state of the old pod has the grace period blanked out (i.e. newPodHasGracePeriod is false) when the feature is disabled regardless of whether the old pod had a grace period or not. If this is not the case, which variable would check whether the grace period field has been dropped for case 2?

// new pod should be changed if the feature gate is disabled
t.Errorf("new pod was not changed")

default:
// new pod should not need to be changed
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod()))
}
}
}
})
})
}
}
}
}
Expand Down