Add a `config.openshift.io/Infrastructure` instance to the cluster #943

ironcladlou · 2018-12-19T11:53:26Z

Add a config.openshift.io/Infrastructure instance to the cluster.

Update the openshift/api package to a version containing the new config type.
Generate the config.openshift.io/Infrastructure custom resource definition manifest.
Generate the CRD instance manifest populated with cloud provider name from the install config.

This enables consumers of cloud provider configuration to migrate away from install config.

/cc @openshift/sig-network-edge @rajatchopra @deads2k

ironcladlou · 2018-12-19T13:21:09Z

/retest

staebler · 2018-12-19T15:11:54Z

pkg/asset/manifests/infrastructure.go

+
+// Infrastructure generates the cluster-infrastructure-*.yml files.
+type Infrastructure struct {
+	config   *configv1.Infrastructure


Do not need to store the config. It is not used.

staebler · 2018-12-19T15:12:58Z

pkg/asset/manifests/infrastructure.go

+
+	crdData, err := content.GetBootkubeTemplate(infraCrdFilename)
+	if err != nil {
+		return err


Wrap this error with a meaningful message.

Fixed — also, I wasn't aware of github.com/pkg/errors, I like it and will propose using it in my team's operators. Thanks!

staebler · 2018-12-19T15:18:27Z

Gopkg.lock

@@ -364,14 +364,14 @@
  version = "1.0.1"

 [[projects]]
-  digest = "1:68214731af5ff5a3bfab4d28571578e5522bc4f667ad1232745d7b4189ccb442"
+  digest = "1:400ab065329172f4f4a88ad94401795780566749b1b3cb73029d0d3971659925"


Put the vendor changes in a separate commit.

Isn't this already the case?
0cd84d5

Ah, yes, sorry for the noise.

staebler · 2018-12-19T15:22:58Z

pkg/asset/manifests/infrastructure.go

+
+	configData, err := yaml.Marshal(i.config)
+	if err != nil {
+		return errors.Wrapf(err, "failed to create %s manifests from InstallConfig", i.Name())


The message for this error does not match what the error is.

staebler · 2018-12-19T15:24:46Z

pkg/asset/manifests/infrastructure.go

+}
+
+// Load loads the already-rendered files back from disk.
+func (i *Infrastructure) Load(f asset.FileFetcher) (bool, error) {


This asset should not be loadable. Its files will be loaded by the Common Manifests asset. For more context, see #877.

Ah, we may need to update the recent DNS config implementation for this too.

I'll refactor DNS once this PR merges

abhinavdahiya · 2018-12-19T15:45:01Z

/hold

Cloud provider is the wrong abstraction in the config. All cloudproviders are going to be external pretty soon. So I rather see Platform been set here...

ironcladlou · 2018-12-19T15:48:59Z

@abhinavdahiya

Cloud provider is the wrong abstraction in the config. All cloudproviders are going to be external pretty soon. So I rather see Platform been set here...

That seems like a discussion we should have had in openshift/api#145. Are you going to open a new API PR with some proposed change?

staebler · 2018-12-19T16:18:06Z

@rajatchopra Is there a distinction between manifests that should be part of manifest-templates and those that should not? I am wondering whether this new Infrastructure Config asset should be output from the manifest-templates target. It is not clear to me why the Ingress Config and Network Config are not.

abhinavdahiya · 2018-12-19T16:25:57Z

@abhinavdahiya

Cloud provider is the wrong abstraction in the config. All cloudproviders are going to be external pretty soon. So I rather see Platform been set here...

That seems like a discussion we should have had in openshift/api#145. Are you going to open a new API PR with some proposed change?

🙁 Yeah let me do that, sorry that PR slipped my radar.

@ironcladlou is this PR blocking you on something important?

ironcladlou · 2018-12-19T18:26:40Z

Opened openshift/api#149 to discuss refactoring the config API

rajatchopra

non-blocking comments.
I notice that even the ingress asset uses a template directly and not through a template asset. Can fix both of these in follow-up PR.

rajatchopra · 2018-12-19T19:12:06Z

pkg/asset/manifests/infrastructure.go

+		return errors.Wrapf(err, "failed to marshal config: %#v", config)
+	}
+
+	crdData, err := content.GetBootkubeTemplate(infraCrdFilename)


There should be a template asset first which this asset can depend upon. If we do it directly, then the 'templates' target will not dump this file (which may be okay, but inconsistent).

rajatchopra · 2018-12-19T19:26:26Z

data/data/manifests/bootkube/cluster-infrastructure-01-crd.yaml

@@ -0,0 +1,16 @@
+apiVersion: apiextensions.k8s.io/v1beta1


Is this a bootkube file? Seems like an openshift file. I mean keep it in data/data/manifests/openshift/

rajatchopra · 2018-12-19T19:30:34Z

@rajatchopra Is there a distinction between manifests that should be part of manifest-templates and those that should not? I am wondering whether this new Infrastructure Config asset should be output from the manifest-templates target. It is not clear to me why the Ingress Config and Network Config are not.

@staebler I put my review comments and then saw your comment. I have the same feedback. There is no distinction as of now, I think we should put all templates/manifests as manifest-template assets first and depend upon them if needed. Unless there is a strong reason otherwise. I don't see the reason here.

ironcladlou · 2019-01-08T22:25:59Z

@rajatchopra @staebler @abhinavdahiya refactored per feedback, PTAL. Tested manually with create manifests but can't yet get a new cluster going to see it all in action.

Are there any automated tests I should incorporate?

ironcladlou · 2019-01-08T22:30:42Z

Should the Infrastructure instance itself also be a template like the CRD? I'm not entirely clear on what the decision making framework is for when an asset should be a Go template in vs. a manifest generated from Go code in pkg/asset/manifests.

ironcladlou · 2019-01-09T18:55:29Z

/retest

ironcladlou · 2019-01-09T20:34:44Z

$ oc get infrastructures/cluster -o yaml
apiVersion: config.openshift.io/v1
kind: Infrastructure
metadata:
  creationTimestamp: 2019-01-09T18:53:37Z
  generation: 1
  name: cluster
  resourceVersion: "227"
  selfLink: /apis/config.openshift.io/v1/infrastructures/cluster
  uid: df61ad74-143f-11e9-a097-02040a65c016
spec: {}
status:
  platform: AWS

ironcladlou · 2019-01-09T20:34:57Z

/retest

Miciah · 2019-01-09T22:51:57Z

/retest

ironcladlou · 2019-01-10T02:03:06Z

@abhinavdahiya @rajatchopra @staebler PTAL, tests are green.

pkg/asset/manifests/infrastructure.go

* Update the `openshift/api` package to a version containing the new config type. * Generate the `config.openshift.io/Infrastructure` custom resource definition manifest. * Generate the CRD instance manifest populated with cloud provider name from the install config. This enables consumers of cloud provider configuration to migrate away from install config.

rajatchopra · 2019-01-11T00:46:19Z

/lgtm

ironcladlou · 2019-01-11T00:55:01Z

/hold cancel

ironcladlou · 2019-01-11T00:55:40Z

@rajatchopra re-tag? I accidentally removed it

ironcladlou · 2019-01-11T00:56:05Z

/retest

abhinavdahiya · 2019-01-11T01:00:46Z

/lgtm

openshift-ci-robot · 2019-01-11T01:00:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, ironcladlou, rajatchopra

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~~ [abhinavdahiya,rajatchopra]

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

ironcladlou · 2019-01-11T01:01:52Z

C'mon, bot.
/retest

Miciah · 2019-01-11T05:55:16Z

could not copy stable imagestreamtag: Timeout: request did not complete within allowed duration

/retest

Miciah · 2019-01-11T06:17:17Z

/retest

Miciah · 2019-01-11T06:48:34Z

level=error msg="\t* module.vpc.aws_route_table_association.worker_routing[0]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.worker_routing.0: timeout while waiting for state to become 'success' (timeout: 5m0s)"
level=error
level=error
level=error msg="\t* module.vpc.aws_route_table_association.route_net[4]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.route_net.4: timeout while waiting for state to become 'success' (timeout: 5m0s)"

/retest

Miciah · 2019-01-11T08:19:10Z

/retest

ironcladlou · 2019-01-11T14:42:58Z

/retest

Miciah · 2019-01-11T16:28:32Z

could not resolve base image: the server was unable to return a response in the time allotted, but may still be processing the request (get imagestreamtags.image.openshift.io pipeline:installer)

/retest

ironcladlou · 2019-01-11T16:28:36Z

/retest

ironcladlou · 2019-01-11T17:27:56Z

/retest

Miciah · 2019-01-11T18:51:59Z

/retest

ironcladlou · 2019-01-11T20:25:41Z

/refresh

ironcladlou · 2019-01-11T20:27:50Z

/retest

ironcladlou · 2019-01-11T22:24:09Z

/retest

ironcladlou · 2019-01-12T01:38:42Z

/retest

* Add the Network.config.openshift.io CRD * Generate the network config from the install config * Remove networkoperator types from install config (but use the same schema) * Move network CRDs to templates to match openshift#943 This doesn't change the json/yaml serialization of the install config, but it changes it internally.

openshift-ci-robot requested review from deads2k and rajatchopra December 19, 2018 11:53

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 19, 2018

staebler reviewed Dec 19, 2018

View reviewed changes

ironcladlou force-pushed the cloudprovider branch 2 times, most recently from fd335ff to f4ed5ff Compare December 19, 2018 15:40

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

rajatchopra approved these changes Dec 19, 2018

View reviewed changes

Miciah mentioned this pull request Dec 22, 2018

Cloud HA strategy should automatically use PROXY support when available. openshift/cluster-ingress-operator#84

Merged

Update openshift/api dependency

6dc1824

ironcladlou force-pushed the cloudprovider branch 2 times, most recently from 60c114f to fa5253f Compare January 8, 2019 22:10

abhinavdahiya reviewed Jan 10, 2019

View reviewed changes

pkg/asset/manifests/infrastructure.go Show resolved Hide resolved

ironcladlou force-pushed the cloudprovider branch from fa5253f to 696475e Compare January 10, 2019 20:54

abhinavdahiya added this to the Freeze milestone Jan 10, 2019

openshift-ci-robot assigned rajatchopra Jan 11, 2019

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 11, 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 Jan 11, 2019

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

openshift-merge-robot merged commit c53de55 into openshift:master Jan 12, 2019

abhinavdahiya mentioned this pull request Jan 15, 2019

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

Merged

Add a config.openshift.io/Infrastructure instance to the cluster #943

Add a config.openshift.io/Infrastructure instance to the cluster #943

Uh oh!

Conversation

ironcladlou commented Dec 19, 2018

Uh oh!

ironcladlou commented Dec 19, 2018

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!

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 Dec 19, 2018

Uh oh!

ironcladlou commented Dec 19, 2018

Uh oh!

staebler commented Dec 19, 2018

Uh oh!

abhinavdahiya commented Dec 19, 2018

Uh oh!

ironcladlou commented Dec 19, 2018

Uh oh!

rajatchopra left a comment

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!

rajatchopra commented Dec 19, 2018

Uh oh!

ironcladlou commented Jan 8, 2019

Uh oh!

ironcladlou commented Jan 8, 2019

Uh oh!

ironcladlou commented Jan 9, 2019

Uh oh!

ironcladlou commented Jan 9, 2019

Uh oh!

ironcladlou commented Jan 9, 2019

Uh oh!

Miciah commented Jan 9, 2019

Uh oh!

ironcladlou commented Jan 10, 2019

Uh oh!

Uh oh!

rajatchopra commented Jan 11, 2019

Uh oh!

ironcladlou commented Jan 11, 2019

Uh oh!

ironcladlou commented Jan 11, 2019

Uh oh!

ironcladlou commented Jan 11, 2019

Uh oh!

abhinavdahiya commented Jan 11, 2019

Uh oh!

openshift-ci-robot commented Jan 11, 2019

Uh oh!

ironcladlou commented Jan 11, 2019

Uh oh!

Add a `config.openshift.io/Infrastructure` instance to the cluster #943

Add a `config.openshift.io/Infrastructure` instance to the cluster #943