[WIP] fix probe-level termination grace period bug and add test cases #827
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
bot
added
do-not-merge/work-in-progress
|
@raisaat: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: raisaat The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // 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()): |
There was a problem hiding this comment.
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:
if enabled || !oldHasGracePeriod: should be unchanged- Now we know it's disabled.
if oldHasGracePeriod: should be dropped - 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.
There was a problem hiding this comment.
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?
|
|
||
| // old pod should never be changed | ||
| if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) { | ||
| t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod())) |
There was a problem hiding this comment.
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
| // 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)() |
|
@raisaat: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
@raisaat: PR needs rebase. 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. |
openshift-ci
bot
added
the
needs-rebase
|
/close |
|
@ehashman: Closed this PR. 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. |
/kind feature