config: add Network type #141

squeed · 2018-11-27T20:28:45Z

Adds the basic network configuration that downstream components may wish to configure.

squeed · 2018-11-27T20:28:54Z

danwinship

Comment on NetworkType needs to be addressed. Other stuff is at your discretion...

danwinship · 2018-11-27T22:17:42Z

config/v1/types_network.go

-	// vxlanPort
-	// 	ClusterNetworks    []ClusterNetworkEntry `json:"clusterNetworks"`
+	// IP address pool to use for pod IPs.
+	ClusterNetworks []ClusterNetwork `json:"clusterNetworks"`


So, totally nitpicking since this may be the last chance to change these...

Technically there is only a single "cluster network", even if it's spread across multiple CIDR blocks. So maybe this should be singular? ("ClusterNetwork") (And the struct type should not be called "ClusterNetwork" since it's only a subset of the cluster network.)

That's a very good point; I was just duplicating what was done by openshift-sdn (in the interest of not rocking the boat too much). But you're right, this is a good chance to make this cleaner.

K8s is not the only one guilty of conflating the word "network" with "contiguous block of addresses."

Also, I know upstream calls it the ClusterCIDR, which makes me wonder if we should standardize on that. I don't like using the suffix "CIDR" to indicate an address block either.

Enough rambling. How about ClusterNetwork []ClusterNetworkBlock

danwinship · 2018-11-27T22:25:14Z

config/v1/types_network.go

+
+	// IP address pool for services.
+	// Currently, we only support a single entry here.
+	ServiceNetworks []string `json:"serviceNetworks"`


Likewise maybe single even though it's array-valued

danwinship · 2018-11-27T22:26:17Z

config/v1/types_network.go

+
+	// NetworkType is the plugin that is to be deployed (e.g. OpenShiftSDN).
+	// N.B. changing this is currently not supported.
+	NetworkType string `json:"networkType"`


You need to explain what the set of possible values is or at least indicate where the canonical list is

changing this is currently not supported.

nit: specify that the change is not supported after installation.

Why not just making this a type and create constants matching the possible values. This will limit the scope of errors. We this in several places already.

danwinship · 2018-11-27T22:30:18Z

config/v1/types_network.go

+
+	// The size of subnet (in bits) to allocate to each node for pod allocations.
+	// For example, 8 indicates a /24 will be given to each node.
+	HostSubnetLength uint32 `json:"hostSubnetLength"`


I have always hated HostSubnetLength's gratuitous backwardness, and managed to fix it when we added multiple CIDR support to ovn-kubernetes, where you now actually say "24" when you want a /24, rather than saying "8" to mean you want a /24. We should do that here too... and then we could perhaps even steal the "double CIDR" syntax that we use in the ovn-kubernetes config, and just specify each cluster network subset as a string, eg, "10.128.0.0/14/23" rather than needing a struct type? (If we keep the struct, then we need to rename HostSubnetLength too, to make it clear that it's not representing the same value as network.openshift.io.ClusterNetwork.HostSubnetLength. Maybe HostSubnetPrefix. Or NodePrefix for that matter.)

NodePrefix seems good to me. I don't like the double-slash.

abhinavdahiya · 2018-11-27T22:42:40Z

config/v1/types_network.go

