Mvp #17

enxebre · 2018-08-10T16:21:36Z

Provides a minimum functional integration for the installer with the machine API and machineSets

Adds operator Config
Adds hack to deploy machineSet object (We need to discuss which operator owns this object. machineConfig? here? its custom operator?)

See:
openshift/installer#119
enxebre/cluster-api-provider-aws#11

enxebre · 2018-08-10T16:28:15Z

FYI @trawler @ingvagabund

ingvagabund

I commented on the individual commits so some comments may no longer apply or need to be moved a few lines down/up.

ingvagabund · 2018-08-13T11:03:55Z

Gopkg.toml

+  version= "v6.0.0"
+[[override]]
+  name = "k8s.io/apimachinery"
+  version = "kubernetes-1.9.0"


We should fix the sigs.k8s.io/cluster-api to the latest release once there is reasonable one available. Meantime, we should fix the revision. The latest one is fbc85d97affbf5c9ad97054c66b23ad2bc0ca097. We want to be as close to the upstream as possible even though we use only the machine types (or more than just those types?). Upstream docs [1] says we can use the revision keyword:

[[constraint]] # Required: the root import path of the project being constrained. name = "github.com/user/project" # Recommended: the version constraint to enforce for the project. # Note that only one of "branch", "version" or "revision" can be specified. version = "1.0.0" branch = "master" revision = "abc123"

[1] https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md

ingvagabund · 2018-08-13T11:10:20Z

cmd/main.go

+
+	go func() {
+		for {
+			// Cluster API client


Given all the variables are local to the for inner block, it would make sense to put the entire block into a named function (for better readability).

ingvagabund · 2018-08-13T11:16:01Z

cmd/main.go

+			// Cluster API client
+			var config *rest.Config
+			var err error
+			if kubeconfig != "" {


func getConfig(kubeconfig string) (*rest.Config, error) { if kubeconfig != "" { glog.V(4).Infof("Loading kube client config from path %q", kubeconfig) return clientcmd.BuildConfigFromFlags("", kubeconfig) } else { glog.V(4).Infof("Using in-cluster kube client config") return rest.InClusterConfig() } }

and then:

config, err := getConfig(kubeconfig)

Do you need to create the config every single loop? Given the kubeconfig is not likely to be changed within the container running the machine api operator.

ingvagabund · 2018-08-13T11:16:51Z

cmd/main.go

+			decode := scheme.Codecs.UniversalDeserializer().Decode
+
+			// Create Cluster object
+			clusterSpec, err := ioutil.ReadFile("examples/cluster.yaml") // just pass the file name


If this is going to be part of the production deployment, I would s/examples/manifests/. It looks more enterprise :). Plus, the content of the examples directory can change over time without any constraints. The manifests is more like Hey, these files might be actually used somewhere in the production code.

ingvagabund · 2018-08-13T11:20:53Z

cmd/main.go

+
+			cluster := obj.(*clusterv1.Cluster)
+			_, err = client.ClusterV1alpha1().Clusters("test").Create(cluster)
+			if err != nil {


v1alphaClient := client.ClusterV1alpha1() if _, err := v1alphaClient.Clusters("test").Create(cluster); err != nil { fmt.Printf("Cannot create cluster, retrying in 5 secs: %v", err) } else { fmt.Printf("Created Cluster object") }

ingvagabund · 2018-08-13T11:28:00Z

cmd/main.go

+			}
+
+			// Create Machine object
+			machineSpec, err := ioutil.ReadFile("examples/machine.yaml") // just pass the file name


The same here: s/examples/manifests/

ingvagabund · 2018-08-13T11:29:11Z

cmd/main.go

+			}
+
+			machine := obj.(*clusterv1.Machine)
+			_, err = client.ClusterV1alpha1().Machines("test").Create(machine)


if _, err = v1alphaClient.Machines("test").Create(machine); err != nil { fmt.Printf("Cannot create machine, retrying in 5 secs: %v", err) } else { fmt.Printf("Created Machine object") }

ingvagabund · 2018-08-13T11:31:21Z

cmd/main.go

+				fmt.Printf("Created Machine object")
+				return
+			}
+			time.Sleep(5 * time.Second)


maybe:

... const retryPeriod = 5 * time.Second ... time.Sleep(retryPeriod)

ingvagabund · 2018-08-13T11:32:10Z

cmd/main.go

@@ -45,5 +119,9 @@ func main() {
 // rendererFromFile reads the config object on demand from the path and then passes it to the
 // renderer.
 func rendererFromFile() []xotypes.UpgradeSpec {
-  return render.MakeRenderer(&render.OperatorConfig{}, manifestDir)()
+	config, err := render.Config(configPath)


Does it build at all? The config variable is not used anywhere in the function's body.

ingvagabund · 2018-08-13T11:38:16Z

cmd/main.go

+			operatorConfig, err := render.Config(configPath)
+			fmt.Printf("configPath %#v", configPath)
+			if err != nil {
+				glog.Exitf("Error reading machine-api-operator config: %v", err)


Exitf logs to the FATAL, ERROR, WARNING, and INFO logs, then calls os.Exit(1). Arguments are handled in the manner of fmt.Printf; a newline is appended if missing. (see [1])

Fatalf logs to the FATAL, ERROR, WARNING, and INFO logs, including a stack trace of all running goroutines, then calls os.Exit(255). Arguments are handled in the manner of fmt.Printf; a newline is appended if missing. (see [2]).

No matter what I still prefer the stack trace to be present. And frankly, this is the first time i see the glog.Exitf function to be used :).

[1] https://godoc.org/github.com/golang/glog#Exitf
[2] https://godoc.org/github.com/golang/glog#Fatalf

enxebre · 2018-08-13T11:47:54Z

@ingvagabund thanks for having a look! To clarify, as stated in the description the way this PR deploys the machineSet it's just a dirty hack for having something to deploy the object and being able to put an e2e working PR against the installer.
We need to discuss which operator properly owns and manage the lifecycle of this object: this operator? machine config operator? needs a custom dedicated operator?)

trawler

cmd/cmd should be added to .gitignore so we don't push binaries into the repo.
same for anything in bin (if it doesn't already exist).

openshift-ci-robot · 2018-08-14T14:28:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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~~ [enxebre]

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

paulfantom · 2018-08-15T06:18:28Z

This PR includes a binary file cmd/cmd, @enxebre please remove it and maybe add exlusion rule to .gitconfig

enxebre · 2018-08-17T10:51:48Z

cleaned this up to a mergeable state. Follow ups:

Include the machineSet object to be managed
Drop x-operator and Rewrite implementation
- May be reuse https://github.com/openshift/library-go/tree/master/pkg/operator/resource similar to https://github.com/openshift/machine-config-operator/pull/11/files
Integrate with https://github.com/openshift/cluster-version-operator when is ready

enxebre · 2018-08-17T11:00:26Z

machines/cluster.yaml

+apiVersion: "cluster.k8s.io/v1alpha1"
+kind: Cluster
+metadata:
+  name: test


name and namespaces need to be consolidated as part of CLOUD-80

enxebre added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 10, 2018

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

ingvagabund reviewed Aug 13, 2018

View reviewed changes

trawler suggested changes Aug 13, 2018

View reviewed changes

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

enxebre removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2018

trawler approved these changes Aug 14, 2018

View reviewed changes

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

enxebre removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2018

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

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

enxebre added 7 commits August 17, 2018 12:12

vendor cluster api

fa06953

add examples

67fd876

add machines folder

8309c02

update manifests

aeea16a

add config params

8d2e9fe

add machineSet deployment hack

ea1e046

add machines folder to dockerfile

f0e4fa5

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

enxebre commented Aug 17, 2018

View reviewed changes

enxebre merged commit 363ab9e into openshift:master Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mvp #17

Mvp #17

enxebre commented Aug 10, 2018 •

edited

Loading

enxebre commented Aug 10, 2018

ingvagabund left a comment

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

ingvagabund Aug 13, 2018

enxebre Aug 17, 2018

ingvagabund Aug 13, 2018

enxebre commented Aug 13, 2018

trawler left a comment

openshift-ci-robot commented Aug 14, 2018

paulfantom commented Aug 15, 2018

enxebre commented Aug 17, 2018 •

edited

Loading

enxebre Aug 17, 2018

Mvp #17

Mvp #17

Conversation

enxebre commented Aug 10, 2018 • edited Loading

enxebre commented Aug 10, 2018

ingvagabund 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

enxebre commented Aug 13, 2018

trawler left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 14, 2018

paulfantom commented Aug 15, 2018

enxebre commented Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

enxebre commented Aug 10, 2018 •

edited

Loading

enxebre commented Aug 17, 2018 •

edited

Loading