operators scaffolding #198

rajatchopra · 2018-08-31T19:23:16Z

Just the scaffolding and some skeleton for the kubecore operator.
Follow up commits will fill up kubecore and figure out what to do with clusterversion operator etc.

@wking @staebler @yifan-gu @abhinavdahiya Feedback please. I still have less grasp about the order of operators (or even the list of them all).

Signed-off-by: Rajat Chopra rchopra@redhat.com

openshift-ci-robot · 2018-08-31T19:23:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rajatchopra]

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

wking · 2018-08-31T22:44:55Z

/lint

openshift-ci-robot

@wking: 11 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2018-08-31T22:45:06Z

pkg/asset/operators/operators.go

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


Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/stock.go

+	NetworkOperator() asset.Asset
+}
+
+type StockImpl struct {


Golint comments: exported type StockImpl should have comment or be unexported. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/stock.go

+
+var _ Stock = (*StockImpl)(nil)
+
+func (s *StockImpl) EstablishStock(rootDir string, stock installconfig.Stock) {


Golint comments: exported method StockImpl.EstablishStock should have comment or be unexported. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/stock.go

+	//s.clusterVersionOperator = &clusterVersionOperator{}
+}
+
+func (s *StockImpl) Operators() asset.Asset              { return s.operators }


Golint comments: exported method StockImpl.Operators should have comment or be unexported. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/stock.go

+}
+
+func (s *StockImpl) Operators() asset.Asset              { return s.operators }
+func (s *StockImpl) ClusterVersionOperator() asset.Asset { return s.clusterVersionOperator }


Golint comments: exported method StockImpl.ClusterVersionOperator should have comment or be unexported. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/stock.go

+func (s *StockImpl) Operators() asset.Asset              { return s.operators }
+func (s *StockImpl) ClusterVersionOperator() asset.Asset { return s.clusterVersionOperator }
+func (s *StockImpl) KubeCoreOperator() asset.Asset       { return s.kubeCoreOperator }
+func (s *StockImpl) NetworkOperator() asset.Asset        { return s.networkOperator }


Golint comments: exported method StockImpl.NetworkOperator should have comment or be unexported. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/stock.go

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


Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/stock.go

+	"github.com/openshift/installer/pkg/asset/installconfig"
+)
+
+type Stock interface {


Golint comments: exported type Stock should have comment or be unexported. More info.

openshift-ci-robot · 2018-08-31T22:45:07Z

pkg/asset/operators/kubeCoreOperator.go

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


Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-08-31T22:45:08Z

pkg/asset/operators/kubeCoreOperator.go

+
+	data, err := yaml.Marshal(coreConfig)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to marshal core config: %v", err)


Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

wking

Sorry for the nit-level review :p, but I thought I'd comment on these while I was wrapping my head around where you were going.

I'm personally not clear on how much of this is going to get stubbed into the asset graph (as you have here) to be set up pre-Terraform (although there are hints of this approach in some stale docs). I'm working on internalizing the existing discussion on this, and then I'll give this PR another look.

wking · 2018-08-31T22:55:52Z

pkg/asset/operators/kubeCoreOperator.go

+	if !ok {
+		return nil, fmt.Errorf("failed to get install config state in the dependent asset states")
+	}
+	if err := yaml.Unmarshal(state.Contents[0].Data, &kco.installConfig); err != nil {


nit: you can use GetInstallConfig (new in #160) for this.

wking · 2018-08-31T22:58:00Z

pkg/asset/operators/kubeCoreOperator.go

+	}
+
+	// installconfig is ready, we can create the core config from it now
+	coreConfig, err := kco.coreConfig()


nit: I'd rather pass the *InstallConfig through as an argument here, instead of holding a copy in the kubeCoreOperator struct itself.

abhinavdahiya · 2018-09-15T01:25:49Z

pkg/asset/operators/kubeAddonOperator.go

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


can you rename the file to kube_addon_operator.go seems like other files follow that convention.

abhinavdahiya · 2018-09-15T01:28:10Z

pkg/asset/operators/kubeAddonOperator.go

+	kao.installConfig = ic
+
+	// installconfig is ready, we can create the addon config from it now
+	addonConfig, err := kao.addonConfig()


can you pass installconfig to the func rather than implicit through kao? eg: https://github.com/openshift/installer/blob/master/pkg/asset/ignition/bootstrap.go#L131

abhinavdahiya · 2018-09-15T01:29:09Z

pkg/asset/operators/kubeAddonOperator.go

+			Kind:       kubeaddon.Kind,
+		},
+	}
+	addonConfig.CloudProvider = tectonicCloudProvider(kao.installConfig.Platform)


tectonicCloudProvider like functions should be part of installconfig package.

abhinavdahiya · 2018-09-15T01:30:20Z

pkg/asset/operators/kubeAddonOperator.go

+		},
+	}
+	addonConfig.CloudProvider = tectonicCloudProvider(kao.installConfig.Platform)
+	addonConfig.ClusterConfig.APIServerURL = kao.getAPIServerURL()


getAPIServerURL: tls package might have required something similar too, maybe some common package?(installconfig)

abhinavdahiya · 2018-09-15T01:31:33Z

pkg/asset/operators/kubeCoreOperator.go

+func (kco *kubeCoreOperator) getBaseAddress() string {
+	return fmt.Sprintf("%s.%s", kco.installConfig.ClusterName, kco.installConfig.BaseDomain)
+}
+


all these functions seem like will be used by multiple assets. there should be a common source of finding these.

abhinavdahiya · 2018-09-15T01:33:51Z

pkg/asset/operators/operators.go

