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

kubeadm: add v1beta4 to scheme; add --allow-experimental-api flag #118866

Merged
merged 2 commits into from Jun 27, 2023
Merged

kubeadm: add v1beta4 to scheme; add --allow-experimental-api flag #118866

merged 2 commits into from Jun 27, 2023

Conversation

neolit123
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

kubeadm: add v1beta4 to the kubeadm API scheme

The highest priority is still v1beta3.

kubeadm: add the --allow-experimental-api flag to "config" commands

Add the flag --allow-experimental-api to the "config migrate" and
"config validate" commands. The flag allows validating / migrating-to
a unreleased / experimental API version.

Add a new experimentalAPIVersions map in validateSupportedVersion()
that contains v1beta4.

Which issue(s) this PR fixes:

xrefs kubernetes/kubeadm#2890

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: add the --allow-experimental-api flag to "kubeadm config migrate/validate" commands. It can be used to migrate or validate WIP / experimental APIs in the future.

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

@neolit123
kubeadm: add v1beta4 to the kubeadm API scheme
0774f77 
The highest priority is still v1beta3.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 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-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 26, 2023
@neolit123
Copy link
Member Author

/priority important-longterm
/triage accepted
/cc @SataQiu @pacoxu @chendave

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 26, 2023
@neolit123
kubeadm: add the --allow-experimental-api flag to "config" commands
f04484f 
Add the flag --allow-experimental-api to the "config migrate" and
"config validate" commands. The flag allows validating / migrating-to
a unreleased / experimental API version.

Add a new experimentalAPIVersions map in validateSupportedVersion()
that contains v1beta4.
@neolit123 neolit123 mentioned this pull request Jun 26, 2023
Open
29 tasks
neolit123
neolit123 commented Jun 26, 2023
View reviewed changes
cmd/kubeadm/app/apis/kubeadm/scheme/scheme.go
utilruntime.Must(scheme.SetVersionPriority(v1beta3.SchemeGroupVersion))
utilruntime.Must(v1beta4.AddToScheme(scheme))
// TODO: https://github.com/kubernetes/kubeadm/issues/2890
// make v1beta4 highest priority
Copy link
Member Author

Choose a reason for hiding this comment

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

this is tracked in the tasklist of #118866

Copy link
Member Author

@neolit123 neolit123 Jun 26, 2023 •
edited

Choose a reason for hiding this comment

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

what happens now is that kubeadm still uses v1beta3 by default, until we adjust the priority here.

cmd/kubeadm/app/util/config/common.go
experimentalAPIVersions := map[string]string{
// TODO: https://github.com/kubernetes/kubeadm/issues/2890
// remove this from experimental once v1beta4 is released
"kubeadm.k8s.io/v1beta4": "v1.28",
Copy link
Member Author

Choose a reason for hiding this comment

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

task tracked in #118866

cmd/kubeadm/app/util/config/common_test.go
Comment on lines +90 to +101
gv: schema.GroupVersion{
Group: KubeadmGroupName,
Version: "v1beta4",
},
allowExperimental: true,
},
{
gv: schema.GroupVersion{
Group: KubeadmGroupName,
Version: "v1beta4",
},
expectedErr: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

there are no /cmd/kubeadm/test integration tests currently, only unit tests.
integration tests are OK, but effectively it would only be testing if the --allow-experimental-api (BOOL CLI flag) is working.

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to see what can be done with the migration unit test

restore TestMigrateOldConfigFromFile in common_test.go once v1beta4 is added. See how we can make this test optional if only one API version is available (e.g. in the future only v1beta4 is avaiable and v1beta3 is removed)

kubernetes/kubeadm#2890

cmd/kubeadm/app/cmd/options/constant.go
@@ -142,4 +142,7 @@ const (

// CleanupTmpDir flag indicates whether reset will cleanup the tmp dir
CleanupTmpDir = "cleanup-tmp-dir"

// AllowExperimentalAPI flag can be used to allow experimental / work in progress APIs
AllowExperimentalAPI = "allow-experimental-api"
Copy link
Member Author

Choose a reason for hiding this comment

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

flag naming is similar to upgrade apply, where we have --allow-foo flags

@neolit123
Copy link
Member Author

unrelated pkg/controller flake
/retest

@chendave
Copy link
Member

/assign

chendave
chendave approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@chendave chendave left a comment

Choose a reason for hiding this comment

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

leaving LGTM to @pacoxu or @SataQiu

SataQiu
SataQiu reviewed Jun 27, 2023
View reviewed changes
Copy link
Member

@SataQiu SataQiu 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 lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a8824ab07c03db5672aab1c899f211a4677df478

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, neolit123, SataQiu

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 merged commit 1c32c3b into kubernetes:master Jun 27, 2023
12 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jun 27, 2023
@pacoxu pacoxu mentioned this pull request Jul 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SataQiu SataQiu SataQiu left review comments

@chendave chendave chendave approved these changes

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

@pacoxu pacoxu Awaiting requested review from pacoxu

Assignees

@chendave chendave

@SataQiu SataQiu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. 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.28
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@neolit123 @chendave @k8s-ci-robot @SataQiu