lavalamp Mar 28, 2022

Both these comments are huge improvements, thanks.

staging/src/k8s.io/api/apps/v1/types.go

lavalamp Mar 25, 2022

This "loop with huge nested if" is not a good way to do a table driven test BTW. Better to extract logic to e.g. functions which are in the test cases and can just be run. I'm not sure if I'm asking for a change or not because I'm still trying to understand the test.

Outdated

.../statefulset/stateful_set_control_test.go

lavalamp Mar 25, 2022

Your PR should not be adding this file.

Outdated

...l.apiserver.k8s.io__v1alpha1_openapi.json

lavalamp Mar 25, 2022

You must need a rebase or something, your PR is not adding this field.

Outdated

api/openapi-spec/swagger.json

lavalamp Mar 25, 2022

This comment should explain how this field interacts with the Partition field. ...The partition field comment is also not helpful and could be expanded if you know what it is supposed to mean.

Outdated

staging/src/k8s.io/api/apps/v1/types.go

lavalamp Mar 25, 2022

If updateMin is not zero, this could miss counting some unhealthy pods?

Outdated

...oller/statefulset/stateful_set_control.go

lavalamp Mar 25, 2022

`Partition` could be nil?

...oller/statefulset/stateful_set_control.go

lavalamp Mar 25, 2022

replace with a call to the function you defined.

Outdated

pkg/apis/apps/validation/validation_test.go

lavalamp Mar 25, 2022

same -- please remove

Outdated

pkg/apis/apps/v1beta2/defaults_test.go

lavalamp Mar 25, 2022

delete these variables now that you have the functions (there are some references too).

Outdated

pkg/apis/apps/v1/defaults_test.go

lavalamp Mar 25, 2022

All these files appear to have a doubled-up openapi-spec directory, perhaps you ran the generator with the wrong working directory?

Outdated

...-known__openid-configuration_openapi.json

lavalamp Mar 25, 2022

This doesn't seem like a logical file for this PR to be adding, is it a merge problem?

Outdated

api/openapi-spec/openapi-spec/README.md

lavalamp Mar 24, 2022

Using the same pointer everywhere makes me nervous (see earlier comments)

Outdated

...egistry/apps/statefulset/strategy_test.go

lavalamp Mar 24, 2022

If you follow my suggestion you don't need the `podsToDelete > 0 check`. But you could replace it with a `deletedPods < podsToDelete` check and lose an if statement inside the loop.

Outdated

...oller/statefulset/stateful_set_control.go

lavalamp Mar 24, 2022

This message doesn't make very much sense -- maybe that was the last pod that needed updating. Also sometimes this message is true, but we didn't get into this loop at all.

Outdated

...oller/statefulset/stateful_set_control.go

lavalamp Mar 24, 2022

Declare `err` right before it's needed, too easy to accidentally reference it if it is declared here.

Outdated

...oller/statefulset/stateful_set_control.go

lavalamp Mar 24, 2022

Predefine this func. This line should just look like `intstrAddr(0)` or something.

Outdated

pkg/apis/apps/validation/validation_test.go

lavalamp Mar 24, 2022

Please lose all of these functions -- they just make things hard to read. I don't know why the preexisting cases have them, you don't have to fix them, but don't propagate this mistake.

Outdated

pkg/apis/apps/validation/validation_test.go

lavalamp Mar 24, 2022

Remove this test case, it doesn't seem different from "update strategy" above?

Outdated

pkg/apis/apps/validation/validation_test.go

lavalamp Mar 24, 2022

Same comments as the v1 tests

Outdated

pkg/apis/apps/v1beta2/defaults_test.go

lavalamp Mar 24, 2022

I'm not usually one to complain about tests, but what failure mode do you expect this test case to catch? I would personally remove this one.

Outdated

pkg/apis/apps/v1/defaults_test.go

lavalamp Mar 24, 2022

same comments as above

Outdated

pkg/apis/apps/v1/defaults_test.go

lavalamp Mar 24, 2022

Don't reuse the same pointer here, if someone modifies the thing pointed at it invalidates the comparison. Also in the real system this field needs to end up nil, so either don't test this case here, or add a comment, or change the behavior.

Outdated

pkg/apis/apps/v1/defaults_test.go

lavalamp Mar 24, 2022

since it is disabled in this test case, I would expect this to end up nil? If some other part of the system clears it then maybe don't test this here, as it is confusing?

Outdated

pkg/apis/apps/v1/defaults_test.go

lavalamp Mar 24, 2022

Why is this in this test case?

Outdated

pkg/apis/apps/v1/defaults_test.go

ravisantoshgudimetla Mar 24, 2022

unnecessary space and have space between L22 and 23.

Outdated

...egistry/apps/statefulset/strategy_test.go

ravisantoshgudimetla Mar 24, 2022

Remove commented code.

Outdated

...oller/statefulset/stateful_set_control.go

soltysh Mar 16, 2022

And another one.

Outdated

...oller/statefulset/stateful_set_control.go

soltysh Mar 16, 2022

Another debug line.

Outdated

...oller/statefulset/stateful_set_control.go

soltysh Mar 16, 2022

Remove debug line.

Outdated

...oller/statefulset/stateful_set_control.go

ravisantoshgudimetla Mar 15, 2022

Same as above.

Outdated

...oller/statefulset/stateful_set_control.go

ravisantoshgudimetla Mar 15, 2022

Remove the debug lines.

Outdated

...oller/statefulset/stateful_set_control.go

ravisantoshgudimetla Mar 15, 2022

Same as Aldo's comments earlier. This needs to be run with the featuregate enabled and disabled.

Outdated

pkg/apis/apps/v1beta2/defaults_test.go

ravisantoshgudimetla Jan 12, 2022

There is a lot of duplication between this method and `TestStatefulSetControlRollingUpdateWithMaxUnavailableInOrderedMode` - Can you make mode as an argument and make the test table driven?

Outdated

.../statefulset/stateful_set_control_test.go

ravisantoshgudimetla Jan 12, 2022

I think the test case should be opaque of the -ve numbers and the controller code should have the validation to check -ve number of pods to be deleted.

Outdated

.../statefulset/stateful_set_control_test.go

ravisantoshgudimetla Jan 12, 2022

This logic is a bit hard to follow, why 6 here?

Outdated

.../statefulset/stateful_set_control_test.go

ravisantoshgudimetla Jan 12, 2022

Add another field to set or unset the flag.

.../statefulset/stateful_set_control_test.go

soltysh Oct 15, 2021

Any particular reason you're changing this condition from `!isTerminating` to `isHealthy` these are slightly different sets. The former will catch any pod unless it's terminating, ie. already removed, the latter only fully running ones.

Outdated

...oller/statefulset/stateful_set_control.go

alculquicondor Sep 20, 2021

reverse the order and add to Fatalf `(-want,+got)`

Outdated

...egistry/apps/statefulset/strategy_test.go

alculquicondor Sep 20, 2021

this is only necessary if running tests in parallel.

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Sep 20, 2021

nit: split in different lines

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Sep 20, 2021

No need for the `func` here.

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Sep 20, 2021

There is no dependency on the feature gate. Remove this line.

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Sep 20, 2021

nit: store the path in a variable ```golang path := fldPath.Child("maxUnavailable") ``` so you can reuse it for all errors.

Outdated

pkg/apis/apps/validation/validation.go

alculquicondor Sep 20, 2021

`var allErrs field.ErrorList`

Outdated

pkg/apis/apps/validation/validation.go

alculquicondor Sep 20, 2021

Add the same test cases as v1

Outdated

pkg/apis/apps/v1beta2/defaults_test.go

alculquicondor Sep 20, 2021

There should be a test were the feature is disabled. The field would have to remain `nil`.

Outdated

pkg/apis/apps/v1/defaults_test.go

alculquicondor Jun 28, 2021

so there was a bug where `StatefulSetMinReadySeconds` were not dropped on Updates? 😢

Outdated

pkg/registry/apps/statefulset/strategy.go

alculquicondor Jun 28, 2021

since you are already changing the log line: use structured logging `klog.InfoS` https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#change-log-functions-to-structured-equivalent

Outdated

...oller/statefulset/stateful_set_control.go

alculquicondor Jun 28, 2021

why not ```golang RollingUpdate: &apps.RolingUpdateStatefulSetStrategy{...} ``` You need the `func` for `MaxUnavailable`, but not here.

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Jun 28, 2021

nit: produce `numberTwo` before calling `return`, so you don't have nested lambda functions.

Outdated

pkg/apis/apps/validation/validation_test.go

ravisantoshgudimetla May 24, 2021

Can you add a test case for maxUnavailable enabled, field used in old and new

Outdated

...egistry/apps/statefulset/strategy_test.go

alculquicondor May 13, 2021

this should only be set if the feature gate is enabled.

Outdated

pkg/apis/apps/v1beta1/defaults.go

kow3ns Mar 10, 2021

So the original code works the way it does because once its established the invariant that all replicas are running and ready it can safely terminate precisely one Pod. This code preserves that invariant up to this point. What I'd expect to see here is code that does the following. Collect all targets in the range between the partition and Spec.Replicas. Count any targets in that range that are unhealthy (i.e. terminated or not running and ready as unavailable). Select the (MaxUnavailable - Unavailable) Pods, in order with respect to their ordinal for termination. Delete those pods and count the successful deletions. Update the status with the correct number of deletions.

Outdated

...oller/statefulset/stateful_set_control.go

kow3ns Mar 10, 2021

This comment needs to be updated

Outdated

...oller/statefulset/stateful_set_control.go

alculquicondor Mar 9, 2021

nit: use `sets.String`

Outdated

...oller/statefulset/stateful_set_control.go

alculquicondor Mar 9, 2021

For future reference: prefer to test exported functions. In this case, the test should be for PrepareForCreate, not dropStatefulSetDisabledFields.

Outdated

...egistry/apps/statefulset/strategy_test.go

krmayankk Mar 8, 2021

@lavalamp this is in the new test i added

Outdated

.../statefulset/stateful_set_control_test.go

krmayankk Mar 3, 2021

@lavalamp @kow3ns added this extra logic for the case when feature is enabled and then disabled again. ptal

Outdated

...oller/statefulset/stateful_set_control.go

alculquicondor Mar 2, 2021

why not use `cmp.Diff` instead of 2 functions?

Outdated

...egistry/apps/statefulset/strategy_test.go

alculquicondor Mar 2, 2021

nit: try the limit: "101%"

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Mar 2, 2021

Isn't this test case repeated above?

Outdated

pkg/apis/apps/validation/validation_test.go

lavalamp Jan 15, 2021

Should this require a non-default value as well? Otherwise it's basically a check that the object was written to while the gate was enabled, which doesn't seem to add much value to me. I'm not sure I see the benefit of feature-gating this in apiserver. The risky stuff is in the controller. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions kind of sort of says you can't do this at all? If we are going to feature gate this, shouldn't we also clear the field on read? But then we change objects in a way that would conflict with copies of those objects in caches. @liggitt may have thoughts.

pkg/registry/apps/statefulset/strategy.go

lavalamp Jan 15, 2021

Personally I think these tests test too much of the system, which makes them very hard to write and read. I would like to see them test all (at least probabalistically) orderings of which pods are unavailable. If I were doing this, I'd probably factor the controller logic to be more testable.

Outdated

.../statefulset/stateful_set_control_test.go

lavalamp Jan 15, 2021

I think you need to count all the pods before stopping any of them. The very first shutdown could have violated this, no? Do I misunderstand the feature?

Outdated

...oller/statefulset/stateful_set_control.go

lavalamp Jan 15, 2021

Not asking for a change since you didn't start this, but just a note that these prefixes should be specific to the case, otherwise there's a lot of ways the test could be passing for spurious reasons.

pkg/apis/apps/validation/validation_test.go

lavalamp Jan 15, 2021

what about 100%, is that OK or not? I'd prefer to test the exact first invalid value.

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Jan 14, 2021

this should be in the previous commit

Outdated

pkg/apis/apps/validation/validation_test.go

alculquicondor Jan 13, 2021

can you use cmp instead?

Outdated

...egistry/apps/statefulset/strategy_test.go

alculquicondor Jan 13, 2021

for consistency, do the same you are doing above: ```golang if statefulset.Spec.UpdateStrategy.RollingUpdate == nil { return false } return statefulset.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable != nil }

Outdated

pkg/registry/apps/statefulset/strategy.go

alculquicondor Jan 13, 2021

1.21

Outdated

pkg/features/kube_features.go

alculquicondor Jan 13, 2021

nit: `fldPath.Child("updateStrategy", "rollingUpdate")`

Outdated

pkg/apis/apps/validation/validation.go

alculquicondor Jan 13, 2021

Do not export this function, unless there is usage that I missed.

Outdated

pkg/apis/apps/validation/validation.go

tnozicka Nov 30, 2020

this seems to undo changes added by your first commit, would you mind cleaning it up? https://github.com/kubernetes/kubernetes/pull/82162/commits/c69cb2e82fe2f6715fd28bb5bd5810aaad014ce2#diff-f4270171dae1b0ad1792c3f5c5ac451e823af053462f464dd38d32fec4846b08R96

Outdated

pkg/apis/apps/validation/validation.go

tnozicka Oct 19, 2020

nit ```suggestion expectedSts *apps.StatefulSet ```

Outdated

...egistry/apps/statefulset/strategy_test.go

tnozicka Oct 19, 2020

t.Run

Outdated

...egistry/apps/statefulset/strategy_test.go

tnozicka Oct 19, 2020

nit: empty line

Outdated

...egistry/apps/statefulset/strategy_test.go

tnozicka Oct 19, 2020

belongs to the first API commit

Outdated

pkg/registry/apps/statefulset/strategy.go

tnozicka Oct 19, 2020

you got the changes also in the wrong (non API commit) https://github.com/kubernetes/kubernetes/pull/82162/commits/6bb42dbd62f581c4af9af646211c28cb73157e8c#diff-453cdbb00bec23e49e2da14c1989f01ef2705560867257fe2630c3f516ec6404R91

Outdated

pkg/registry/apps/statefulset/strategy.go

tnozicka Oct 19, 2020

```suggestion "StatefulSet %s/%s is waiting for pods to become available. (Currently unavailable: %v, maxUnavailable: %v)", ```

Outdated

...oller/statefulset/stateful_set_control.go

tnozicka Oct 19, 2020

nit: fix the capital letters and add a dot below

Outdated

...oller/statefulset/stateful_set_control.go

tnozicka Oct 19, 2020

Are old but healthy pods counted against maxUnavailable?

Outdated

...oller/statefulset/stateful_set_control.go

tnozicka Oct 19, 2020

```suggestion return nil, err ```

Outdated

...oller/statefulset/stateful_set_control.go

tnozicka Oct 19, 2020

this looks like a job for validation, rather then silently ignoring it

Outdated

...oller/statefulset/stateful_set_control.go

tnozicka Oct 19, 2020

nit: you've already set the default value above in https://github.com/kubernetes/kubernetes/pull/82162/commits/6bb42dbd62f581c4af9af646211c28cb73157e8c#diff-a1e25eb50e4b2aed8947c2caa5d6d1ddfef1a5f2f95fdf71f2388c657823ee13R520

...oller/statefulset/stateful_set_control.go

tnozicka Oct 19, 2020

2 sounds like `numberTwo`, not `zeroNum` (similarly applies to the other case bellow)

Outdated

pkg/apis/apps/validation/validation_test.go

tnozicka Oct 19, 2020

why isn't this code included in the `ValidateRollingUpdateStatefulSet` function?

Outdated

pkg/apis/apps/validation/validation.go

liggitt Nov 14, 2019

need tests demonstrating this works properly for all combinations of enabled/disabled, old object with feature present/absent, new object with feature present/absent. see various `func TestDrop...` tests as an example

Outdated

pkg/registry/apps/statefulset/strategy.go

lavalamp Sep 19, 2019

Should also prevent using a percentage in this case, no?

Outdated

pkg/apis/apps/validation/validation.go

lavalamp Sep 3, 2019

May it be zero?

Outdated

staging/src/k8s.io/api/apps/v1beta2/types.go

Resolved conversations (17)

lavalamp Mar 24, 2022

My thoughts on this function -- this is a lot of logic change. If it goes wrong even when the feature isn't used, how do users disable it? It doesn't check the feature flag. I would keep the old version of this logic, and decide whether to use the old or new version by whether the flag is on or not.

...oller/statefulset/stateful_set_control.go

lavalamp Mar 24, 2022

Instead why not do this on line 630: ``` if unavailablePods >= maxUnavailable { return } ```

Outdated

...oller/statefulset/stateful_set_control.go

lavalamp Mar 24, 2022

Does this tolerate `nil`s?

Outdated

...oller/statefulset/stateful_set_control.go

ravisantoshgudimetla Jan 12, 2022

I think we also need to check based on `updateMin`

Outdated

...oller/statefulset/stateful_set_control.go

ravisantoshgudimetla Jan 12, 2022

I think this can be simplified by making `podsToDelete=0` when it is <0.

Outdated

...oller/statefulset/stateful_set_control.go

soltysh Oct 15, 2021

In the case when your entire StatefulSet is 5, for example, and all of the pods are unavailable, with `maxUnavilable` set to 1, the result of this equation can be negative, which will trip the condition below you're checking for equality. You should change the condition to cover that case or ensure that `podsToDelete` is never negative. That should be exercised in the units and it's potentially what @lavalamp mentioned.

...oller/statefulset/stateful_set_control.go

alculquicondor Sep 20, 2021

I would discourage the use of a helper function like this to build test tables. DRY principles don't necessarily apply to tests. A couple of references, if you are interested in reading more: https://mtlynch.io/good-developers-bad-tests/ https://marcingryszko.medium.com/good-vs-bad-test-helpers-62d552004bc5

...egistry/apps/statefulset/strategy_test.go

alculquicondor Sep 20, 2021

No need to include this line (it's false by default)

...egistry/apps/statefulset/strategy_test.go

ravisantoshgudimetla May 24, 2021

Better to check featuregate here

pkg/apis/apps/validation/validation.go

alculquicondor Mar 9, 2021

Use `k8s.io/apimachinery/pkg/api/errors.IsNotFound`. If that's true, the deletion can be considered successful.

...oller/statefulset/stateful_set_control.go

lavalamp Jan 15, 2021

shouldn't you add this pod to the list of unavailable pods? it's definitely absolutely about to become unavailable.

...oller/statefulset/stateful_set_control.go

alculquicondor Jan 14, 2021

nit: `pointer.Int32ptr(0)`

pkg/apis/apps/v1/defaults.go

LouisPlisso Jan 22, 2020

We do have a usecase for MaxUnavailable to be 0: can this just be a warning or be provided a way to force MaxUnavailable to 0?

pkg/apis/apps/validation/validation.go

liggitt Nov 14, 2019

is 1 the current implicit behavior?

pkg/apis/apps/types.go

liggitt Nov 14, 2019

I was surprised this didn't change fixture test data under k8s.io/api/testdata. Turns out, our test generator wasn't populating *intstr.IntOrString objects. Fix is in https://github.com/kubernetes/kubernetes/pull/85258. Once that merges, rebase and run `UPDATE_COMPATIBILITY_FIXTURE_DATA=true go test ./vendor/k8s.io/api/` to update fixture data with this new field.

staging/src/k8s.io/api/apps/v1/types.go

lavalamp Oct 15, 2019

@liggitt did we decide feature gates should cause us to clear the relevant fields? I thought we just prevented people from creating new objects setting the field?

Outdated

pkg/registry/apps/statefulset/strategy.go

lavalamp Sep 3, 2019

What is 10% of 1? 10% of 2?

staging/src/k8s.io/api/apps/v1beta2/types.go