KEP-3017: Promote unhealthyPodEvictionPolicy for PDBs to Beta

[atiratree]

• One-line PR description: Promote unhealthyPodEvictionPolicy for PDBs to Beta

• Issue link: PodHealthyPolicy for PodDisruptionBudget #3017

• Other comments:

[atiratree]

/assign @ravisantoshgudimetla
/assign @soltysh
/assign @liggitt

[liggitt]

overall lgtm

[liggitt]

I'm on the fence whether to default the field or not... I'm not sure how much it would actually inform/drive user awareness. @soltysh, thoughts?

[soltysh]

I'm not sure if defaulting will help with the adoption or rise awareness, @wojtek-t also pointed out that here that it might cause issues for existing CI/CD flows where users have predefined PDBs with nil values for those fields which would cause them being pushed to the server. I'm inclined to leave it as is, for now.

Do we have a precedence before for changing defaults in similar situations? If we decide to do that, I'd probably go the path of adding a warning for nil values and setting the default in the next release, to give users one release to adopt, maybe?

[liggitt]

there's plenty of precedence for defaulting of newly introduced fields, conditional on their feature being enabled, which is what is being considered here:

git grep 'Gate.Enabled' -- 'pkg/apis/*defaults.go'
pkg/apis/apps/v1/defaults.go:           if utilfeature.DefaultFeatureGate.Enabled(features.MaxUnavailableStatefulSet) {
pkg/apis/apps/v1/defaults.go:   if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetAutoDeletePVC) {
pkg/apis/apps/v1beta1/defaults.go:      if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetAutoDeletePVC) {
pkg/apis/apps/v1beta1/defaults.go:              if utilfeature.DefaultFeatureGate.Enabled(features.MaxUnavailableStatefulSet) {
pkg/apis/apps/v1beta2/defaults.go:              if utilfeature.DefaultFeatureGate.Enabled(features.MaxUnavailableStatefulSet) {
pkg/apis/apps/v1beta2/defaults.go:      if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetAutoDeletePVC) {
pkg/apis/storage/v1/defaults.go:        if obj.Spec.SELinuxMount == nil && utilfeature.DefaultFeatureGate.Enabled(features.SELinuxMountReadWriteOncePod) {
pkg/apis/storage/v1beta1/defaults.go:   if obj.Spec.SELinuxMount == nil && utilfeature.DefaultFeatureGate.Enabled(features.SELinuxMountReadWriteOncePod) {

that said, there's also evidence that some clients don't handle defaulted fields correctly / gracefully in general (including the kubelet choking on new defaulted fields inside podspec - kubernetes/kubernetes#63814, or clients doing diffs being unhappy with defaulted fields - argoproj/argo-cd#11143), so unless there's a strong reason to default, I'd probably leave the field nil by default and document which explicit option that corresponds to

[wojtek-t]

so unless there's a strong reason to default, I'd probably leave the field nil by default and document which explicit option that corresponds to

+1 (unless there are other reasons not mentioned here).

[soltysh]

👍 for nil, in that case.

[atiratree]

ack, updated the KEP to keep the field nil by default

[logicalhan]

It does not change the default behavior. Users will have to specify a policy on the PDB for behavior to be affected.

What happens if a policy is specified and then the feature is disabled?

[logicalhan]

Also, https://github.com/kubernetes/enhancements/blob/458d4689ad80b05ad938e97c5c29f354acf3df66/keps/sig-apps/3017-pod-healthy-policy-for-pdb/README.md#what-are-the-slis-service-level-indicators-an-operator-can-use-to-determine-the-health-of-the-service is not filled out. Neither is https://github.com/kubernetes/enhancements/blob/458d4689ad80b05ad938e97c5c29f354acf3df66/keps/sig-apps/3017-pod-healthy-policy-for-pdb/README.md#how-can-someone-using-this-feature-know-that-it-is-working-for-their-instance.

[wojtek-t]

Few more comments (on top of what @logicalhan already pointed below too):

L364: "No, but they will be added for alpha"

Alpha has happened - can you update with the test that were added?

L370: "It does not change the default behavior. Users will have to specify a policy
on the PDB for behavior to be affected."

That's not really answer to the question. The question is not what changes, but how can a rollout fail? (e.g. apiserver can start crashing, etc.)

L375: Please specify the exact metric that reports that

L378: was this test run? If so, please update. If not, please ensure this will happen prior promoting feature gate to beta.

L400: the whole monitoring section is not filled-in

[atiratree]

was this test run? If so, please update. If not, please ensure this will happen prior promoting feature gate to beta.

I will try to run this test soon

[atiratree]

following up here #3777 (review)

[soltysh]

@atiratree try addressing the comments from Han and Wojtek asap

[atiratree]

Thanks for reviews everyone. I have tried to resolve all the feedback. Can you please take a look again @logicalhan @wojtek-t @soltysh @liggitt

[atiratree]

Is it enough to have the E2E tests before GA or should it be sooner?

[soltysh]

Yeah, having a test run at beta, when we enable the feature gate as the default is usually sufficient.

[soltysh]

/lgtm
/approve
from sig-apps pov

[soltysh]

Yeah, having a test run at beta, when we enable the feature gate as the default is usually sufficient.

[wojtek-t]

Just two more minor comments - other than that LGTM from PRR perspective.

[wojtek-t]

Those are all great and needed tests, but they are more of a feature tests.

Enablement/disablement tests are tests that verify what happens if we change the FG in the middle of the test.

Wlll you be able to add a test like this one described in the template comment:
https://github.com/kubernetes/enhancements/blame/master/keps/NNNN-kep-template/README.md#L505-L509
[and add a TODO for it here?]

[atiratree]

Actually, there are 2 tests of these that do that https://github.com/kubernetes/kubernetes/blob/06914bdaf51fc1b91501c332bd69d439cd370581/pkg/registry/policy/poddisruptionbudget/strategy_test.go#L96-L114. Nevertheless I have added a TODO to add more comprehensive tests for disablement/enablement,

[atiratree]

I have done the manual upgrade->downgrade->upgrade test, but the outcome is not the one as originally described by @mortent.

That is, when we downgrade to 1.25 the unhealthyPodEvictionPolicy field is preserved in etcd, until an update happens to the PDB. Eg by dirsuption controller when updating status, which is triggered for example by eviction that we do in step 5. The field is then removed from etcd and will not be present when we upgrade to 1.26.

Only hacky way to preserve it is to ensure there is no update to the PDB.

I am not sure if this is expected behaviour for new fields. I have found something relevant mentioned here https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md

It must be possible to rollback to a previous version of API server that does not include your change and have no impact on API objects which do not use your change. API objects that use your change will be impacted in case of a rollback.

so it seems we should expect impact on API objects in case of a rollback, so the current behaviour would be correct?

I have updated the KEP to reflect that, but I am not sure if I am not missing something. Can @liggitt or anybody else confirm that?

[wojtek-t]

OK - this was rollback to the version that doesn't recognize this field, so if something was touching this object, the fact that this field is dropped isn't unexpected.

I think it would be useful to make this test again but switching from 1.27 -> 1.26 -> 1.27, in which case you would be downgrading to a version that already supports that and see that (an alternative is to just disable FG within 1.26 version).

If you could add a TODO for doing that, it would be great.

[atiratree]

Thanks for confirming.

Good idea, I have added a TODO to test the 1.27 upgrade path

[wojtek-t]

@atiratree - I just have one super minor comment.
I'm approving it so that you don't have to wait for me, but please apply before merging.

/approve PRR
/hold

[wojtek-t]

Thanks for pointing me at the tests above - I missed that somehow when looking into all those tests initially.

This is already great - if you have cycles adding more is of course appreciated, but the ones that you mention above actually make me already happy :)

[atiratree]

ok, in that case I have removed the TODO for more tests :)

[wojtek-t]

OK - this was rollback to the version that doesn't recognize this field, so if something was touching this object, the fact that this field is dropped isn't unexpected.

I think it would be useful to make this test again but switching from 1.27 -> 1.26 -> 1.27, in which case you would be downgrading to a version that already supports that and see that (an alternative is to just disable FG within 1.26 version).

If you could add a TODO for doing that, it would be great.

[soltysh]

All the comments from Wojtek got addressed
/hold cancel
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh, wojtek-t

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:

• keps/prod-readiness/OWNERS [wojtek-t]
• keps/sig-apps/OWNERS [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wojtek-t]

For posterity - this LGTM - thanks!