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

External OpenStack Cloud Controller Manager implementation #5491

Merged
merged 4 commits into from
Feb 18, 2020
Merged

External OpenStack Cloud Controller Manager implementation #5491

merged 4 commits into from
Feb 18, 2020

Conversation

alijahnas
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR is the implementation of the external OpenStack Cloud Controller Manager.
As the in-tree k8s cloud controllers are deprecated, the external cloud controllers should now be used. This cloud controller allows to use Octavia as a backend for service load balancing.

  • Tested on OpenStack Rocky

Special notes for your reviewer:
In order to keep both working in a first phase, I added new OpenStack conf variables which are the same as the old ones but prefixed with external_

Does this PR introduce a user-facing change?:
Users should use the new external OpenStack cloud controller rather than the old in-tree one.

@k8s-ci-robot k8s-ci-robot added 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. labels Dec 23, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 23, 2019
@alijahnas alijahnas mentioned this pull request Dec 23, 2019
Merged
@holmsten
Copy link
Contributor

@alijahnas can you rebase this one as well, to get CI tests to pass?

@alijahnas
Copy link
Contributor Author

@holmsten Rebase done. Thanks 👍

@StevenReitsma
Copy link
Contributor

According to the first step in https://github.com/kubernetes/cloud-provider-openstack/blob/master/docs/using-controller-manager-with-kubeadm.md#steps it's necessary to mount the cloud-config in the default Kubernetes controller manager as well. I don't see this in your code, is it strictly required?

StevenReitsma
StevenReitsma suggested changes Jan 20, 2020
View reviewed changes
Copy link
Contributor

@StevenReitsma StevenReitsma left a comment

Choose a reason for hiding this comment

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

I've been working on something similar this week so I thought I'd review your PR! Thanks a bunch for working on this.

inventory/sample/group_vars/all/all.yml Outdated Show resolved Hide resolved
roles/kubernetes-apps/cloud_controller/openstack/defaults/main.yml Outdated Show resolved Hide resolved
...s/cloud_controller/openstack/templates/external-openstack-cloud-controller-manager-ds.yml.j2 Outdated Show resolved Hide resolved
...troller/openstack/templates/external-openstack-cloud-controller-manager-role-bindings.yml.j2 Outdated Show resolved Hide resolved
roles/kubernetes-apps/meta/main.yml Outdated Show resolved Hide resolved
roles/kubespray-defaults/defaults/main.yaml Show resolved Hide resolved
@StevenReitsma
Copy link
Contributor

@alijahnas I tested the latest update of this PR and it works perfectly for my setup! Amazing work. Could you also check out my PR for a couple of minor Cinder fixes? #5561

@alijahnas
Copy link
Contributor Author

@alijahnas I tested the latest update of this PR and it works perfectly for my setup! Amazing work. Could you also check out my PR for a couple of minor Cinder fixes? #5561

Thanks for testing! Sure I will check your PR 👍

@holmsten
Copy link
Contributor

How does an upgrade from the internal controller to the external one work? Is this supported in existing clusters?

@StevenReitsma
Copy link
Contributor

Reading through https://github.com/kubernetes/cloud-provider-openstack/blob/master/docs/migrate-to-ccm-with-csimigration.md it should be fine to switch from the in-tree CCM to the external CCM if you weren't using any of the Cinder functionality. However, switching from in-tree Cinder to the external CSI plugin will be more problematic and requires migration.

The thing is that the CCM and CSI plugins are now decoupled, which makes it hard to guess what kind of migration the user wants to do on their cluster. For example, how should the migration look if a user has the in-tree CCM enabled and only wants to deploy the external CCM but keep the volume functionality of the in-tree CCM? It's hard to cover all use cases so maybe we should decide which migration paths to support in kubespray, if any.

@alijahnas
Copy link
Contributor Author

How does an upgrade from the internal controller to the external one work? Is this supported in existing clusters?

I tried to ask in the provider-openstack slack but didn't get a proper reply. I can't find any migration paths besides the ones for the CSI driver. I am not sure if the loadbalancers created by the internal CCM are managed by the external CCM if they run side by side for the transition.

@StevenReitsma
Copy link
Contributor

@holmsten What do you suggest we do about migration? It think it would be nice to get this merged regardless.

@alijahnas
Copy link
Contributor Author

alijahnas commented Feb 11, 2020 •
edited
Loading

@holmsten What do you suggest we do about migration? It think it would be nice to get this merged regardless.

I think this could be a nice start for transitioning to the external cloud providers.

@huxcrux
Copy link

huxcrux commented Feb 15, 2020 •
edited
Loading

This looks good and since the PR doesn't remove support for the in-tree provider I completely agrees that it would be great to have this PR merged and available in the next release

@holmsten
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2020
@StevenReitsma
Copy link
Contributor

/assign @woopstar

@Miouge1
Copy link
Contributor

Miouge1 commented Feb 18, 2020

Great to see! Thank you @alijahnas !

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alijahnas, Miouge1

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 Feb 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 646fd5f into kubernetes-sigs:master Feb 18, 2020
@alijahnas alijahnas mentioned this pull request Feb 18, 2020
Closed
6 tasks
@alijahnas alijahnas deleted the openstack_controller branch February 18, 2020 15:53
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Mar 7, 2020
@alijahnas @LuckySB
External OpenStack Cloud Controller Manager implementation (kubernete…
978f29a 
…s-sigs#5491)

* External OpenStack Cloud Controller Manager implementation

* Adding controller image tag

* Minor fixes

* Restructuring the external cloud controller to work with KubeADM
@Miouge1 Miouge1 mentioned this pull request Mar 11, 2020
Merged
@pierreyves-lebrun pierreyves-lebrun mentioned this pull request Mar 31, 2020
Closed
@Miouge1 Miouge1 mentioned this pull request Apr 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StevenReitsma StevenReitsma StevenReitsma requested changes

@mikejoh mikejoh mikejoh left review comments

@Atoms Atoms Awaiting requested review from Atoms

@holmsten holmsten Awaiting requested review from holmsten

Assignees

@woopstar woopstar

@holmsten holmsten

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. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@alijahnas @holmsten @StevenReitsma @huxcrux @Miouge1 @k8s-ci-robot @mikejoh @woopstar