fix: --chunk-size with selector returns missing result

[Abirdcfly]

Signed-off-by: Abirdcfly [email protected]

What type of PR is this?

/kind bug

What this PR does / why we need it:

It is easy to reproduce this bug:

# 1. create a 1.24 cluster
$ kind create cluster
# 2. create 2 deploy replicas=5
$ kubectl create deployment nginx1 --image=nginx --replicas=5 
$ kubectl create deployment nginx2 --image=nginx --replicas=5
# 3. You should get 5 results, but only get 4.
$ kubectl get po -l app=nginx1 --chunk-size=2
# 4. But if you use --chunk-size=1, you get the correct 5 results again
$ kubectl get po -l app=nginx1 --chunk-size=1

I have tested several versions and only the >= 1.24 version has errors.

I found that once I revert #108569, the error disappears. 😂 This means that it is #108569 that exposes the bug.

But the version provided by the reporter is v1.23.6-gke.1700. I did not reproduce the bug in the 1.23.6 cluster created by kind.

So I think this fix should at least cherry-pick to 1.23 to help other distributions fix this issue.

Which issue(s) this PR fixes:

Fixes #110625

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a 1.24 regression that caused the wrong result length when using --chunk-size and --selector together

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[Abirdcfly]

/sig api-machinery

[Abirdcfly]

/assign @apelisse @lavalamp

[Abirdcfly]

/retest

[Abirdcfly]

/cc @smarterclayton
in case I misunderstood the original meaning of the function

/cc @wojtek-t
because it looks like you are focusing on that

/triage accepted

[k8s-ci-robot]

@Abirdcfly: GitHub didn't allow me to request PR reviews from the following users: focusing, that, because, it, like, are, on, I, misunderstood, function, meaning, of, you, the, original, looks, in, case.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @smarterclayton in case I misunderstood the original meaning of the function

/cc @wojtek-t because it looks like you are focusing on that

/triage accepted

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.

[lavalamp]

This is kind of a terrifying function and it needs a bunch of unit tests. ... it looks to have been broken quite badly in ways that should cause drastic problems everywhere it's used, so either it's barely ever called, or we're reading it wrong, but either way I need to see a unit test.

[lavalamp]

@logicalhan do you know anything about this function?

[Abirdcfly]

so either it's barely ever called

I agree with this opinion. I have tested several versions and only the >= 1.24 version has errors.

The current exposure of this problem depends on #108569 code modifications and #110625 feedback. (Thanks @mborsz and @yokomotod!)

@yokomotod's test case is typical, in fact, if only some objects with the same lables were created, this problem would not be reproduced, otherwise we would not have found a problem here.

Maybe other scenarios also have the problem, but we did not find the feedback

[lavalamp]

Can you open an issue to capture these two points? (I won't block this PR on them since the bug looks disastrous)

• I am skeptical that this function is actually an improvement over just letting the go runtime pick the size. I'd like to see benchmarks with it swapped out for just calling append.
• If we do keep it, this desperately needs to be rewritten to use the new type parameters instead of reflection.

[Abirdcfly]

Create #110734 to track these 2 points

[lavalamp]

/lgtm
/approve

Thanks for this fix!!

Can you file the issue I mentioned above so we don't forget?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Abirdcfly, lavalamp

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:

• staging/src/k8s.io/apiserver/pkg/storage/OWNERS [lavalamp]
• test/integration/apiserver/OWNERS [lavalamp]

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

[wojtek-t]

Thanks for the fix!