Remove requirement of having deprecated clusterNetworkCIDR/hostSubnetLength in master.networkConfig by JacobTanenbaum · Pull Request #18669 · openshift/origin

JacobTanenbaum · 2018-02-19T22:10:17Z

Currently in order for openshift to start the first entry in clusterNetworks and the old clusterNetworkCIDR/hostSubnetLength have to match. Change it
so the older clusterNetworkCIDR/hostSubnetLength fields are no longer required.

Bug 1534779

JacobTanenbaum · 2018-02-20T18:36:01Z

@pravisankar PTAL

pravisankar

Did you test if this works with 'oc cluster up'? (#16627)

pravisankar · 2018-02-20T22:52:42Z

pkg/cmd/server/apis/config/validation/master.go

-	}
-	if len(config.NetworkConfig.ClusterNetworks) > 0 && config.NetworkConfig.DeprecatedClusterNetworkCIDR != config.NetworkConfig.ClusterNetworks[0].CIDR {
-		validationResults.AddErrors(field.Invalid(fldPath.Child("clusterNetworkCIDR"), config.NetworkConfig.DeprecatedClusterNetworkCIDR, "cannot set clusterNetworkCIDR and clusterNetworks, please use clusterNetworks"))
+	fmt.Printf("KEYWORD: len(config.NetworkConfig.ClusterNetworks)=%d\n", len(config.NetworkConfig.ClusterNetworks))


Delete this debug printf

pravisankar · 2018-02-20T22:52:59Z

pkg/cmd/server/apis/config/validation/master.go

+		}
+
+	} else if len(config.NetworkConfig.ClusterNetworks) == 1 {
+		fmt.Printf("KEYWORD: I THINK YOU ARE HERE!\n")


delete this line

pravisankar · 2018-02-20T22:55:27Z

pkg/cmd/server/apis/config/validation/master.go

+	} else if len(config.NetworkConfig.ClusterNetworks) == 1 {
+		fmt.Printf("KEYWORD: I THINK YOU ARE HERE!\n")
+		if config.NetworkConfig.DeprecatedHostSubnetLength != config.NetworkConfig.ClusterNetworks[0].HostSubnetLength && config.NetworkConfig.DeprecatedHostSubnetLength != 0 {
+			fmt.Printf("\tKEYWORD: condif.NetworkConfig.DeprecatedHostsubnetLength: %d\n\tKEYWORD:config.NetworkConfig.ClusterNEtworks[0].HostSubnetLength:%d\n", config.NetworkConfig.DeprecatedHostSubnetLength, config.NetworkConfig.ClusterNetworks[0].HostSubnetLength)


smarterclayton · 2018-02-21T14:07:21Z

This is not backwards compatible - old config would be broken if you merged this. We don't drop support for old config without a new major api version.

JacobTanenbaum · 2018-02-21T14:22:38Z

@pravisankar Yes this works with 'oc cluster up'

pravisankar · 2018-02-21T18:52:36Z

/lgtm

imcsk8 · 2018-02-21T18:55:38Z

looks good

dcbw · 2018-02-21T19:11:43Z

@JacobTanenbaum what happens when there's old-style config?

pravisankar · 2018-02-21T22:56:29Z

On Wed, Feb 21, 2018 at 11:11 AM, Dan Williams ***@***.***> wrote: @JacobTanenbaum <https://github.com/jacobtanenbaum> what happens when there's old-style config?

As part of conversions, it will be converted to new-style/internal config

…

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#18669 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABM0hr27B8uTxJJjHEViopEy4-boVwHBks5tXGp1gaJpZM4SLKYa> .

pravisankar · 2018-02-22T22:22:42Z

Copying for posterity:

Jacob: I tested an old config file that just had the deprecated fields and it worked with this PR. I thought my changes to the conversion function changed the deprecated fields into the new struct internally. Unless my understanding is wrong old configs would only be wrong if a user tried to add the new clusterNetworks config struct while using the deprecated fields

Clayton: Ok, I misread the PR then. If an old config file with only the old fields specified still works, then you can relax the requirement that the old field be specified.

JacobTanenbaum · 2018-03-07T19:03:53Z

@knobunc @smarterclayton is there something I need to do to push this forward?

…Length in master.networkConfig Currently in order for openshift to start the first entry in clusterNetworks and the old clusterNetworkCIDR/hostSubnetLength have to match. Change it so the older clusterNetworkCIDR/hostSubnetLength fields are no longer required. Bug 1534779

JacobTanenbaum · 2018-03-08T16:14:32Z

/test extended_networking_minimal

pravisankar

/lgtm

dcbw · 2018-03-20T16:28:53Z

/approve

knobunc · 2018-03-26T13:09:06Z

/approve

openshift-ci-robot · 2018-03-26T13:09:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, JacobTanenbaum, knobunc, pravisankar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/cmd/OWNERS~~ [dcbw,knobunc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-merge-robot · 2018-03-26T13:31:20Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-bot · 2018-03-26T13:59:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2018-03-26T15:02:12Z

Automatic merge from submit-queue.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 19, 2018

JacobTanenbaum changed the title ~~Remove requirement of having deprecated clusterNetworkCIDR/hostSubnet…~~ Remove requirement of having deprecated clusterNetworkCIDR/hostSubnetLength in master.networkConfig Feb 19, 2018

openshift-ci-robot requested review from 0xmichalis and smarterclayton February 19, 2018 22:10

pravisankar added component/networking kind/bug Categorizes issue or PR as related to a bug. labels Feb 20, 2018

pravisankar reviewed Feb 20, 2018

View reviewed changes

JacobTanenbaum force-pushed the BZ1534779 branch from 8b286d4 to 603136a Compare February 21, 2018 14:00

pravisankar approved these changes Feb 21, 2018

View reviewed changes

openshift-ci-robot assigned pravisankar Feb 21, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2018

JacobTanenbaum added 3 commits March 8, 2018 09:43

modification of the serialization_test.go

01ed557

update to TestMasterConfig

e5b0d93

JacobTanenbaum force-pushed the BZ1534779 branch from 603136a to e5b0d93 Compare March 8, 2018 14:46

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2018

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2018

pravisankar approved these changes Mar 8, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2018

openshift-merge-robot merged commit 5fbfe32 into openshift:master Mar 26, 2018

Conversation

JacobTanenbaum commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JacobTanenbaum commented Feb 20, 2018

Uh oh!

pravisankar left a comment

Choose a reason for hiding this comment

Uh oh!

pravisankar Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

pravisankar Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

pravisankar Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Feb 21, 2018

Uh oh!

JacobTanenbaum commented Feb 21, 2018

Uh oh!

pravisankar commented Feb 21, 2018

Uh oh!

imcsk8 commented Feb 21, 2018

Uh oh!

dcbw commented Feb 21, 2018

Uh oh!

pravisankar commented Feb 21, 2018 via email

Uh oh!

pravisankar commented Feb 22, 2018

Uh oh!

JacobTanenbaum commented Mar 7, 2018

Uh oh!

JacobTanenbaum commented Mar 8, 2018

Uh oh!

pravisankar left a comment

Choose a reason for hiding this comment

Uh oh!

dcbw commented Mar 20, 2018

Uh oh!

knobunc commented Mar 26, 2018

Uh oh!

openshift-ci-robot commented Mar 26, 2018

Uh oh!

openshift-merge-robot commented Mar 26, 2018

Uh oh!

openshift-bot commented Mar 26, 2018

Uh oh!

openshift-merge-robot commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

JacobTanenbaum commented Feb 19, 2018 •

edited

Loading