Allow expanding ClusterNetwork #6349
Conversation
| return fielderrors.NewFieldInvalid("Network", obj.Network, err.Error()) | ||
| } | ||
| newSize, _ := newNet.Mask.Size() | ||
| if newSize < oldSize && newNet.Contains(oldBase) { |
There was a problem hiding this comment.
I dislike 'size' since this was making me think "Why would we only allow it if the size of the network was smaller?"
Of course this is actually the number of leading 1s. Can we call it oldOnes and newOnes? So some future idiot me doesn't get confused? but LGTM even the way it is if you don't want to change it.
There was a problem hiding this comment.
size reads better to me. Maybe just a comment.
There was a problem hiding this comment.
repushed with
// oldSize/newSize is, eg the "16" in "10.1.0.0/16", so "newSize < oldSize" means
// the new network is larger
Add missing check that HostSubnetLength doesn't change. Allow ClusterNetwork to change as long as the new value is a strict superset of the old one.
danwinship
force-pushed
the
clusternetwork-validation
branch
from
7d91d51 to
c638c87
Compare
|
[merge] |
|
[Test]ing while waiting on the merge queue |
|
[test] |
1 similar comment
|
[test] |
|
LGTM, similarly we can also allow expanding service/portal network |
|
[test] |
|
Evaluated for origin test up to c638c87 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7933/) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4404/) (Image: devenv-rhel7_2987) |
|
I believe that #6149 is this flake. So |
|
Evaluated for origin merge up to c638c87 |
Merged by openshift-bot
We were disallowing all changes to ClusterNetwork, but in fact, everything should work just fine if you change it to a new value that fully contains the old network. Eg, given the 10.1.0.0/16 default, switching to 10.0.0.0/15 (ie, 10.0.0.0/16 + 10.1.0.0.0/16) is not a problem. You need to keep the same HostSubnetLength though (which we weren't checking before).
(Given this possibility, maybe we should change the default network to something like 10.128.0.0/16, so users can grow it later without overlapping 10.0.0.0/16 (since that's probably the /16 that's most likely to already be in use for something non-OpenShift-related.))
(Note that currently openshift-sdn has code that redundantly validates ClusterNetwork changes, so this patch alone isn't enough to expose the new functionality.)
@openshift/networking PTAL