ko-364 fix managed by #84
Conversation
| var service corev1.Service | ||
| service.ObjectMeta.Namespace = dc.Namespace | ||
| service.ObjectMeta.Labels = labels | ||
| service.Spec.Selector = labels | ||
| service.Spec.Selector = selector |
There was a problem hiding this comment.
We were including the managed-by label in the selector which is both not necessary and unhelpful for when we update the managed-by value to cass-operator (it will cause the services to, temporarily, not have any pods associated with them).
| // label of "cassandra-operator" we have since fixed this to be "cass-operator", | ||
| // but unfortunately, we cannot modify the labels in the volumeClaimTemplates of a | ||
| // StatefulSet. Consequently, we must preserve the old labels in this case. | ||
| usesDefunct := usesDefunctPvcManagedByLabel(sts) |
There was a problem hiding this comment.
Yep. It's dumb.
| if shouldUpdateLabels { | ||
| logger.Info("Updating labels", | ||
| // if we found the service already, check if they need updating | ||
| if !resourcesHaveSameHash(currentService, desiredSvc) { |
There was a problem hiding this comment.
It turns out this change isn't necessary because we removed managed-by from the selector of our services. However, it's still a good idea so here it is.
| @@ -0,0 +1,4 @@ | |||
| apiVersion: v2 | |||
There was a problem hiding this comment.
I need access to the 1.1.0 helm chart somehow. It was easiest to just include it in the testdata. Open to suggestions.
There was a problem hiding this comment.
we could get it with git, but this is fine with me
johntrimble
requested review from
bradfordcp,
jimdickinson,
respringer and
sandoichi
as code owners
| @@ -1,7 +1,7 @@ | |||
| // Copyright DataStax, Inc. | |||
| // Please see the included license file for details. | |||
|
|
|||
| package config_change | |||
| package config_change_condition | |||
| fmt.Sprintf("--namespace=%s", namespace), | ||
| } | ||
|
|
||
| func buildOverrideArgs(overrides map[string]string) []string { |
| ManagedByLabelValue = "cassandra-operator" | ||
| ManagedByLabel = "app.kubernetes.io/managed-by" | ||
| ManagedByLabelValue = "cass-operator" | ||
| ManagedByLabelDefunctValue = "cassandra-operator" |
| @@ -104,18 +114,52 @@ func newNamespacedNameForStatefulSet( | |||
| } | |||
| } | |||
|
|
|||
| // Create a statefulset object for the Datacenter. | |||
| func newStatefulSetForCassandraDatacenterWithDefunctPvcManagedBy( | |||
There was a problem hiding this comment.
maybe add a comment for why this is here - your other comment is really great - just edited the intro...
// We have to account for the fact that they might use the old managed-by label value
// (oplabels.ManagedByLabelDefunctValue) for CassandraDatacenters originally
// created in version 1.1.0 or earlier.
| @@ -211,7 +212,7 @@ func TestCassandraDatacenter_buildPodTemplateSpec_labels_merge(t *testing.T) { | |||
|
|
|||
| expected := dc.GetRackLabels("testrack") | |||
| expected[api.CassNodeState] = stateReadyToStart | |||
| expected["app.kubernetes.io/managed-by"] = "cassandra-operator" | |||
There was a problem hiding this comment.
I feel so much shame right now, but this is how name changes go - one leak and the costs add up later
| shouldUpdateLabels := !reflect.DeepEqual(currentLabels, desiredLabels) | ||
| if shouldUpdateLabels { | ||
| logger.Info("Updating labels", | ||
| // if we found the service already, check if they need updating |
There was a problem hiding this comment.
getting the services updating in a proper/idiomatic-for-us way was a great side benefit of this work, great job!
No description provided.