asset/manifests: bootstrap manifest generation #286

rajatchopra · 2018-09-19T20:34:35Z

asset/manifests: bootstrap manifest generation

All manifest files required for bootstrap operations are generated. The operators covered include:
1. network-operator
2. kube-core-operator
3. kube-addon-operator
4. machine-api-operator
The config files are generated in the root directory. And the manifest files go in <root>/manifests/ directory.

This PR supersedes #198

abhinavdahiya · 2018-09-19T20:44:00Z

pkg/asset/operators/operators.go

+		TectonicNetworkOperatorImage:    "quay.io/coreos/tectonic-network-operator-dev:3b6952f5a1ba89bb32dd0630faddeaf2779c9a85",
+		WorkerIgnConfig:                 "", // FIXME: this means that depending on ignition assets (risk of cyclical dependencies)
+	}
+


nit: We can loop it?

Didn't grasp what could be looped. Could do if they were all of one type, but the templates have them mixed. And more could be coming.

abhinavdahiya · 2018-09-19T20:46:07Z

This looks good. Its missing tectonic/resource/manifests.

/cc @crawford

crawford · 2018-09-19T22:44:31Z

cmd/openshift-install/main.go

@@ -13,6 +13,7 @@ import (
 var (
 	installConfigCommand   = kingpin.Command("install-config", "Generate the Install Config asset")
 	ignitionConfigsCommand = kingpin.Command("ignition-configs", "Generate the Ignition Config assets")
+	operatorsCommand       = kingpin.Command("operators", "Generate the Operator assets")


Use "manifests" instead.

crawford · 2018-09-19T22:46:32Z

pkg/asset/operators/kube-addon-operator.go

+}
+
+func (kao *kubeAddonOperator) manifest() (string, error) {
+	return "", nil


Does this need a body?

currently all in operators.go. The ones marked for kube-addon will be moved here. So, its a TODO really. Removing it for the time being then.

crawford · 2018-09-19T22:46:58Z

pkg/asset/operators/network-operator.go

+
+	tectonicnetwork "github.com/coreos/tectonic-config/config/tectonic-network"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	//"github.com/openshift/openshift-network-operator/pkg/apis/networkoperator"


crawford · 2018-09-19T22:48:23Z

pkg/asset/operators/tnco.go

+	tncoConfig.ControllerConfig.ClusterDNSIP = ip.String()
+
+	tncoConfig.ControllerConfig.Platform = tectonicCloudProvider(tnco.installConfig.Platform)
+	tncoConfig.ControllerConfig.CloudProviderConfig = "" // TODO(yifan): Get CloudProviderConfig.


Do these need to be implemented?

Yeah. I still have to figure this one out. The cloudproviderconfig especially.

abhinavdahiya · 2018-09-19T23:25:41Z

pkg/asset/operators/tnco.go

@@ -0,0 +1,95 @@
+package operators


@rajatchopra can you drop this file. This is not required.

pkg/asset/operators/operators.go

abhinavdahiya · 2018-09-19T23:27:52Z

pkg/asset/operators/operators.go

+	kubeSys, err := configMap("kube-system", "cluster-config-v1", genericData{
+		"kco-config":     string(kco.Data),
+		"network-config": string(no.Data),
+		"tnco-config":    string(tnco.Data),


drop this not required.

crawford · 2018-09-19T23:46:25Z

cmd/openshift-install/main.go

@@ -13,6 +13,7 @@ import (
 var (
 	installConfigCommand   = kingpin.Command("install-config", "Generate the Install Config asset")
 	ignitionConfigsCommand = kingpin.Command("ignition-configs", "Generate the Ignition Config assets")
+	manifestsCommand       = kingpin.Command("manifests", "Generate the 'manifests' assets")


"Generate the Kubernetes manifests"

crawford · 2018-09-19T23:46:37Z

cmd/openshift-install/main.go

@@ -33,6 +34,10 @@ func main() {
 			assetStock.MasterIgnition(),
 			assetStock.WorkerIgnition(),
 		}
+	case manifestsCommand.FullCommand():
+		targetAssets = []asset.Asset{
+			assetStock.Operators(),


Just change the top level function call or the entire package naming?

I'd prefer the entire package be renamed.

@crawford Done. If this all looks good to you, I can squash the commits once more for the merge.

abhinavdahiya

/lgtm

/hold
@rajatchopra /hold cancel when @crawford has /lgtmd as he has some requests.

abhinavdahiya · 2018-09-20T19:14:20Z

/lgtm

All manifest files required for bootstrap operations are generated. The operators covered include: 1. network-operator 2. kube-core-operator 3. kube-addon-operator 4. machine-api-operator The config files are generated in the root directory. And the manifest files go in <root>/manifests/ directory. asset/cluster: Added the BUILD.bazel file asset/stock: Changed stock to call manifests as well. Modified the BUILD.bazel file cmd/openshift-install: 'manifests' is the new target to get all operator configs and manifests

crawford · 2018-09-20T19:52:59Z

/lgtm

openshift-ci-robot · 2018-09-20T19:53:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, 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,crawford,rajatchopra]

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

crawford · 2018-09-20T19:54:04Z

/hold cancel

We'd added this in 4d636d3 (asset/manifests: bootstrap manifest generation, 2018-08-31, openshift#286) to support the machine-API operator which had been generating worker machine-sets. But since e2dc955 (pkg/asset: add ClusterK8sIO, machines.Worker assets, 2018-10-15, openshift#468), we've been creating those machine-sets ourselves. And the machine-API operator dropped their consuming code in openshift/machine-api-operator@c59151f6 (delete machine/cluster object loops, 2018-10-22, openshift/machine-api-operator#286). Dropping this dependency fixes bootstrap Ignition config edits during a multi-step deploy. For example, with: $ openshift-install --dir=wking create ignition-configs $ tree wking wking ├── bootstrap.ign ├── master-0.ign └── worker.ign before this commit, any edits to bootstrap.ign were clobbered in a subsequent 'create cluster' call, because: 1. The bootstrap Ignition config depends on the manifests. 2. The manifests depended on the worker Ignition config. 3. The worker Ignition config is on disk, so it gets marked dirty.

We set this in both kube-apiserver-secret.go and openshift-apiserver-secret.go, and we started setting this in db65ea2 (modules/tls: modularize TLS provisioning, 2017-09-13, coreos/tectonic-installer#1811). But we have been using the kube-ca value for it since at least 4d636d3 (asset/manifests: bootstrap manifest generation, 2018-08-31, openshift#286) and I see no other OpenShift consumers [1]. [1]: https://github.com/search?q=org%3Aopenshift+oidc-ca.crt&type=Code

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 19, 2018

openshift-ci-robot requested review from aaronlevy and crawford September 19, 2018 20:34

rajatchopra force-pushed the operators_wip branch from 4e89514 to ea0890a Compare September 19, 2018 20:41

abhinavdahiya reviewed Sep 19, 2018

View reviewed changes

rajatchopra force-pushed the operators_wip branch from ea0890a to 8222476 Compare September 19, 2018 20:51

wking mentioned this pull request Sep 19, 2018

operators scaffolding #198

Closed

crawford suggested changes Sep 19, 2018

View reviewed changes

rajatchopra force-pushed the operators_wip branch from bd11735 to 63d7f6a Compare September 19, 2018 23:02

abhinavdahiya suggested changes Sep 19, 2018

View reviewed changes

abhinavdahiya reviewed Sep 19, 2018

View reviewed changes

pkg/asset/operators/operators.go Outdated Show resolved Hide resolved

abhinavdahiya suggested changes Sep 19, 2018

View reviewed changes

crawford suggested changes Sep 19, 2018

View reviewed changes

abhinavdahiya approved these changes Sep 19, 2018

View reviewed changes

openshift-ci-robot assigned abhinavdahiya Sep 19, 2018

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 19, 2018

rajatchopra force-pushed the operators_wip branch from d7cacca to 37c3d69 Compare September 19, 2018 23:49

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

rajatchopra force-pushed the operators_wip branch 3 times, most recently from 35a0632 to ec6cba5 Compare September 20, 2018 19:04

rajatchopra changed the title ~~Operators wip~~ Bootstrap manifests target for installer Sep 20, 2018

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

rajatchopra force-pushed the operators_wip branch from ec6cba5 to 3858ec4 Compare September 20, 2018 19:17

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2018

rajatchopra changed the title ~~Bootstrap manifests target for installer~~ bootstrap manifest generation Sep 20, 2018

rajatchopra force-pushed the operators_wip branch from 3858ec4 to 4d636d3 Compare September 20, 2018 19:19

crawford changed the title ~~bootstrap manifest generation~~ asset/manifests: bootstrap manifest generation Sep 20, 2018

crawford approved these changes Sep 20, 2018

View reviewed changes

openshift-ci-robot assigned crawford Sep 20, 2018

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

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

openshift-merge-robot merged commit 39865b7 into openshift:master Sep 20, 2018

wking mentioned this pull request Oct 24, 2018

pkg/asset/manifests/operators: Drop worker-ignition dependency #529

Merged

wking mentioned this pull request Oct 29, 2018

Drop oidc-ca.crt? #564

Closed

wking mentioned this pull request Oct 29, 2018

pkg/asset/manifests: Drop oidc-ca.crt #568

Merged

wking mentioned this pull request Dec 9, 2018

Unable to retrieve pull secret openshift-operator-lifecycle-manager/coreos-pull-secret for openshift-operator-lifecycle-manager/olm-operator... operator-framework/operator-lifecycle-manager#607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asset/manifests: bootstrap manifest generation #286

asset/manifests: bootstrap manifest generation #286

rajatchopra commented Sep 19, 2018 •

edited

Loading

abhinavdahiya Sep 19, 2018

rajatchopra Sep 19, 2018

abhinavdahiya commented Sep 19, 2018

crawford Sep 19, 2018

crawford Sep 19, 2018

rajatchopra Sep 19, 2018

crawford Sep 19, 2018

rajatchopra Sep 19, 2018

crawford Sep 19, 2018

rajatchopra Sep 19, 2018

abhinavdahiya Sep 19, 2018

abhinavdahiya Sep 19, 2018

crawford Sep 19, 2018

crawford Sep 19, 2018

rajatchopra Sep 19, 2018

crawford Sep 20, 2018

rajatchopra Sep 20, 2018

abhinavdahiya left a comment

abhinavdahiya commented Sep 20, 2018

crawford commented Sep 20, 2018

openshift-ci-robot commented Sep 20, 2018

crawford commented Sep 20, 2018

asset/manifests: bootstrap manifest generation #286

asset/manifests: bootstrap manifest generation #286

Conversation

rajatchopra commented Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya commented Sep 19, 2018

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

abhinavdahiya left a comment

Choose a reason for hiding this comment

abhinavdahiya commented Sep 20, 2018

crawford commented Sep 20, 2018

openshift-ci-robot commented Sep 20, 2018

crawford commented Sep 20, 2018

rajatchopra commented Sep 19, 2018 •

edited

Loading