Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ko-364 fix managed by #84

Merged
merged 15 commits into from
May 20, 2020
Merged
5 changes: 3 additions & 2 deletions mage/ginkgo/lib.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import (
)

const (
EnvNoCleanup = "M_NO_CLEANUP"
EnvNoCleanup = "M_NO_CLEANUP"
OperatorImage = "datastax/cass-operator:latest"
)

func duplicate(value string, count int) string {
Expand Down Expand Up @@ -364,7 +365,7 @@ func (ns *NsWrapper) WaitForOperatorReady() {
}

func (ns NsWrapper) HelmInstall(chartPath string) {
var overrides = map[string]string{"image": "datastax/cass-operator:latest"}
var overrides = map[string]string{"image": OperatorImage}
err := helm_util.Install(chartPath, "cass-operator", ns.Namespace, overrides)
mageutil.PanicOnError(err)
}
28 changes: 22 additions & 6 deletions mage/helm/lib.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,8 @@ import (
shutil "github.com/datastax/cass-operator/mage/sh"
)

func Install(chartPath string, releaseName string, namespace string, overrides map[string]string) error {
args := []string{
"install",
fmt.Sprintf("--namespace=%s", namespace),
}

func buildOverrideArgs(overrides map[string]string) []string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

args := []string{}
if overrides != nil && len(overrides) > 0 {
var overrideString = ""
for key, val := range overrides {
Expand All @@ -27,7 +23,27 @@ func Install(chartPath string, releaseName string, namespace string, overrides m

args = append(args, "--set", overrideString)
}
return args
}

func Install(chartPath string, releaseName string, namespace string, overrides map[string]string) error {
args := []string{
"install",
fmt.Sprintf("--namespace=%s", namespace),
}

args = append(args, buildOverrideArgs(overrides)...)
args = append(args, releaseName, chartPath)
return shutil.RunV("helm", args...)
}

func Upgrade(chartPath string, releaseName string, namespace string, overrides map[string]string) error {
args := []string{
"upgrade",
fmt.Sprintf("--namespace=%s", namespace),
}

args = append(args, buildOverrideArgs(overrides)...)
args = append(args, releaseName, chartPath)
return shutil.RunV("helm", args...)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
},
}

// NOTE: We do not currently watch PVC resources, but if we did, we'd 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.

err = c.Watch(
&source.Kind{Type: &appsv1.StatefulSet{}},
&handler.EnqueueRequestForOwner{
Expand Down
9 changes: 7 additions & 2 deletions operator/pkg/oplabels/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,19 @@
package oplabels

const (
ManagedByLabel = "app.kubernetes.io/managed-by"
ManagedByLabelValue = "cassandra-operator"
ManagedByLabel = "app.kubernetes.io/managed-by"
ManagedByLabelValue = "cass-operator"
ManagedByLabelDefunctValue = "cassandra-operator"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣 👍

)

func AddManagedByLabel(m map[string]string) {
m[ManagedByLabel] = ManagedByLabelValue
}

func AddDefunctManagedByLabel(m map[string]string) {
m[ManagedByLabel] = ManagedByLabelDefunctValue
}

func HasManagedByCassandraOperatorLabel(m map[string]string) bool {
v, ok := m[ManagedByLabel]
return ok && v == ManagedByLabelValue
Expand Down
50 changes: 47 additions & 3 deletions operator/pkg/reconciliation/constructor.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ func newServiceForCassandraDatacenter(dc *api.CassandraDatacenter) *corev1.Servi
Name: "mgmt-api", Port: 8080, TargetPort: intstr.FromInt(8080),
},
}

addHashAnnotation(service)

return service
}

Expand All @@ -63,6 +66,9 @@ func newSeedServiceForCassandraDatacenter(dc *api.CassandraDatacenter) *corev1.S

service.Spec.Selector = buildLabelSelectorForSeedService(dc)
service.Spec.PublishNotReadyAddresses = true

addHashAnnotation(service)

return service
}

Expand All @@ -72,6 +78,9 @@ func newAllPodsServiceForCassandraDatacenter(dc *api.CassandraDatacenter) *corev
service := makeGenericHeadlessService(dc)
service.ObjectMeta.Name = dc.GetAllPodsServiceName()
service.Spec.PublishNotReadyAddresses = true

addHashAnnotation(service)

return service
}

