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.

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

feat: add EMULATED_VERSION env var and --emulated-version flag params to k8s binaries flags in hack/local-up-cluster.sh #126789

Merged

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Aug 19, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

As part of kubernetes/enhancements#4330, we want to add the --emulated-version flag as an option (not set by default) through our test and dev infrastructure to make it easier for users and test automation to use the flag.

This PR adds a new environment variable EMULATED_VERSION in hack/local-cluster-up.sh and adds the --emulated-version flag to the kubernetes binaries that support it currently:

  • kube-apiserver
  • kube-controller-manager
  • kube-scheduler

From looking at similar PRs that update hack/local-up-cluster.sh similarly I believe no other files need to be touched other than the local-up-cluster.sh script directly:

Which issue(s) this PR fixes:

Fixes #126788

Special notes for your reviewer:

Manually tested via running hack/local-up-cluster.sh w/ and w/o EMULATED_VERSION env var set

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. 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-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 19, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @aaron-prindle. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Aug 19, 2024
@k8s-ci-robot k8s-ci-robot requested review from SataQiu and sttts August 19, 2024 17:11
@Jefftree
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 19, 2024
@@ -632,6 +633,7 @@ EOF
--etcd-servers="http://${ETCD_HOST}:${ETCD_PORT}" \
--service-cluster-ip-range="${SERVICE_CLUSTER_IP_RANGE}" \
--feature-gates="${FEATURE_GATES}" \
--emulated-version="${EMULATED_VERSION}" \
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we use --emulated-version=kube=${EMULATED_VERSION} to be aligned with #125742? It is defaulted to kube but consistency would be nice.

Copy link
Contributor Author

@aaron-prindle aaron-prindle Aug 19, 2024

Choose a reason for hiding this comment

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

Ah good catch thanks! I've updated the value for all three flag entries as recommended. Now they are:
--emulated-version=kube=${EMULATED_VERSION}

Copy link
Contributor Author

@aaron-prindle aaron-prindle Aug 19, 2024

Choose a reason for hiding this comment

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

I've undone this change as there was some test failures and UX issues in my testing with the "kube=" prefix added to the flag entires. The default value for EMULATED_VERSION is "" so the value passed is kube="". Going to keep the initial syntax as adding logic to prepend a suffix or use single quotes is different than other env vars in the file and also the prepending logic might confuse users. With the initial approach users can set EMULATED_VERSION=1.31 OR EMULATED_VERSION=kube=1.31

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the script to instead use:

EMULATED_VERSION=${EMULATED_VERSION:+kube=$EMULATED_VERSION}

This way it is consistent with #125742 and has the default values we want

Copy link
Member

Choose a reason for hiding this comment

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

perfect, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

aside: in most other scripts we make the "public API" use KUBE_ but we should probably remain self-consistent within this script/tool

@aaron-prindle aaron-prindle force-pushed the emu-version-local-up-cluster branch from 516776a to 31499b6 Compare August 19, 2024 19:28
@Jefftree
Copy link
Member

/sig api-machinery
/lgtm
/assign @BenTheElder
/cc @jpbetz

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 19, 2024
@k8s-ci-robot k8s-ci-robot requested a review from jpbetz August 19, 2024 19:39
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 19, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1cb6d75f4b43f05ca7d36598436f2caf119e06b5

@aaron-prindle
Copy link
Contributor Author

/retest

@aaron-prindle aaron-prindle force-pushed the emu-version-local-up-cluster branch from 31499b6 to 0cc09d4 Compare August 19, 2024 20:33
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 19, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7286740c464dc2625a64efa5c43852738f96cef6

@BenTheElder
Copy link
Member

cc @liggitt

I'd prefer to get dims's input for local-up-cluster as kinda the main steward at this point, but this looks straight forward enough, I don't think we're trying to enable skew here with older releases that lack these flags.

@BenTheElder
Copy link
Member

er cc @dims for FYI later at least

Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2024
@BenTheElder
Copy link
Member

/retest

the e2e-gce failure was #126800

… to k8s binaries flags in hack/local-up-cluster.sh
@aaron-prindle aaron-prindle force-pushed the emu-version-local-up-cluster branch from 6a59542 to a83f0f6 Compare August 20, 2024 21:18
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 20, 2024
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2024
@aaron-prindle aaron-prindle force-pushed the emu-version-local-up-cluster branch from a83f0f6 to 780cfa5 Compare August 20, 2024 21:20
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 20, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-prindle, BenTheElder

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 Aug 20, 2024
@k8s-ci-robot
Copy link
Contributor

@aaron-prindle: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-local-e2e 780cfa5 link false /test pull-kubernetes-local-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

@Jefftree
Copy link
Member

/lgtm

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

LGTM label has been added.

Git tree hash: 39057e4fb2f234c8c91df68c76e136bf1c1d88d4

@aaron-prindle
Copy link
Contributor Author

/retest

@Jefftree
Copy link
Member

@BenTheElder it seems like the job pull-kubernetes-local-e2e has not succeeded recently: https://prow.k8s.io/job-history/gs/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-local-e2e. It's not blocking but is this something that is already tracked?

@k8s-ci-robot k8s-ci-robot merged commit 563ab1b into kubernetes:master Aug 21, 2024
14 of 15 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Aug 21, 2024
@fedebongio
Copy link
Contributor

/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 Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/provider/gcp Issues or PRs related to gcp provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging this pull request may close these issues.

local-up-cluster.sh currently does not have support for the --emulated-version flag
5 participants