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

Allow emulation version to be set in integration test #127643

Merged
merged 3 commits into from
Sep 27, 2024
Merged

Allow emulation version to be set in integration test #127643

merged 3 commits into from
Sep 27, 2024

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Sep 26, 2024 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Allow emulation version to be set in integration test. Use a VAP test as example for creating a disablement test.

Which issue(s) this PR fixes:

Fixes #127639

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. 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 Sep 26, 2024
@k8s-ci-robot k8s-ci-robot requested review from aojea and sttts September 26, 2024 02:02
@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 Sep 26, 2024
@Jefftree Jefftree mentioned this pull request Sep 26, 2024
Merged
@Jefftree
Copy link
Member Author

/assign @jpbetz @aaron-prindle
/cc @cici37
/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Sep 26, 2024
@k8s-ci-robot k8s-ci-robot requested a review from cici37 September 26, 2024 14:42
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 26, 2024
@cici37
Copy link
Contributor

cici37 commented Sep 26, 2024

/lgtm
Thank you!

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

LGTM label has been added.

Git tree hash: ba3d3aed0a15a1c4f1369ffa572fb09f4710c970

@aaron-prindle
Copy link
Contributor

/lgtm

@Jefftree @jpbetz
Update cmd/kube-apiserver/app/testing/testserver.go
396d365 
Co-authored-by: Joe Betz <jpbetz@google.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2024
@Jefftree @jpbetz
Update cmd/kube-apiserver/app/testing/testserver.go
ff13ffc 
Co-authored-by: Joe Betz <jpbetz@google.com>
@jpbetz
Copy link
Contributor

jpbetz commented Sep 26, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, jpbetz

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 Sep 26, 2024
@Jefftree
Copy link
Member Author

/hold

Hmm noticed the integration test is failing in Cici's PR: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/127302/pull-kubernetes-integration/1839361843622055936. Going to double check this fix before merging

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2024
@cici37
Copy link
Contributor

cici37 commented Sep 26, 2024

/hold

Hmm noticed the integration test is failing in Cici's PR: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/127302/pull-kubernetes-integration/1839361843622055936. Going to double check this fix before merging

Just going to mention it. Thank you!

@cici37
Copy link
Contributor

cici37 commented Sep 26, 2024

looks like when SetFeatureGateDuringTest, the emulation version recognized is using the default version:

kubernetes/staging/src/k8s.io/component-base/featuregate/feature_gate.go

Line 383 in ff13ffc

errs := f.unsafeSetFromMap(enabled, enabledRaw, f.EmulationVersion())

@Jefftree
Copy link
Member Author

Yeah I see. So we would need to set the feature gate after the emulation version is set, and currently with the PR it's the other way around.

@Jefftree
Copy link
Member Author

From talking to Joe, feature gates are generally set before we call StartTestServer so emulation version must be set before that and imo we shouldn't set the emulation version this late in StartTestServer.

We should also return an error if a feature gate was overridden and we try to set the emulation version after.

@Jefftree
Copy link
Member Author

/hold cancel

This is needed for #127302. Will address #127643 (comment) in a follow up

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2024
@jpbetz jpbetz mentioned this pull request Sep 27, 2024
Merged
@jpbetz
Copy link
Contributor

jpbetz commented Sep 27, 2024

/lgtm

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

LGTM label has been added.

Git tree hash: db1f9416b9651fdb061c5242d92f9fc87e90874f

@k8s-ci-robot k8s-ci-robot merged commit 80941e3 into kubernetes:master Sep 27, 2024
15 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Sep 27, 2024
@Jefftree Jefftree deleted the set-emulation-integration-test branch October 29, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea Awaiting requested review from aojea

@sttts sttts Awaiting requested review from sttts

@cici37 cici37 Awaiting requested review from cici37

@aaron-prindle aaron-prindle Awaiting requested review from aaron-prindle

@jpbetz jpbetz Awaiting requested review from jpbetz

Assignees

@jpbetz jpbetz

@aaron-prindle aaron-prindle

@cici37 cici37

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. 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-none Denotes a PR that doesn't merit a release note. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.

Emulation Version cannot be set in integration test
5 participants
@Jefftree @cici37 @k8s-ci-robot @aaron-prindle @jpbetz