Expand All @@ -82,10 +91,11 @@ func newAllPodsServiceForCassandraDatacenter(dc *api.CassandraDatacenter) *corev
func makeGenericHeadlessService(dc *api.CassandraDatacenter) *corev1.Service {
labels := dc.GetDatacenterLabels()
oplabels.AddManagedByLabel(labels)
selector := dc.GetDatacenterLabels()
var service corev1.Service
service.ObjectMeta.Namespace = dc.Namespace
service.ObjectMeta.Labels = labels
service.Spec.Selector = labels
service.Spec.Selector = selector
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

service.Spec.Type = "ClusterIP"
service.Spec.ClusterIP = "None"
return &service
Expand All @@ -104,18 +114,52 @@ func newNamespacedNameForStatefulSet(
}
}

// Create a statefulset object for the Datacenter.
func newStatefulSetForCassandraDatacenterWithDefunctPvcManagedBy(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rackName string,
dc *api.CassandraDatacenter,
replicaCount int) (*appsv1.StatefulSet, error) {

return newStatefulSetForCassandraDatacenterHelper(rackName, dc, replicaCount, true)
}

func usesDefunctPvcManagedByLabel(sts *appsv1.StatefulSet) bool {
usesDefunct := false
for _, pvc := range sts.Spec.VolumeClaimTemplates {
value, ok := pvc.Labels[oplabels.ManagedByLabel]
if ok && value == oplabels.ManagedByLabelDefunctValue {
usesDefunct = true
break
}
}

return usesDefunct
}

func newStatefulSetForCassandraDatacenter(
rackName string,
dc *api.CassandraDatacenter,
replicaCount int) (*appsv1.StatefulSet, error) {

return newStatefulSetForCassandraDatacenterHelper(rackName, dc, replicaCount, false)
}