@@ -13,20 +13,50 @@ type Network struct {
 	// Standard object's metadata.
 	metav1.ObjectMeta `json:"metadata,omitempty"`

-	// spec holds user settable values for configuration
+	// Spec holds user settable values for configuration.


@deads2k mentioned in #139 (comment)
spec to match the json tag

abhinavdahiya · 2018-11-27T22:46:33Z

config/v1/types_network.go

 type NetworkStatus struct {
+	// IP address pool to use for pod IPs.


Not sure when ObservedGeneration is required. cc @deads2k

I don't think we require OG in config.

Not sure when ObservedGeneration is required. cc @deads2k

We need it when we're reconciling the workload resources to a particular level. I don't anticipate seeing it in these types.

abhinavdahiya · 2018-11-27T22:47:59Z

config/v1/types_network.go

 	Spec NetworkSpec `json:"spec"`
-	// status holds observed values from the cluster. They may not be overridden.
+	// Status holds observed values from the cluster. They may not be overridden.


squeed · 2018-12-03T15:30:08Z

Updated, PTAL, all.

danwinship · 2018-12-04T18:59:03Z

config/v1/types_network.go

+	// The complete block for pod IPs.
+	CIDR string `json:"cidr"`
+
+	// The size (prefix) of block to allocate to each node.


Maybe explicitly note that this is the inverse of the 3.x "HostSubnetLength"

danwinship · 2018-12-04T18:59:28Z

lgtm

squeed · 2018-12-06T10:54:15Z

Here's another open question: Should we put the observed MTU in the NetworkStatus? Should administrators be able to configure the MTU in the NetworkSpec?

danwinship · 2018-12-06T14:23:35Z

We believe that we will be able to automatically guess the correct cluster-wide MTU for the vast majority of installations, so most users won't need to worry about it, so I'd say it doesn't need to be in the cluster NetworkSpec. (If it turns out that our powers of MTU prognostication aren't as good as we think, then we can add it later.)

OTOH, putting the observed MTU in the NetworkStatus makes it easier for admins to see when we guessed it wrong, so I think it does make sense to put it there.

squeed · 2018-12-12T14:16:03Z

OK, updated to include MTU. This is ready for final approval / merge.

config/v1/types_network.go

danwinship · 2018-12-18T20:38:42Z

config/v1/types_network.go

+	NetworkType string `json:"networkType"`
+
+	// ClusterNetworkMTU is the MTU for inter-pod networking.
+	ClusterNetworkMTU int `json:"podNetworkMTU"`


need to update the json name to match the field name

soltysh

Dan's comment is still outstanding also see my proposal about type for NetworkType

soltysh · 2018-12-19T14:36:29Z

config/v1/types_network.go

+
+	// NetworkType is the plugin that is to be deployed (e.g. OpenShiftSDN).
+	// N.B. changing this is currently not supported.
+	NetworkType string `json:"networkType"`


Why not just making this a type and create constants matching the possible values. This will limit the scope of errors. We this in several places already.

squeed · 2018-12-19T14:48:54Z

@soltysh because we need to have some way of handling third-party network providers. It seems useful for the administrator to be able to specify, say, Calico, rather than Other. The effect on the network operator will be the same -- it will not install a network plugin -- but the true configuration is still discoverable.

soltysh · 2018-12-19T14:51:46Z

@squeed gotcha, that's fair. So only that nit from Dan is outstanding here.

Adds the basic network configuration that downstream components may wish to configure.

squeed · 2018-12-19T14:55:46Z

And, updated.

soltysh

/lgtm
/approve

openshift-ci-robot · 2018-12-19T14:58:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, squeed

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:

~~OWNERS~~ [soltysh]

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

openshift-ci-robot requested review from abhinavdahiya and adambkaplan November 27, 2018 20:28

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

danwinship suggested changes Nov 27, 2018

View reviewed changes

abhinavdahiya reviewed Nov 27, 2018

View reviewed changes

squeed force-pushed the network-status branch from 48e3c78 to 630636c Compare December 3, 2018 15:29

squeed mentioned this pull request Dec 4, 2018

Convert cluster.openshift.io/Network in to the NetworkConfig CRD; update Network status openshift/cluster-network-operator#47

Merged

danwinship reviewed Dec 4, 2018

View reviewed changes

squeed force-pushed the network-status branch from 630636c to 474bf73 Compare December 12, 2018 14:15

danwinship reviewed Dec 13, 2018

View reviewed changes

config/v1/types_network.go Outdated Show resolved Hide resolved

danwinship mentioned this pull request Dec 18, 2018

readme: lots more documentation openshift/cluster-network-operator#50

Merged

squeed force-pushed the network-status branch from 474bf73 to 49b414d Compare December 18, 2018 19:21

danwinship reviewed Dec 18, 2018

View reviewed changes

soltysh suggested changes Dec 19, 2018

View reviewed changes

config: add Network type

f578b4a

Adds the basic network configuration that downstream components may wish to configure.

squeed force-pushed the network-status branch from 49b414d to f578b4a Compare December 19, 2018 14:55

soltysh approved these changes Dec 19, 2018

View reviewed changes

openshift-ci-robot assigned soltysh Dec 19, 2018

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

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

openshift-merge-robot merged commit cfbaa98 into openshift:master Dec 19, 2018

squeed deleted the network-status branch May 29, 2019 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: add Network type #141

config: add Network type #141

squeed commented Nov 27, 2018

squeed commented Nov 27, 2018

danwinship left a comment

danwinship Nov 27, 2018

squeed Nov 28, 2018

danwinship Nov 27, 2018

danwinship Nov 27, 2018

abhinavdahiya Nov 27, 2018

soltysh Dec 19, 2018

danwinship Nov 27, 2018

squeed Nov 28, 2018

abhinavdahiya Nov 27, 2018

abhinavdahiya Nov 27, 2018

soltysh Nov 30, 2018

deads2k Dec 3, 2018

abhinavdahiya Nov 27, 2018

squeed commented Dec 3, 2018

danwinship Dec 4, 2018

danwinship commented Dec 4, 2018

squeed commented Dec 6, 2018

danwinship commented Dec 6, 2018

squeed commented Dec 12, 2018

danwinship Dec 18, 2018

soltysh left a comment

soltysh Dec 19, 2018

squeed commented Dec 19, 2018

soltysh commented Dec 19, 2018

squeed commented Dec 19, 2018

soltysh left a comment

openshift-ci-robot commented Dec 19, 2018

		type NetworkStatus struct {
		// IP address pool to use for pod IPs.

config: add Network type #141

config: add Network type #141

Conversation

squeed commented Nov 27, 2018

squeed commented Nov 27, 2018

danwinship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeed commented Dec 3, 2018

Choose a reason for hiding this comment

danwinship commented Dec 4, 2018

squeed commented Dec 6, 2018

danwinship commented Dec 6, 2018

squeed commented Dec 12, 2018

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeed commented Dec 19, 2018

soltysh commented Dec 19, 2018

squeed commented Dec 19, 2018

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 19, 2018