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 OpenStack external CCM setup. #615

Closed

Conversation

ainmosni
Copy link
Contributor

@ainmosni ainmosni commented Aug 4, 2019

What this PR does / why we need it:
Adds support for the external OpenStack cloud controll manager.

As the internal OpenStack code in the controller manager
will be gone soon, we'll need to switch to the external cloud controller
manager, this PR adds the configuration for it.

* Adds support for the external OpenStack Cloud Control Manager.

As the internal OpenStack code in the controller manager
will be gone soon, we'll need to switch to the external cloud controller
manager, this commit adds the configuration for it.

Signed-off-by: Daniël Franke <daniel@ams-sec.org>
@kubermatic-bot kubermatic-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 4, 2019
@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign xmudrii
You can assign the PR to them by writing /assign @xmudrii in a comment when ready.

The full list of commands accepted by this bot can be found 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

@kubermatic-bot
Copy link
Contributor

Hi @ainmosni. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubermatic-bot kubermatic-bot added dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2019
I forgot to add the NodeSelector and the SecurityContext, this commit
adds them.

Signed-off-by: Daniël Franke <daniel@ams-sec.org>
@xmudrii
Copy link
Member

xmudrii commented Aug 5, 2019

Hello @ainmosni,

thank you for the PR! However, before we can merge this, there are several things we need to take care of and decide about. The switch to the external CCMs is a little bit tough topic because there are some points that are still unclear.

You can find some of the discussion in #616

@ainmosni
Copy link
Contributor Author

ainmosni commented Aug 5, 2019

@xmudrii Yeah the migration will be painful but as there's no real development on the in-tree one and the in-tree code will be removed sooner rather later, it might be worth to pull the bandaid off sooner rather than later. I can definitely take a look at the OpenStack side to help with the migration options.

@kron4eg
Copy link
Member

kron4eg commented Aug 5, 2019

@xmudrii Yeah the migration will be painful but as there's no real development on the in-tree one and the in-tree code will be removed sooner rather later

AFAIK, timeline is set to 1.17.x

, it might be worth to pull the bandaid off sooner rather than later.

Indeed, that's why we create #616.

I can definitely take a look at the OpenStack side to help with the migration options.

There is a problem with openstack CCM. Its releases are bound to specific minor kubernetes versions. So it's not that easy to jump on it and we need to take into account older version (since we support k8s starting from 1.13).

@ainmosni
Copy link
Contributor Author

ainmosni commented Aug 5, 2019

Yeah I kept that in mind in this patch, AFAICS they only started to tag their images since 1.15, they used latest before that.

One thing that I didn't put in this patch but we should probably add is support for the cinder CSI plugin, so that PVCs keep working.

@kron4eg
Copy link
Member

kron4eg commented Aug 5, 2019

OK, I've got a confirmation that it's safe to use OS CCM 1.15 in 1.13, 1.14 and 1.15 clusters (but no 1.12, but since we don't support 1.12 anyway, we are safe).

@ainmosni
Copy link
Contributor Author

ainmosni commented Aug 5, 2019

@kron4eg Yeah that's what I gathered from their examples as well. It's a bit more complicated with cinder-csi (which isn't in this PR) as the CSI sidecar containers are k8s version specific. I was thinking about adding those, but changed my mind to keep the PR smaller.

Copy link
Member

@kron4eg kron4eg left a comment

Choose a reason for hiding this comment

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

It seems this PR lack the handling of in-tree VS out-of-tree cloud providers.

I mean, we have

func (p CloudProviderSpec) CloudProviderInTree() bool {
which currently hardcodes who is considered as in-tree.

So we need a way (which we currently don't have) to handle situation when in-tree cloud-provider is requested (in config) but also external: true. In such situation we believe we should resort to provider CCM (if available).

But all of this definitely is out of scope of this PR because it required refactoring.

pkg/templates/externalccm/openstack.go Show resolved Hide resolved
Signed-off-by: Daniël Franke <daniel@ams-sec.org>
Signed-off-by: Daniël Franke <daniel@ams-sec.org>
@kubermatic-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@kubermatic-bot kubermatic-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 3, 2019
@kron4eg
Copy link
Member

kron4eg commented Feb 25, 2020

/remove-lifecycle stale

@kubermatic-bot kubermatic-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 25, 2020
@ivomarino
Copy link

Hi all, is this PR going to happen "soon" or what's the best actual solution on this topic? Thanks

@kron4eg
Copy link
Member

kron4eg commented Mar 4, 2020

@ivomarino yes, in the nearest future, I'd say. I'm working on integrating changes from this PR.

@kron4eg
Copy link
Member

kron4eg commented Mar 10, 2020

This PR is replaced by #820

@ainmosni thanks for initiating this!

@kron4eg kron4eg closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

5 participants