Refactor Elasticsearch node pool controller into Actions structures #241

munnerz · 2018-02-06T11:36:28Z

What this PR does / why we need it:

This gives us a generic way to implement 'Actions' against a given 'State' structure, and additionally introduces a unit testing framework that can be reused between controllers for testing Actions.

See #228

fixes #215

Special notes for your reviewer:

Stacks on #240, #239

Release note:

Updated Elasticsearch controller that carefully manages upgrade rollouts

munnerz · 2018-02-06T17:04:13Z

/hold

wallrj

Hey @munnerz

I really like this change.
It's taken me a long time to absorb it and I haven't finished the review, but I'm being kicked out of the office now.
So here are some comments to be considering.

wallrj · 2018-02-06T13:39:36Z

pkg/controllers/elasticsearch/actions/exclude_shards.go

+
+func (e *ExcludeShards) Execute(state *controllers.State) error {
+	return nil
+}


Seems incomplete.

👍 removed

wallrj · 2018-02-06T13:42:08Z

pkg/controllers/elasticsearch/actions/scale.go

+func (s *Scale) Execute(state *controllers.State) error {
+	if s.NodePool == nil || s.Cluster == nil {
+		return fmt.Errorf("internal error: nodepool and cluster must be set")
+	}


I feel like this should never happen in production and if these are somehow nil, we should let it panic and restart.
I don't see any such checks in the other actions....so far.

I'd rather not have any 'leaf' bits of code throw panics. If the call-site wants to panic as a result of an error, then sure. But this really should be set. I was just trying to be safe here and catch it, but you are correct it should be all or nothing. I'll update other actions to do the same...

I can't see any way that we'd create a scale action with a nil nodepool or nil cluster pointer.
IMO If we're going to do this check it should be done in the nextAction function before we create the action rather than having to do it everytime we attempt to use the pointer.
Happy to change it in a followup branch if you agree.

This is more just about making our structures safe - imo, anywhere you touch an exported field you should probably check if it's nil. We could 'gate' these fields with a NewConstructor as an alternative.

wallrj · 2018-02-06T13:45:42Z

pkg/controllers/elasticsearch/actions/scale.go

+	statefulSet, err := state.StatefulSetLister.StatefulSets(s.Cluster.Namespace).Get(statefulSetName)
+	if k8sErrors.IsNotFound(err) {
+		state.Recorder.Eventf(s.Cluster, core.EventTypeWarning, "Err"+s.Name(), "Cannot get statefulset %q for node pool %q: %v", statefulSetName, s.NodePool.Name, err)
+		return nil


Shouldn't this return the error?
If we've got the state changes correct, we should have always created a statefulset before we attempt to scale, right?

We return nil to prevent an endless loop in the case where the cluster doesn't exist. Reasoning being that when the StatefulSet is added to the listers store, it will trigger the informers OnAdd to fire again, thus re-triggering this.

we should have always created a statefulset before we attempt to scale, right?

Race issues could cause the StatefulSet to no longer exist after the initial check for it existing happens. Also the lister may be stale, which could cause a 404.

wallrj · 2018-02-06T13:56:27Z

pkg/controllers/elasticsearch/actions/scale.go

+	// TODO: not sure if we should treat nil as 1 or 0 instead of erroring
+	if currentReplicas == nil {
+		return fmt.Errorf("current number of replicas on statefulset cannot be nil")
+	}


statefulset controller seems to dereference it...which I think ends up being 0 for a nil pointer.

https://github.com/kubernetes/kubernetes/blame/master/pkg/controller/statefulset/stateful_set_control.go#L278

Dereferencing any nil pointer will cause a panic: https://play.golang.org/p/6eQA-QmxJWv

Right, sorry. That was a stupid comment. But in that case, I take it that the statefulset controller doesn't handle statefulSet.Spec.Replicas either.

In which case I assume that there is a validation / defaulting mechanism in the API server which checks for nil / missing replicas. Ensuring that it will always have a value by the time it reaches controllers.

wallrj · 2018-02-06T14:09:29Z

pkg/controllers/elasticsearch/actions/scale.go

+	// ensure that at least one data, ingest and master node exists already
+	if !utilapi.ContainsElasticsearchRole(s.NodePool.Roles, v1alpha1.ElasticsearchRoleData) {
+		return true, nil
+	}


It's a general point, but maybe it's useful to be able to scale down to 0.
We do this with GKE clusters, so why not with Navigator Clusters?

Perhaps - but this could create unstable clusters, and potentially then causing a lot of churn in control loops (Pilots might be updating their resources with error conditions frequently, causing the navigator control loop to keep resyncing).

I'm not against scale-to-zero, but I don't think it needs to be incorporated right now/as part of this PR?

wallrj · 2018-02-06T17:57:39Z

internal/test/util/contains.go

+		return false
+	}
+	return true
+}


👍 This looks useful.
Consider adding it to a public package so that we can use it in the implementation too.

These packages can be used from any subpackage of github.com/jetstack/navigator. This specifically stops anything outside of our repo from importing it. We can use this to convey the fact certain functions/interfaces, whilst exported, are NOT supported long-term. More info: https://docs.google.com/document/d/1e8kOo3r51b2BWtTs_1uADIA5djfXhPT36s6eHVRIvaU/edit

wallrj · 2018-02-06T17:59:36Z

internal/test/util/generate/generate.go

+			},
+		},
+	}
+}


👍 This looks like a really nice pattern for generating the resources we need in tests.
Is this what they do in kubernetes too?

Nope - it made things a lot cleaner though 😄

wallrj · 2018-02-06T18:01:05Z

pkg/apis/navigator/validation/elasticsearch.go

@@ -45,8 +45,7 @@ func ValidateElasticsearchClusterNodePool(np *navigator.ElasticsearchClusterNode
 		el = append(el, field.Invalid(fldPath.Child("replicas"), np.Replicas, "must be greater than zero"))
 	}
 	// TODO: call k8s.io/kubernetes/pkg/apis/core/validation.ValidateResourceRequirements on np.Resources
-	// this will require vendoring kubernetes/kubernetes and switching to use the corev1 ResourceRequirements
-	// struct
+	// this will require vendoring kubernetes/kubernetes.


❓ Might be simpler with latest version of dep which shouldn't need to vendor the entire kubernetes tree....assuming these validation functions don't have too many dependencies.

wallrj · 2018-02-06T18:08:51Z

pkg/controllers/elasticsearch/actions/create_nodepool.go

+	_, err = state.Clientset.AppsV1beta1().StatefulSets(toCreate.Namespace).Create(toCreate)
+	if err != nil {
+		return err
+	}


Should we use IsAlreadyExistsError(err) here and return nil in that case?
I feel like it might be good if Execute was guaranteed to be idempotent.
I feel like (not tested or verified) that there might be cases where we create the stateful set, but it doesn't get immediately listed and the reconciliation code elsewhere might calculate another create action.
In that case, we don't want to return an error because that will result in the cluster being put back in the queue.
Not sure, but worth discussing I think.

I'd rather hold on this one for now. It's an overbearing requirement to impose on all actions, and (right now) there would no noticeable gain. Right now, if the call site wants to check if a CreateNodePool failed because it already exists, they can call IsAlreadyExists themselves and take appropriate action.

Once we start implementing more features with Actions, I hope it'll become clearer whether we want to require all actions be idempotent.

wallrj · 2018-02-06T18:09:19Z

pkg/controllers/elasticsearch/actions/create_nodepool.go

-					Resources: apiv1.ResourceRequirements{
-						Requests: np.Resources.Requests,
-						Limits:   np.Resources.Limits,
+						InitialDelaySeconds: 240,


This figure changed from 60 to 240. How come?

During testing I was having issues where the liveness probe wasn't passing within 60s, causing crash loops.

Really, we should probably change the Elasticsearch Pilot liveness probe to just check that Pilot is running (i.e. Pilot should always return true, and not attempt to query ES itself)

I think that's what it does already.

func (p *Pilot) LivenessCheck() probe.Check { return probe.CombineChecks( func() error { return nil }, ) }

jetstack-ci-bot · 2018-02-07T08:00:52Z

@munnerz PR needs rebase

wallrj

Some further comments.

wallrj · 2018-02-07T10:18:34Z

internal/test/unit/framework/state_fixture_test.go

+	// }
+	// if len(pilots) != 0 {
+	// 	t.Errorf("Expected pilot to be deleted, but Pilot is still present in lister: %v", pilots)
+	// }


Although, if we're waiting for longer than the resync period in WaitForResync above, the lister should (most of the time) not list the deleted object.
How flaky is this in practice?

wallrj · 2018-02-07T10:27:43Z

internal/test/util/contains.go

+	elemsLen := elemsV.Len()
+	if listLen < elemsLen {
+		return false
+	}


This means that ContainsAll([1], [1,1]) == false, which might cause confusion.
It's a shame golang doesn't allow you to create generic sets.

I guess this is just a property of the function - it could be said that [1] does not contain all the elements of [1, 1] (depending on how you're looking at it)

wallrj · 2018-02-07T10:44:07Z

pkg/controllers/elasticsearch/cluster_control.go

+	}
+	if err != nil {
+		return nil, err
+	}


What happens if someone accidentally deletes a Navigator statefulset during an ongoing version upgrade from ES v2 to ES v4?

Navigator will attempt to recreate the statefulset, but

with a pod template containing the new version and

start the newer version of the pilot and

start the new version of ES

without starting the intermediate ES v3 pilot and waiting for it to complete data migration.

Perhaps instead, the CreateNodePool action needs to look at the last reported ESCluster.Status.Version (if it exists) and start that instead.

Does a similar problem exist for an ongoing scale down?
If the statefulset is deleted, a new one will be recreated with the desired (smaller) replica count whereas it should probably be recreated with the last known good replica count and wait for the shards to be drained from the chosen node?

This all supposes that the PVs from the deleted statefulset would be re-used by the new statefulset. Is that how it works?

Or perhaps in the case where a statefulset goes missing, navigator should just stop, log an error event and wait for a human to repair it.

And then, what would those steps be?
If a CassandraCluster resource is accidentally deleted, you still have all the data on, now orphan, PVs.
How would you recreate the cluster and safely re-use the data on those PVs?

What happens if someone accidentally deletes a Navigator statefulset during an ongoing version upgrade from ES v2 to ES v4?

This scenario is definitely something we are susceptible to. I think some examination of how the Deployment controller behaves in this case would be useful. If it does manage to handle it gracefully, my guess is because it stores a lot of information about the upgrade/rolling update back on the Deployment resource itself. We can definitely do something similar here.. borrowing a lot of logic from the Deployment controller itself.

I'm thinking (/hoping) that this doesn't need to be a release blocker however, as this actions change is quite architectural and it'd be good to get Cassandra working in a similar way too.

Does a similar problem exist for an ongoing scale down?

Yes

This all supposes that the PVs from the deleted statefulset would be re-used by the new statefulset. Is that how it works?

Yep, that is how it works

And then, what would those steps be?
If a CassandraCluster resource is accidentally deleted, you still have all the data on, now orphan, PVs.
How would you recreate the cluster and safely re-use the data on those PVs?

Creating a statefulset with the same name (and same PVC template) should do it 😄

wallrj · 2018-02-07T11:00:54Z

pkg/controllers/elasticsearch/cluster_control.go

+		NavigatorClientset: e.navigatorClient,
+		Recorder:           e.recorder,
+		StatefulSetLister:  e.statefulSetLister,
+		ConfigMapLister:    e.configMapLister,


Do any of the actions need to list ConfigMaps in this branch?
I can't find it used in any of the actions.

Not right now - once this is merged I'll be incorporating it though.

munnerz · 2018-02-13T13:46:25Z

@wallrj PR ready for another review. I've addressed changes in later commits to make it easier to see my newest changes.

/hold cancel

wallrj · 2018-02-15T09:48:29Z

internal/test/util/contains.go

+
+func FuzzyEqualLists(left, right interface{}) bool {
+	return ContainsAll(left, right) && ContainsAll(right, left)
+}


wallrj

Thanks @munnerz

The changes all look good to me and I'm looking forward to adapting the cassandra controller to this pattern.
I made a few further comments, but nothing that needs to be addressed here.

Merge at will!

/lgtm

jetstack-bot · 2018-02-15T10:13:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [wallrj]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jetstack-ci-bot · 2018-02-15T10:13:57Z

/test all [submit-queue is verifying that this PR is safe to merge]

jetstack-ci-bot · 2018-02-15T10:40:38Z

Automatic merge from submit-queue.

jetstack-bot added the release-note label Feb 6, 2018

jetstack-bot requested a review from wallrj February 6, 2018 11:36

jetstack-bot added the size/XXL label Feb 6, 2018

munnerz force-pushed the refactor-es-controller branch 2 times, most recently from ed142dc to 82941f9 Compare February 6, 2018 11:49

munnerz mentioned this pull request Feb 6, 2018

Add ginkgo e2e testing framework #242

Closed

munnerz force-pushed the refactor-es-controller branch 2 times, most recently from bc8dfa0 to 71296a7 Compare February 6, 2018 14:14

jetstack-bot added the do-not-merge/hold label Feb 6, 2018

munnerz mentioned this pull request Feb 6, 2018

Refactor Elasticsearch controller to use a semi-state machine design & add ginkgo based e2e tests #228

Closed

wallrj suggested changes Feb 6, 2018

View reviewed changes

munnerz force-pushed the refactor-es-controller branch from cd43d5a to 9c61c2b Compare February 6, 2018 20:37

jetstack-ci-bot added the needs-rebase label Feb 7, 2018

wallrj suggested changes Feb 7, 2018

View reviewed changes

James Munnelly added 7 commits February 13, 2018 11:33

Refactor Elasticsearch node pool controller into Actions structures

e9688f4

Run dep ensure

7d6a16d

Replace SuccessSync e2e test with CreateNodePool check

63ff69c

Fix Version comparison since changes to pilot status version

851c49e

Ensure Pilot version matches desired cluster version

791c720

Catch the case where the next partition to use is -1

2f2efd6

Only set version if non-empty in Pilot generator

9c6a9d7

munnerz force-pushed the refactor-es-controller branch from 9c61c2b to 9c6a9d7 Compare February 13, 2018 11:37

jetstack-ci-bot removed the needs-rebase label Feb 13, 2018

James Munnelly added 5 commits February 13, 2018 13:36

Stop statefixture after start

45fdd02

Refactor comments on state fixture test

03e567d

Add FuzzyEqualLists function

3fc2689

Remove unimplemented exclude shards action

1037359

Remove unused Message() function on Action interface

8373f85

jetstack-bot removed the do-not-merge/hold label Feb 13, 2018

James Munnelly added 2 commits February 13, 2018 13:53

Use FuzzyEqualLists in scale_test.go

ea88b71

Add test names to TestDeterminePilotsToBeRemoved

12e64a9

munnerz force-pushed the refactor-es-controller branch from 2c726fb to 12e64a9 Compare February 13, 2018 13:53

munnerz added this to the v0.1 milestone Feb 13, 2018

munnerz added the area/elasticsearch label Feb 13, 2018

wallrj reviewed Feb 15, 2018

View reviewed changes

wallrj approved these changes Feb 15, 2018

View reviewed changes

jetstack-bot assigned wallrj Feb 15, 2018

jetstack-bot added the lgtm label Feb 15, 2018

jetstack-bot added the approved label Feb 15, 2018

jetstack-ci-bot merged commit 56634a6 into jetstack:master Feb 15, 2018

wallrj mentioned this pull request Feb 16, 2018

Refactor Cassandra cluster controller into Actions #253

Closed

wallrj mentioned this pull request May 15, 2018

Support ElasticsearchCluster scale down #98

Closed

Refactor Elasticsearch node pool controller into Actions structures #241

Refactor Elasticsearch node pool controller into Actions structures #241

Conversation

munnerz commented Feb 6, 2018 • edited Loading

munnerz commented Feb 6, 2018

wallrj 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

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

jetstack-ci-bot commented Feb 7, 2018

wallrj 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

munnerz commented Feb 13, 2018

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

jetstack-bot commented Feb 15, 2018

jetstack-ci-bot commented Feb 15, 2018

jetstack-ci-bot commented Feb 15, 2018

munnerz commented Feb 6, 2018 •

edited

Loading