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

K8SSAND-1503 ⁃ Unable to remove the additional labels on the seed service through CR #330

Closed
hoyhbx opened this issue May 11, 2022 · 5 comments · Fixed by #344
Closed
Assignees
Labels
bug Something isn't working zh:Review

Comments

@hoyhbx
Copy link

hoyhbx commented May 11, 2022

What happened?
We found it impossible to remove the additional labels by unsetting the key-value pairs under the field spec.additionalServiceConfig.seedService.additionalLabels. We first added a label labelKey: labelValue under the additionalLabels field, and cass-operator correctly adds the label to the seed service. However, later we wanted to remove the labelKey: labelValue label from the seed service by deleting it from the CR. However, by doing so, cass-operator does not remove the additionalLabel from the seed service.

We also tried to change the labelKey: labelVaule to labelKey: null, but cass-operator still is not able to update the labels in seed service.

Did you expect to see something different?
We expected that the CR represents the desired state of the application, based on the declarative model of Kubernetes. So we expected that when remove the label from the spec.additionalServiceConfig.seedService.additionalLabels field, cass-operator would automatically remove this label from the seed service.

How to reproduce it (as minimally and precisely as possible):

  1. Deploy cass-operator and the server-storage storage class

  2. Deploy a basic CassandraDB using the example CR:
    kubectl apply -f example.yaml

    example.yaml
    apiVersion: cassandra.datastax.com/v1beta1
    kind: CassandraDatacenter
    metadata:
      name: cassandra-datacenter
    spec:
      clusterName: cluster1
      config:
        cassandra-yaml:
          authenticator: org.apache.cassandra.auth.PasswordAuthenticator
          authorizer: org.apache.cassandra.auth.CassandraAuthorizer
          role_manager: org.apache.cassandra.auth.CassandraRoleManager
        jvm-options:
          initial_heap_size: 800M
          max_heap_size: 800M
      managementApiAuth:
        insecure: {}
      serverType: cassandra
      serverVersion: 3.11.7
      size: 1
      storageConfig:
        cassandraDataVolumeClaimSpec:
          accessModes:
          - ReadWriteOnce
          resources:
            requests:
              storage: 3Gi
          storageClassName: server-storage
```
  1. Add additional labels to seed service by applying the cr1.yaml

    cr1.yaml
    apiVersion: cassandra.datastax.com/v1beta1
    kind: CassandraDatacenter
    metadata:
      name: cassandra-datacenter
    spec:
      additionalServiceConfig:
        seedService:
          additionalLabels:
            labelKey: labelValue
      clusterName: cluster1
      config:
        cassandra-yaml:
          authenticator: org.apache.cassandra.auth.PasswordAuthenticator
          authorizer: org.apache.cassandra.auth.CassandraAuthorizer
          role_manager: org.apache.cassandra.auth.CassandraRoleManager
        jvm-options:
          initial_heap_size: 800M
          max_heap_size: 800M
      managementApiAuth:
        insecure: {}
      serverType: cassandra
      serverVersion: 3.11.7
      size: 1
      storageConfig:
        cassandraDataVolumeClaimSpec:
          accessModes:
          - ReadWriteOnce
          resources:
            requests:
              storage: 3Gi
          storageClassName: server-storage
```
  1. Observe that the seed service now has the additional label labelKey: labelValue
  2. Delete the additionalLabels by applying the example CR again
  3. Observe that the seed service still has the label labelKey: labelValue

Environment

  • Cass Operator version:

    docker.io/k8ssandra/cass-operator:v1.10.3

    * Kubernetes version information:
Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.1", GitCommit:"5e58841cce77d4bc13713ad2b91fa0d961e69192", GitTreeState:"clean", BuildDate:"2021-05-12T14:11:29Z", GoVersion:"go1.16.3", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.0", GitCommit:"cb303e613a121a29364f75cc67d3d580833a7479", GitTreeState:"clean", BuildDate:"2021-04-08T16:25:06Z", GoVersion:"go1.16.1", Compiler:"gc", Platform:"linux/amd64"}
  • Kubernetes cluster kind:
