Create Cassandra Pilot resources #153

wallrj · 2017-11-27T17:33:00Z

For every pod in the nodepool, create a corresponding Pilot resource.
Delete Pilot resources for which there is no corresponding Pod.
Ignore Pods that are not owned by the cluster.
Stop and error if we encounter Pilots with expected name, but not owned by the cluster.

Fixes: #152

Release note:

NONE

wallrj · 2017-11-28T09:38:39Z

This isn't working yet.
Forgot that the ownerreferences are to the statefulset not to the cassandracluster.
Looking at whether it's possible to set additional ownerreferences from the statefulset template.

wallrj · 2017-12-14T09:02:53Z

/test e2e

wallrj · 2017-12-14T09:06:51Z

I've updated this PR against master.
There are two other PRs stacked on top of this (#142, #162)
But I've marked those as WIP to avoid confusion.
Ready for review.
/cc @munnerz

wallrj · 2017-12-14T10:08:01Z

/test e2e

munnerz

Looks good - only a few changes to make!

munnerz · 2017-12-14T11:05:08Z

pkg/controllers/cassandra/pilot/pilot.go

+			}
+		}
+	}
+	return nil


So I'm not too sure if we should be doing this right now (deleting 'unused' pilot resources). It's difficult to define unused, and in fact, the lack of a pod does not immediately imply that the Pilot resource is no longer in use. I'd rather do something along the lines of waiting N seconds since the last heartbeat (provided no corresponding pod exists) before deleting.

Right now the Elasticsearch controller doesn't actually clean up old Pilots, it just cleans up old StatefulSets (i.e. ones no longer specified as a nodepool on the ElasticsearchCluster resource). Should we make these the same? Consistency in this sort of thing probably makes it easier to reason about. At some point I think we can refactor the Pilot control loop to make it mostly generic between DB types.

So I'm not too sure if we should be doing this right now (deleting 'unused' pilot resources). It's difficult to define unused, and in fact, the lack of a pod does not immediately imply that the Pilot resource is no longer in use. I'd rather do something along the lines of waiting N seconds since the last heartbeat (provided no corresponding pod exists) before deleting.

But what's the bug that the heartbeat / delay would fix?
If there is a bug, then I can add the heartbeat / delay in a followup branch.
But for now, removing unused Pilot resources seems like good housekeeping.

The heartbeat is a suggestion for how it can be handled, but as you say, should be in a follow up.

I think that we can't say the lack of pod = the pilot doesn't exist. In the ES controller for example, the 'master' Pilot uses Pilot resources to determine whether a cluster is in a state ready to scale down for example. This scenario, as far as I can tell, will cause failures if we auto-delete Pilots if no Pod exists:

User requests scale down of cluster

Navigator updates a Pilot resource to 'decommission' it

Because the user is a sadist, or because something happened, the pod that is being decommissioned gets deleted.

This should cause the StatefulSet controller to re-create the pod, really, as the Pilot is not finished decommissioning so it needs to come back in order to finish

However, because we immediately deleted the Pilot resource upon the Pod disappearing, the 'knowledge' of those remaining documents/indices left on that node (i.e. the pilot.status.elasticsearch.documentCount field) has been lost.

So now our master Pilot is not aware of those extra documents, nor the fact that a Pilot is being decommissioned either (as that knowledge has also been lost).

At this point we've reached a bit of a weird state - especially if we consider that the only state about the state of the world is stored in the k8s api and anything in-memory should not be relied upon

I do agree we need to do house keeping, but I'm thinking we should consider how this is done carefully as 'unused' is a tricky term. Additionally, given the Pilot resource in the API is one of the users primary points for debugging issues with their clusters, by deleting the Pilots so frequently we are also destroying a lot of useful debugging information for users. I'd much prefer the behaviour between all DB types to be consistent - the first steps in debugging the ES cluster should be the same as Cassandra, and vice versa.

^ my only outstanding/current issue with this PR before I'm happy to merge 😄 .

Let me know if you disagree, but any change we make here needs to be consistent with ES too given this touches a user-facing resource (the Pilot resource)

It seems to me that in the scenario above,

Accidentally deleted pod gets restarted by StatefulSet Controller.

Pilot process waits for appearance of Pilot

Pilot starts ES sub-process

Pilot re-updates the Pilot status with document count 0

navigator controller waits for document count 0 and then decrements the Statefulset replicas count

and removes the drained pod and corresponding pilot.

But I agree that we should keep ES and Cass in sync, so I'll remove the Pilot cleanup code for now.

munnerz · 2017-12-14T11:14:47Z

pkg/controllers/cassandra/pilot/pilot.go

+	}
+	desiredPilot = existingPilot.DeepCopy()
+	updatePilotForCluster(cluster, pod, desiredPilot)
+	_, err = client.Update(desiredPilot)


So from what I can see here, this will cause Navigator to go into a sync loop, continuously updating the Pilot resource. We don't ever check if the Pilot is already reconciled (e.g. it's spec is up to date), so we end out updating to contain the same spec as is already there. This will still however increment the ResourceVersion, thus causing this loop to be triggered once again (and so on).

There are a couple of ways (that I can think of) to deal with this:

Perform a reflect.DeepEqual on the pilot.Spec before calling Update. This has the downside of being inefficient (DeepEqual is an expensive call), however it is accurate and easy to do.

Write a hash of the spec into the annotations of the Pilot, so that we can quickly compare the old and new hashes.

Manually compare each field, but this is tedious and error prone.

Can you think of anything else here?

Additionally, we need to update the strategy.go file for Cassandra resources to not perform updates to the cassandracluster.status block when plain Update is called (and instead, UpdateStatus should be used if the Status block needs updating).

Right now, each time Update is called we will wipe out all fields in Status, causing Pilots and Navigator to fight with each other indefinitely (causing more loops)

Write a hash of the spec into the annotations of the Pilot, so that we can quickly compare the old and new hashes.

I've gone with that option. I found the ES hashing code and adapted that. Also added labels to the hash since the controller updates those too.

Right now, each time Update is called we will wipe out all fields in Status, causing Pilots and Navigator to fight with each other indefinitely (causing more loops)

I don't think that's the case. I'm taking the latest Pilot from the lister and then updating labels and in future, spec Not replacing Pilot.Status.

munnerz · 2017-12-14T11:26:42Z

pkg/controllers/util_api.go

+	appslisters "k8s.io/client-go/listers/apps/v1beta1"
+)
+
+func PodControlledByCluster(


Can we also update the Elasticsearch controller to use this function too? This version doesn't depend on any types like *ElasticsearchCluster like the one the the controller currently uses

munnerz · 2017-12-14T11:29:23Z

pkg/controllers/cassandra/pilot/pilot.go

+	pilot := &v1alpha1.Pilot{}
+	ownerRefs := pilot.GetOwnerReferences()
+	ownerRefs = append(ownerRefs, util.NewControllerRef(cluster))
+	pilot.SetOwnerReferences(ownerRefs)


Don't need so many steps to do this bit - we aren't mutating an existing Pilot, but creating a new one instead (so don't need to use function accessors)

munnerz · 2017-12-14T11:29:52Z

pkg/controllers/cassandra/pilot/pilot.go

+) *v1alpha1.Pilot {
+	pilot.SetName(pod.GetName())
+	pilot.SetNamespace(cluster.GetNamespace())
+	pilot.SetLabels(util.ClusterLabels(cluster))


We should be careful not to override any user provided labels, or labels that may have been added by the pilot (we don't do this right now, but we might do at some point)

* For every pod in the nodepool, create a corresponding Pilot resource. * Ignore Pods that are not owned by the cluster. * Stop and error if we encounter Pilots with expected name, but not owned by the cluster. Fixes: jetstack#152

wallrj · 2018-01-09T10:54:35Z

/test e2e

munnerz · 2018-01-09T11:35:31Z

/retest

wallrj · 2018-01-09T12:22:58Z

/retest

munnerz · 2018-01-09T15:06:39Z

/retest

munnerz · 2018-01-09T15:44:29Z

I restarted the build infra workers

/retest

munnerz · 2018-01-09T16:43:29Z

/retest

munnerz · 2018-01-09T16:44:50Z

pkg/controllers/cassandra/pilot/pilot.go

+		p.ObjectMeta,
+		p.Labels,
+	}
+	hasher := fnv.New32()


Can this be pulled out to be a global var (to save calling New32 each time)?

Happy to be a follow up as I'll be making similar changes in ES

munnerz · 2018-01-09T16:58:34Z

pkg/controllers/cassandra/pilot/pilot.go

+			c.statefulSets,
+		)
+		if err != nil {
+			return clusterPods, err


Should probably return nil, err here instead of a partial list of pods.

munnerz · 2018-01-09T17:01:15Z

pkg/controllers/cassandra/pilot/pilot.go

+	}
+	err = util.OwnerCheck(existingPilot, cluster)
+	if err != nil {
+		return err


TODO: should there be some way to detect that this function has failed because the existing pilot is owned by another cluster? (not important for merge)

munnerz · 2018-01-09T17:06:28Z

/lgtm
/approve

jetstack-ci-bot · 2018-01-09T17:06:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

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

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-01-09T17:06:39Z

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

jetstack-ci-bot · 2018-01-09T17:21:04Z

Automatic merge from submit-queue.

jetstack-bot added the release-note-none label Nov 27, 2017

jetstack-ci-bot assigned munnerz Nov 27, 2017

jetstack-bot added the size/L label Nov 27, 2017

wallrj changed the title ~~Create Cassandra Pilot resources~~ WIP: Create Cassandra Pilot resources Nov 28, 2017

jetstack-bot added the do-not-merge/work-in-progress label Nov 28, 2017

wallrj force-pushed the 152-cassandra-pilot-resource branch 3 times, most recently from 93abcb6 to 8d2c57d Compare November 28, 2017 22:55

wallrj changed the title ~~WIP: Create Cassandra Pilot resources~~ Create Cassandra Pilot resources Nov 28, 2017

jetstack-bot removed the do-not-merge/work-in-progress label Nov 28, 2017

wallrj force-pushed the 152-cassandra-pilot-resource branch from 8d2c57d to fd4494e Compare November 30, 2017 15:53

jetstack-bot added size/XL and removed size/L labels Nov 30, 2017

wallrj force-pushed the 152-cassandra-pilot-resource branch from fd4494e to faf367c Compare December 13, 2017 17:44

jetstack-bot added size/L and removed size/XL labels Dec 13, 2017

wallrj changed the base branch from 23-cassandra to master December 13, 2017 17:44

jetstack-bot requested a review from munnerz December 14, 2017 09:06

wallrj force-pushed the 152-cassandra-pilot-resource branch from faf367c to b56499f Compare December 14, 2017 09:47

munnerz reviewed Dec 14, 2017

View reviewed changes

wallrj force-pushed the 152-cassandra-pilot-resource branch from b56499f to cefc69a Compare January 2, 2018 21:37

jetstack-bot added size/XL and removed size/L labels Jan 2, 2018

wallrj force-pushed the 152-cassandra-pilot-resource branch from cefc69a to a619d42 Compare January 2, 2018 21:38

wallrj mentioned this pull request Jan 3, 2018

WIP: Use Kubernetes hashutils #182

Closed

Create Cassandra Pilot resources

a4c6d7a

* For every pod in the nodepool, create a corresponding Pilot resource. * Ignore Pods that are not owned by the cluster. * Stop and error if we encounter Pilots with expected name, but not owned by the cluster. Fixes: jetstack#152

wallrj force-pushed the 152-cassandra-pilot-resource branch from a619d42 to a4c6d7a Compare January 9, 2018 10:54

wallrj mentioned this pull request Jan 9, 2018

Intermittent TEST FAILURE: Elasticsearch pilot did not update the document count #185

Closed

munnerz reviewed Jan 9, 2018

View reviewed changes

jetstack-bot added the lgtm label Jan 9, 2018

jetstack-ci-bot added the approved label Jan 9, 2018

jetstack-ci-bot merged commit 0e6f38c into jetstack:master Jan 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Cassandra Pilot resources #153

Create Cassandra Pilot resources #153

wallrj commented Nov 27, 2017

wallrj commented Nov 28, 2017 •

edited

Loading

wallrj commented Dec 14, 2017

wallrj commented Dec 14, 2017

wallrj commented Dec 14, 2017

munnerz left a comment

munnerz Dec 14, 2017

wallrj Jan 2, 2018

munnerz Jan 8, 2018

munnerz Jan 8, 2018

wallrj Jan 9, 2018

munnerz Dec 14, 2017

munnerz Dec 14, 2017

wallrj Jan 2, 2018

wallrj Jan 2, 2018

munnerz Dec 14, 2017

wallrj Jan 2, 2018

munnerz Dec 14, 2017

wallrj Jan 2, 2018

munnerz Dec 14, 2017

wallrj Jan 2, 2018

wallrj commented Jan 9, 2018

munnerz commented Jan 9, 2018

wallrj commented Jan 9, 2018

munnerz commented Jan 9, 2018

munnerz commented Jan 9, 2018

munnerz commented Jan 9, 2018

munnerz Jan 9, 2018

munnerz Jan 9, 2018

munnerz Jan 9, 2018

munnerz commented Jan 9, 2018

jetstack-ci-bot commented Jan 9, 2018

jetstack-ci-bot commented Jan 9, 2018

jetstack-ci-bot commented Jan 9, 2018

Create Cassandra Pilot resources #153

Create Cassandra Pilot resources #153

Conversation

wallrj commented Nov 27, 2017

wallrj commented Nov 28, 2017 • edited Loading

wallrj commented Dec 14, 2017

wallrj commented Dec 14, 2017

wallrj commented Dec 14, 2017

munnerz 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

wallrj commented Jan 9, 2018

munnerz commented Jan 9, 2018

wallrj commented Jan 9, 2018

munnerz commented Jan 9, 2018

munnerz commented Jan 9, 2018

munnerz commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munnerz commented Jan 9, 2018

jetstack-ci-bot commented Jan 9, 2018

jetstack-ci-bot commented Jan 9, 2018

jetstack-ci-bot commented Jan 9, 2018

wallrj commented Nov 28, 2017 •

edited

Loading