StatefulSet Override #175

ChunyiLyu · 2020-06-30T08:10:19Z

This closes #143

Commits will be squashed when merging

Summary Of Changes

spec.override.statefulSet allows users to specify a patch to apply on top of generated StatefulSet definition

Added new types: EmbeddedObjectMeta, StatefulSetSpec, PodTemplateSpec, and PersistentVolumeClaim

Context

Why adding our own definitions of ObjectMeta, StatefulSetSpec? When using statefulset object directly in crd spec, embedded objectMeta get pruned (objectMeta is embedded in statefulSet itself, podTemplate, and VolumeClaimTemplates). ObjectMeta is treated as a "known type" in controller-tools, and it is generated with no fields when used in CRD spec (related code )
So with preserved-unknown-fields set to false, schema validation will prune objectMeta since it doesn't have any properties associate with it. This pruning behavior is due to preserveUnknownFields: false, which is going to be the default behavior for crd v1. There is a stale kubebuilder issue which proposes to skip pruning on embedded objectMeta, but due to its inactivity, we cannot rely on the change. The same problem is described in this kubebuilder issue.
For the custom embeddedObjectMeta object, I decided to only include fields Name, Namespace, Labels, and Annotations, I think it should be enough for most use cases.
I made all fields in the custom StatefulSetSpec to be optional. This gives us a better user experience since users will no longer be required to input serviceName, or selector to the sts spec when they try to modify other fields.

Testing

This feature is tested in unit, and integration tests. I decided not to add a system tests for it because verifying that the generated StatefulSet definition is correct is enough.

Required minimal version

We are using k8s api version 1.17 which means that k8s api object, such as corev1.PodTemplate has a new set of golang marker defined that is only added at version 1.16 and 1.17 for server side apply functionality. With this change, our CRD will longer deploy to a 1.15 cluster because k8s api does not recognize these new Golang markers.

We will need to document required version, as well as upgrade CI clusters that we are using.

Cannot be installed on k8s 1.18

why
issue in controller-tool which talks about the same issue

Example

To try this feature by hand, here is a rabbitmqcluster manifest which adds a new PVC, a mount, and uses override to overwrite the replicas count.

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name: sample
spec:
  replicas: 1
  resources: {}
  override:
    statefulSet:
      spec:
        replicas: 3
#        selector:
#          matchLabels:
#            app.kubernetes.io/name: sample
#        serviceName: sample-rabbitmq-headless
        template:
          spec:
            containers:
              - name: rabbitmq
                image: "rabbitmq:3.8"
                volumeMounts:
                  - name: persistence-foo
                    mountPath: /tmp/rabbitmq/test
        volumeClaimTemplates:
          - apiVersion: v1
            kind: PersistentVolumeClaim
            metadata:
              name: persistence
              namespace: rabbitmq-system
              labels:
                "app.kubernetes.io/name": "sample"
            spec:
              storageClassName: standard
              accessModes:
                - ReadWriteOnce
              resources:
                requests:
                  storage: 3Gi
          - apiVersion: v1
            kind: PersistentVolumeClaim
            metadata:
              name: persistence-foo
              namespace: rabbitmq-system
              labels:
                "app.kubernetes.io/name": "sample"
            spec:
              storageClassName: standard
              accessModes:
                - ReadWriteOnce
              resources:
                requests:
                  storage: 1Gi

- new crd properties: spec.override.statefulSet, which is our own definition of appsv1.StatefulSet, which allows users to provide a patch that will be applied on top of generated statefulset definition as an override. - new type EmbeddedObjectMeta, which is a partially metav1.ObjectMeta; it is used in StatefulSet, PodTemplateSpec, and PersistentVolumeClaim. - custome types are needed because of the pruning behavior of `preserveUnknownFields: false`, which is going to be the default behavior for crd v1. - all fields in the custom StatefulSetSpec are optional; including selector, serviceName...etc

api/v1beta1/rabbitmqcluster_types.go

j4mcs · 2020-07-02T10:17:59Z

controllers/rabbitmqcluster_controller_test.go

+							Spec: &rabbitmqv1beta1.StatefulSetSpec{
+								VolumeClaimTemplates: []rabbitmqv1beta1.PersistentVolumeClaim{
+									{
+										EmbeddedObjectMeta: rabbitmqv1beta1.EmbeddedObjectMeta{


Would it compile if (as part of the test) you built this as a native StatefulSet, marshalled it as JSON, then unmarshalled it as our custom struct?

The reason I ask is because it would be a much better developer experience for this to be opaque to the user. Obviously document that only certain fields in object meta are available. But I think it's a tough sell if they have to build a different struct than the native resource

I think it will compile James! I didn't look into this because I liked the flexibility of not having any required field in StatefulSetSpec with the current implementation. If I were to build and also patch as the native StatefulSetSpec, all the required fields will stay the same as k8s sts and people will need to specific things like serviceName, and selectors all the time again.

mkuratczyk · 2020-07-03T10:58:36Z

What I like:

no longer need to specify stuff I don't want to change (selector, serviceName)
I was able to make some modifications to the statefulset (ie. it works)

One limitations I see:

If I add a volume with a different name then I expect to add a new volume without changing the default one ("persistence"). I guess currently the whole volumeClaimTemplates is replaced with the value provided while ideally, this should happen on a per volume basis (add/replace volume with the same name)

Zerpet

It's looking good. I haven't manually tested this in Kind yet. I left a few comments; the main thing that poped out was the implementation of label/annotation override.

internal/resource/statefulset.go

controllers/rabbitmqcluster_controller_test.go

Zerpet · 2020-07-06T10:47:50Z

internal/resource/statefulset_test.go

+			It("overrides statefulSet.spec.selector", func() {
+				instance.Spec.Override.StatefulSet = &rabbitmqv1beta1.StatefulSet{
+					Spec: &rabbitmqv1beta1.StatefulSetSpec{
+						Selector: &metav1.LabelSelector{
+							MatchLabels: map[string]string{
+								"my-label": "my-label",
+							},
+						},
+					},
+				}
+
+				stsBuilder := cluster.StatefulSet()
+				obj, err := stsBuilder.Build()


I think we should make changes in the instance of the "resource builder" builder i.e. cluster.instance; same as we do in internal/resource/statefulset_test.go#L312, in the Update context. The end result will be the same because instance is a pointer used in cluster, but I think it reads better if we modify cluster.instance.

Would you mind if I address that as a separate PR? I think you have a valid point here, but the rest of the Build test specs are following the same pattern. I would like to have a separate PR to address all of these.

Sure, make it part of another refactoring PR 👌

Zerpet · 2020-07-06T10:54:09Z

internal/resource/statefulset_test.go

+									Resources: corev1.ResourceRequirements{
+										Requests: corev1.ResourceList{
+											corev1.ResourceStorage: *instance.Spec.Persistence.Storage,
+										},
+									},
+									StorageClassName: &storageClass,
+								},
+							},
+						},
+					},
+				}
+				cluster = &resource.RabbitmqResourceBuilder{
+					Instance: &instance,
+					Scheme:   scheme,
+				}
+				stsBuilder := cluster.StatefulSet()


Regenerating RabbitmqResourceBuilder doesn't feel right because the BeforeEach at Describe level is doing this for us. I believe we should modify cluster.instance with any properties we want to assert on, then call Build(), instead of regenerating the RabbitmqResourceBuilder entirely. Same as we do in Update context further down.

Would you mind if I address that as a separate PR together with the previous comment you made? Again, the rest of the Build test specs are following the same pattern. I would like to have a separate PR to address all of these.

internal/resource/statefulset.go

ChunyiLyu · 2020-07-07T08:44:47Z

@mkuratczyk

One limitations I see:

If I add a volume with a different name then I expect to add a new volume without changing the default one ("persistence"). I guess currently the whole volumeClaimTemplates is replaced with the value provided while ideally, this should happen on a per volume basis (add/replace volume with the same name)

The limitation is the "desired behavior" of PVC, because in the type StatefulSetSpec, there is no merge behavior specified for VolumeClaimTemplates , so the default behavior is a complete replace the list. We are defining our own types here, so we can have a custom behavior for PVCs to provide better user experience. However, I have to say that we will then deviate from k8s api, and without knowing why PVCs are specified like this, this is an unknown and risk for me.

Zerpet · 2020-07-09T11:24:53Z

internal/resource/statefulset_test.go

+			It("overrides statefulSet.spec.selector", func() {
+				instance.Spec.Override.StatefulSet = &rabbitmqv1beta1.StatefulSet{
+					Spec: &rabbitmqv1beta1.StatefulSetSpec{
+						Selector: &metav1.LabelSelector{
+							MatchLabels: map[string]string{
+								"my-label": "my-label",
+							},
+						},
+					},
+				}
+
+				stsBuilder := cluster.StatefulSet()
+				obj, err := stsBuilder.Build()


Sure, make it part of another refactoring PR 👌

ferozjilla

All looks good to me

- this is related and a followup to feature in PR #175 - SetControllerReference should be called after override is applied since the object's name and namespace could be modified by the override. - delete the unit test which asserts that sts's ns can be overwritten by override; the test is incorrect and will never pass, since cluster-scoped resources (sts in this case), can not be owned by objects in a other namespace.

- this is related to PR #175 - ObjectMeta.Labels and ObjectMeta.Annotations are merged when patching instead of replace.

- this is related and a followup to feature in PR #175 - SetControllerReference should be called after override is applied since the object's name and namespace could be modified by the override. - delete the unit test which asserts that sts's ns can be overwritten by override; the test is incorrect and will never pass, since cluster-scoped resources (sts in this case), can not be owned by objects in a other namespace.

- this is related to PR #175 - ObjectMeta.Labels and ObjectMeta.Annotations are merged when patching instead of replace.

ChunyiLyu force-pushed the override branch from 6387104 to 60c7dec Compare June 30, 2020 08:15

ChunyiLyu added 2 commits June 30, 2020 13:43

Extract applyStsOverride() helper functions

924c6d4

Add update integration tests for sts override

2f22674

coro reviewed Jul 1, 2020

View reviewed changes

api/v1beta1/rabbitmqcluster_types.go Outdated Show resolved Hide resolved

Modify override related fields descriptions

a3f792a

j4mcs reviewed Jul 2, 2020

View reviewed changes

Zerpet reviewed Jul 6, 2020

View reviewed changes

Update copyObjectMeta()

323260c

Zerpet approved these changes Jul 9, 2020

View reviewed changes

ChunyiLyu mentioned this pull request Jul 10, 2020

Add example for statefulSetOverride #195

Merged

j4mcs approved these changes Jul 10, 2020

View reviewed changes

ChunyiLyu merged commit db9724f into master Jul 10, 2020

j4mcs deleted the override branch July 10, 2020 10:06

ferozjilla approved these changes Jul 10, 2020

View reviewed changes

Zerpet mentioned this pull request Jul 10, 2020

Create a workflow to automatically import RabbitMQ definitions #104

Closed

ChunyiLyu mentioned this pull request Jul 10, 2020

Small refactorings for internal/resource #199

Merged

ChunyiLyu added a commit that referenced this pull request Jul 16, 2020

Labels and annotations override should be merged not replaced

05a2662

- this is related to PR #175 - ObjectMeta.Labels and ObjectMeta.Annotations are merged when patching instead of replace.

coro mentioned this pull request Aug 4, 2020

StatefulSet Override is not compatible with Kubernetes 1.18 #230

Closed

ChunyiLyu added a commit that referenced this pull request Aug 25, 2020

Labels and annotations override should be merged not replaced

57498a3

- this is related to PR #175 - ObjectMeta.Labels and ObjectMeta.Annotations are merged when patching instead of replace.

coro mentioned this pull request Oct 3, 2022

Use apps/v1 type for StatefulSetSpec in RabbitmqCluster override #1152

Closed

DanielePalaia mentioned this pull request Oct 26, 2022

Use apps/v1 type for StatefulSetSpec in RabbitmqCluster override #1170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StatefulSet Override #175

StatefulSet Override #175

ChunyiLyu commented Jun 30, 2020 •

edited

Loading

j4mcs Jul 2, 2020

j4mcs Jul 2, 2020

ChunyiLyu Jul 7, 2020

mkuratczyk commented Jul 3, 2020

Zerpet left a comment

Zerpet Jul 6, 2020

ChunyiLyu Jul 8, 2020

Zerpet Jul 9, 2020

Zerpet Jul 6, 2020

ChunyiLyu Jul 8, 2020 •

edited

Loading

ChunyiLyu commented Jul 7, 2020 •

edited

Loading

Zerpet Jul 9, 2020

ferozjilla left a comment

StatefulSet Override #175

StatefulSet Override #175

Conversation

ChunyiLyu commented Jun 30, 2020 • edited Loading

Summary Of Changes

Context

Testing

Required minimal version

Cannot be installed on k8s 1.18

Example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkuratczyk commented Jul 3, 2020

Zerpet 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

ChunyiLyu Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

ChunyiLyu commented Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

ferozjilla left a comment

Choose a reason for hiding this comment

ChunyiLyu commented Jun 30, 2020 •

edited

Loading

ChunyiLyu Jul 8, 2020 •

edited

Loading

ChunyiLyu commented Jul 7, 2020 •

edited

Loading