From 4a7b32977df53e3d4cc94290118e78052c512d34 Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Wed, 10 Jul 2024 09:36:40 +0200 Subject: [PATCH] fix(feature): only relies on desired state for reconcilation (#1103) This change ensures that management state is based exclusively on a resource defined in desired state, i.e. the one provided by the controller itself. This can be, but is not limited to, manifest files provided through embedded file system. Although the actual instance on the cluster (the target) might be in a different state, we intentionally do not address this scenario due to the lack of clear requirements on the extent to which users can modify such resource. This change allows to take control over previously undefined resources, so those which are created by `Feature` API without explicit management state defined. As long as original Feature does not have `Managed()` called in the builder chain, nor used manifests have the label explicitly defined the behaviour remain unchanged, that means the resources being part of such a feature are not managed and thus not reconciled after initial apply. `opendatahub.io/managed` is already exposed as an annotation and used by [other components](https://docs.redhat.com/en/documentation/red_hat_openshift_ai_self-managed/2.10/html/installing_and_uninstalling_openshift_ai_self-managed/preparing-openshift-ai-for-ibm-cpd_prepare-openshift-ai-ibm-cpd#editing-model-inferencing-configuration-ibm-cpd_prepare-openshift-ai-ibm-cpd). Follow up to #974 (cherry picked from commit e12d7d7911fe2eec27687a2eadadf6e2f588e9a8) --- pkg/deploy/deploy.go | 3 +- pkg/feature/manifest.go | 17 ------ pkg/feature/raw_resources.go | 54 +++++++++++++++++-- .../templates/local-gateway-svc.tmpl.yaml | 2 +- .../fixtures/templates/managed-svc.tmpl.yaml | 2 +- .../templates/unmanaged-svc.tmpl.yaml | 2 +- .../features/resources_int_test.go | 28 ++++++---- 7 files changed, 71 insertions(+), 37 deletions(-) diff --git a/pkg/deploy/deploy.go b/pkg/deploy/deploy.go index 5ea6cedbd7a..86ad9bce180 100644 --- a/pkg/deploy/deploy.go +++ b/pkg/deploy/deploy.go @@ -46,6 +46,7 @@ import ( "sigs.k8s.io/kustomize/kyaml/filesys" "github.com/opendatahub-io/opendatahub-operator/v2/components" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/plugins" ) @@ -222,7 +223,7 @@ func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Un if enabled { // Exception to not update kserve with managed annotation // do not reconcile kserve resource with annotation "opendatahub.io/managed: false" - if found.GetAnnotations()["opendatahub.io/managed"] == "false" && componentName == "kserve" { + if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" { return nil } return updateResource(ctx, cli, obj, found, owner, componentName) diff --git a/pkg/feature/manifest.go b/pkg/feature/manifest.go index 89b270675e5..4e4d4c2089a 100644 --- a/pkg/feature/manifest.go +++ b/pkg/feature/manifest.go @@ -12,8 +12,6 @@ import ( "github.com/ghodss/yaml" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" ) type Manifest interface { @@ -108,21 +106,6 @@ func (t *templateManifest) MarkAsManaged(objects []*unstructured.Unstructured) { } } -func markAsManaged(objs []*unstructured.Unstructured) { - for _, obj := range objs { - objAnnotations := obj.GetAnnotations() - if objAnnotations == nil { - objAnnotations = make(map[string]string) - } - - // If resource already has an annotation, leave it as defined - if _, exists := objAnnotations[annotations.ManagedByODHOperator]; !exists { - objAnnotations[annotations.ManagedByODHOperator] = "true" - obj.SetAnnotations(objAnnotations) - } - } -} - func loadManifestsFrom(fsys fs.FS, path string) ([]Manifest, error) { var manifests []Manifest diff --git a/pkg/feature/raw_resources.go b/pkg/feature/raw_resources.go index 2c5c3b0efda..892efee1e5e 100644 --- a/pkg/feature/raw_resources.go +++ b/pkg/feature/raw_resources.go @@ -56,14 +56,11 @@ func applyResources(ctx context.Context, cli client.Client, objects []*unstructu return nil } - isManaged, isAnnotated := target.GetAnnotations()[annotations.ManagedByODHOperator] - if isAnnotated && isManaged == "true" { - // reconcile the target since we manage it + if shouldReconcile(source) { if errUpdate := patchUsingApplyStrategy(ctx, cli, source, target); errUpdate != nil { - return fmt.Errorf("failed to update source %s/%s: %w", namespace, name, errUpdate) + return fmt.Errorf("failed to reconcile resource %s/%s: %w", namespace, name, errUpdate) } } - // source exists and is not manged, skip reconcile allowing users to tweak it } return nil @@ -105,3 +102,50 @@ func patchUsingMergeStrategy(ctx context.Context, cli client.Client, patch *unst return nil } + +// At this point we only look at what is defined for the resource in the desired state (source), +// so the one provided by the operator (e.g. embedded manifest files). +// +// Although the actual instance on the cluster (the target) might be in a different state, +// we intentionally do not address this scenario due to the lack of clear requirements +// on the extent to which users can modify it. Additionally, addressing this would +// necessitate extra reporting (e.g., via status.conditions on a FeatureTracker) +// to highlight discrepancies between the actual and desired state when users opt out +// of operator management/reconciliation. +func shouldReconcile(source *unstructured.Unstructured) bool { + if isManaged(source) { + return true + } + + if isUnmanaged(source) { + return false + } + + // In all the other cases preserve original behaviour + return false +} + +func markAsManaged(objs []*unstructured.Unstructured) { + for _, obj := range objs { + objAnnotations := obj.GetAnnotations() + if objAnnotations == nil { + objAnnotations = make(map[string]string) + } + + // If resource already has management mode defined, it should take precedence + if _, exists := objAnnotations[annotations.ManagedByODHOperator]; !exists { + objAnnotations[annotations.ManagedByODHOperator] = "true" + obj.SetAnnotations(objAnnotations) + } + } +} + +func isUnmanaged(obj *unstructured.Unstructured) bool { + managed, isDefined := obj.GetAnnotations()[annotations.ManagedByODHOperator] + return isDefined && managed == "false" +} + +func isManaged(obj *unstructured.Unstructured) bool { + managed, isDefined := obj.GetAnnotations()[annotations.ManagedByODHOperator] + return isDefined && managed == "true" +} diff --git a/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml b/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml index 92760cdd564..005db64383c 100644 --- a/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml +++ b/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml @@ -2,7 +2,7 @@ apiVersion: v1 kind: Service metadata: annotations: - test-annotation: "original-value" + test: "original-value" labels: experimental.istio.io/disable-gateway-port-translation: "true" name: knative-local-gateway diff --git a/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml b/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml index 56654bb8863..766eb0170e1 100644 --- a/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml +++ b/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml @@ -5,7 +5,7 @@ metadata: namespace: {{ .TargetNamespace }} annotations: opendatahub.io/managed: "true" - test-annotation: "original-value" + test: "original-value" spec: ports: - name: http2 diff --git a/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml b/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml index a4318bb3413..0e2fb8280d2 100644 --- a/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml +++ b/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml @@ -5,7 +5,7 @@ metadata: namespace: {{ .TargetNamespace }} annotations: opendatahub.io/managed: "false" - test-annotation: "original-value" + test: "original-value" spec: ports: - name: http2 diff --git a/tests/integration/features/resources_int_test.go b/tests/integration/features/resources_int_test.go index 7c7404cbfed..b26a0c3148a 100644 --- a/tests/integration/features/resources_int_test.go +++ b/tests/integration/features/resources_int_test.go @@ -2,6 +2,7 @@ package features_test import ( "context" + "fmt" "path" corev1 "k8s.io/api/core/v1" @@ -26,9 +27,9 @@ var _ = Describe("Applying and updating resources", func() { ) const ( - testAnnotationKey = "test-annotation" - newValue = "new-value" - originalValue = "original-value" + testKey = "test" + testNewValue = "new-value" + testOriginalValue = "original-value" ) BeforeEach(func(ctx context.Context) { @@ -71,7 +72,7 @@ var _ = Describe("Applying and updating resources", func() { ) // when - service.Annotations[testAnnotationKey] = newValue + service.Annotations[testKey] = testNewValue Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -80,7 +81,7 @@ var _ = Describe("Applying and updating resources", func() { updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, originalValue), + HaveKeyWithValue(testKey, testOriginalValue), ) }) @@ -101,7 +102,7 @@ var _ = Describe("Applying and updating resources", func() { // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc") Expect(err).ToNot(HaveOccurred()) - service.Annotations[testAnnotationKey] = newValue + service.Annotations[testKey] = testNewValue Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -110,7 +111,7 @@ var _ = Describe("Applying and updating resources", func() { updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc") Expect(err).ToNot(HaveOccurred()) Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, newValue), + HaveKeyWithValue(testKey, testNewValue), ) }) @@ -133,7 +134,7 @@ var _ = Describe("Applying and updating resources", func() { // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) - service.Annotations[testAnnotationKey] = newValue + service.Annotations[testKey] = testNewValue Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -142,7 +143,7 @@ var _ = Describe("Applying and updating resources", func() { updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, newValue), + HaveKeyWithValue(testKey, testNewValue), ) }) @@ -164,7 +165,10 @@ var _ = Describe("Applying and updating resources", func() { // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc") Expect(err).ToNot(HaveOccurred()) - service.Annotations[testAnnotationKey] = newValue + service.Annotations[testKey] = testNewValue + service.Spec.ClusterIP = "" + service.Spec.Type = corev1.ServiceTypeExternalName + service.Spec.ExternalName = "test-external-name" Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -173,8 +177,10 @@ var _ = Describe("Applying and updating resources", func() { updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc") Expect(err).ToNot(HaveOccurred()) Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, originalValue), + HaveKeyWithValue(testKey, testOriginalValue), ) + Expect(updatedService.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP)) + fmt.Println(updatedService.Spec.ExternalName) }) })