network: move from operator-specific CRD to cluster config #1013

squeed · 2019-01-08T00:07:13Z

Add the Network.config.openshift.io CRD
Generate the network config from the install config
Change the install config schema to match the cluster config

squeed · 2019-01-08T00:07:37Z

/hold
This won't pass CI until openshift/cluster-network-operator#47 merges

squeed · 2019-01-08T00:07:55Z

@wking this is the replacement to that previous PR.

abhinavdahiya · 2019-01-08T00:44:10Z

pkg/types/installconfig.go

why is this not list of https://github.com/openshift/api/blob/master/config/v1/types_network.go#L58 ?

Up to you. The cleanest would probably just be to use configv1.NetworkSpec directly. It depends on what you would like the installer to look like. Keep in mind that NetworkSpec may slowly change over time (additive only, of course).

squeed · 2019-01-09T22:23:51Z

@abhinavdahiya @wking ping on this. I'd like to resolve the discussion of embedding vs. copying the Network.config.openshift.io datatype.

abhinavdahiya · 2019-01-09T22:30:28Z

@abhinavdahiya @wking ping on this. I'd like to resolve the discussion of embedding vs. copying the Network.config.openshift.io datatype.

I am in favor of keeping them separate and derive Network.config.openshift.io from install-config parts.

also I don't think we should expand clusternetwork to a list, that keeps the installation simple and defers to day-2 for customization.

squeed · 2019-01-14T17:36:19Z

@abhinavdahiya we have to support multiple cidrs from day-1 - there are existing use-cases for it, and we don't yet support increasing them.

I can update the PR to just copy the objects.

abhinavdahiya · 2019-01-14T17:48:21Z

@abhinavdahiya we have to support multiple cidrs from day-1 - there are existing use-cases for it, and we don't yet support increasing them.

I can update the PR to just copy the objects.

+1 on clusternetwork being a list and also a installer type that converts to config.openshift.io type.

abhinavdahiya · 2019-01-14T17:51:41Z

with feature freeze and install-config versioned to v1beta1 657cb8c this PR needs

exception
version bump to v1beta2
migration from v1beta1 to v1beta2

/cc @crawford for tracking.

squeed · 2019-01-15T11:30:46Z

@abhinavdahiya on it. Should I create a completely separate v1beta1 type for the beta1 -> beta2 migration, or just use optional fields?

squeed · 2019-01-15T14:29:40Z

In order to avoid bumping the type, I'll split this in to two PRs. One that just creates the new operator type, and a later one that changes install config to what we'd prefer.

squeed · 2019-01-15T18:15:15Z

