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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Jun 23, 2022
Merged

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

merged 1 commit into from Jun 23, 2022

Conversation

Abirdcfly
Copy link
Member

@Abirdcfly Abirdcfly commented Jun 19, 2022 •
edited by liggitt

Signed-off-by: Abirdcfly fp544037857@gmail.com

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.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 19, 2022
@k8s-ci-robot k8s-ci-robot added area/apiserver area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 19, 2022
@Abirdcfly
Copy link
Member Author

/sig api-machinery

@Abirdcfly
Copy link
Member Author

/assign @apelisse @lavalamp

@Abirdcfly
Copy link
Member Author

/retest

@Abirdcfly
Copy link
Member Author

Abirdcfly commented Jun 21, 2022 •
edited

/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 k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 21, 2022
@k8s-ci-robot
Copy link
Contributor

@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.

apelisse
apelisse reviewed Jun 21, 2022
View reviewed changes
test/cmd/get.sh Outdated Show resolved Hide resolved
lavalamp
lavalamp reviewed Jun 21, 2022
View reviewed changes
test/cmd/get.sh Outdated Show resolved Hide resolved
lavalamp
lavalamp reviewed Jun 21, 2022
View reviewed changes
staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go
@@ -779,7 +779,7 @@ func growSlice(v reflect.Value, maxCapacity int, sizes ...int) {
return
}
if v.Len() > 0 {
extra := reflect.MakeSlice(v.Type(), 0, max)
extra := reflect.MakeSlice(v.Type(), v.Len(), max)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@logicalhan do you know anything about this function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create #110734 to track these 2 points

@Abirdcfly
fix: --chunk-size with selector returns missing result
fba2976 
Signed-off-by: Abirdcfly <fp544037857@gmail.com>
@lavalamp
Copy link
Member

/lgtm
/approve

Thanks for this fix!!

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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2022
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2022
@Abirdcfly Abirdcfly mentioned this pull request Jun 22, 2022
Open
@k8s-ci-robot k8s-ci-robot merged commit dee37aa into kubernetes:master Jun 23, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 23, 2022
@Abirdcfly Abirdcfly mentioned this pull request Jun 23, 2022
Merged
@Abirdcfly Abirdcfly deleted the fixchunksize branch June 24, 2022 02:03
This was referenced Jun 24, 2022
Merged
Merged
Closed
@lavalamp lavalamp mentioned this pull request Jun 24, 2022
Merged
k8s-ci-robot added a commit that referenced this pull request Jun 30, 2022
@k8s-ci-robot
Merge pull request #110735 from Abirdcfly/automated-cherry-pick-of-#1…
9da76d2 
…10652-upstream-release-1.24

Automated cherry pick of #110652: fix: --chunk-size with selector returns missing result
k8s-ci-robot added a commit that referenced this pull request Jun 30, 2022
@k8s-ci-robot
Merge pull request #110758 from Abirdcfly/automated-cherry-pick-of-#1…
8f4b8fa 
…10652-upstream-release-1.22

Automated cherry pick of #110652: fix: --chunk-size with selector returns missing result
k8s-ci-robot added a commit that referenced this pull request Jun 30, 2022
@k8s-ci-robot
Merge pull request #110757 from Abirdcfly/automated-cherry-pick-of-#1…
b85c292 
…10652-upstream-release-1.23

Automated cherry pick of #110652: fix: --chunk-size with selector returns missing result
@openshift-ci-robot openshift-ci-robot mentioned this pull request Jun 30, 2022
Merged
This was referenced Jun 30, 2022
Closed
Closed
@openshift-ci-robot openshift-ci-robot mentioned this pull request Jul 1, 2022
Merged
@wojtek-t
Copy link
Member

Thanks for the fix!

@ialidzhikov ialidzhikov mentioned this pull request Sep 20, 2022
Closed
@openshift-ci-robot openshift-ci-robot mentioned this pull request Oct 4, 2022
Merged
@liggitt liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Jan 30, 2023
@zjy365 zjy365 mentioned this pull request Feb 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@brianpursley brianpursley Awaiting requested review from brianpursley

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@apelisse apelisse Awaiting requested review from apelisse

Assignees

@lavalamp lavalamp

@apelisse apelisse

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

--chunk-size with selector returns missing result, so describe node / drain could be works wrong
6 participants
@Abirdcfly @k8s-ci-robot @lavalamp @wojtek-t @apelisse @liggitt