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: Add (duplicated) v1alpha2 Config API #63788

Merged
merged 5 commits into from
May 16, 2018

Conversation

luxas
Copy link
Member

@luxas luxas commented May 14, 2018

What this PR does / why we need it:
Work in progress PR to add a (initially duplicated) v1alpha2 we can iterate on during the cycle.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of kubernetes/community#2131

Special notes for your reviewer:
This PR depends on:

The first commit is from #63799.
The second commit duplicates v1alpha1, but updates timestamps, and doesn't require the upgrade.go.
The third commit does the mechanical bump of using v1alpha1 -> v1alpha2
The fourth commit updates bazel

Release note:

[action required] kubeadm now uses an upgraded API version for the configuration file, `kubeadm.k8s.io/v1alpha2`. kubeadm in v1.11 will still be able to read `v1alpha1` configuration, and will automatically convert the configuration to `v1alpha2` internally and when storing the configuration in the ConfigMap in the cluster.

@kubernetes/sig-cluster-lifecycle-pr-reviews @liztio

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 14, 2018
@k8s-ci-robot k8s-ci-robot requested review from dixudx and kad May 14, 2018 11:21
@@ -0,0 +1,66 @@
/*
Copyright 2016 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

may as well bump this to 2018

@@ -0,0 +1,137 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

probably don't need to copy this over? or if we do, we should delete it from v1alpha1

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, I will delete this. This isn't needed in v1alpha2

return nil
}

func Convert_v1alpha1_MasterConfiguration_To_kubeadm_MasterConfiguration(in *MasterConfiguration, out *kubeadm.MasterConfiguration, s conversion.Scope) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this handle the proxy featureflags?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, Migrate takes care of that, as that happens at unmarshal time, not conversion time

@liztio
Copy link
Contributor

liztio commented May 14, 2018

this may be easier to read once #63782 is rebased onto it

@luxas
Copy link
Member Author

luxas commented May 14, 2018

this may be easier to read once #63782 is rebased onto it

yes, very much easier after the first PRs are merged.

@timothysc
Copy link
Member

I'll wait on this till the other ones are fix'd up. Please ping when this one is ready.

@luxas luxas changed the title [WIP] kubeadm: Add v1alpha2 Config API kubeadm: Add (duplicated) v1alpha2 Config API May 15, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2018
Copy link
Contributor

@liztio liztio left a comment

Choose a reason for hiding this comment

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

I think next time it would've been better to add the code in one PR, then switch it as the default in the next one. But I understand we were under a time crunch.

@liztio
Copy link
Contributor

liztio commented May 15, 2018

/lgtm

@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 comment for consistent failures.

1 similar comment
@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 comment for consistent failures.

@timothysc
Copy link
Member

poke when this is all ready to go.

@luxas
Copy link
Member Author

luxas commented May 15, 2018

This is ready to go, if you only review 9a26430
The first commit is #63799 and the others autogenerated stuff

@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 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 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 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 comment for consistent failures.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2018
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 16, 2018
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2018
@luxas
Copy link
Member Author

luxas commented May 16, 2018

Readding the LGTM label, I just rebased upon #63799.
Readding approved label as well as I had to add the API package to hack/.golint_failures. Unnecessary to ask for approval from hack/OWNERS for that.

@luxas luxas added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 16, 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: liztio, luxas

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-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 254664d into kubernetes:master May 16, 2018
k8s-github-robot pushed a commit that referenced this pull request May 16, 2018
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

 kubeadm: Remove the `.CloudProvider` and `.PrivilegedPods` configuration option

**What this PR does / why we need it**:
Removes the `.CloudProvider` option, it has been experimental for a long time. People should now use external cloud providers, which is beta in v1.11. Most importantly, you can get the exact same behavior in the API by utilizing the `.*ExtraArgs` and `.*ExtraVolumes` fields.
Removes `.PrivilegedPods` as that serves a super small edge case with the legacy cloud provider, and only for openstack.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Part of kubernetes/community#2131

**Special notes for your reviewer**:
Depends on PRs:
 - [x] #63799
 - [x] #63788

**Release note**:

```release-note
[action required] In the new v1alpha2 kubeadm Configuration API, the `.CloudProvider` and `.PrivilegedPods` fields don't exist anymore.
Instead, you should use the out-of-tree cloud provider implementations which are beta in v1.11.
If you have to use the legacy in-tree cloud providers, you can rearrange your config like the example below.
If you need to use the `.PrivilegedPods` functionality, you can still edit the manifests in
`/etc/kubernetes/manifests/`, and set `.SecurityContext.Privileged=true` for the apiserver
and controller manager.
---
kind: MasterConfiguration
apiVersion: kubeadm.k8s.io/v1alpha2
apiServerExtraArgs:
  cloud-provider: "{cloud}"
  cloud-config: "{path}"
apiServerExtraVolumes:
- name: cloud
  hostPath: "{path}"
  mountPath: "{path}"
controllerManagerExtraArgs:
  cloud-provider: "{cloud}"
  cloud-config: "{path}"
controllerManagerExtraVolumes:
- name: cloud
  hostPath: "{path}"
  mountPath: "{path}"
---
```
@kubernetes/sig-cluster-lifecycle-pr-reviews @dims @liztio
k8s-github-robot pushed a commit that referenced this pull request May 17, 2018
Automatic merge from submit-queue (batch tested with PRs 63871, 63927, 63966, 63957, 63844). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm: Remove the never-used .Etcd.SelfHosted field

**What this PR does / why we need it**:
These API types were added to support the self-hosting etcd feature, which in the end never was merged. Hence these API types are unused and should be removed. Perfect timing to do that is now in our new `v1alpha2` scheme.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Part of kubernetes/community#2131

**Special notes for your reviewer**:
Depends on PRs:
 - [x] #63799
 - [x] #63788

**Release note**:

```release-note
kubeadm has removed `.Etcd.SelfHosting` from its configuration API. It was never used in practice.
```
@kubernetes/sig-cluster-lifecycle-pr-reviews @liztio
k8s-github-robot pushed a commit that referenced this pull request May 20, 2018
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Add roundtrip, defaulting, upgrading and validation unit tests for the kubeadm API types

**What this PR does / why we need it**:
Follows up from #63799, as well as net-new unit testing for our serialization/deserialization package. This tests our API machinery pretty much end to end.

This is more important now given we now support two external types: #63788

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Part of kubernetes/community#2131

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```
@kubernetes/sig-cluster-lifecycle-pr-reviews @liztio
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. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants