*: implement how to deploy an etcd cluster #3

fanminshi · 2018-04-09T22:31:05Z

The first, vault operator needs to create an etcd cluster before proceeding to other parts of reconciliation.
This pr shows that the reconciliation loop operates on the model of finding the current state and apply the appropriate action.

For example,

check if the vault cluster is ClusterPhaseInitial state.
- If it is, then proceed to determine if vault cluster has created the etcd-cluster.
- If etcd cluster has not been created, then apply action to create it.
- if etcd cluster has been created, then check if it at the state of correct size.
- if etcd cluster is not at the right size, apply action by breaking from reconciliation loop and wait for watch to trigger
  the Handle again.
- if etcd cluster is at the state of correct size, then proceed to figure next vault cluster's state and apply the corresponding actions.
if vault cluster not ClusterPhaseInitial state, then proceed to figure next vault cluster's state and apply the corresponding actions.

cc/ @hasbro17

fanminshi · 2018-04-09T23:42:08Z

Manual Test:

# setup etcd-operator
$ kubectl create -f deploy/etcd_crds.yaml
$ kubectl create -f deploy/etcd-operator-deploy.yaml

# setup vault-operator
$ kubectl create -f deploy/operator.yaml
$ kubectl create -f deploy/cr.yaml

Output:

$ kubectl get po
NAME                             READY     STATUS    RESTARTS   AGE
etcd-operator-6cccdc5566-lc5vj   3/3       Running   0          7m
example-etcd-2vnt6rjmjh          1/1       Running   0          1m
example-etcd-f65s5mzp9f          1/1       Running   0          2m
example-etcd-p77v54smvx          1/1       Running   0          2m
vault-operator-788978b8b-6sfhc   1/1       Running   0          2m

hasbro17 · 2018-04-10T17:23:32Z

vault-operator/Gopkg.toml

@@ -10,6 +10,10 @@
  name = "k8s.io/client-go"
  version = "kubernetes-1.9.3"

+[[override]]
+name = "github.com/coreos/etcd-operator"
+branch = "master"


Set this to a fixed version so that the builds are reproducible, i.e this doesn't fail anytime the etcd-operator master breaks something.

[[override]] name = "github.com/coreos/etcd-operator" version = "=v0.9.1"

@hasbro17 the issue with v0.9.1 is that it uses an outdated generated deepcopy function that's not compatible with api-machinery 1.9.3. hence, if I were to import the above, the vault-operator won't compile. However, the outdated deepcopy definition is updated in the master via coreos/etcd-operator#1727

I'll update this once we have release a new version of etcd-operator.

Alright, put a TODO for now.

hasbro17 · 2018-04-10T17:39:12Z

vault-operator/pkg/stub/vault.go

@@ -0,0 +1,132 @@
+package stub
+
+// reconcileVault reconciles the vault cluster's state to the spec specified by vr


Move this comment down to the function header

hasbro17 · 2018-04-10T18:01:56Z

vault-operator/pkg/stub/vault.go

+	// Simulate initializer.
+	changed := vr.SetDefaults()
+	if changed {
+		err := action.Update(vr)


Just return the err directly if it's not being wrapped:

if change { return action.Update(vr) }

hasbro17 · 2018-04-10T18:19:40Z

vault-operator/pkg/stub/vault.go

+		// If not, we need to wait until etcd cluster is up before proceeding to the next state.
+		// Hence, we return from here and let the Watch triggers the handler again.
+		rdy, err := isEtcdClusterReady(ec)
+		if rdy || err != nil {


This logic isn't very clear. Make it more explicit:

ready, err := isEtcdClusterReady(ec) if err != nil { return fmt.Errorf("failed to check if etcd cluster is ready: %v", err) } if !ready { logrus.Infof("Waiting for EtcdCluster (%v) to become ready: %v", ec.Name) return nil }

Make sure to add the logging since unlike the original implementation there is no limited retry period in checking the EtcdCluster readiness and it could keep waiting without providing any information about the reason.

Also correct me if I'm wrong but with the current logic if ready=true then that would just return nil everytime and we won't proceed beyond this.

Also correct me if I'm wrong but with the current logic if ready=true then that would just return nil everytime and we won't proceed beyond this.

You are correct. Good catch!

Also we need to update vr.Status.Phase to Running after it's confirmed the EtcdCluster is ready.

@hasbro17 will update the phase when implementing https://github.com/coreos-inc/vault-operator/blob/master/pkg/operator/sync.go#L183

hasbro17 · 2018-04-10T18:23:46Z

vault-operator/pkg/stub/vault.go

+	trueVar := true
+	return metav1.OwnerReference{
+		APIVersion: api.SchemeGroupVersion.String(),
+		Kind:       "VaultService",


Use:

Kind: api.VaultServiceKind,

Currently, we don't auto generate the VaultServiceKind in the register.go. However, we might want to do that in the future. For this pr, I'll just manually add VaultServiceKind into register.go

fanminshi · 2018-04-10T18:55:40Z

all fixed PTAL cc/ @hasbro17

hasbro17 · 2018-04-10T19:53:44Z

vault-operator/pkg/stub/vault.go

+		// Check if etcd cluster is up and running.
+		// If not, we need to wait until etcd cluster is up before proceeding to the next state;
+		// Hence, we return from here and let the Watch triggers the handler again.
+		rdy, err := isEtcdClusterReady(ec)


Minor nit but can you change rdy to ready. I feel that's easier to read and it's short enough already.

sounds good!

hasbro17 · 2018-04-10T19:58:10Z

LGTM

fanminshi force-pushed the implement_deploy branch 4 times, most recently from 0a1eca0 to c019f65 Compare April 9, 2018 23:23

fanminshi force-pushed the implement_deploy branch from 814bec3 to 859b6fd Compare April 9, 2018 23:43

hasbro17 reviewed Apr 10, 2018

View reviewed changes

fanminshi added 2 commits April 10, 2018 10:39

dep: update required dependencies and gitignore

f675a54

apis: register etcd operator scheme

2a0ba82

fanminshi force-pushed the implement_deploy branch 3 times, most recently from da6f119 to 757d31c Compare April 10, 2018 17:42

hasbro17 reviewed Apr 10, 2018

View reviewed changes

fanminshi force-pushed the implement_deploy branch from 757d31c to 05c5464 Compare April 10, 2018 18:55

hasbro17 reviewed Apr 10, 2018

View reviewed changes

fanminshi added 2 commits April 10, 2018 13:01

pkg/stub: implement etcdCluster deployment state and actions

82990e7

deploy: add etcd operator related manifests

22ac71c

fanminshi force-pushed the implement_deploy branch from 05c5464 to 22ac71c Compare April 10, 2018 20:01

fanminshi merged commit e26698b into operator-framework:master Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: implement how to deploy an etcd cluster #3

*: implement how to deploy an etcd cluster #3

fanminshi commented Apr 9, 2018

fanminshi commented Apr 9, 2018

hasbro17 Apr 10, 2018

fanminshi Apr 10, 2018 •

edited

Loading

fanminshi Apr 10, 2018

hasbro17 Apr 10, 2018

fanminshi Apr 10, 2018

hasbro17 Apr 10, 2018

fanminshi Apr 10, 2018

hasbro17 Apr 10, 2018

hasbro17 Apr 10, 2018

fanminshi Apr 10, 2018

hasbro17 Apr 10, 2018

fanminshi Apr 10, 2018

hasbro17 Apr 10, 2018

fanminshi Apr 10, 2018

fanminshi commented Apr 10, 2018

hasbro17 Apr 10, 2018

fanminshi Apr 10, 2018

hasbro17 commented Apr 10, 2018

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

		// reconcileVault reconciles the vault cluster's state to the spec specified by vr

*: implement how to deploy an etcd cluster #3

*: implement how to deploy an etcd cluster #3

Conversation

fanminshi commented Apr 9, 2018

fanminshi commented Apr 9, 2018

Choose a reason for hiding this comment

fanminshi Apr 10, 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

fanminshi commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasbro17 commented Apr 10, 2018

fanminshi Apr 10, 2018 •

edited

Loading