Delete left over pilots

[wallrj]

Contrary to the original idea of creating pilots during scale-out and deleting them during scale-in,
I've instead extended the existing pilot sync method to delete pilots with an index higher than the Replicas value of the corresponding StatefulSet.

Fixes: #322

Release note:

NONE

[jetstack-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: kragniz

Assign the PR to them by writing /assign @kragniz in a comment when ready.

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

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

[wallrj]

/retest

[munnerz]

I'm not sure we should be using currentReplicas here:

FIELD: currentReplicas <integer>

DESCRIPTION:
     currentReplicas is the number of Pods created by the StatefulSet controller
     from the StatefulSet version indicated by currentRevision.

During an update of the statefulset, this number will represent the total number of out of date replicas (i.e. it will gradually reduce from N to 0 during the upgrade, and then once the upgrade is complete will reset back to N).

FWIW, here are the various StatefulSet.status fields:

FIELDS:
   collisionCount	<integer>
     collisionCount is the count of hash collisions for the StatefulSet. The
     StatefulSet controller uses this field as a collision avoidance mechanism
     when it needs to create the name for the newest ControllerRevision.

   conditions	<[]Object>
     Represents the latest available observations of a statefulset's current
     state.

   currentReplicas	<integer>
     currentReplicas is the number of Pods created by the StatefulSet controller
     from the StatefulSet version indicated by currentRevision.

   currentRevision	<string>
     currentRevision, if not empty, indicates the version of the StatefulSet
     used to generate Pods in the sequence [0,currentReplicas).

   observedGeneration	<integer>
     observedGeneration is the most recent generation observed for this
     StatefulSet. It corresponds to the StatefulSet's generation, which is
     updated on mutation by the API Server.

   readyReplicas	<integer>
     readyReplicas is the number of Pods created by the StatefulSet controller
     that have a Ready Condition.

   replicas	<integer> -required-
     replicas is the number of Pods created by the StatefulSet controller.

   updateRevision	<string>
     updateRevision, if not empty, indicates the version of the StatefulSet used
     to generate Pods in the sequence [replicas-updatedReplicas,replicas)

   updatedReplicas	<integer>
     updatedReplicas is the number of Pods created by the StatefulSet controller
     from the StatefulSet version indicated by updateRevision.

I'd guess either replicas, or currentReplicas+updatedReplicas is what we need to use?

We could alternatively always base it on the statefulset.spec.replicas field - as once the StatefulSet has been scaled down, the cluster itself should already be in a state whereby it can scale down (i.e. all data evacuated from the node). Perhaps I've missed an edge case here though?

[munnerz]

Sorry - I'd left this review sitting around in my browser cache/github somewhere!

[munnerz]

Why do we need a list of pods if we're just creating a Pilot per StatefulSet replica? Can we not infer the name of each pod from the name of the StatefulSet and its index?

[munnerz]

I'm confused by this function.

First, we get the statefulset. Then we list all pods managed by that set and create Pilots for each of them.
We then list all Pilots for the StatefulSet, and iterate over them, determining whether they should or shouldn't exist and deleting them.

Instead, we could create a function getExpectedPilotsForStatefulSet which could return a []string of Pilots that are expected to exist.

We could then get a list of all existing pilots for the StatefulSet, 'diff' the two lists into a 'to delete' and 'to create', and then go ahead and delete/create the Pilots.

I've done a similar thing in cert-manager: https://github.com/jetstack/cert-manager/blob/master/pkg/issuer/acme/prepare.go#L107-L108

Here, we first run a function that creates a list of challenges that should exist. We then pass that list to a cleanup function, which deletes any challenges that do exist but are not expected to exist anymore.

The later code then goes ahead and creates/realises those challenges (i.e. the 'pilot creation' part).

[wallrj]

You're right, I'd over complicated it.
Was trying to imagine (and handle) a case where the statefulset had been scaled in but the pods hadn't yet been deleted but the pilot had been accidentally deleted.
I wanted to re-create the pilot in that case, so that the pod (c* node) could discover its desired configuration and decommission and remove itself from the cluster.
But discussing with @kragniz and I now realise that the statefulset.spec.replicas value doesn't change until C* node / Pod / Pilot has been decomissioned and reported that fact via the pilot status.

I've re-implemented as you suggested, as a couple of lists (actual / expected pilots) and a couple of sets (pilotsToCreate / pilotsToDelete).

See what you think.

[munnerz]

Can you collapse these sorts of things into single lines. My own text editor does line wrapping the works for me, and this makes it harder to quickly scan code for what is going on. What is actually a log message looks like an actual block of code when written like this and indented.

[wallrj]

Yeah, but my editor doesn't wrap these lines (nor does Github in its web based diffs).
I find it much easier to read the code this way.

[munnerz]

if/else is a pattern avoided in Go (and linters will warn you about doing it).

This could be rewritten to:

if err != nil {
    if k8sErrors.IsNotFound(err) {
        continue
    }
    return errors.Wrapf(err, "unable to delete pilot %s/%s", pilot.Namespace, pilot.Name)
}

glog.V(4).Infof("Deleted pilot %s/%s.", pilot.Namespace, pilot.Name)

[wallrj]

Agreed. Done.

[munnerz]

Why does this return (int, bool)? int, error makes sense (parsing the string may fail) - but I can't work out how the bool is used.

A comment to explain this function's behaviour would also help 😄

[munnerz]

Ah I see the bool means 'found' - an error would would make more sense here, and would allow you to provide more context on why something failed.

[wallrj]

I removed this function, it's no longer needed.

[munnerz]

Can you fix up grouping of imports here?

import (
//stdlib

//external pkgs

//navigator pkgs
)

[wallrj]

Seems a bit nit picky 😉
My editor isn't set up to automatically format the code that way.
If it's important, then suggest we make it part of verify-lint.sh

[wallrj]

Ok.

• updated to latest goimports
• installed https://direnv.net/ and https://github.com/wbolster/emacs-direnv
• added an wrapper script in github.com/jetstack/bin/goimports which calls goimports as goimports -local github.com/jetstack/

But it still doesn't format the imports as we want them.

There's a discussion of this here: golang/go#20818

Also tried https://github.com/aristanetworks/goarista/tree/master/cmd/importsort, but that sorts them differently again.

I've fixed it by hand ☹️

[munnerz]

It'd be great if we can avoid such a huge stack of objects declared here. It's really difficult when looking at a particular test case to see what's being tested (i.e. the inputs and outputs).

Not sure if there's an easy way to do this, but right now I am definitely struggling to understand all this.

I managed to get around this in cert-manager by accepting a pointer to the fixture itself as part of a 'PreFn': https://github.com/jetstack/cert-manager/blob/master/pkg/issuer/acme/http/ingress_test.go#L21-L54

If not, comments throughout all of this may help make this more readable 😄

[wallrj]

With the revised implementation I found I could remove many of these.
I tried to make the variable names self-explanatory.
Not sure what more I can add with comments.

[munnerz]

Flatten to a single line

[munnerz]

Also why is this being set to nil? Doesn't make sense to me? 🤔

[wallrj]

Exists selections can not have a value.

[munnerz]

Brief comment would be great

[wallrj]

Done.

[wallrj]

/retest

[munnerz]

It'd be good to see the API surface for this use annotations that aren't so cassandra specific. I think general creation of Pilots for pods should be fairly generic between implementations, else administrators will have to understand behaviour for each controllers implementations when debugging problems.

Perhaps this could be refactored to have some form of watch on pods with a create-pilot label to trigger creation of a Pilot resource?

We can include all the existing create/delete logic, but have it triggered via watches on pods (i.e. its workqueue should be filled with Pod resources)

Then, each controller implementation (elasticsearch or cassandra) can just update the Pilot with any specific configuration that's required.

To do this, we'll need to somehow access the replicas field of the pod-owning resource (e.g. StatefulSet). If we expand beyond StatefulSets in future, the logic here can be extended to use either the 'scale' subresource or some other kind of polymorphic client.

• If a pod is created with the required label, a blank Pilot resource is 'ensured' (i.e. create if does not exist)
• If a Pilot is deleted, trigger a resync of the pod it was created for

This means we create Pilots when Pods are created, and we delete them when Pods are deleted (provided their index < the replicas count on the owner). We can then also consider adding a 5m delay or similar on deletion, if required (which would apply universally across controllers)

[munnerz]

This appears to be set, but not actually used (at least as part of this PR).

It also could introduce some issues around generalising this code between controllers if we ever have a controller that doesn't use a StatefulSet.

Can we remove it for now in favour of discussion at a later date of storing the index vs the pod name on the Pilot?

[wallrj]

Supplanted by #353