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

Kubeadm remove v1alpha2 api #69055

Merged

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR removes v1alpha2 API from kubeadm.

Which issue(s) this PR fixes:
Ref: kubernetes/kubeadm#911

Special notes for your reviewer:
This PR is the first of a series or PRs that will remove v1alpha2, cleanup all the references to v1alpha2 objects, and finally add v1beta1 api version; the work is divided into multiple PRs hopefully more easy to review.

This PR leaves some TODOs as a placeholder for implementing v1alpha3-->v1beta1 roundtrip tests in following PRs.

Release note:

[action required] kubeadm: The `v1alpha2` config API has been removed.
Please convert your `v1alpha2` configuration files to `v1alpha3` using the
`kubeadm config migrate` command of kubeadm v1.12.x

@kubernetes/sig-cluster-lifecycle-pr-reviews
sig cluster-lifecycle
kind api-change
area kubeadm

/cc @timothysc @luxas

@k8s-ci-robot k8s-ci-robot added 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/XXL Denotes a PR that changes 1000+ 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. area/kubeadm approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 25, 2018
@neolit123
Copy link
Member

thanks @fabriziopandini
i will have a look a bit later.

/kind api-change

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 25, 2018
@timothysc
Copy link
Member

Please merge this with #69056 into a single changeset for v1alpha2 for historical purposes.

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2018
@timothysc
Copy link
Member

Also can you squash commits to 2.

  • removal of v1alpha2
  • autogenerated

@fabriziopandini
Copy link
Member Author

Dear k8s ci robot
/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce-100-performance

Copy link
Member

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

Please address the comment, or possibly follow up on a separate PR.

oldKnownAPIVersions := map[string]string{
"kubeadm.k8s.io/v1alpha1": "v1.11",
"kubeadm.k8s.io/v1alpha2": "v1.12",
Copy link
Member

Choose a reason for hiding this comment

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

b/c you are removing alpha 2 should you remove v1.11?

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 piece of code gives warning to user if the pass to kubeadm an older version of the config, and tell what version of kubeadm can be used for executing config migrate.
I think that giving a warning also for v1alpha1 doesn't hurts...

@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, 2018
@timothysc timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: fabriziopandini, timothysc

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

@chuckha
Copy link
Contributor

chuckha commented Sep 27, 2018

Is the goal here to have one alpha version and n beta versions?

Seems like removing the v1alpha2 type is just as dangerous as making a breaking change to v1alpha1 like in the 1.10 release. It feels like at this point we'd want to rename v1alpha2 to v1alpha1 and v1alpha3 to v1beta1.

@timothysc
Copy link
Member

@chuckha - It's explicitly stated that the support model for kubeadm needs to upgrade through. So a 1.11 version can only upgrade from 1.10. Maintaining code and machinery of past versions (that aren't supported) is an albatross so we continually shed the code every cycle.

@chuckha
Copy link
Contributor

chuckha commented Sep 27, 2018

@timothysc right, I get that, but if I'm reading this right, we're getting rid of the newer version (v1alpha2) and keeping the older one (v1alpha1), I'd suggest renaming v1alpha2 to v1alpha1 and getting rid of the current v1alpha1.

@neolit123
Copy link
Member

neolit123 commented Sep 27, 2018

@fabriziopandini
LGTM

@chuckha @timothysc

Seems like removing the v1alpha2 type is just as dangerous as making a breaking change to v1alpha1 like in the 1.10 release

i know that removing v1alpha1 early in the 1.12 cycle broke kubernetes-anywhere because config --migrate no longer worked on master and we no longer supported the old config.

this early removal method, forced us to handle versions in bash, which is a bad outcome:
https://github.com/kubernetes/kubernetes-anywhere/blob/master/phase2/kubeadm/configure-vm-kubeadm-master.sh

i will send a PR for k-a, because i'm pretty sure this PR will make the master tests red.

@fabriziopandini
Copy link
Member Author

@chuckha

I get that, but if I'm reading this right, we're getting rid of the newer version (v1alpha2) and keeping the older one (v1alpha1)

Current version is v1alpha3; this PR is getting rid of v1alpha2
Soonish I will add v1beta1 and all the machinery for converting v1alpha3->v1beta1

@neolit123

i will send a PR for k-a, because i'm pretty sire this PR will make the master tests red.

Thanks. We can make k/a use v1alpha3 for kubeadm v>=1.12

@timothysc
Copy link
Member

/test pull-kubernetes-integration
/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot k8s-ci-robot merged commit 587914c into kubernetes:master Sep 27, 2018
@fabriziopandini fabriziopandini deleted the kubeadm-remove-v1alpha2 branch September 28, 2018 07:49
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/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants