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

Initial deprecation of kubeadm v1beta1 apis #83276

Merged
merged 1 commit into from Oct 8, 2019

Conversation

Klaven
Copy link
Contributor

@Klaven Klaven commented Sep 29, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR deprecates the v1beta1 apies in kubeadm in favor of the v1beta2 apis.
Which issue(s) this PR fixes:

Fixes #

There is no issue filed, was discussed in 18th September 2019 kubeadm office hours when version 1.17 was planned.

Special notes for your reviewer:
I hope this is going on the right path, I looked up /api/apps/v1beta1 was deprecated and kind of just followed along. Please let me know anything else that needs done and I will get it done. Thank you!

Does this PR introduce a user-facing change?:

Action Required: `kubeadm.k8s.io/v1beta1` has been deprecated, you should update your config to use newer non-deprecated API versions.

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


@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 29, 2019
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 29, 2019
@neolit123
Copy link
Member

/assign @rosti

@@ -113,6 +115,7 @@ type ClusterConfiguration struct {
ClusterName string `json:"clusterName,omitempty"`
}

// DEPRECATED
Copy link
Member

Choose a reason for hiding this comment

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

i think we should add the message:

DEPRECATED - This group version of X is deprecated by apis/kubeadm/v1beta2/X.

to:

InitConfiguration
JoinConfiguration
ClusterConfiguration

but not have it for the other objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for what @neolit123 said

@neolit123
Copy link
Member

/kind cleanup
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 29, 2019
@neolit123
Copy link
Member

in the release note:

you should update to use the kubeadm/app/apis/kubeadm/v1beta2

please omit the.

Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @Klaven !
We also need to modify validateSupportedVersion (can be found here) in the following ways:

  • Remove the allowDeprecated param and consider it always false.
  • Add the v1beta1 version string to the (now empty) map of deprecated versions.
  • Instead of returning an error, print a warning, that the version is deprecated and users need to use kubeadm config migrate to switch to latest.

@@ -113,6 +115,7 @@ type ClusterConfiguration struct {
ClusterName string `json:"clusterName,omitempty"`
}

// DEPRECATED
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for what @neolit123 said

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 1, 2019
@Klaven
Copy link
Contributor Author

Klaven commented Oct 1, 2019

* Remove the `allowDeprecated` param and consider it always `false`.

@rosti I am not sure I agree with this. If I did it would let deprecated api be used with init and join. I split the function so that it only returned an error if not allowed, and if allowed it logs a warning. Now that I think about it... it really is not needed anyhow as when you are allowed to use it you are running migrate anyhow and the warning is just going to tell you the same. What do you think?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2019
@rosti
Copy link
Contributor

rosti commented Oct 2, 2019

@Klaven the idea is to have beta level APIs working out of the box. No matter if they are deprecated or not. Hence, we need to log a warning.
However, I think, that you are right for the flag. Let's keep it that way, just change the error return into a klog.Warning.

@Klaven Klaven force-pushed the pr_v1beta1_dep branch 2 times, most recently from ee3e065 to beba0dc Compare October 4, 2019 00:35
@Klaven
Copy link
Contributor Author

Klaven commented Oct 4, 2019

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@neolit123
Copy link
Member

/retest

@@ -72,7 +74,7 @@ func validateSupportedVersion(gv schema.GroupVersion, allowDeprecated bool) erro
}

if _, present := deprecatedAPIVersions[gvString]; present && !allowDeprecated {
return errors.Errorf("your configuration file uses a deprecated API spec: %q. Please use 'kubeadm config migrate --old-config old.yaml --new-config new.yaml', which will write the new, similar spec using a newer API version.", gv.String())
klog.Errorf("your configuration file uses a deprecated API spec: %q. Please use 'kubeadm config migrate --old-config old.yaml --new-config new.yaml', which will write the new, similar spec using a newer API version.", gv.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Errorf/Warningf
Furthermore, no need to call gv.String(), just pass gv, .String() will be called under the hood.

@rosti
Copy link
Contributor

rosti commented Oct 4, 2019

Another thing, can you change the release note to something like this?

Action Required: `kubeadm.k8s.io/v1beta1` has been deprecated, you should update your config to use newer non-deprecated API versions.

@Klaven
Copy link
Contributor Author

Klaven commented Oct 7, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @Klaven!
/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Klaven, rosti

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 Oct 7, 2019
@craiglpeters craiglpeters added this to In progress in Provider Azure Oct 7, 2019
@craiglpeters craiglpeters removed this from In progress in Provider Azure Oct 7, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

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/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants