diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go index 3b62a9e8..4f1f4e39 100644 --- a/controllers/cluster_controller.go +++ b/controllers/cluster_controller.go @@ -24,7 +24,10 @@ import ( "context" "encoding/json" "fmt" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer/yaml" "k8s.io/apimachinery/pkg/types" "regexp" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -41,6 +44,7 @@ import ( "github.com/FoundationDB/fdb-kubernetes-operator/internal" "sigs.k8s.io/controller-runtime/pkg/predicate" + sigyaml "sigs.k8s.io/yaml" fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2" "github.com/FoundationDB/fdb-kubernetes-operator/pkg/podclient" @@ -87,6 +91,7 @@ type FoundationDBClusterReconciler struct { // ClusterLabelKeyForNodeTrigger if set will trigger a reconciliation for all FoundationDBClusters that host a Pod // on the affected node. ClusterLabelKeyForNodeTrigger string + decodingSerializer runtime.Serializer } // NewFoundationDBClusterReconciler creates a new FoundationDBClusterReconciler with defaults. @@ -95,6 +100,7 @@ func NewFoundationDBClusterReconciler(podLifecycleManager podmanager.PodLifecycl PodLifecycleManager: podLifecycleManager, } r.PodClientProvider = r.newFdbPodClient + r.decodingSerializer = yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) return r } @@ -508,7 +514,7 @@ func (r *FoundationDBClusterReconciler) newFdbPodClient(cluster *fdbv1beta2.Foun // updateOrApply updates the status either with server-side apply or if disabled with the normal update call. func (r *FoundationDBClusterReconciler) updateOrApply(ctx context.Context, cluster *fdbv1beta2.FoundationDBCluster) error { if r.ServerSideApply { - // TODO(johscheuer): We have to set the TypeMeta otherwise the Patch command will fail. This is the rudimentary + // We have to set the TypeMeta otherwise the Patch command will fail. This is the rudimentary // support for server side apply which should be enough for the status use case. The controller runtime will // add some additional support in the future: https://github.com/kubernetes-sigs/controller-runtime/issues/347. patch := &fdbv1beta2.FoundationDBCluster{ @@ -523,7 +529,22 @@ func (r *FoundationDBClusterReconciler) updateOrApply(ctx context.Context, clust Status: cluster.Status, } - return r.Status().Patch(ctx, patch, client.Apply, client.FieldOwner("fdb-operator")) //, client.ForceOwnership) + // We are converting the patch into an *unstructured.Unstructured to remove fields that use a default value. + // If we are not doing this, empty (nil) fields will be evaluated as if they were set by the default value. + // In some previous testing we discovered some issues with that behaviour. With the *unstructured.Unstructured + // we make sure that only fields that are actually set will be applied. + outBytes, err := sigyaml.Marshal(patch) + if err != nil { + return err + } + + unstructuredPatch := &unstructured.Unstructured{} + _, _, err = r.decodingSerializer.Decode(outBytes, nil, unstructuredPatch) + if err != nil { + return err + } + + return r.Status().Patch(ctx, unstructuredPatch, client.Apply, client.FieldOwner("fdb-operator")) //, client.ForceOwnership) } return r.Status().Update(ctx, cluster) diff --git a/e2e/Makefile b/e2e/Makefile index 718b1115..9a420787 100644 --- a/e2e/Makefile +++ b/e2e/Makefile @@ -38,6 +38,13 @@ FEATURE_DNS?=false FEATURE_LOCALITIES?=false # Allows to specify the tag for a specific FDB version. Format is 7.1.57:7.1.57-testing,7.3.35:7.3.35-debugging FDB_VERSION_TAG_MAPPING?= +# If the FEATURE_SERVER_SIDE_APPLY environment variable is not defined the test suite will be randomly enable (1) or disable (0) +# the server side apply feature. +ifndef FEATURE_SERVER_SIDE_APPLY + FEATURE_SERVER_SIDE_APPLY=$(shell shuf -i 0-1 -n 1) +else + FEATURE_SERVER_SIDE_APPLY=false +endif # Make bash pickier about errors. SHELL=/bin/bash -euo pipefail @@ -136,4 +143,5 @@ nightly-tests: run --storage-engine=$(STORAGE_ENGINE) \ --fdb-version-tag-mapping=$(FDB_VERSION_TAG_MAPPING) \ --unified-fdb-image=$(UNIFIED_FDB_IMAGE) \ + --feature-server-side-apply=$(FEATURE_SERVER_SIDE_APPLY) \ | grep -v 'constructing many client instances from the same exec auth config can cause performance problems during cert rotation' &> $(BASE_DIR)/../logs/$<.log diff --git a/e2e/fixtures/fdb_operator_client.go b/e2e/fixtures/fdb_operator_client.go index fe2c92ce..a04c97a6 100644 --- a/e2e/fixtures/fdb_operator_client.go +++ b/e2e/fixtures/fdb_operator_client.go @@ -485,7 +485,9 @@ spec: - --max-concurrent-reconciles=5 - --zap-log-level=debug - --minimum-required-uptime-for-cc-bounce=60s - #- --server-side-apply +{{ if .EnableServerSideApply }} + - --server-side-apply +{{ end }} # We are setting low values here as the e2e test are taking down processes multiple times # and having a high wait time between recoveries will increase the reliability of the cluster but also # increase the time our e2e test take. @@ -518,6 +520,8 @@ type operatorConfig struct { MemoryRequests string // Defines the user that runs the current e2e tests. User string + // EnableServerSideApply if true, the operator will make use of server side apply. + EnableServerSideApply bool } // SidecarConfig represents the configuration for a sidecar. This can be used for templating. @@ -603,15 +607,16 @@ func (factory *Factory) getOperatorConfig(namespace string) *operatorConfig { } return &operatorConfig{ - OperatorImage: factory.GetOperatorImage(), - SecretName: factory.GetSecretName(), - BackupSecretName: factory.GetBackupSecretName(), - Namespace: namespace, - SidecarVersions: factory.GetSidecarConfigs(), - ImagePullPolicy: factory.getImagePullPolicy(), - CPURequests: cpuRequests, - MemoryRequests: MemoryRequests, - User: factory.options.username, + OperatorImage: factory.GetOperatorImage(), + SecretName: factory.GetSecretName(), + BackupSecretName: factory.GetBackupSecretName(), + Namespace: namespace, + SidecarVersions: factory.GetSidecarConfigs(), + ImagePullPolicy: factory.getImagePullPolicy(), + CPURequests: cpuRequests, + MemoryRequests: MemoryRequests, + User: factory.options.username, + EnableServerSideApply: factory.options.featureOperatorServerSideApply, } } diff --git a/e2e/fixtures/options.go b/e2e/fixtures/options.go index 948e2c3e..ba65c337 100644 --- a/e2e/fixtures/options.go +++ b/e2e/fixtures/options.go @@ -31,30 +31,31 @@ import ( // FactoryOptions defines the (command line) options that are support for the e2e test cases. type FactoryOptions struct { - namespace string - chaosNamespace string - context string - fdbImage string - unifiedFDBImage string - sidecarImage string - operatorImage string - dataLoaderImage string - registry string - fdbVersion string - username string - storageClass string - upgradeString string - cloudProvider string - clusterName string - storageEngine string - fdbVersionTagMapping string - enableChaosTests bool - enableDataLoading bool - cleanup bool - featureOperatorDNS bool - featureOperatorLocalities bool - featureOperatorUnifiedImage bool - dumpOperatorState bool + namespace string + chaosNamespace string + context string + fdbImage string + unifiedFDBImage string + sidecarImage string + operatorImage string + dataLoaderImage string + registry string + fdbVersion string + username string + storageClass string + upgradeString string + cloudProvider string + clusterName string + storageEngine string + fdbVersionTagMapping string + enableChaosTests bool + enableDataLoading bool + cleanup bool + featureOperatorDNS bool + featureOperatorLocalities bool + featureOperatorUnifiedImage bool + featureOperatorServerSideApply bool + dumpOperatorState bool } // BindFlags binds the FactoryOptions flags to the provided FlagSet. This can be used to extend the current test setup @@ -192,6 +193,12 @@ func (options *FactoryOptions) BindFlags(fs *flag.FlagSet) { true, "defines if the operator tests should print the state of the cluster and the according Pods for better debugging.", ) + fs.BoolVar( + &options.featureOperatorServerSideApply, + "feature-server-side-apply", + false, + "defines if the operator should make use of server side apply.", + ) fs.StringVar(&options.clusterName, "cluster-name", "",