+	addon := dependencies[o.assetStock.KubeAddonOperator()].Contents[0]
+
+	// kco+no+tnco go to kube-system config map
+	kubeSys, err := configMap("kube-system", "cluster-config-v1", genericData{


This asset generate seems to be just cluster config, Can we have these configs be separate assets?

abhinavdahiya · 2018-09-15T01:35:52Z

pkg/asset/operators/utils.go

+	return str, nil
+}
+
+func marshalYAML(obj interface{}) (string, error) {


this seems to be a function that us already present in some common package?

Ack.
Maybe all of this file's contents can be moved out. This is the common utils package being born.

crawford · 2018-09-17T23:35:45Z

pkg/asset/operators/tnco.go

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


We don't install TNCO anymore. Please use MCO instead.

crawford · 2018-09-18T00:20:09Z

pkg/asset/operators/stock.go

+	kubeCoreOperator       asset.Asset
+	networkOperator        asset.Asset
+	tnco                   asset.Asset
+	addon                  asset.Asset


Please use consistent names.

crawford · 2018-09-18T00:21:34Z

pkg/asset/operators/stock.go

+
+// StockImpl implements the Stock interface for operators
+type StockImpl struct {
+	operators              asset.Asset


This name isn't very accurate. It's two different configs that really should be separate assets.

I didn't get this. Explain? Its the parent (gathering asset) and the dependent assets.

Sorry, that comment was supposed to be on the StockImpl in pkg/asset/operators/stock.go.

crawford · 2018-09-18T00:22:04Z

pkg/asset/operators/stock.go

+		directory:          rootDir,
+	}
+	// TODO:
+	//s.clusterVersionOperator = &clusterVersionOperator{}


Without this populated, the asset store panics.

Fixed that. The main.go has an operators command. It runs and dumps the config.yaml.

operator config and manifests for: 1. tnco (to be replaced by mco) 2. network-operator 3. kube-core-operator 4. kube-addon-operator

crawford · 2018-09-18T15:43:04Z

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")


This should be called "manifests".

crawford · 2018-09-18T15:45:22Z

cmd/openshift-install/main.go

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


What about all of the other manifests? This only renders openshift-config.yaml and cluster-config.yaml (which is very confusing given the name of this asset.

crawford · 2018-09-18T15:50:48Z

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

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// kubeAddonOperator generates the network-operator-*.yml files


Use yaml instead of yml.

crawford · 2018-09-18T15:50:59Z

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

+				Data: addonConfig,
+			},
+			{
+				Name: filepath.Join(kao.directory, "kube-addon-operator-manifests.yml"),


Singular manifest.

crawford · 2018-09-18T15:51:46Z

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

+}
+
+func (kao *kubeAddonOperator) getAPIServerURL() string {
+	return fmt.Sprintf("https://%s-api.%s:6443", kao.installConfig.ClusterName, kao.installConfig.BaseDomain)


This ends up as https://-api.base.domain:6443 on my system.

crawford · 2018-09-18T16:00:03Z

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

+	}
+	addonConfig.CloudProvider = tectonicCloudProvider(kao.installConfig.Platform)
+	addonConfig.ClusterConfig.APIServerURL = kao.getAPIServerURL()
+	registrySecret, err := generateRandomID(16)


Just use

installer/vendor/k8s.io/apimachinery/pkg/util/rand/rand.go

Line 91 in d45ee35

func String(n int) string {

instead.

wking · 2018-09-19T21:12:36Z

Replaced by #286.

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2018

openshift-ci-robot requested review from abhinavdahiya and crawford August 31, 2018 19:23

openshift-ci-robot reviewed Aug 31, 2018

View reviewed changes

wking reviewed Aug 31, 2018

View reviewed changes

rajatchopra force-pushed the operators branch 2 times, most recently from 4e1d28d to 85e32be Compare September 11, 2018 23:28

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2018

abhinavdahiya reviewed Sep 15, 2018

View reviewed changes

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

rajatchopra force-pushed the operators branch 3 times, most recently from 4b51486 to 1312e0a Compare September 17, 2018 21:46

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

crawford reviewed Sep 17, 2018

View reviewed changes

pkg/asset/operators/tnco.go

@@ -0,0 +1,90 @@

package operators

Copy link

Contributor

crawford Sep 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't install TNCO anymore. Please use MCO instead.

crawford suggested changes Sep 18, 2018

View reviewed changes

Rajat Chopra added 2 commits September 17, 2018 20:35

pkg/assets/operators:

b55ca60

operator config and manifests for: 1. tnco (to be replaced by mco) 2. network-operator 3. kube-core-operator 4. kube-addon-operator

Remove panic. Feedback fixes.

101b1dc

rajatchopra force-pushed the operators branch from 1312e0a to 101b1dc Compare September 18, 2018 00:36

crawford suggested changes Sep 18, 2018

View reviewed changes

rajatchopra mentioned this pull request Sep 19, 2018

asset/manifests: bootstrap manifest generation #286

Merged

wking closed this Sep 19, 2018

rajatchopra deleted the operators branch October 11, 2018 20:11


		var _ Stock = (*StockImpl)(nil)

		func (s *StockImpl) EstablishStock(rootDir string, stock installconfig.Stock) {

operators scaffolding #198

operators scaffolding #198

Conversation

rajatchopra commented Aug 31, 2018

openshift-ci-robot commented Aug 31, 2018

wking commented Aug 31, 2018

openshift-ci-robot 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

wking 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

abhinavdahiya Sep 15, 2018 • edited Loading

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

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

wking commented Sep 19, 2018

abhinavdahiya Sep 15, 2018 •

edited

Loading