OK, this is updated. It doesn't change the serialized InstallConfig type (though it does change the go representation). Not using cluster.openshift.io is a bug, so this doesn't need an exception (I asked for one, and was told it's not necessary).

@abhinavdahiya ptal, thanks!

abhinavdahiya · 2019-01-15T18:25:24Z

pkg/validate/validate.go

can we keep some validations on allowed network types?

Maybe. If users want to define their own networking (e.g. Calico), then they need to opt-out of the operator. However, there's still some value in them setting the network type to Calico and not None.

We discussed this for a while when designing the API object and decided that all values are "allowed" for network type, just that the operator won't react to them.

abhinavdahiya · 2019-01-15T18:27:12Z

pkg/asset/manifests/network.go

/cc @staebler / @rajatchopra asked improvements to this on #943. the result looked like 696475e

I can move the CRDs in to a separate template manifest... up to y'all.

I get an impression from #943 that we do.

abhinavdahiya · 2019-01-15T18:28:51Z

@crawford this is making changes to install-config go definition but i think it makes no change to the serialized form... so not sure where this falls on version change... ?

squeed · 2019-01-17T12:54:41Z

/retest (it's a flake)

knobunc · 2019-01-17T17:36:15Z

/retest
(@squeed the bot is finnicky and won't take words after the command)

crawford · 2019-01-17T18:40:03Z

this is making changes to install-config go definition but i think it makes no change to the serialized form... so not sure where this falls on version change...

The serialized form is what we are versioning. If the changes are internal, we don't need to bump the version.

abhinavdahiya · 2019-02-01T00:08:08Z

I think https://github.com/openshift/cluster-config-operator will help installer to remove all *config.openshift.io CRDs. so for now its fine as-is.

/lgtm

openshift-ci-robot · 2019-02-01T00:08:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [abhinavdahiya]

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

abhinavdahiya · 2019-02-01T00:09:35Z

/hold cancel

PodCIDR was removed in dafc79f (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift#1013).

Through da6d45b (Merge pull request openshift#890 from staebler/asset_loading_tests, 2019-02-04). Background for the networking.type validation entry is in this thread [1]. The OpenStack HAProxy entry has wording based on [2] and Russell's out-of-band suggestions. Forwarding static-pod longs to systemd is still in flight with [3]. [1]: openshift#1013 (comment) [2]: https://github.com/openshift/installer/pull/1185/files#r253714521 [3]: openshift/cluster-bootstrap#11

Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013) and openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209

Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013), openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154), and openshift/installer@9ad20c35 (pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31, openshift/installer#1170). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 8, 2019

openshift-ci-robot requested review from aaronlevy and steveej January 8, 2019 00:07

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2019

squeed force-pushed the cluster-network-config branch from 8e703b2 to d9f99e7 Compare January 8, 2019 00:23

abhinavdahiya reviewed Jan 8, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2019

squeed force-pushed the cluster-network-config branch from d9f99e7 to ea1aaa8 Compare January 15, 2019 18:05

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 15, 2019

abhinavdahiya reviewed Jan 15, 2019

View reviewed changes

squeed force-pushed the cluster-network-config branch from ea1aaa8 to 54e076f Compare January 24, 2019 17:29

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 24, 2019

squeed mentioned this pull request Jan 31, 2019

[WIP] [Do Not Merge] pkg/validate: 'Raw' is a valid type of network #1159

Closed

abhinavdahiya mentioned this pull request Feb 1, 2019

operator: use infra and network manifests to create controllerconfigspec openshift/machine-config-operator#357

Merged

openshift-ci-robot assigned abhinavdahiya Feb 1, 2019

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

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2019

openshift-merge-robot merged commit 9626766 into openshift:master Feb 1, 2019

wking mentioned this pull request Feb 4, 2019

pkg/types/installconfig: Drop PodCIDR reference from ClusterNetworks #1190

Merged

wking mentioned this pull request Feb 5, 2019

CHANGELOG: Document changes since 0.11.0 #1191

Merged

wking mentioned this pull request Feb 14, 2019

*: Bump to install-config v0.12.0 openshift/hive#222

Merged

network: move from operator-specific CRD to cluster config #1013

network: move from operator-specific CRD to cluster config #1013

Uh oh!

Conversation

squeed commented Jan 8, 2019

Uh oh!

squeed commented Jan 8, 2019

Uh oh!

squeed commented Jan 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squeed commented Jan 9, 2019

Uh oh!

abhinavdahiya commented Jan 9, 2019

Uh oh!

squeed commented Jan 14, 2019

Uh oh!

abhinavdahiya commented Jan 14, 2019

Uh oh!

abhinavdahiya commented Jan 14, 2019

Uh oh!

squeed commented Jan 15, 2019

Uh oh!

squeed commented Jan 15, 2019

Uh oh!

squeed commented Jan 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Jan 15, 2019

Uh oh!

squeed commented Jan 17, 2019

Uh oh!

knobunc commented Jan 17, 2019

Uh oh!

crawford commented Jan 17, 2019

Uh oh!

abhinavdahiya commented Feb 1, 2019

Uh oh!

openshift-ci-robot commented Feb 1, 2019

Uh oh!

abhinavdahiya commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants