[Dev Tooling] generateSubnets should check if subnets are already taken #3706
Conversation
mrWinston
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
SrinivasAtmakuri,
mociarain,
AldoFusterTurpin,
kimorris27 and
tsatam
as code owners
There was a problem hiding this comment.
@mrWinston Thank you for this. Looks good. I just left some suggestions in case you find them useful. Thanks!
mrWinston
force-pushed
the
mrwinston/hotfix-dev-cluster-create-better-subnet-creation
branch
from
f0fdcfd to
858084a
Compare
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
|
|
||
| func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, workerSubnet string, err error) { | ||
| // pick a random 23 in range [10.3.0.0, 10.127.255.0], making sure it doesn't |
There was a problem hiding this comment.
idea: I wonder if this randomness is actually what shoots us in the foot. What if instead, we got very structured with this.
What if, given that both masters and workers are /24 nets, we adopted a CIDR convention rather than shooting in the dark x many times? I'm thinking something like the 10.3.0.0/ space is entirely dedicated to dev clusters, and we take that a slice at a time:
10.3.0.0/24
10.3.1.0/24
10.3.2.0/24
10.3.3.0/24
10.3.4.0/24
10.3.5.0/24
10.3.6.0/24
...
Now, to find the next "open" subnet, all you have to do is iterate over this CIDR convention, and see if it shows up in the VNet or not, and it supports a scale I think we can easily handle (255 subnets, or 127 clusters per region). Thoughts?
There was a problem hiding this comment.
It was discussed here as a potential follow-up improvement: #3706 (comment)
There was a problem hiding this comment.
Ideas aside, this worked for me on the first try locally, so huge +1 considering this is a critical blocker on local testing, thank you so much for this - I no longer need Tanmay's famous hack:
while ! go run ./hack/cluster create; do echo "failed, trying again"; sleep 10; done;
| @@ -345,22 +352,111 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str | |||
| return nil | |||
| } | |||
|
|
|||
| func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, workerSubnet string) { | |||
| // pick a random 23 in range [10.3.0.0, 10.127.255.0] | |||
| // ipRangesContainCIDR checks, weather any of the ipRanges overlap with the cidr string. In case cidr isn't valid, false is returned. | |||
There was a problem hiding this comment.
nit: weather should be whether
mociarain
deleted the
mrwinston/hotfix-dev-cluster-create-better-subnet-creation
branch
Which issue this PR addresses:
This PR fixes errors like:
when creating a cluster using
go run ./hack/cluster create.What this PR does / why we need it:
Instead of just randomly generating the subnet cidrs,
generateSubnetsnow first retrieves all subnets in thedev-vnetand makes sure that the subnet cidrs don't overlap with any of the preexisting ones.Is there any documentation that needs to be updated for this PR?
How do you know this will function as expected in production?