minikube start --vm-driver=docker --cpus 4 --memory 4096 --kubernetes-version v1.21.0

Anything else we need to know?:
The root cause is inside the

desiredSvc.Labels = utils.MergeMap(map[string]string{}, currentService.Labels, desiredSvc.Labels)
where the labels in the desired service are merged into the existing service label map. The comment states that the reason of the merge is because cass-operator does not want to mess up the labels that are added by other sources.

A possible fix is to keep track of the labels that are added by the user using the spec.additionalServiceConfig.seedService.additionalLabels field, so that when users delete some labels from the field, cass-operator can delete these from the service.

The similar problem happens to the annotations too:

desiredSvc.Annotations = utils.MergeMap(map[string]string{}, currentService.Annotations, desiredSvc.Annotations)

┆Issue is synchronized with this Jira Task by Unito
┆friendlyId: K8SSAND-1503
┆priority: Medium

@hoyhbx hoyhbx added the bug Something isn't working label May 11, 2022
@sync-by-unito sync-by-unito bot changed the title Unable to remove the additional labels on the seed service through CR K8SSAND-1503 ⁃ Unable to remove the additional labels on the seed service through CR May 11, 2022
@jsanda
Copy link
Contributor

jsanda commented May 11, 2022

The MergeMap function performs a union. I think we can fix this by instead taking the intersection between currentService and desiredService and taking the values from desiredSvc.

@hoyhbx
Copy link
Author

hoyhbx commented May 12, 2022

I think this is actually a bit hard to fix cleanly.

I guess the MergeMap function was used because the design is not to remove labels set by others.
For example, if users specified additionalLabels as:

key1: value1
key2: value2

then some other sources added some labels to the service, so service has labels as:

key1: value1
key2: value2
externalKey: value3

When users change the additionalLabels to (removing the key2: value2)

key1: value1

The cass-operator should only remove the key2: value2 from the service labels, expecting:

key1: value1
externalKey: value3

I think to achieve this behavior, we need to keep track what labels are added through the CR's additionalLabels field.

@hoyhbx
Copy link
Author

hoyhbx commented May 12, 2022

I am not sure why

key1: null

would not update the labels though.

@jsanda
Copy link
Contributor

jsanda commented May 12, 2022

I may have spoken incorrectly when I suggested taking the intersection. It should be pretty straightforward though. The operator's job is to make sure that the actual labels match the desired labels as specified by additionalLabels. We also have to take into consideration the labels added by cass-operator. Here is an example of the labels added by the operator for a cluster named test:

app.kubernetes.io/instance: cassandra-test
app.kubernetes.io/managed-by: cass-operator
app.kubernetes.io/name: cassandra
app.kubernetes.io/version: 4.0.3
cassandra.datastax.com/cluster: test

Now let's say I configure additionalLabels as follows:

key1: value1
key2: value2

The service should then have:

app.kubernetes.io/instance: cassandra-test
app.kubernetes.io/managed-by: cass-operator
app.kubernetes.io/name: cassandra
app.kubernetes.io/version: 4.0.3
cassandra.datastax.com/cluster: test
key1: value1
key2: value2

Now let's update additionalLabels so it has:

key1: value1.1
key3: value3

3 changes have been made

  • The value of key1 has changed
  • key2 has been removed
  • key3 has been added

The actual service labels should then be as follows:

app.kubernetes.io/instance: cassandra-test
app.kubernetes.io/managed-by: cass-operator
app.kubernetes.io/name: cassandra
app.kubernetes.io/version: 4.0.3
cassandra.datastax.com/cluster: test
key1: value1.1
key3: value3

@hoyhbx
Copy link
Author

hoyhbx commented May 14, 2022

Yeah I totally agree

@burmanm burmanm added backport Feature requires backporting to older release branches and removed backport Feature requires backporting to older release branches labels Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working zh:Review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants