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.

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

promote PodReadinessGate feature to GA #74434

Merged
merged 1 commit into from Mar 8, 2019

Conversation

@freehan
Copy link
Member

commented Feb 22, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR contains code changes necessary to graduate the PodReadinessGate feature from Beta to GA
Enhancement: kubernetes/enhancements#580
KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/0007-pod-ready%2B%2B.md

Does this PR introduce a user-facing change?:

NONE

PodReadinessGate feature is now GA. The feature gate will not allow disabling it.
@@ -470,7 +470,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
RunAsGroup: {Default: false, PreRelease: utilfeature.Alpha},
VolumeSubpath: {Default: true, PreRelease: utilfeature.GA},
BalanceAttachedNodeVolumes: {Default: false, PreRelease: utilfeature.Alpha},
PodReadinessGates: {Default: true, PreRelease: utilfeature.Beta},
PodReadinessGates: {Default: true, PreRelease: utilfeature.GA},

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 23, 2019

Member

typically, when promoting a feature gate to GA, we should:

This comment has been minimized.

Copy link
@freehan

freehan Feb 25, 2019

Author Member

Thanks! I have made the corresponding changes.

@freehan freehan force-pushed the freehan:pod-ready-ga branch from 31da70d to a9deb52 Feb 25, 2019
@freehan freehan added this to the v1.14 milestone Feb 25, 2019
@freehan freehan referenced this pull request Feb 25, 2019
@liggitt

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

if the gate is locked to enabled, we should drop these:

if !utilfeature.DefaultFeatureGate.Enabled(features.PodReadinessGates) && !podReadinessGatesInUse(oldPodSpec) {
podSpec.ReadinessGates = nil
}

// podReadinessGatesInUse returns true if the pod spec is non-nil and has ReadinessGates
func podReadinessGatesInUse(podSpec *api.PodSpec) bool {
if podSpec == nil {
return false
}
if podSpec.ReadinessGates != nil {
return true
}
return false
}

func TestDropReadinessGates(t *testing.T) {
podWithoutReadinessGates := func() *api.Pod {
return &api.Pod{
Spec: api.PodSpec{
ReadinessGates: nil,
},
}
}
podWithReadinessGates := func() *api.Pod {
return &api.Pod{
Spec: api.PodSpec{
ReadinessGates: []api.PodReadinessGate{
{
ConditionType: api.PodConditionType("example.com/condition1"),
},
{
ConditionType: api.PodConditionType("example.com/condition2"),
},
},
},
}
}
podInfo := []struct {
description string
hasPodReadinessGates bool
pod func() *api.Pod
}{
{
description: "has ReadinessGates",
hasPodReadinessGates: true,
pod: podWithReadinessGates,
},
{
description: "does not have ReadinessGates",
hasPodReadinessGates: false,
pod: podWithoutReadinessGates,
},
{
description: "is nil",
hasPodReadinessGates: false,
pod: func() *api.Pod { return nil },
},
}
for _, enabled := range []bool{true, false} {
for _, oldPodInfo := range podInfo {
for _, newPodInfo := range podInfo {
oldPodHasReadinessGates, oldPod := oldPodInfo.hasPodReadinessGates, oldPodInfo.pod()
newPodHasReadinessGates, newPod := newPodInfo.hasPodReadinessGates, newPodInfo.pod()
if newPod == nil {
continue
}
t.Run(fmt.Sprintf("featue enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodReadinessGates, enabled)()
var oldPodSpec *api.PodSpec
if oldPod != nil {
oldPodSpec = &oldPod.Spec
}
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()))
}
switch {
case enabled || oldPodHasReadinessGates:
// new pod should not be changed if the feature is enabled, or if the old pod had ReadinessGates
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
}
case newPodHasReadinessGates:
// new pod should be changed
if reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod was not changed")
}
// new pod should not have ReadinessGates
if !reflect.DeepEqual(newPod, podWithoutReadinessGates()) {
t.Errorf("new pod had ReadinessGates: %v",
diff.ObjectReflectDiff(newPod, podWithoutReadinessGates()))
}
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()))
}
}
})
}
}
}
}

@freehan freehan force-pushed the freehan:pod-ready-ga branch from a9deb52 to 562bc03 Feb 26, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/XS labels Feb 26, 2019
@freehan

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

Thank you pointing that out!

@liggitt

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

/retest

mechanics look good, will defer to @thockin on API promotion

Copy link
Member

left a comment

Before I can approve this, I think we need to have the discussion about polarity of conditions. We started in email, but we need to resolve it.

TL;DR We document that conditions should be "bad", so that True means something unusual. This uses True as a good thing.

  1. We could flip this feature (and drag it through a beta 2 and other changes) to align with that (condition must be present AND False). E.g. "LoadBalancerReady" now becomes "WaitingForLoadBalancer".

  2. We could change our conventions to say that polarity is situational, but "True = bad" is preferred.

  3. We could add an optional polarity flag to conditions and let implementors define them how they want.

@liggitt @smarterclayton @lavalamp - anyone have thoughts?

@freehan

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

We could flip this feature (and drag it through a beta 2 and other changes) to align with that (condition must be present AND False). E.g. "LoadBalancerReady" now becomes "WaitingForLoadBalancer".

For a boolean condition, it feels more intuitive to default it to "False" when condition is not present.

I do not think we can just flip this feature with the existing field. We will need to add new field to conform with the convention and deprecate the old field and allow users to migrate in between.

We could change our conventions to say that polarity is situational, but "True = bad" is preferred.

I am not sure why we added the polarity recommendation in API convention in the first place. But my understanding is that it falls under this assumption: Because an object would not know what are the possible conditions out there. If additional conditions present, it should indicate problems and the object should be considered in "bad" state. If no conditions present, the object is considered "good". If we do the reverse, it would be hard/racy to determine if an object is in "good" state because additional conditions may always pop up. Do I understand it correctly?

However, for PodReadinessGate feature, we defined ReadinessGates in PodSpec and disallow updates, so the corresponding conditions are expected to be presented. Hence, I feel like the assumption above does not hold true.

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

For a boolean condition, it feels more intuitive to default it to "False" when condition is not present.

I agree. For things that must be present to indicate readiness, I map "all systems go" to true values much more intuitively. "Ready=true" is immediately understandable. "NotReady=false" makes me go into a slow-motion double-negative headspace.

Keeping readinessGates the same polarity as the pod Ready condition also seems more coherent to me.

We could change our conventions to say that polarity is situational, but "True = bad" is preferred.

I do think it is situational. One rationalization might be to consider whether absence of a condition is considered good or bad, and make that align with an explicit false value:

  • for readinessGates, absence is bad, so absence/false=bad, true=good
  • for node conditions like disk pressure, absence is assumed to be ok, so absence/false=good, true=bad
@soggiest

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Hello! Code freeze for 1.14 is coming up in 2 days. Will this be merged in the next week?

@liggitt @thockin

@thockin

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@thockin

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

To expedite this, I spoke with Jordan some more. The conclusion is that we both agree that the UX of conditions is important and in this case it makes more sense to keep the polarity as-is. If I had a time machine, I might have argued harder to design this to be polarity-consistent with guidance. Alas, my time machine is in the shop until last week.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 8, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@freehan

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

/sig network

@k8s-ci-robot k8s-ci-robot added sig/network and removed needs-sig labels Mar 8, 2019
@k8s-ci-robot k8s-ci-robot merged commit 1fc2396 into kubernetes:master Mar 8, 2019
16 checks passed
16 checks passed
cla/linuxfoundation freehan authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.