Openstack fixes #9554
Openstack fixes #9554
Conversation
Openstack can use floating IPs as master API address. Setting these defauls and using floating ips ends up in a nil pointer error somewhere in the lbaasv2 code
Needed somewhere anyway. Failing to create this one errors with missing task
Originally, floating ips depended on instances, but this causes a dependency cycle now that bootstrap scripts require all IPs for the API cert. This also requires using networking API for creating floating ips instead of compute so that we can name (and later tag) the floating IPs, which is necessary to know which floating IP belongs to which instance prior to association
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
I do want test this before it is merged to master. In my opinion it is quite big change and can break things
So after this PR we should still have working clusters with loadbalancer (octavia) and without loadbalancer (3 masters should have floatingips attached to port and then hopefully port is attached to instance)
I try to test this when I have time (I am currently on vacation still)
/hold
| } | ||
| default: | ||
| if !b.UsesSSHBastion() { | ||
| if b.Cluster.Spec.Topology.Nodes == "public" { |
There was a problem hiding this comment.
is this change backwards compatible to old clusters?
There was a problem hiding this comment.
The reason I added this was that I got floating IPs on all nodes otherwise. Even with private topology.
There is a lot of validation missing for configuring an openstack cluster, where e.g private topology on masters + floating ips on masters should be conflicting. In addition there is ig.spec.associatePublicIP that comes into play too, where floating IPs should be added regardless.
Happy to amend this any other way.
k8s-ci-robot
added
the
do-not-merge/hold
|
Thanks for the early feedback. Yeah, I am hoping to get a cluster with Octavia to test this as well. Multi-master clusters were broken until just now, so didn't have chance to test that, but single master works just fine. |
|
If the instance can figure out its fixed IP, then nodeup can add that IP into the cert. |
|
fixed ip is available in OpenStack metadata API in each Instance (and that also should match to interface ip address in that instance) |
|
That should work. I think that is a largely independent change from this one, so I can see if I can do a parallel PR on that. |
| @@ -45,6 +46,7 @@ type Instance struct { | |||
| Metadata map[string]string | |||
| AvailabilityZone *string | |||
| SecurityGroups []string | |||
| FloatingIP *FloatingIP | |||
There was a problem hiding this comment.
Find() needs to fill in this field.
There was a problem hiding this comment.
I agree that at some point Find should fill in this. But right now, there is a CheckChanges that only returns true when the instance doesn't exist. So an instance will basically never be updated once created. That is, if I understand fi.task correctly.
One can easily run into an issue where floating IP and instance is created, but association fails for some reason, and it would be nice to have kops detect and amend that. But that's the case for a number of things here. Had a plan to clean that part up later as well.
|
Looks okay, but I don't know OpenStack. |
|
I will test this when I get internet to my apartment (next week) |
|
I cannot get this PR working https://gist.github.com/zetaab/e0990214a939830a36c16c022fcb600b |
|
@olemarkus thanks a lot! This will fix all other cases than LB use-case. Anyway lets make separate PR from that. /approve |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus, 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
The refactoring done lately have created cyclic dependencies in cloudup tasks when provisioning clusters that use floating or fixed ips for accessing the master API.
This PR breaks this cycle by provisioning floating IPs before the instances.
Using fixed IPs for the API is probably not possible to fix since they are created together with the instance, so one will always have the bootstrapscript -> instance -> bootstrapscript cycle. I think that should be removed in a later PR.
/cc @zetaab