// Create a statefulset object for the Datacenter.
func newStatefulSetForCassandraDatacenterHelper(
rackName string,
dc *api.CassandraDatacenter,
replicaCount int,
useDefunctManagedByForPvc bool) (*appsv1.StatefulSet, error) {

replicaCountInt32 := int32(replicaCount)

// see https://github.com/kubernetes/kubernetes/pull/74941
// pvc labels are ignored before k8s 1.15.0
pvcLabels := dc.GetRackLabels(rackName)
oplabels.AddManagedByLabel(pvcLabels)
if useDefunctManagedByForPvc {
oplabels.AddDefunctManagedByLabel(pvcLabels)
} else {
oplabels.AddManagedByLabel(pvcLabels)
}

statefulSetLabels := dc.GetRackLabels(rackName)
oplabels.AddManagedByLabel(statefulSetLabels)
Expand Down
27 changes: 24 additions & 3 deletions operator/pkg/reconciliation/reconcile_racks.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,28 @@ func (rc *ReconciliationContext) CheckRackCreation() result.ReconcileResult {
return result.Continue()
}

func (rc *ReconciliationContext) desiredStatefulSetForExistingStatefulSet(sts *appsv1.StatefulSet, rackName string) (desiredSts *appsv1.StatefulSet, err error) {
dc := rc.Datacenter

// have to use zero here, because each statefulset is created with no replicas
// in GetStatefulSetForRack()
replicas := 0

// when Cass Operator was released, we accidentally used the incorrect managed-by
// 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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It's dumb.


if usesDefunct {
desiredSts, err = newStatefulSetForCassandraDatacenterWithDefunctPvcManagedBy(rackName, dc, replicas)
} else {
desiredSts, err = newStatefulSetForCassandraDatacenter(rackName, dc, replicas)
}

return
}

func (rc *ReconciliationContext) CheckRackPodTemplate() result.ReconcileResult {
logger := rc.ReqLogger
dc := rc.Datacenter
Expand All @@ -161,9 +183,8 @@ func (rc *ReconciliationContext) CheckRackPodTemplate() result.ReconcileResult {
}
statefulSet := rc.statefulSets[idx]

// have to use zero here, because each statefulset is created with no replicas
// in GetStatefulSetForRack()
desiredSts, err := newStatefulSetForCassandraDatacenter(rackName, dc, 0)
desiredSts, err := rc.desiredStatefulSetForExistingStatefulSet(statefulSet, rackName)

if err != nil {
logger.Error(err, "error calling newStatefulSetForCassandraDatacenter")
return result.Error(err)
Expand Down
28 changes: 16 additions & 12 deletions operator/pkg/reconciliation/reconcile_services.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
package reconciliation

import (
"reflect"

"github.com/datastax/cass-operator/operator/internal/result"
api "github.com/datastax/cass-operator/operator/pkg/apis/cassandra/v1beta1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"

"github.com/datastax/cass-operator/operator/pkg/utils"
)

func (rc *ReconciliationContext) CreateHeadlessServices() result.ReconcileResult {
Expand Down Expand Up @@ -90,19 +90,23 @@ func (rc *ReconciliationContext) CheckHeadlessServices() result.ReconcileResult
return result.Error(err)

} else {
// if we found the service already, check if the labels are right
currentLabels := currentService.GetLabels()
desiredLabels := desiredSvc.GetLabels()
shouldUpdateLabels := !reflect.DeepEqual(currentLabels, desiredLabels)
if shouldUpdateLabels {
logger.Info("Updating labels",
// if we found the service already, check if they need updating
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting the services updating in a proper/idiomatic-for-us way was a great side benefit of this work, great job!

if !resourcesHaveSameHash(currentService, desiredSvc) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resourceVersion := currentService.GetResourceVersion()
// preserve any labels and annotations that were added to the service post-creation
desiredSvc.Labels = utils.MergeMap(map[string]string{}, currentService.Labels, desiredSvc.Labels)
desiredSvc.Annotations = utils.MergeMap(map[string]string{}, currentService.Annotations, desiredSvc.Annotations)

logger.Info("Updating service",
"service", currentService,
"current", currentLabels,
"desired", desiredLabels)
currentService.SetLabels(desiredLabels)
"desired", desiredSvc)

desiredSvc.DeepCopyInto(currentService)

currentService.SetResourceVersion(resourceVersion)

if err := client.Update(rc.Ctx, currentService); err != nil {
logger.Error(err, "Unable to update service with labels",
logger.Error(err, "Unable to update service",
"service", currentService)
return result.Error(err)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright DataStax, Inc.
// Please see the included license file for details.

package config_change
package config_change_condition
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


import (
"fmt"
Expand Down
4 changes: 4 additions & 0 deletions tests/testdata/cass-operator-1.1.0-chart/Chart.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
apiVersion: v2
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need access to the 1.1.0 helm chart somehow. It was easiest to just include it in the testdata. Open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could get it with git, but this is fine with me

name: cass-operator
version: 1.0.0
description: Helm chart for Cass Operator.
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
creationTimestamp: null
name: {{ .Values.clusterRoleName }}
rules:
- apiGroups:
- admissionregistration.k8s.io
resources:
- validatingwebhookconfigurations
verbs:
- create
- get
- update
resourceNames:
- "cassandradatacenter-webhook-registration"
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: {{ .Values.clusterRoleBindingName }}
subjects:
- kind: ServiceAccount
name: {{ .Values.serviceAccountName }}
namespace: {{ .Release.Namespace }}
roleRef:
kind: ClusterRole
name: {{ .Values.clusterRoleName }}
apiGroup: rbac.authorization.k8s.io
Loading