External out-of-tree CloudControllerManager support for openstack #6444
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
| Version: fi.String(version), | ||
| Manifest: fi.String(location), | ||
| Selector: map[string]string{"k8s-addon": key}, | ||
| KubernetesVersion: ">=1.11.0", |
There was a problem hiding this comment.
I would imagine someone should just do
fmt.Sprintf(">=%s", version)
here to remove the duplicate setting of this version?
There was a problem hiding this comment.
This whole tree could be simplified to only set the key and the version if we did the string manipulation here, however I feel more comfortable just putting the old path into an else as well...
There was a problem hiding this comment.
Honestly I'm not sure if the older CCM works at all. I'd keep your branch explicit, and then we can look at the other side of the else statement separately!
|
this is already working, but this contains many commits from another PRs. Lets wait until those are solved, and then I can maybe rebase this (and fix reviewed things) |
|
/sig openstack |
k8s-ci-robot
added
the
area/provider/openstack
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
zetaab
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
It seems that now this works against master branch. So feel free to review it :) |
|
/assign @chrisz100 |
|
/assign @justinsb Justin: I was thinking is this correct way of doing these external cloudproviders? Soon we are going to see more of these from another clouds as well. Like from AWS and from GCP (in-tree cloudproviders are deprecated). At least this works for me in openstack |
upup/models/cloudup/resources/addons/openstack.addons.k8s.io/k8s-1.11.yaml.template
Outdated
Show resolved
Hide resolved
| Version: fi.String(version), | ||
| Manifest: fi.String(location), | ||
| Selector: map[string]string{"k8s-addon": key}, | ||
| KubernetesVersion: ">=1.11.0", |
There was a problem hiding this comment.
Honestly I'm not sure if the older CCM works at all. I'd keep your branch explicit, and then we can look at the other side of the else statement separately!
| image: "{{- .ExternalCloudControllerManager.Image }}" | ||
| args: | ||
| - /bin/openstack-cloud-controller-manager | ||
| - --v={{ .ExternalCloudControllerManager.LogLevel }} |
There was a problem hiding this comment.
Not sure if this means it's required to specify logLevel or if we default it somewhere. As it's feature-flagged though, it's fine not to default :-)
There was a problem hiding this comment.
we need specify loglevel, there is no default value
|
LGTM but I would really like to see this not using the unprivileged apiserver port - it looks like you don't need it AFAICT |
|
/hold some problems currently with this code |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
removed
the
do-not-merge/hold
|
great, found correct RBAC rules for this. Now it works without that master port |
|
@justinsb is this ok now? |
|
/assign @mikesplain |
|
@justinsb could you check this? |
k8s-ci-robot
added
the
lgtm
|
@justinsb ping |
|
As it’s feature flagged and I’d love to get openstack compability through... /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrisz100, zetaab 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 |
k8s-ci-robot
added
the
approved
|
/retest |
fixes #6439 for openstack
This adds possibility to use kops with external CCM. In case of openstack that provider contains more features than in tree one. Also in-tree one is deprecated https://github.com/kubernetes/kubernetes/tree/master/pkg/cloudprovider and codes is going to be removed in some point.