use existing network and subnet in OpenStack #7699
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
zetaab
changed the title
zetaab
force-pushed
the
existnetandsubnet
branch
2 times, most recently
from
b69c9f8
to
4f8b605
Compare
|
@mitch000001 I combined both network and subnet PRs and rebased against master. Could you review this PR |
|
/test pull-kops-e2e-kubernetes-aws |
|
W0928 10:39:16.189] error running tasks: deadline exceeded executing task VPC/e2e-a3de3751a9-ff1eb.test-cncf-aws.k8s.io. Example error: error creating VPC: VpcLimitExceeded: The maximum number of VPCs has been reached. aws test will fail to error like that |
|
/test pull-kops-e2e-kubernetes-aws |
| return err | ||
| if len(c.Zones) > 0 && len(c.UtilitySubnetIDs) > 0 { | ||
| if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderAWS { | ||
| zoneToSubnetProviderID, err = getZoneToSubnetProviderID(cluster.Spec.NetworkID, c.Zones[0][:len(c.Zones[0])-1], c.UtilitySubnetIDs) |
There was a problem hiding this comment.
Nit: we should probably put the word aws in getZoneToSubnetProviderIDandOpenstack in getSubnetProviderID
| } | ||
|
|
||
| for index, subnet := range subnets { | ||
| zone := c.zones[int(index)%len(c.zones)] |
There was a problem hiding this comment.
This seems fragile - should c.zones be a map?
There was a problem hiding this comment.
this is similar logic that we have in https://github.com/kubernetes/kops/blob/master/pkg/model/openstackmodel/servergroup.go#L99 or https://github.com/kubernetes/kops/blob/master/pkg/model/openstackmodel/servergroup.go#L109
the problem is that in openstack the subnet does not have any relation to zones
There was a problem hiding this comment.
I kind of like the idea mapping the zones and the subnets. Possibly according to Cluster.Spec.Subnets as there are already zones in it.
Another option is to annotate the zones onto the subnets (also via tags?).
|
A few nits, but otherwise LGTM You have a conflict though :-( Does this need to make 1.14? |
zetaab
force-pushed
the
existnetandsubnet
branch
from
4f8b605
to
b69007c
Compare
|
I think we should not put this to 1.14 or even 1.15. It is quite big change and I would like to see this going through alpha and beta stages before stable. Also this is going to have many conflicts if we cherry-pick this to 1.15. So I would say kops 1.16 alpha 1 is first release which contains this PR. |
zetaab
force-pushed
the
existnetandsubnet
branch
from
b03fd16
to
19d6c68
Compare
zetaab
force-pushed
the
existnetandsubnet
branch
from
19d6c68
to
c1f86f8
Compare
|
/test pull-kops-verify-bazel |
|
@justinsb friendly ping :) I have solved now quite many conflicts |
/test pull-kops-verify-generated |
|
@justinsb could you check this. I would like to see this in 1.16 alpha 1 |
| } | ||
|
|
||
| if needRouter { |
There was a problem hiding this comment.
And we could even check if there is a router already. But the main question is if the router is connected to the internet, because that is normally why we want to have it in the first place.
| var err error | ||
| for _, sp := range b.Cluster.Spec.Subnets { | ||
| if sp.Type == kops.SubnetTypePrivate { | ||
| lbSubnetName, err = b.findSubnetNameByID(sp.ProviderID, sp.Name) |
There was a problem hiding this comment.
So, previously we used one subnet of the masters, but now we just use one subnet independent if there are masters in there or not?
There was a problem hiding this comment.
yes, if you have better ideas how to do it. Feel free to say
| } | ||
|
|
||
| for index, subnet := range subnets { | ||
| zone := c.zones[int(index)%len(c.zones)] |
There was a problem hiding this comment.
I kind of like the idea mapping the zones and the subnets. Possibly according to Cluster.Spec.Subnets as there are already zones in it.
Another option is to annotate the zones onto the subnets (also via tags?).
|
@mikesplain @justinsb ping, could you check this |
|
I think this looks good - let's get it into 1.16.0-alpha.1 :-) /approve |
k8s-ci-robot
added
lgtm
|
I don't think I can resolve the merge conflict though :-( - do you mind rebasing @zetaab ? |
zetaab
force-pushed
the
existnetandsubnet
branch
from
c1f86f8
to
f65b0f3
Compare
k8s-ci-robot
removed
the
lgtm
|
Thanks @zetaab /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, 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 |
It is now possible to use existing OpenStack project network by specifying --os-network when creating cluster. More info in documentation section
There is experimental feature to use existing network and subnets in OpenStack project. When you create new cluster you can specify flag --subnets and it will then use existing subnet. There is similar flag for utility subnets --utility-subnets .