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

Add apimachinery changes to enable cloud controller manager #3408

Merged
merged 1 commit into from
Sep 22, 2017

Conversation

wlan0
Copy link
Member

@wlan0 wlan0 commented Sep 17, 2017

This is part I of multi part Pull Request to enable the
cloud-controller-manager through kops. This specific PR introduces the
cloud-controller-manager api, and puts it behind a feature flag.

Please feel free to merge this.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 17, 2017
@wlan0
Copy link
Member Author

wlan0 commented Sep 17, 2017

/assign chrislovecnm
/assign justinsb

@wlan0
Copy link
Member Author

wlan0 commented Sep 17, 2017

@chrislovecnm Just as we discussed this is the first PR in a series of PR to enable cloud controller manager support in kops. This adds the apimachinery changes required for this change.

We decided to do apimachinery changes first because then it gives people enough time to review this, and make changes if needed. This is also feature gated, and if enabled, it no-ops. There is no harm in merging this.

@chrislovecnm
Copy link
Contributor

Ci is not happy

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Questions for you

KubeDNS *KubeDNSConfig `json:"kubeDNS,omitempty"`
KubeAPIServer *KubeAPIServerConfig `json:"kubeAPIServer,omitempty"`
KubeControllerManager *KubeControllerManagerConfig `json:"kubeControllerManager,omitempty"`
CloudControllerManager *CloudControllerManagerConfig `json:"cloudControllerManager,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to nit pick. ExternalCloudControllerManager?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@@ -103,6 +103,9 @@ type CreateClusterOptions struct {
// Egress configuration - FOR TESTING ONLY
Egress string

Copy link
Contributor

Choose a reason for hiding this comment

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

You won't have any changes in this file at all, just remove them

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it felt like featureflag + cli was overkill. I'll remove it

@@ -369,6 +370,10 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e
}
cluster.Spec.Channel = c.Channel

if featureflag.EnableExternalCloudController.Enabled() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is needed or not, but I added it anyways since this is the only way to enable CCM now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't even need it here. This is simply to get the API in unwired.

KubeDNS *KubeDNSConfig `json:"kubeDNS,omitempty"`
KubeAPIServer *KubeAPIServerConfig `json:"kubeAPIServer,omitempty"`
KubeControllerManager *KubeControllerManagerConfig `json:"kubeControllerManager,omitempty"`
ExternalCloudControllerManager *CloudControllerManagerConfig `json:"cloudControllerManager,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to ExternalCloudControllerManager

This is part I of multi part Pull Request to enable the
cloud-controller-manager through kops. This specific PR introduces the
cloud-controller-manager api, and puts it behind a feature flag.

Please feel free to merge this.
@wlan0
Copy link
Member Author

wlan0 commented Sep 21, 2017

@chrislovecnm Updated the PR

@wlan0
Copy link
Member Author

wlan0 commented Sep 21, 2017

The logs make me think that the Jenkins failure is a flake.

@chrislovecnm
Copy link
Contributor

/test pull-kops-e2e-kubernetes-aws

@chrislovecnm
Copy link
Contributor

/lgtm

@wlan0 we need release notes that this is just getting put in place if we cut a release before these are wired in.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2017
@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. .

@k8s-github-robot k8s-github-robot merged commit 784da37 into kubernetes:master Sep 22, 2017
k8s-github-robot pushed a commit that referenced this pull request Oct 21, 2017
Automatic merge from submit-queue.

Add Cloud Controller Manager addon

This adds the CCM addon for the Kubernetes cluster. 

This is a follow-up PR to #3408. 

cc @chrislovecnm @andrewsykim
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. 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. needs-changes size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants