Add CreateNodePool and ScaleOut actions for Cassandra #256

wallrj · 2018-02-22T16:37:45Z

These become the only changes supported by the Cassandra controller until ScaleIn and CassandraUpgrade actions are implemented in followup branches.

Fixes: #253

Release note:

NONE

jetstack-ci-bot · 2018-02-27T07:53:04Z

@wallrj PR needs rebase

jetstack-ci-bot · 2018-02-27T22:17:07Z

@wallrj PR needs rebase

* In jetstack#256 I want to refactor nodepool.Sync so that its only responsibility is to update the NodePool status. * And the seed labelling seems like it is a separate concern that can live in its own module. * With its own unit tests.

Automatic merge from submit-queue. Move seed pod labelling to a separate controller * In #256 I want to refactor nodepool.Sync so that its only responsibility is to update the NodePool status. * And the seed labelling seems like it is a separate concern that can live in its own module. * With its own unit tests. **Release note**: ```release-note NONE ```

wallrj · 2018-03-02T15:48:39Z

rebased

wallrj · 2018-03-02T16:31:35Z

/retest

kragniz

This generally looks good, I'm happy to merge

kragniz · 2018-03-07T11:30:21Z

pkg/apis/navigator/v1alpha1/types.go

@@ -44,7 +44,7 @@ type CassandraClusterSpec struct {
 // CassandraClusterNodePool describes a node pool within a CassandraCluster.
 type CassandraClusterNodePool struct {
 	Name     string `json:"name"`
-	Replicas int64  `json:"replicas"`
+	Replicas int32  `json:"replicas"`


Why are these changed to int32s?

I changed it to match the type of StatefulSet.Status.ReadyReplicas.

https://github.com/kubernetes/kubernetes/blob/v1.9.3/pkg/apis/apps/types.go#L171

Missing corresponding change in the internal api version

munnerz · 2018-03-13T14:22:38Z

internal/test/util/generate/generate.go

+	if actual.Namespace != expected.Namespace {
+		t.Errorf("Namespace %q != %q", expected.Namespace, actual.Namespace)
+	}
+	if expected.Replicas != nil {


What if expected.Replicas == nil but actual.Spec.Replicas != nil?

munnerz · 2018-03-13T14:25:20Z

internal/test/util/generate/generate.go

@@ -118,3 +120,46 @@ func StatefulSet(c StatefulSetConfig) *apps.StatefulSet {
 		},
 	}
 }
+
+func AssertStatefulSetMatches(t *testing.T, expected StatefulSetConfig, actual *apps.StatefulSet) {


Not sure if we should put assertions into this package. generate.AssertStatefulSetMatches doesn't really 'flow' when read out loud.

Perhaps create internal/test/util/assert and call assert.StatefulSetMatches?

Also, given this is used for unit tests, can we just use reflect.DeepEqual instead? This method could easily return true for two very different statefulsets with its current implementation, which if reused could cause confusion (and also, if this function is extended in future to check more fields, it may cause tests to fail, thus making this not very reusable)

Removed that assertion and simplified the tests.

munnerz · 2018-03-13T14:26:23Z

pkg/apis/navigator/v1alpha1/types.go

@@ -44,7 +44,7 @@ type CassandraClusterSpec struct {
 // CassandraClusterNodePool describes a node pool within a CassandraCluster.
 type CassandraClusterNodePool struct {
 	Name     string `json:"name"`
-	Replicas int64  `json:"replicas"`
+	Replicas int32  `json:"replicas"`


Missing corresponding change in the internal api version

munnerz · 2018-03-13T14:27:33Z

pkg/controllers/cassandra/actions/create_nodepool.go

+	ss := nodepool.StatefulSetForCluster(a.Cluster, a.NodePool)
+	_, err := s.Clientset.AppsV1beta1().StatefulSets(ss.Namespace).Create(ss)
+	if k8sErrors.IsAlreadyExists(err) {
+		return nil


Should return an error here always if err != nil (these actions shouldn't be idempotent, in favour of nextAction choosing what to execute next sensibly)

There are a few good reasons for them to be idempotent:

the CassandraCluster.Status.NodePools map (upon which this action is calculated) may not have been updated.

future actions (e.g. ScaleIn) may perform a more complex sequence of steps which need to be "ratcheted" through, by repeated calls to ScaleIn.Execute, e.g.

Decommission node

Decrease the statefulset replica count.
In which case each of those intermediate steps must be idempotent and in which case the final step may as well be idempotent.

The lister used to calculate the action may not have the most up-to-date resources, so it appears there isn't a StatefulSet, until you attempt to create it, which fails with AlreadyExists (although I might be making that up, because the current implementation doesn't check the lister first)

On the whole, it makes sense to me that these actions should all be idempotent, but happy to discuss further.

munnerz · 2018-03-13T14:30:49Z

pkg/controllers/cassandra/actions/scaleout.go

+	if ss.Spec.Replicas == nil || *ss.Spec.Replicas < a.NodePool.Replicas {
+		ss.Spec.Replicas = &a.NodePool.Replicas
+		_, err = s.Clientset.AppsV1beta1().StatefulSets(ss.Namespace).Update(ss)
+		if err == nil {


For convention with everywhere else, let's return err if it does not equal nil, and if not log the event and return nil.

i.e.

if err != nil { return err } <log event> return nil

munnerz · 2018-03-13T14:54:33Z

pkg/controllers/cassandra/nodepool/nodepool.go

@@ -34,77 +36,65 @@ func NewControl(
 	}
 }

-func (e *defaultCassandraClusterNodepoolControl) removeUnusedStatefulSets(
+func (e *defaultCassandraClusterNodepoolControl) clusterStatefulSets(


Can you collapse the method signature to a single line?

munnerz · 2018-03-13T14:58:55Z

pkg/controllers/cassandra/nodepool/nodepool.go

+			}
+		} else {
+			delete(cluster.Status.NodePools, np.Name)
+		}


Can you refactor out this else? (turn it into an if with continue after it). Reduces the amount of nesting going on here.

The whole function has been simplified and merged in a separate PR #299

munnerz · 2018-03-13T15:00:58Z

pkg/controllers/cassandra/nodepool/nodepool.go

+				cluster.Status.NodePools[np.Name] = nps
+			}
+			if nps.ReadyReplicas != ss.Status.ReadyReplicas {
+				nps.ReadyReplicas = ss.Status.ReadyReplicas


It took a little while to spot that this is where we actually set the fields in the node pool status - perhaps create a function statusForNodePool to handle this? (which returns a NodePoolStatus structure)

munnerz · 2018-03-13T15:02:10Z

pkg/controllers/cassandra/nodepool/nodepool.go

+// (the statefulset has been removed by a DeleteNodePool action - not yet implemented)
+func (e *defaultCassandraClusterNodepoolControl) Sync(cluster *v1alpha1.CassandraCluster) error {
+	if cluster.Status.NodePools == nil {
+		cluster.Status.NodePools = map[string]v1alpha1.CassandraClusterNodePoolStatus{}


Not 100% sure how maps are ordered when serialised - perhaps we shouldn't be using a map here at all in favour of a slice that has a name field?

This version of the function was just wrong.
Instead, in #299 I re-populate the CassandraCluster.Status.NodePools each time, based on the statefulsets that are owned by the cluster, and which have the expected name.

munnerz · 2018-03-13T15:05:37Z

pkg/controllers/cassandra/testing/gen.go

+		}
+		// 20% chance of ScaleOut
+		if rand.Intn(4) == 0 {
+			np.Replicas++


Will increase it by 1 ever result in it being particularly large? Should we not just set this to some random value output by the rng?

(maybe you are following some best practice I am not aware of though!)

the initial value is between 0-5 (see code above) so it should never be more than 6.
This is a way to simulate an occasional difference between the Spec.NodePool.Replicas and Status.NodePools.ReadyReplicas

jetstack-ci-bot · 2018-03-13T21:42:22Z

@wallrj PR needs rebase

…ned by the cluster * Update E2E tests to check the CassandraCluster status rather than checking statefulset status. * Issue an API UpdateStatus with the CassandraCluster.Status after running all control.Sync methods. * Modify the RBAC role to allow the controller to update CassandraCluster.Status * Add a statefulset informer callback to trigger a controller sync when our statefulsets change. Extracted from: jetstack#256 Part of: jetstack#253

wallrj · 2018-03-24T18:52:44Z

Noticed a weird E2E test error:

W0324 14:27:19.643] + kubectl --namespace=test-cassandra-1521900884-833 exec cass-test-ringnodes-0 -- bash -c 'kill -SIGSTOP -- $(ps -u cassandra -o pid=) && ps faux'
I0324 14:27:20.340] rpc error: code = 2 desc = oci runtime error: exec failed: container_linux.go:262: starting container process caused "process_linux.go:86: adding pid 17816 to cgroups caused \"failed to write 17816 to cgroup.procs: write /sys/fs/cgroup/cpu,cpuacct/kubepods/poddc1388be-2f6e-11e8-8210-5254001464f2/f148ee0881cb5644368505fc324414543c01c6046f6e9850c52cc4f48ce72801/cgroup.procs: invalid argument\""

https://jetstack-build-infra.appspot.com/build/jetstack-logs/pr-logs/pull/jetstack_navigator/256/navigator-e2e-v1-7/1071/

Maybe caused by opencontainers/runc#1326 or moby/moby#31230

jetstack-bot · 2018-03-27T11:41:55Z

@wallrj: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
navigator-e2e-v1-10	`414be89`	link	`/test e2e v1.10`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

* These become the only changes supported by the Cassandra controller. * ScaleIn and CassandraUpgrade actions will be implemented in followup branches. * Some initial documentation on supported configuration changes. Fixes: jetstack#253

munnerz · 2018-03-27T16:56:01Z

/lgtm
/approve

jetstack-bot · 2018-03-27T16:56:15Z

[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.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [munnerz]

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

jetstack-bot added release-note-none do-not-merge/work-in-progress size/XXL labels Feb 22, 2018

jetstack-bot requested a review from munnerz February 22, 2018 16:37

wallrj changed the title ~~WIP: Cassandra Actions~~ Add CreateNodePool and ScaleOut actions for Cassandra Feb 26, 2018

jetstack-bot removed the do-not-merge/work-in-progress label Feb 26, 2018

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

wallrj mentioned this pull request Feb 27, 2018

Changing CassandraCluster persistence settings should not be allowed #233

Closed

wallrj force-pushed the 286-cassandra-actions branch from d1d6c3f to 42f02a1 Compare February 27, 2018 13:34

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

wallrj force-pushed the 286-cassandra-actions branch from 42f02a1 to 5c14447 Compare February 27, 2018 13:39

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

wallrj mentioned this pull request Feb 28, 2018

Move seed pod labelling to a separate controller #270

Merged

wallrj force-pushed the 286-cassandra-actions branch from 5c14447 to 46059e8 Compare March 2, 2018 15:45

jetstack-bot added size/XL and removed size/XXL labels Mar 2, 2018

jetstack-ci-bot removed the needs-rebase label Mar 2, 2018

wallrj mentioned this pull request Mar 6, 2018

WIP: cassandra upgrade #277

Closed

kragniz approved these changes Mar 7, 2018

View reviewed changes

wallrj force-pushed the 286-cassandra-actions branch from 46059e8 to ca8ae71 Compare March 8, 2018 10:36

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

munnerz requested review from munnerz and kragniz and removed request for kragniz March 13, 2018 10:34

munnerz assigned wallrj Mar 13, 2018

munnerz removed their request for review March 13, 2018 10:34

kragniz approved these changes Mar 13, 2018

View reviewed changes

munnerz reviewed Mar 13, 2018

View reviewed changes

jetstack-ci-bot added the needs-rebase label Mar 13, 2018

kragniz mentioned this pull request Mar 16, 2018

Cassandra scale in action #289

Open

wallrj mentioned this pull request Mar 21, 2018

Update CassandraCluster.Status.NodePools #299

Merged

wallrj force-pushed the 286-cassandra-actions branch from 490933c to 8b5ab5c Compare March 24, 2018 14:04

jetstack-bot removed the needs-rebase label Mar 24, 2018

wallrj force-pushed the 286-cassandra-actions branch from 8b5ab5c to 49253c8 Compare March 24, 2018 18:07

wallrj force-pushed the 286-cassandra-actions branch from 49253c8 to 414be89 Compare March 27, 2018 10:58

wallrj added 2 commits March 27, 2018 16:36

Make service control follow the same interface pattern as other controls

ff7e744

wallrj force-pushed the 286-cassandra-actions branch from 414be89 to 36b773f Compare March 27, 2018 15:39

jetstack-bot assigned munnerz Mar 27, 2018

jetstack-bot added the lgtm label Mar 27, 2018

jetstack-bot added the approved label Mar 27, 2018

jetstack-bot merged commit d0e60bd into jetstack:master Mar 27, 2018

wallrj deleted the 286-cassandra-actions branch March 27, 2018 18:11

wallrj mentioned this pull request Apr 9, 2018

Cassandra ScaleOut should only change the statefulset when all pods are ready #316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CreateNodePool and ScaleOut actions for Cassandra #256

Add CreateNodePool and ScaleOut actions for Cassandra #256

wallrj commented Feb 22, 2018 •

edited

Loading

jetstack-ci-bot commented Feb 27, 2018

jetstack-ci-bot commented Feb 27, 2018

wallrj commented Mar 2, 2018

wallrj commented Mar 2, 2018

kragniz left a comment

kragniz Mar 7, 2018

wallrj Mar 8, 2018

munnerz Mar 13, 2018

munnerz Mar 13, 2018

munnerz Mar 13, 2018

wallrj Mar 27, 2018

munnerz Mar 13, 2018

munnerz Mar 13, 2018

wallrj Mar 24, 2018 •

edited

Loading

munnerz Mar 13, 2018

munnerz Mar 13, 2018

munnerz Mar 13, 2018

wallrj Mar 24, 2018

munnerz Mar 13, 2018

munnerz Mar 13, 2018

wallrj Mar 24, 2018

munnerz Mar 13, 2018

munnerz Mar 13, 2018

wallrj Mar 24, 2018

jetstack-ci-bot commented Mar 13, 2018

wallrj commented Mar 24, 2018

jetstack-bot commented Mar 27, 2018 •

edited

Loading

munnerz commented Mar 27, 2018

jetstack-bot commented Mar 27, 2018

Add CreateNodePool and ScaleOut actions for Cassandra #256

Add CreateNodePool and ScaleOut actions for Cassandra #256

Conversation

wallrj commented Feb 22, 2018 • edited Loading

jetstack-ci-bot commented Feb 27, 2018

jetstack-ci-bot commented Feb 27, 2018

wallrj commented Mar 2, 2018

wallrj commented Mar 2, 2018

kragniz 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

wallrj Mar 24, 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

jetstack-ci-bot commented Mar 13, 2018

wallrj commented Mar 24, 2018

jetstack-bot commented Mar 27, 2018 • edited Loading

munnerz commented Mar 27, 2018

jetstack-bot commented Mar 27, 2018

wallrj commented Feb 22, 2018 •

edited

Loading

wallrj Mar 24, 2018 •

edited

Loading

jetstack-bot commented Mar 27, 2018 •

edited

Loading