From aeb9cf98cab5d527d40e21faf49a54968b131427 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 21 Nov 2022 00:51:25 -0800 Subject: [PATCH 01/32] Not working --- pkg/controllers/work/apply_controller.go | 93 ++++++++++++++++++++---- pkg/controllers/work/manager.go | 4 +- pkg/controllers/work/patch_util.go | 16 ++-- 3 files changed, 90 insertions(+), 23 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 2f033f5e3..3d471e648 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -340,17 +340,23 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), } - // compute the hash without taking into consider the last applied annotation - if err := setManifestHashAnnotation(manifestObj); err != nil { + manifestHash, err := computeManifestHash(manifestObj) + if err != nil { return nil, ManifestNoChangeAction, err } // extract the common create procedure to reuse var createFunc = func() (*unstructured.Unstructured, applyAction, error) { - // record the raw manifest with the hash annotation in the manifest - if err := setModifiedConfigurationAnnotation(manifestObj); err != nil { + // record the raw manifest + lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) + if err != nil { + return nil, ManifestNoChangeAction, err + } + // add owner reference of applied work for config map. + if err := r.createConfigMap(ctx, manifestObj, gvr, manifestHash, lastModifiedConfig); err != nil { return nil, ManifestNoChangeAction, err } + // another possible case where configmap gets created but manifest doesn't get created. actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { @@ -382,9 +388,16 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } + // get Config map for curObj + var configMap v1.ConfigMap + configMapName := getConfigMapName(curObj, gvr) + if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "default"}, &configMap); err != nil { + klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) + } + // We only try to update the object if its spec hash value has changed. - if manifestObj.GetAnnotations()[manifestHashAnnotation] != curObj.GetAnnotations()[manifestHashAnnotation] { - return r.patchCurrentResource(ctx, gvr, manifestObj, curObj) + if manifestHash != configMap.Data[manifestHashAnnotation] { + return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, &configMap) } return curObj, ManifestNoChangeAction, nil @@ -392,24 +405,20 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // patchCurrentResource uses three way merge to patch the current resource with the new manifest we get from the work. func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr schema.GroupVersionResource, - manifestObj, curObj *unstructured.Unstructured) (*unstructured.Unstructured, applyAction, error) { + manifestObj, curObj *unstructured.Unstructured, manifestHash string, configMap *v1.ConfigMap) (*unstructured.Unstructured, applyAction, error) { manifestRef := klog.ObjectRef{ Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), } klog.V(2).InfoS("manifest is modified", "gvr", gvr, "manifest", manifestRef, - "new hash", manifestObj.GetAnnotations()[manifestHashAnnotation], - "existing hash", curObj.GetAnnotations()[manifestHashAnnotation]) + "new hash", manifestHash, + "existing hash", configMap.Data[manifestHashAnnotation]) // we need to merge the owner reference between the current and the manifest since we support one manifest // belong to multiple work so it contains the union of all the appliedWork manifestObj.SetOwnerReferences(mergeOwnerReference(curObj.GetOwnerReferences(), manifestObj.GetOwnerReferences())) - // record the raw manifest with the hash annotation in the manifest - if err := setModifiedConfigurationAnnotation(manifestObj); err != nil { - return nil, ManifestNoChangeAction, err - } // create the three-way merge patch between the current, original and manifest similar to how kubectl apply does - patch, err := threeWayMergePatch(curObj, manifestObj) + patch, err := threeWayMergePatch(curObj, manifestObj, configMap) if err != nil { klog.ErrorS(err, "failed to generate the three way patch", "gvr", gvr, "manifest", manifestRef) return nil, ManifestNoChangeAction, err @@ -426,6 +435,27 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche klog.ErrorS(patchErr, "failed to patch the manifest", "gvr", gvr, "manifest", manifestRef) return nil, ManifestNoChangeAction, patchErr } + // calculate last modified config. possibility where manifest gets patched, but we don't set the last modified config below + lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) + if err != nil { + // error message for manifest with namespace ? + klog.ErrorS(err, "failed to compute last modified configuration", "manifest", manifestObj.GetName()) + return nil, ManifestNoChangeAction, err + } + //updatedConfigMap := &v1.ConfigMap{ + // ObjectMeta: metav1.ObjectMeta{ + // Name: configMap.Name, + // Namespace: "default", + // }, + // Data: map[string]string{}, + //} + //updatedConfigMap.Data[manifestHashAnnotation] = manifestHash + configMap.Data[lastAppliedConfigAnnotation] = lastModifiedConfig + // TODO Failing: Operation cannot be fulfilled on configmaps "v1-clusterroles-test-cluster-role": StorageError: invalid object, Code: 4, Key: /registry/configmaps/default/v1-clusterroles-test-cluster-role, ResourceVersion: 0, AdditionalErrorMsg: Precondition failed: UID in precondition: 4aec6193-45df-478e-aa4e-75cba10aeac0, UID in object meta: + if err := r.client.Update(ctx, configMap); err != nil { + klog.ErrorS(err, "failed to update config map", "configMap", configMap.Name) + return nil, ManifestNoChangeAction, err + } klog.V(2).InfoS("manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) return manifestObj, ManifestUpdatedAction, nil } @@ -516,8 +546,8 @@ func computeManifestHash(obj *unstructured.Unstructured) (string, error) { // remove the last applied Annotation to avoid unlimited recursion annotation := manifest.GetAnnotations() if annotation != nil { - delete(annotation, manifestHashAnnotation) - delete(annotation, lastAppliedConfigAnnotation) + //delete(annotation, manifestHashAnnotation) + //delete(annotation, lastAppliedConfigAnnotation) if len(annotation) == 0 { manifest.SetAnnotations(nil) } else { @@ -629,6 +659,37 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } +func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, manifestObj *unstructured.Unstructured, gvr schema.GroupVersionResource, manifestHash, lastModifiedConfig string) error { + configMapName := getConfigMapName(manifestObj, gvr) + configMap := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: configMapName, + Namespace: "default", + }, + Data: map[string]string{}, + } + configMap.Data[manifestHashAnnotation] = manifestHash + configMap.Data[lastAppliedConfigAnnotation] = lastModifiedConfig + err := r.spokeClient.Create(ctx, configMap) + if err != nil { + klog.ErrorS(err, "config map create failed", "configMap", configMapName) + return err + } + klog.V(2).InfoS("config map was created", "configMap", configMapName) + return nil +} + +func getConfigMapName(manifestObj *unstructured.Unstructured, gvr schema.GroupVersionResource) string { + var configMapName string + isNamespaced := len(manifestObj.GetNamespace()) > 0 + if isNamespaced { + configMapName = gvr.Version + "-" + gvr.Resource + "-" + manifestObj.GetName() + "-" + manifestObj.GetNamespace() + } else { + configMapName = gvr.Version + "-" + gvr.Resource + "-" + manifestObj.GetName() + } + return configMapName +} + // generateWorkAppliedCondition generate applied status condition for work. // If one of the manifests is applied failed on the spoke, the applied status condition of the work is false. func generateWorkAppliedCondition(manifestConditions []workv1alpha1.ManifestCondition, observedGeneration int64) metav1.Condition { diff --git a/pkg/controllers/work/manager.go b/pkg/controllers/work/manager.go index 067471bc7..2342a1a2e 100644 --- a/pkg/controllers/work/manager.go +++ b/pkg/controllers/work/manager.go @@ -32,9 +32,9 @@ import ( const ( workFinalizer = "fleet.azure.com/work-cleanup" - manifestHashAnnotation = "fleet.azure.com/spec-hash" + manifestHashAnnotation = "fleet-spec-hash" - lastAppliedConfigAnnotation = "fleet.azure.com/last-applied-configuration" + lastAppliedConfigAnnotation = "fleet-last-applied-configuration" ConditionTypeApplied = "Applied" ConditionTypeAvailable = "Available" diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index 10d433b1d..143014600 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" @@ -25,17 +26,14 @@ func init() { // threeWayMergePatch creates a patch by computing a three-way diff based on // an object's current state, modified state, and last-applied-state recorded in its annotation. -func threeWayMergePatch(currentObj, manifestObj client.Object) (client.Patch, error) { +func threeWayMergePatch(currentObj, manifestObj client.Object, configMap *v1.ConfigMap) (client.Patch, error) { //TODO: see if we should use something like json.ConfigCompatibleWithStandardLibrary.Marshal to make sure that // the json we created is compatible with the format that json merge patch requires. current, err := json.Marshal(currentObj) if err != nil { return nil, err } - original, err := getOriginalConfiguration(currentObj) - if err != nil { - return nil, err - } + original := []byte(configMap.Data[lastAppliedConfigAnnotation]) manifest, err := json.Marshal(manifestObj) if err != nil { return nil, err @@ -109,6 +107,14 @@ func setModifiedConfigurationAnnotation(obj runtime.Object) error { return metadataAccessor.SetAnnotations(obj, annotations) } +func computeModifiedConfiguration(obj runtime.Object) (string, error) { + modified, err := json.Marshal(obj) + if err != nil { + return "", err + } + return string(modified), nil +} + // getOriginalConfiguration gets original configuration of the object // form the annotation, or return an error if no annotation found. func getOriginalConfiguration(obj runtime.Object) ([]byte, error) { From 7c095bd5eafdcc16ca331612523b77a9aedb053e Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 21 Nov 2022 10:10:30 -0800 Subject: [PATCH 02/32] Working update --- pkg/controllers/work/apply_controller.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 3d471e648..0b2957fc1 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -391,7 +391,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // get Config map for curObj var configMap v1.ConfigMap configMapName := getConfigMapName(curObj, gvr) - if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "default"}, &configMap); err != nil { + if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "fleet-system"}, &configMap); err != nil { klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) } @@ -442,17 +442,9 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche klog.ErrorS(err, "failed to compute last modified configuration", "manifest", manifestObj.GetName()) return nil, ManifestNoChangeAction, err } - //updatedConfigMap := &v1.ConfigMap{ - // ObjectMeta: metav1.ObjectMeta{ - // Name: configMap.Name, - // Namespace: "default", - // }, - // Data: map[string]string{}, - //} - //updatedConfigMap.Data[manifestHashAnnotation] = manifestHash configMap.Data[lastAppliedConfigAnnotation] = lastModifiedConfig // TODO Failing: Operation cannot be fulfilled on configmaps "v1-clusterroles-test-cluster-role": StorageError: invalid object, Code: 4, Key: /registry/configmaps/default/v1-clusterroles-test-cluster-role, ResourceVersion: 0, AdditionalErrorMsg: Precondition failed: UID in precondition: 4aec6193-45df-478e-aa4e-75cba10aeac0, UID in object meta: - if err := r.client.Update(ctx, configMap); err != nil { + if err := r.spokeClient.Update(ctx, configMap); err != nil { klog.ErrorS(err, "failed to update config map", "configMap", configMap.Name) return nil, ManifestNoChangeAction, err } @@ -664,7 +656,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, manifestObj * configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: configMapName, - Namespace: "default", + Namespace: "fleet-system", }, Data: map[string]string{}, } From 8fa0974579b2a023955332ec92ed21733dc1c2b1 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 22 Nov 2022 00:21:36 -0800 Subject: [PATCH 03/32] compute manifestHash again --- pkg/controllers/work/apply_controller.go | 47 +++++++++++-------- pkg/controllers/work/manager.go | 2 + pkg/controllers/work/patch_util.go | 5 +- .../placement/select-named-resource.yaml | 6 +-- 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 0b2957fc1..59f3ded31 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -356,7 +356,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. if err := r.createConfigMap(ctx, manifestObj, gvr, manifestHash, lastModifiedConfig); err != nil { return nil, ManifestNoChangeAction, err } - // another possible case where configmap gets created but manifest doesn't get created. + // possible case where configmap gets created but manifest doesn't get created. actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { @@ -389,15 +389,16 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // get Config map for curObj - var configMap v1.ConfigMap + var curObjConfigMap v1.ConfigMap configMapName := getConfigMapName(curObj, gvr) - if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "fleet-system"}, &configMap); err != nil { + if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "fleet-system"}, &curObjConfigMap); err != nil { klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) + return nil, ManifestNoChangeAction, err } // We only try to update the object if its spec hash value has changed. - if manifestHash != configMap.Data[manifestHashAnnotation] { - return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, &configMap) + if manifestHash != curObjConfigMap.Data[manifestHashAnnotation] { + return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, &curObjConfigMap) } return curObj, ManifestNoChangeAction, nil @@ -405,20 +406,20 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // patchCurrentResource uses three way merge to patch the current resource with the new manifest we get from the work. func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr schema.GroupVersionResource, - manifestObj, curObj *unstructured.Unstructured, manifestHash string, configMap *v1.ConfigMap) (*unstructured.Unstructured, applyAction, error) { + manifestObj, curObj *unstructured.Unstructured, manifestHash string, curObjConfigMap *v1.ConfigMap) (*unstructured.Unstructured, applyAction, error) { manifestRef := klog.ObjectRef{ Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), } klog.V(2).InfoS("manifest is modified", "gvr", gvr, "manifest", manifestRef, "new hash", manifestHash, - "existing hash", configMap.Data[manifestHashAnnotation]) + "existing hash", curObjConfigMap.Data[manifestHashKey]) // we need to merge the owner reference between the current and the manifest since we support one manifest // belong to multiple work so it contains the union of all the appliedWork manifestObj.SetOwnerReferences(mergeOwnerReference(curObj.GetOwnerReferences(), manifestObj.GetOwnerReferences())) // create the three-way merge patch between the current, original and manifest similar to how kubectl apply does - patch, err := threeWayMergePatch(curObj, manifestObj, configMap) + patch, err := threeWayMergePatch(curObj, manifestObj, curObjConfigMap) if err != nil { klog.ErrorS(err, "failed to generate the three way patch", "gvr", gvr, "manifest", manifestRef) return nil, ManifestNoChangeAction, err @@ -435,17 +436,24 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche klog.ErrorS(patchErr, "failed to patch the manifest", "gvr", gvr, "manifest", manifestRef) return nil, ManifestNoChangeAction, patchErr } - // calculate last modified config. possibility where manifest gets patched, but we don't set the last modified config below - lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) + // calculate manifest hash & last modified config again to set the new manifest hash & last applied config after patch gets applied + // possibility where manifest gets patched, but we don't set the last modified config. + updatedManifestHash, err := computeManifestHash(manifestObj) + if err != nil { + // namespace for manifest can be empty + klog.ErrorS(err, "failed to compute new manifest hash", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) + return nil, ManifestNoChangeAction, err + } + updatedLastModifiedConfig, err := computeModifiedConfiguration(manifestObj) if err != nil { - // error message for manifest with namespace ? - klog.ErrorS(err, "failed to compute last modified configuration", "manifest", manifestObj.GetName()) + // namespace for manifest can be empty + klog.ErrorS(err, "failed to compute new last modified configuration", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) return nil, ManifestNoChangeAction, err } - configMap.Data[lastAppliedConfigAnnotation] = lastModifiedConfig - // TODO Failing: Operation cannot be fulfilled on configmaps "v1-clusterroles-test-cluster-role": StorageError: invalid object, Code: 4, Key: /registry/configmaps/default/v1-clusterroles-test-cluster-role, ResourceVersion: 0, AdditionalErrorMsg: Precondition failed: UID in precondition: 4aec6193-45df-478e-aa4e-75cba10aeac0, UID in object meta: - if err := r.spokeClient.Update(ctx, configMap); err != nil { - klog.ErrorS(err, "failed to update config map", "configMap", configMap.Name) + curObjConfigMap.Data[manifestHashKey] = updatedManifestHash + curObjConfigMap.Data[lastAppliedConfigKey] = updatedLastModifiedConfig + if err := r.spokeClient.Update(ctx, curObjConfigMap); err != nil { + klog.ErrorS(err, "failed to update config map", "configMap", curObjConfigMap.Name) return nil, ManifestNoChangeAction, err } klog.V(2).InfoS("manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) @@ -538,8 +546,6 @@ func computeManifestHash(obj *unstructured.Unstructured) (string, error) { // remove the last applied Annotation to avoid unlimited recursion annotation := manifest.GetAnnotations() if annotation != nil { - //delete(annotation, manifestHashAnnotation) - //delete(annotation, lastAppliedConfigAnnotation) if len(annotation) == 0 { manifest.SetAnnotations(nil) } else { @@ -660,8 +666,8 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, manifestObj * }, Data: map[string]string{}, } - configMap.Data[manifestHashAnnotation] = manifestHash - configMap.Data[lastAppliedConfigAnnotation] = lastModifiedConfig + configMap.Data[manifestHashKey] = manifestHash + configMap.Data[lastAppliedConfigKey] = lastModifiedConfig err := r.spokeClient.Create(ctx, configMap) if err != nil { klog.ErrorS(err, "config map create failed", "configMap", configMapName) @@ -674,6 +680,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, manifestObj * func getConfigMapName(manifestObj *unstructured.Unstructured, gvr schema.GroupVersionResource) string { var configMapName string isNamespaced := len(manifestObj.GetNamespace()) > 0 + // Need to ensure if configName won't exceed length limit if isNamespaced { configMapName = gvr.Version + "-" + gvr.Resource + "-" + manifestObj.GetName() + "-" + manifestObj.GetNamespace() } else { diff --git a/pkg/controllers/work/manager.go b/pkg/controllers/work/manager.go index 2342a1a2e..6d2ac2bd0 100644 --- a/pkg/controllers/work/manager.go +++ b/pkg/controllers/work/manager.go @@ -33,8 +33,10 @@ const ( workFinalizer = "fleet.azure.com/work-cleanup" manifestHashAnnotation = "fleet-spec-hash" + manifestHashKey = "fleet-spec-hash" lastAppliedConfigAnnotation = "fleet-last-applied-configuration" + lastAppliedConfigKey = "fleet-last-applied-configuration" ConditionTypeApplied = "Applied" ConditionTypeAvailable = "Available" diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index 143014600..892384ca9 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -26,14 +26,14 @@ func init() { // threeWayMergePatch creates a patch by computing a three-way diff based on // an object's current state, modified state, and last-applied-state recorded in its annotation. -func threeWayMergePatch(currentObj, manifestObj client.Object, configMap *v1.ConfigMap) (client.Patch, error) { +func threeWayMergePatch(currentObj, manifestObj client.Object, currentObjConfigMap *v1.ConfigMap) (client.Patch, error) { //TODO: see if we should use something like json.ConfigCompatibleWithStandardLibrary.Marshal to make sure that // the json we created is compatible with the format that json merge patch requires. current, err := json.Marshal(currentObj) if err != nil { return nil, err } - original := []byte(configMap.Data[lastAppliedConfigAnnotation]) + original := []byte(currentObjConfigMap.Data[lastAppliedConfigKey]) manifest, err := json.Marshal(manifestObj) if err != nil { return nil, err @@ -107,6 +107,7 @@ func setModifiedConfigurationAnnotation(obj runtime.Object) error { return metadataAccessor.SetAnnotations(obj, annotations) } +// setModifiedConfigurationAnnotation serializes the object into byte stream and returns it. func computeModifiedConfiguration(obj runtime.Object) (string, error) { modified, err := json.Marshal(obj) if err != nil { diff --git a/test/integration/manifests/placement/select-named-resource.yaml b/test/integration/manifests/placement/select-named-resource.yaml index e6f4a4998..e7c7b3ba0 100644 --- a/test/integration/manifests/placement/select-named-resource.yaml +++ b/test/integration/manifests/placement/select-named-resource.yaml @@ -7,8 +7,4 @@ spec: - group: rbac.authorization.k8s.io version: v1 kind: ClusterRole - name: test-cluster-role - - group: apiextensions.k8s.io - version: v1 - kind: CustomResourceDefinition - name: clonesets.apps.kruise.io \ No newline at end of file + name: test-cluster-role \ No newline at end of file From 50a72277d147ce89306733fe6748381d49c21cbd Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 22 Nov 2022 19:46:04 -0800 Subject: [PATCH 04/32] More comments to address --- pkg/controllers/work/apply_controller.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 59f3ded31..ce74793c9 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -352,10 +352,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. if err != nil { return nil, ManifestNoChangeAction, err } - // add owner reference of applied work for config map. - if err := r.createConfigMap(ctx, manifestObj, gvr, manifestHash, lastModifiedConfig); err != nil { - return nil, ManifestNoChangeAction, err - } // possible case where configmap gets created but manifest doesn't get created. actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) @@ -363,6 +359,10 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) return actual, ManifestCreatedAction, nil } + // add owner reference of applied work for config map. + if err := r.createConfigMap(ctx, manifestObj, gvr, manifestHash, lastModifiedConfig); err != nil { + return nil, ManifestNoChangeAction, err + } return nil, ManifestNoChangeAction, err } @@ -393,6 +393,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. configMapName := getConfigMapName(curObj, gvr) if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "fleet-system"}, &curObjConfigMap); err != nil { klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) + // recreate configmap return nil, ManifestNoChangeAction, err } @@ -436,8 +437,11 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche klog.ErrorS(patchErr, "failed to patch the manifest", "gvr", gvr, "manifest", manifestRef) return nil, ManifestNoChangeAction, patchErr } + // Use manifest generation to check if configmap is outdated + // transition annotation to configmaps for older manifests // calculate manifest hash & last modified config again to set the new manifest hash & last applied config after patch gets applied // possibility where manifest gets patched, but we don't set the last modified config. + // ensure line 451 is needed, and modularize code to make it readable. updatedManifestHash, err := computeManifestHash(manifestObj) if err != nil { // namespace for manifest can be empty From 56c4391fa6a689d891a08bb90c6cffbc12a9e74a Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 28 Nov 2022 17:07:09 -0800 Subject: [PATCH 05/32] Address issues --- pkg/controllers/work/apply_controller.go | 92 ++++++++++++++++++------ 1 file changed, 72 insertions(+), 20 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index ce74793c9..4d83174f4 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -20,6 +20,7 @@ import ( "context" "crypto/sha256" "encoding/json" + "errors" "fmt" "time" @@ -381,6 +382,9 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } + // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. + r.migrateToConfigMap(ctx, curObj, gvr) + // check if the existing manifest is managed by the work if !isManifestManagedByWork(curObj.GetOwnerReferences()) { err = fmt.Errorf("resource is not managed by the work controller") @@ -393,8 +397,22 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. configMapName := getConfigMapName(curObj, gvr) if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "fleet-system"}, &curObjConfigMap); err != nil { klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) - // recreate configmap - return nil, ManifestNoChangeAction, err + // recreated configmap if it doesn't exist, what potential issues can it cause ?. + lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) + if err != nil { + return nil, ManifestNoChangeAction, err + } + // add owner reference of applied work for config map. + if err := r.createConfigMap(ctx, manifestObj, gvr, manifestHash, lastModifiedConfig); err != nil { + return nil, ManifestNoChangeAction, err + } + } + + // Use manifest generation to check if configmap is outdated + if curObj.GetGeneration() != curObjConfigMap.GetGeneration() { + if err := r.updateConfigMap(ctx, &curObjConfigMap, curObj); err != nil { + return nil, ManifestNoChangeAction, err + } } // We only try to update the object if its spec hash value has changed. @@ -437,27 +455,11 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche klog.ErrorS(patchErr, "failed to patch the manifest", "gvr", gvr, "manifest", manifestRef) return nil, ManifestNoChangeAction, patchErr } - // Use manifest generation to check if configmap is outdated - // transition annotation to configmaps for older manifests + // calculate manifest hash & last modified config again to set the new manifest hash & last applied config after patch gets applied // possibility where manifest gets patched, but we don't set the last modified config. // ensure line 451 is needed, and modularize code to make it readable. - updatedManifestHash, err := computeManifestHash(manifestObj) - if err != nil { - // namespace for manifest can be empty - klog.ErrorS(err, "failed to compute new manifest hash", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) - return nil, ManifestNoChangeAction, err - } - updatedLastModifiedConfig, err := computeModifiedConfiguration(manifestObj) - if err != nil { - // namespace for manifest can be empty - klog.ErrorS(err, "failed to compute new last modified configuration", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) - return nil, ManifestNoChangeAction, err - } - curObjConfigMap.Data[manifestHashKey] = updatedManifestHash - curObjConfigMap.Data[lastAppliedConfigKey] = updatedLastModifiedConfig - if err := r.spokeClient.Update(ctx, curObjConfigMap); err != nil { - klog.ErrorS(err, "failed to update config map", "configMap", curObjConfigMap.Name) + if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj); err != nil { return nil, ManifestNoChangeAction, err } klog.V(2).InfoS("manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) @@ -681,6 +683,56 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, manifestObj * return nil } +func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured) error { + updatedManifestHash, err := computeManifestHash(obj) + if err != nil { + // namespace for manifest can be empty + klog.ErrorS(err, "failed to compute new manifest hash", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return err + } + updatedLastModifiedConfig, err := computeModifiedConfiguration(obj) + if err != nil { + // namespace for manifest can be empty + klog.ErrorS(err, "failed to compute new last modified configuration", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return err + } + configMap.Data[manifestHashKey] = updatedManifestHash + configMap.Data[lastAppliedConfigKey] = updatedLastModifiedConfig + if err := r.spokeClient.Update(ctx, configMap); err != nil { + klog.ErrorS(err, "failed to update config map", "configMap", configMap.Name) + return err + } + return nil +} + +// We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. +func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, obj *unstructured.Unstructured, gvr schema.GroupVersionResource) error { + annots, err := metadataAccessor.Annotations(obj) + if err != nil { + return fmt.Errorf("cannot access metadata.annotations: %w", err) + } + if annots == nil { + return errors.New("object does not have manifestHash/lastAppliedConfiguration") + } + if annots[manifestHashAnnotation] == nil && annots[lastAppliedConfigAnnotation] == nil { + + } + manifestHash, ok := annots[manifestHashAnnotation] + if !ok { + return errors.New("object does not have manifestHash") + } + lastModifiedConfig, ok := annots[lastAppliedConfigAnnotation] + if !ok { + return errors.New("object does not have lastAppliedConfigAnnotation") + } + delete(annots, manifestHashAnnotation) + delete(annots, lastAppliedConfigAnnotation) + if err := r.createConfigMap(ctx, obj, gvr, manifestHash, lastModifiedConfig); err != nil { + return err + } + return nil +} + func getConfigMapName(manifestObj *unstructured.Unstructured, gvr schema.GroupVersionResource) string { var configMapName string isNamespaced := len(manifestObj.GetNamespace()) > 0 From 8a8006e1f0c7c0da2044c78961156427c3a89ff0 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 28 Nov 2022 17:47:57 -0800 Subject: [PATCH 06/32] fix migrateToConfigMap --- pkg/controllers/work/apply_controller.go | 29 +++++++++++------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 4d83174f4..0a11f2dac 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -383,7 +383,10 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. - r.migrateToConfigMap(ctx, curObj, gvr) + if err := r.migrateToConfigMap(ctx, curObj, gvr); err != nil { + klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName()) + return nil, ManifestNoChangeAction, err + } // check if the existing manifest is managed by the work if !isManifestManagedByWork(curObj.GetOwnerReferences()) { @@ -714,21 +717,15 @@ func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, obj *unstr if annots == nil { return errors.New("object does not have manifestHash/lastAppliedConfiguration") } - if annots[manifestHashAnnotation] == nil && annots[lastAppliedConfigAnnotation] == nil { - - } - manifestHash, ok := annots[manifestHashAnnotation] - if !ok { - return errors.New("object does not have manifestHash") - } - lastModifiedConfig, ok := annots[lastAppliedConfigAnnotation] - if !ok { - return errors.New("object does not have lastAppliedConfigAnnotation") - } - delete(annots, manifestHashAnnotation) - delete(annots, lastAppliedConfigAnnotation) - if err := r.createConfigMap(ctx, obj, gvr, manifestHash, lastModifiedConfig); err != nil { - return err + // Assuming that manifest has both annotations on it + manifestHash, ok1 := annots[manifestHashAnnotation] + lastModifiedConfig, ok2 := annots[lastAppliedConfigAnnotation] + if ok1 || ok2 { + delete(annots, manifestHashAnnotation) + delete(annots, lastAppliedConfigAnnotation) + if err := r.createConfigMap(ctx, obj, gvr, manifestHash, lastModifiedConfig); err != nil { + return err + } } return nil } From af67b7fb18b3dd0fa66b89a36376c39d25d9e0ed Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 28 Nov 2022 20:25:00 -0800 Subject: [PATCH 07/32] clean up code --- pkg/controllers/work/apply_controller.go | 73 +++++++++++++++--------- 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 0a11f2dac..334429acb 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -51,6 +51,7 @@ import ( const ( workFieldManagerName = "work-api-agent" + fleetSystemNamespace = "fleet-system" ) // ApplyWorkReconciler reconciles a Work object @@ -382,12 +383,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } - // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. - if err := r.migrateToConfigMap(ctx, curObj, gvr); err != nil { - klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName()) - return nil, ManifestNoChangeAction, err - } - // check if the existing manifest is managed by the work if !isManifestManagedByWork(curObj.GetOwnerReferences()) { err = fmt.Errorf("resource is not managed by the work controller") @@ -395,25 +390,21 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } - // get Config map for curObj - var curObjConfigMap v1.ConfigMap - configMapName := getConfigMapName(curObj, gvr) - if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: "fleet-system"}, &curObjConfigMap); err != nil { - klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) - // recreated configmap if it doesn't exist, what potential issues can it cause ?. - lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) - if err != nil { - return nil, ManifestNoChangeAction, err - } - // add owner reference of applied work for config map. - if err := r.createConfigMap(ctx, manifestObj, gvr, manifestHash, lastModifiedConfig); err != nil { - return nil, ManifestNoChangeAction, err - } + // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. + if err := r.migrateToConfigMap(ctx, curObj, gvr); err != nil { + klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName()) + return nil, ManifestNoChangeAction, err + } + + // get config map or create it if it doesn't exist. + curObjConfigMap, err := r.getConfigMap(ctx, curObj, gvr) + if err != nil { + return nil, ManifestNoChangeAction, err } // Use manifest generation to check if configmap is outdated if curObj.GetGeneration() != curObjConfigMap.GetGeneration() { - if err := r.updateConfigMap(ctx, &curObjConfigMap, curObj); err != nil { + if err := r.updateConfigMap(ctx, curObjConfigMap, curObj); err != nil { return nil, ManifestNoChangeAction, err } } @@ -666,12 +657,12 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } -func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, manifestObj *unstructured.Unstructured, gvr schema.GroupVersionResource, manifestHash, lastModifiedConfig string) error { - configMapName := getConfigMapName(manifestObj, gvr) +func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, obj *unstructured.Unstructured, gvr schema.GroupVersionResource, manifestHash, lastModifiedConfig string) error { + configMapName := getConfigMapName(obj, gvr) configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: configMapName, - Namespace: "fleet-system", + Namespace: fleetSystemNamespace, }, Data: map[string]string{}, } @@ -686,6 +677,31 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, manifestObj * return nil } +func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, obj *unstructured.Unstructured, gvr schema.GroupVersionResource) (*v1.ConfigMap, error) { + var configMap v1.ConfigMap + configMapName := getConfigMapName(obj, gvr) + if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { + klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) + // recreated configmap if it doesn't exist, what potential issues can it cause ?. + manifestHash, err := computeManifestHash(obj) + if err != nil { + klog.ErrorS(err, "failed to compute manifest hash", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return nil, err + } + lastModifiedConfig, err := computeModifiedConfiguration(obj) + if err != nil { + klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return nil, err + } + // add owner reference of applied work for config map. + if err := r.createConfigMap(ctx, obj, gvr, manifestHash, lastModifiedConfig); err != nil { + klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return nil, err + } + } + return &configMap, nil +} + func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured) error { updatedManifestHash, err := computeManifestHash(obj) if err != nil { @@ -710,6 +726,7 @@ func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1 // We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, obj *unstructured.Unstructured, gvr schema.GroupVersionResource) error { + // we can also use obj.getAnnotations(). annots, err := metadataAccessor.Annotations(obj) if err != nil { return fmt.Errorf("cannot access metadata.annotations: %w", err) @@ -730,14 +747,14 @@ func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, obj *unstr return nil } -func getConfigMapName(manifestObj *unstructured.Unstructured, gvr schema.GroupVersionResource) string { +func getConfigMapName(obj *unstructured.Unstructured, gvr schema.GroupVersionResource) string { var configMapName string - isNamespaced := len(manifestObj.GetNamespace()) > 0 + isNamespaced := len(obj.GetNamespace()) > 0 // Need to ensure if configName won't exceed length limit if isNamespaced { - configMapName = gvr.Version + "-" + gvr.Resource + "-" + manifestObj.GetName() + "-" + manifestObj.GetNamespace() + configMapName = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() + "-" + obj.GetNamespace() } else { - configMapName = gvr.Version + "-" + gvr.Resource + "-" + manifestObj.GetName() + configMapName = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() } return configMapName } From 4a1597349b4e60cb8bbebe26951d0fd0a7701c06 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 28 Nov 2022 21:01:12 -0800 Subject: [PATCH 08/32] Make methods readable --- pkg/controllers/work/apply_controller.go | 26 ++++++++++++------------ 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 334429acb..52b2553f8 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -300,7 +300,7 @@ func (r *ApplyWorkReconciler) applyManifests(ctx context.Context, manifests []wo default: addOwnerRef(owner, rawObj) - appliedObj, result.action, result.err = r.applyUnstructured(ctx, gvr, rawObj) + appliedObj, result.action, result.err = r.applyUnstructured(ctx, gvr, rawObj, owner) result.identifier = buildResourceIdentifier(index, rawObj, gvr) logObjRef := klog.ObjectRef{ Name: result.identifier.Name, @@ -337,7 +337,7 @@ func (r *ApplyWorkReconciler) decodeManifest(manifest workv1alpha1.Manifest) (sc // Determines if an unstructured manifest object can & should be applied. If so, it applies (creates) the resource on the cluster. func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema.GroupVersionResource, - manifestObj *unstructured.Unstructured) (*unstructured.Unstructured, applyAction, error) { + manifestObj *unstructured.Unstructured, owner metav1.OwnerReference) (*unstructured.Unstructured, applyAction, error) { manifestRef := klog.ObjectRef{ Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), @@ -361,8 +361,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) return actual, ManifestCreatedAction, nil } - // add owner reference of applied work for config map. - if err := r.createConfigMap(ctx, manifestObj, gvr, manifestHash, lastModifiedConfig); err != nil { + if err := r.createConfigMap(ctx, gvr, manifestObj, owner, manifestHash, lastModifiedConfig); err != nil { return nil, ManifestNoChangeAction, err } return nil, ManifestNoChangeAction, err @@ -391,13 +390,13 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. - if err := r.migrateToConfigMap(ctx, curObj, gvr); err != nil { + if err := r.migrateToConfigMap(ctx, gvr, curObj, owner); err != nil { klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName()) return nil, ManifestNoChangeAction, err } // get config map or create it if it doesn't exist. - curObjConfigMap, err := r.getConfigMap(ctx, curObj, gvr) + curObjConfigMap, err := r.getConfigMap(ctx, gvr, curObj, owner) if err != nil { return nil, ManifestNoChangeAction, err } @@ -411,7 +410,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // We only try to update the object if its spec hash value has changed. if manifestHash != curObjConfigMap.Data[manifestHashAnnotation] { - return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, &curObjConfigMap) + return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, curObjConfigMap) } return curObj, ManifestNoChangeAction, nil @@ -657,7 +656,7 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } -func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, obj *unstructured.Unstructured, gvr schema.GroupVersionResource, manifestHash, lastModifiedConfig string) error { +func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) error { configMapName := getConfigMapName(obj, gvr) configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -668,6 +667,8 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, obj *unstruct } configMap.Data[manifestHashKey] = manifestHash configMap.Data[lastAppliedConfigKey] = lastModifiedConfig + // add applied work owner reference. + addOwnerRef(owner, configMap) err := r.spokeClient.Create(ctx, configMap) if err != nil { klog.ErrorS(err, "config map create failed", "configMap", configMapName) @@ -677,7 +678,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, obj *unstruct return nil } -func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, obj *unstructured.Unstructured, gvr schema.GroupVersionResource) (*v1.ConfigMap, error) { +func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, error) { var configMap v1.ConfigMap configMapName := getConfigMapName(obj, gvr) if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { @@ -693,8 +694,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, obj *unstructure klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } - // add owner reference of applied work for config map. - if err := r.createConfigMap(ctx, obj, gvr, manifestHash, lastModifiedConfig); err != nil { + if err := r.createConfigMap(ctx, gvr, obj, owner, manifestHash, lastModifiedConfig); err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } @@ -725,7 +725,7 @@ func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1 } // We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. -func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, obj *unstructured.Unstructured, gvr schema.GroupVersionResource) error { +func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference) error { // we can also use obj.getAnnotations(). annots, err := metadataAccessor.Annotations(obj) if err != nil { @@ -740,7 +740,7 @@ func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, obj *unstr if ok1 || ok2 { delete(annots, manifestHashAnnotation) delete(annots, lastAppliedConfigAnnotation) - if err := r.createConfigMap(ctx, obj, gvr, manifestHash, lastModifiedConfig); err != nil { + if err := r.createConfigMap(ctx, gvr, obj, owner, manifestHash, lastModifiedConfig); err != nil { return err } } From e168bf53c1d2eca7b1580d11c9a1c01cbd3f907d Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 28 Nov 2022 21:05:30 -0800 Subject: [PATCH 09/32] Minor fixes --- pkg/controllers/work/manager.go | 4 ++-- pkg/controllers/work/patch_util.go | 2 +- .../manifests/placement/select-named-resource.yaml | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/work/manager.go b/pkg/controllers/work/manager.go index 6d2ac2bd0..947bc07ef 100644 --- a/pkg/controllers/work/manager.go +++ b/pkg/controllers/work/manager.go @@ -32,10 +32,10 @@ import ( const ( workFinalizer = "fleet.azure.com/work-cleanup" - manifestHashAnnotation = "fleet-spec-hash" + manifestHashAnnotation = "fleet.azure.com/spec-hash" manifestHashKey = "fleet-spec-hash" - lastAppliedConfigAnnotation = "fleet-last-applied-configuration" + lastAppliedConfigAnnotation = "fleet.azure.com/last-applied-configuration" lastAppliedConfigKey = "fleet-last-applied-configuration" ConditionTypeApplied = "Applied" diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index 892384ca9..5e97cb783 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -4,8 +4,8 @@ import ( "encoding/json" "errors" "fmt" - v1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" diff --git a/test/integration/manifests/placement/select-named-resource.yaml b/test/integration/manifests/placement/select-named-resource.yaml index e7c7b3ba0..d714aadf4 100644 --- a/test/integration/manifests/placement/select-named-resource.yaml +++ b/test/integration/manifests/placement/select-named-resource.yaml @@ -7,4 +7,8 @@ spec: - group: rbac.authorization.k8s.io version: v1 kind: ClusterRole - name: test-cluster-role \ No newline at end of file + name: test-cluster-role + - group: apiextensions.k8s.io + version: v1 + kind: CustomResourceDefinition + name: clonesets.apps.kruise.io From 40c224a6930ac57022e4475b08caf6988ee02f40 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 28 Nov 2022 21:07:56 -0800 Subject: [PATCH 10/32] Remove comments --- pkg/controllers/work/apply_controller.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 52b2553f8..863e52453 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -354,7 +354,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. if err != nil { return nil, ManifestNoChangeAction, err } - // possible case where configmap gets created but manifest doesn't get created. actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { @@ -451,7 +450,6 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche // calculate manifest hash & last modified config again to set the new manifest hash & last applied config after patch gets applied // possibility where manifest gets patched, but we don't set the last modified config. - // ensure line 451 is needed, and modularize code to make it readable. if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj); err != nil { return nil, ManifestNoChangeAction, err } From ef2e3a93bcb8959ed4b3660fc072390f82740732 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 29 Nov 2022 13:34:25 -0800 Subject: [PATCH 11/32] Concern comments --- pkg/controllers/work/apply_controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 863e52453..65150e3ce 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -665,7 +665,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr } configMap.Data[manifestHashKey] = manifestHash configMap.Data[lastAppliedConfigKey] = lastModifiedConfig - // add applied work owner reference. + // add applied work owner reference. let's check if it works addOwnerRef(owner, configMap) err := r.spokeClient.Create(ctx, configMap) if err != nil { @@ -750,6 +750,7 @@ func getConfigMapName(obj *unstructured.Unstructured, gvr schema.GroupVersionRes isNamespaced := len(obj.GetNamespace()) > 0 // Need to ensure if configName won't exceed length limit if isNamespaced { + // This can exceed 253 characters. configMapName = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() + "-" + obj.GetNamespace() } else { configMapName = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() From ba872251f4121a12a4d365a469ad463bb720e7df Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 29 Nov 2022 16:07:35 -0800 Subject: [PATCH 12/32] Use generateName for configMap --- pkg/controllers/work/apply_controller.go | 37 ++++++++++++++++++------ 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 65150e3ce..f82235b21 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -22,6 +22,7 @@ import ( "encoding/json" "errors" "fmt" + "k8s.io/apimachinery/pkg/runtime" "time" "go.uber.org/atomic" @@ -358,11 +359,12 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) + // create configmap + if err := r.createConfigMap(ctx, gvr, manifestObj, owner, manifestHash, lastModifiedConfig); err != nil { + return nil, ManifestNoChangeAction, err + } return actual, ManifestCreatedAction, nil } - if err := r.createConfigMap(ctx, gvr, manifestObj, owner, manifestHash, lastModifiedConfig); err != nil { - return nil, ManifestNoChangeAction, err - } return nil, ManifestNoChangeAction, err } @@ -655,11 +657,11 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) error { - configMapName := getConfigMapName(obj, gvr) + configMapName := "configmap-" configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Name: configMapName, - Namespace: fleetSystemNamespace, + GenerateName: configMapName, + Namespace: fleetSystemNamespace, }, Data: map[string]string{}, } @@ -667,12 +669,29 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr configMap.Data[lastAppliedConfigKey] = lastModifiedConfig // add applied work owner reference. let's check if it works addOwnerRef(owner, configMap) - err := r.spokeClient.Create(ctx, configMap) + unstructuredCM, err := runtime.DefaultUnstructuredConverter.ToUnstructured(configMap) + if err != nil { + return err + } + unstructuredConfigMap := &unstructured.Unstructured{ + Object: unstructuredCM, + } + + gvr = schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "configmaps", + } + + actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(fleetSystemNamespace).Create( + ctx, unstructuredConfigMap, metav1.CreateOptions{FieldManager: workFieldManagerName}) + + //err := r.spokeClient.Create(ctx, configMap) if err != nil { - klog.ErrorS(err, "config map create failed", "configMap", configMapName) + klog.ErrorS(err, "config map create failed", "configMap") return err } - klog.V(2).InfoS("config map was created", "configMap", configMapName) + klog.V(2).InfoS("config map was created", "configMap", actual.GetName()) return nil } From f18946839a0b41c5a02899433e3ce587bfc8e018 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 29 Nov 2022 19:07:53 -0800 Subject: [PATCH 13/32] refactor code --- go.mod | 1 + go.sum | 1 + pkg/controllers/work/apply_controller.go | 96 +++++++++++++----------- pkg/controllers/work/manager.go | 2 + 4 files changed, 57 insertions(+), 43 deletions(-) diff --git a/go.mod b/go.mod index caec03fe4..c595f0608 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( k8s.io/api v0.25.2 k8s.io/apiextensions-apiserver v0.24.2 k8s.io/apimachinery v0.25.2 + k8s.io/apiserver v0.24.2 k8s.io/client-go v0.25.2 k8s.io/component-base v0.24.2 k8s.io/klog/v2 v2.70.1 diff --git a/go.sum b/go.sum index 04a920b29..82fba03f2 100644 --- a/go.sum +++ b/go.sum @@ -961,6 +961,7 @@ k8s.io/apiextensions-apiserver v0.24.2/go.mod h1:e5t2GMFVngUEHUd0wuCJzw8YDwZoqZf k8s.io/apimachinery v0.24.2/go.mod h1:82Bi4sCzVBdpYjyI4jY6aHX+YCUchUIrZrXKedjd2UM= k8s.io/apimachinery v0.25.2 h1:WbxfAjCx+AeN8Ilp9joWnyJ6xu9OMeS/fsfjK/5zaQs= k8s.io/apimachinery v0.25.2/go.mod h1:hqqA1X0bsgsxI6dXsJ4HnNTBOmJNxyPp8dw3u2fSHwA= +k8s.io/apiserver v0.24.2 h1:orxipm5elPJSkkFNlwH9ClqaKEDJJA3yR2cAAlCnyj4= k8s.io/apiserver v0.24.2/go.mod h1:pSuKzr3zV+L+MWqsEo0kHHYwCo77AT5qXbFXP2jbvFI= k8s.io/client-go v0.24.2/go.mod h1:zg4Xaoo+umDsfCWr4fCnmLEtQXyCNXCvJuSsglNcV30= k8s.io/client-go v0.25.2 h1:SUPp9p5CwM0yXGQrwYurw9LWz+YtMwhWd0GqOsSiefo= diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index f82235b21..74aaf2384 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -22,7 +22,6 @@ import ( "encoding/json" "errors" "fmt" - "k8s.io/apimachinery/pkg/runtime" "time" "go.uber.org/atomic" @@ -34,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apiserver/pkg/storage/names" "k8s.io/client-go/dynamic" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" @@ -53,6 +53,7 @@ import ( const ( workFieldManagerName = "work-api-agent" fleetSystemNamespace = "fleet-system" + configMapNamePrefix = "configmap-" ) // ApplyWorkReconciler reconciles a Work object @@ -355,12 +356,14 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. if err != nil { return nil, ManifestNoChangeAction, err } + configMapName := r.getRandomConfigMapName(ctx) + setConfigMapNameAnnotation(configMapName, manifestObj) actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) // create configmap - if err := r.createConfigMap(ctx, gvr, manifestObj, owner, manifestHash, lastModifiedConfig); err != nil { + if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { return nil, ManifestNoChangeAction, err } return actual, ManifestCreatedAction, nil @@ -397,7 +400,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // get config map or create it if it doesn't exist. - curObjConfigMap, err := r.getConfigMap(ctx, gvr, curObj, owner) + curObjConfigMap, err := r.getConfigMap(ctx, curObj, owner) if err != nil { return nil, ManifestNoChangeAction, err } @@ -656,8 +659,7 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } -func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) error { - configMapName := "configmap-" +func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, configMapName string, owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) error { configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ GenerateName: configMapName, @@ -669,38 +671,24 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr configMap.Data[lastAppliedConfigKey] = lastModifiedConfig // add applied work owner reference. let's check if it works addOwnerRef(owner, configMap) - unstructuredCM, err := runtime.DefaultUnstructuredConverter.ToUnstructured(configMap) - if err != nil { - return err - } - unstructuredConfigMap := &unstructured.Unstructured{ - Object: unstructuredCM, - } - - gvr = schema.GroupVersionResource{ - Group: "", - Version: "v1", - Resource: "configmaps", - } - - actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(fleetSystemNamespace).Create( - ctx, unstructuredConfigMap, metav1.CreateOptions{FieldManager: workFieldManagerName}) - - //err := r.spokeClient.Create(ctx, configMap) - if err != nil { + if err := r.spokeClient.Create(ctx, configMap); err != nil { klog.ErrorS(err, "config map create failed", "configMap") return err } - klog.V(2).InfoS("config map was created", "configMap", actual.GetName()) + klog.V(2).InfoS("config map was created", "configMap", configMap.GetName()) return nil } -func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, error) { - var configMap v1.ConfigMap - configMapName := getConfigMapName(obj, gvr) - if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { - klog.ErrorS(err, "cannot retrieve config map", "configMap", configMapName) - // recreated configmap if it doesn't exist, what potential issues can it cause ?. +func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, obj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, error) { + var configMap *v1.ConfigMap + annotations := obj.GetAnnotations() + configMapName, ok := annotations[configMapNameAnnotation] + if !ok { + return nil, errors.New("manifest doesn't have a config map") + } + err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, configMap) + if apierrors.IsNotFound(err) { + // create config map since it's not found manifestHash, err := computeManifestHash(obj) if err != nil { klog.ErrorS(err, "failed to compute manifest hash", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) @@ -711,12 +699,16 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } - if err := r.createConfigMap(ctx, gvr, obj, owner, manifestHash, lastModifiedConfig); err != nil { + if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } + // get newly created config map recursively. + if configMap, err = r.getConfigMap(ctx, obj, owner); err != nil { + return nil, err + } } - return &configMap, nil + return configMap, nil } func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured) error { @@ -749,7 +741,7 @@ func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, gvr schema return fmt.Errorf("cannot access metadata.annotations: %w", err) } if annots == nil { - return errors.New("object does not have manifestHash/lastAppliedConfiguration") + return errors.New("manifest does not have any annotations") } // Assuming that manifest has both annotations on it manifestHash, ok1 := annots[manifestHashAnnotation] @@ -757,26 +749,44 @@ func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, gvr schema if ok1 || ok2 { delete(annots, manifestHashAnnotation) delete(annots, lastAppliedConfigAnnotation) - if err := r.createConfigMap(ctx, gvr, obj, owner, manifestHash, lastModifiedConfig); err != nil { + configMapName := r.getRandomConfigMapName(ctx) + setConfigMapNameAnnotation(configMapName, obj) + // update manifest + if _, err := r.spokeDynamicClient.Resource(gvr).Namespace(obj.GetNamespace()).Update(ctx, obj, metav1.UpdateOptions{}); err != nil { + klog.ErrorS(err, "failed to update manifest with config map name annotation") + return err + } + if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { return err } } return nil } -func getConfigMapName(obj *unstructured.Unstructured, gvr schema.GroupVersionResource) string { +func (r *ApplyWorkReconciler) getRandomConfigMapName(ctx context.Context) string { + generator := names.SimpleNameGenerator var configMapName string - isNamespaced := len(obj.GetNamespace()) > 0 - // Need to ensure if configName won't exceed length limit - if isNamespaced { - // This can exceed 253 characters. - configMapName = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() + "-" + obj.GetNamespace() - } else { - configMapName = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() + var configMap v1.ConfigMap + for { + configMapName := generator.GenerateName(configMapNamePrefix) + if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { + if apierrors.IsNotFound(err) { + break + } + } } return configMapName } +func setConfigMapNameAnnotation(configMapName string, obj *unstructured.Unstructured) { + annotations := obj.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[configMapNameAnnotation] = configMapName + obj.SetAnnotations(annotations) +} + // generateWorkAppliedCondition generate applied status condition for work. // If one of the manifests is applied failed on the spoke, the applied status condition of the work is false. func generateWorkAppliedCondition(manifestConditions []workv1alpha1.ManifestCondition, observedGeneration int64) metav1.Condition { diff --git a/pkg/controllers/work/manager.go b/pkg/controllers/work/manager.go index 947bc07ef..a48b537cc 100644 --- a/pkg/controllers/work/manager.go +++ b/pkg/controllers/work/manager.go @@ -32,6 +32,8 @@ import ( const ( workFinalizer = "fleet.azure.com/work-cleanup" + configMapNameAnnotation = "fleet.azure.com/config-map-name" + manifestHashAnnotation = "fleet.azure.com/spec-hash" manifestHashKey = "fleet-spec-hash" From 04a0414893ae919701610427ceb8b3fa17e30748 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 30 Nov 2022 00:22:36 -0800 Subject: [PATCH 14/32] testing --- pkg/controllers/work/apply_controller.go | 82 ++++++++++++++---------- 1 file changed, 49 insertions(+), 33 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 74aaf2384..69bc82f8e 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -351,13 +351,14 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // extract the common create procedure to reuse var createFunc = func() (*unstructured.Unstructured, applyAction, error) { - // record the raw manifest + // set config map name annotation on manifest + configMapName := r.getRandomConfigMapName(ctx) + setConfigMapNameAnnotation(configMapName, manifestObj) + // record the raw manifest with config map name annotation lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) if err != nil { return nil, ManifestNoChangeAction, err } - configMapName := r.getRandomConfigMapName(ctx) - setConfigMapNameAnnotation(configMapName, manifestObj) actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { @@ -386,6 +387,8 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } + klog.InfoS("generation for curObj", "generation", curObj.GetGeneration()) + // check if the existing manifest is managed by the work if !isManifestManagedByWork(curObj.GetOwnerReferences()) { err = fmt.Errorf("resource is not managed by the work controller") @@ -393,18 +396,24 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } - // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. - if err := r.migrateToConfigMap(ctx, gvr, curObj, owner); err != nil { - klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName()) - return nil, ManifestNoChangeAction, err - } - // get config map or create it if it doesn't exist. curObjConfigMap, err := r.getConfigMap(ctx, curObj, owner) if err != nil { return nil, ManifestNoChangeAction, err } + klog.InfoS("generation for configmap", "generation", curObjConfigMap.GetGeneration()) + + // set config map name annotation on manifest object to prevent deletion during patch. + setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) + + klog.InfoS("Before trying to migrate") + // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. + if err := r.migrateToConfigMap(ctx, gvr, curObj, owner); err != nil { + klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName()) + return nil, ManifestNoChangeAction, err + } + // Use manifest generation to check if configmap is outdated if curObj.GetGeneration() != curObjConfigMap.GetGeneration() { if err := r.updateConfigMap(ctx, curObjConfigMap, curObj); err != nil { @@ -548,6 +557,7 @@ func computeManifestHash(obj *unstructured.Unstructured) (string, error) { // remove the last applied Annotation to avoid unlimited recursion annotation := manifest.GetAnnotations() if annotation != nil { + delete(annotation, configMapNameAnnotation) if len(annotation) == 0 { manifest.SetAnnotations(nil) } else { @@ -662,8 +672,8 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, configMapName string, owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) error { configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: configMapName, - Namespace: fleetSystemNamespace, + Name: configMapName, + Namespace: fleetSystemNamespace, }, Data: map[string]string{}, } @@ -680,35 +690,41 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, configMapName } func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, obj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, error) { - var configMap *v1.ConfigMap + var configMap v1.ConfigMap annotations := obj.GetAnnotations() configMapName, ok := annotations[configMapNameAnnotation] if !ok { return nil, errors.New("manifest doesn't have a config map") } - err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, configMap) - if apierrors.IsNotFound(err) { - // create config map since it's not found - manifestHash, err := computeManifestHash(obj) - if err != nil { - klog.ErrorS(err, "failed to compute manifest hash", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - return nil, err - } - lastModifiedConfig, err := computeModifiedConfiguration(obj) - if err != nil { - klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - return nil, err - } - if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { - klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - return nil, err - } - // get newly created config map recursively. - if configMap, err = r.getConfigMap(ctx, obj, owner); err != nil { + if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { + if apierrors.IsNotFound(err) { + // create config map since it's not found + manifestHash, err := computeManifestHash(obj) + if err != nil { + klog.ErrorS(err, "failed to compute manifest hash", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return nil, err + } + lastModifiedConfig, err := computeModifiedConfiguration(obj) + if err != nil { + klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return nil, err + } + if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { + klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return nil, err + } + // get newly created config map recursively. + newConfigMap, err := r.getConfigMap(ctx, obj, owner) + if err != nil { + return nil, err + } + return newConfigMap, nil + } else { + klog.ErrorS(err, "failed to retrieve config map for object", "manifest", obj.GetName(), "config map", configMapName) return nil, err } } - return configMap, nil + return &configMap, nil } func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured) error { @@ -768,7 +784,7 @@ func (r *ApplyWorkReconciler) getRandomConfigMapName(ctx context.Context) string var configMapName string var configMap v1.ConfigMap for { - configMapName := generator.GenerateName(configMapNamePrefix) + configMapName = generator.GenerateName(configMapNamePrefix) if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { if apierrors.IsNotFound(err) { break From 61552a68efa10dac65aeaf1baca54e38b757d313 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 30 Nov 2022 00:43:08 -0800 Subject: [PATCH 15/32] edit comment --- pkg/controllers/work/apply_controller.go | 4 ++-- .../manifests/placement/select-named-resource.yaml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 69bc82f8e..458092e92 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -554,7 +554,7 @@ func (r *ApplyWorkReconciler) SetupWithManager(mgr ctrl.Manager) error { // we have modified. func computeManifestHash(obj *unstructured.Unstructured) (string, error) { manifest := obj.DeepCopy() - // remove the last applied Annotation to avoid unlimited recursion + // remove the config map annotation to avoid unlimited recursion, need to think ? annotation := manifest.GetAnnotations() if annotation != nil { delete(annotation, configMapNameAnnotation) @@ -713,7 +713,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, obj *unstructure klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } - // get newly created config map recursively. + // get newly created config map recursively, doesn't look clean newConfigMap, err := r.getConfigMap(ctx, obj, owner) if err != nil { return nil, err diff --git a/test/integration/manifests/placement/select-named-resource.yaml b/test/integration/manifests/placement/select-named-resource.yaml index d714aadf4..e6f4a4998 100644 --- a/test/integration/manifests/placement/select-named-resource.yaml +++ b/test/integration/manifests/placement/select-named-resource.yaml @@ -11,4 +11,4 @@ spec: - group: apiextensions.k8s.io version: v1 kind: CustomResourceDefinition - name: clonesets.apps.kruise.io + name: clonesets.apps.kruise.io \ No newline at end of file From 25e0f1a92e5ad428162b94600d80d01f7e3b775a Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 30 Nov 2022 09:59:58 -0800 Subject: [PATCH 16/32] fix getRandomConfigMapName --- pkg/controllers/work/apply_controller.go | 33 ++++++++++++++---------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 458092e92..aaf9fca66 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -352,7 +352,11 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // extract the common create procedure to reuse var createFunc = func() (*unstructured.Unstructured, applyAction, error) { // set config map name annotation on manifest - configMapName := r.getRandomConfigMapName(ctx) + configMapName, err := r.getRandomConfigMapName(ctx) + if err != nil { + klog.ErrorS(err, "failed to get random config map name for manifest", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) + return nil, ManifestNoChangeAction, err + } setConfigMapNameAnnotation(configMapName, manifestObj) // record the raw manifest with config map name annotation lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) @@ -396,7 +400,14 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } - // get config map or create it if it doesn't exist. + klog.InfoS("Before trying to migrate") + // Migrate old objects with annotations mnifest hash and last modified annotation to use configmap instead, will be removed after migration is complete. + if err := r.migrateToConfigMap(ctx, gvr, curObj, owner); err != nil { + klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) + return nil, ManifestNoChangeAction, err + } + + // get config map or create it if it doesn't exist, need to set curObj generation to config map when creating it again curObjConfigMap, err := r.getConfigMap(ctx, curObj, owner) if err != nil { return nil, ManifestNoChangeAction, err @@ -407,13 +418,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // set config map name annotation on manifest object to prevent deletion during patch. setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) - klog.InfoS("Before trying to migrate") - // Migrate old objects with annotations to use configmap instead, will be removed after migration is complete. - if err := r.migrateToConfigMap(ctx, gvr, curObj, owner); err != nil { - klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName()) - return nil, ManifestNoChangeAction, err - } - // Use manifest generation to check if configmap is outdated if curObj.GetGeneration() != curObjConfigMap.GetGeneration() { if err := r.updateConfigMap(ctx, curObjConfigMap, curObj); err != nil { @@ -463,7 +467,6 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche } // calculate manifest hash & last modified config again to set the new manifest hash & last applied config after patch gets applied - // possibility where manifest gets patched, but we don't set the last modified config. if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj); err != nil { return nil, ManifestNoChangeAction, err } @@ -765,7 +768,10 @@ func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, gvr schema if ok1 || ok2 { delete(annots, manifestHashAnnotation) delete(annots, lastAppliedConfigAnnotation) - configMapName := r.getRandomConfigMapName(ctx) + configMapName, err := r.getRandomConfigMapName(ctx) + if err != nil { + return err + } setConfigMapNameAnnotation(configMapName, obj) // update manifest if _, err := r.spokeDynamicClient.Resource(gvr).Namespace(obj.GetNamespace()).Update(ctx, obj, metav1.UpdateOptions{}); err != nil { @@ -779,7 +785,7 @@ func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, gvr schema return nil } -func (r *ApplyWorkReconciler) getRandomConfigMapName(ctx context.Context) string { +func (r *ApplyWorkReconciler) getRandomConfigMapName(ctx context.Context) (string, error) { generator := names.SimpleNameGenerator var configMapName string var configMap v1.ConfigMap @@ -789,9 +795,10 @@ func (r *ApplyWorkReconciler) getRandomConfigMapName(ctx context.Context) string if apierrors.IsNotFound(err) { break } + return "", err } } - return configMapName + return configMapName, nil } func setConfigMapNameAnnotation(configMapName string, obj *unstructured.Unstructured) { From f3e0007e24156acf03c4a1f59bee65affa9abbe5 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 30 Nov 2022 10:20:32 -0800 Subject: [PATCH 17/32] Use manifest key for check --- pkg/controllers/work/apply_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index aaf9fca66..4a9104d2c 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -426,7 +426,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // We only try to update the object if its spec hash value has changed. - if manifestHash != curObjConfigMap.Data[manifestHashAnnotation] { + if manifestHash != curObjConfigMap.Data[manifestHashKey] { return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, curObjConfigMap) } From 1fcf980923d8e5251a213027969bb63794a09259 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 1 Dec 2022 09:13:27 -0800 Subject: [PATCH 18/32] Address comments --- pkg/controllers/work/apply_controller.go | 170 +++++++++++------------ pkg/controllers/work/manager.go | 12 +- 2 files changed, 85 insertions(+), 97 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 4a9104d2c..a867dfa0d 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -351,24 +351,19 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // extract the common create procedure to reuse var createFunc = func() (*unstructured.Unstructured, applyAction, error) { + configMapName := getRandomConfigMapName() // set config map name annotation on manifest - configMapName, err := r.getRandomConfigMapName(ctx) - if err != nil { - klog.ErrorS(err, "failed to get random config map name for manifest", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) - return nil, ManifestNoChangeAction, err - } setConfigMapNameAnnotation(configMapName, manifestObj) - // record the raw manifest with config map name annotation - lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) - if err != nil { - return nil, ManifestNoChangeAction, err - } actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) - // create configmap - if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { + // record the raw manifest with config map name annotation + lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) + if err != nil { + return nil, ManifestNoChangeAction, err + } + if err, _ := r.createConfigMap(ctx, gvr, actual, owner, configMapName, manifestHash, lastModifiedConfig); err != nil { return nil, ManifestNoChangeAction, err } return actual, ManifestCreatedAction, nil @@ -400,15 +395,8 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } - klog.InfoS("Before trying to migrate") - // Migrate old objects with annotations mnifest hash and last modified annotation to use configmap instead, will be removed after migration is complete. - if err := r.migrateToConfigMap(ctx, gvr, curObj, owner); err != nil { - klog.ErrorS(err, "cannot migrate manifest to use config map", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) - return nil, ManifestNoChangeAction, err - } - - // get config map or create it if it doesn't exist, need to set curObj generation to config map when creating it again - curObjConfigMap, err := r.getConfigMap(ctx, curObj, owner) + // get config map or create it if it doesn't exist + curObjConfigMap, err := r.getConfigMap(ctx, gvr, curObj, owner, manifestHash) if err != nil { return nil, ManifestNoChangeAction, err } @@ -418,13 +406,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // set config map name annotation on manifest object to prevent deletion during patch. setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) - // Use manifest generation to check if configmap is outdated - if curObj.GetGeneration() != curObjConfigMap.GetGeneration() { - if err := r.updateConfigMap(ctx, curObjConfigMap, curObj); err != nil { - return nil, ManifestNoChangeAction, err - } - } - // We only try to update the object if its spec hash value has changed. if manifestHash != curObjConfigMap.Data[manifestHashKey] { return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, curObjConfigMap) @@ -459,19 +440,19 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche return nil, ManifestNoChangeAction, err } // Use client side apply the patch to the member cluster - manifestObj, patchErr := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()). + updatedManifestObj, patchErr := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()). Patch(ctx, manifestObj.GetName(), patch.Type(), data, metav1.PatchOptions{FieldManager: workFieldManagerName}) if patchErr != nil { klog.ErrorS(patchErr, "failed to patch the manifest", "gvr", gvr, "manifest", manifestRef) return nil, ManifestNoChangeAction, patchErr } - // calculate manifest hash & last modified config again to set the new manifest hash & last applied config after patch gets applied + // calculate the values based on obj from hub cluster, only track revisions from hub cluster changes if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj); err != nil { return nil, ManifestNoChangeAction, err } klog.V(2).InfoS("manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) - return manifestObj, ManifestUpdatedAction, nil + return updatedManifestObj, ManifestUpdatedAction, nil } // generateWorkCondition constructs the work condition based on the apply result @@ -672,7 +653,7 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } -func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, configMapName string, owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) error { +func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured, owner metav1.OwnerReference, configMapName, manifestHash, lastModifiedConfig string) (error, *v1.ConfigMap) { configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: configMapName, @@ -682,46 +663,56 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, configMapName } configMap.Data[manifestHashKey] = manifestHash configMap.Data[lastAppliedConfigKey] = lastModifiedConfig - // add applied work owner reference. let's check if it works + // add applied work owner reference addOwnerRef(owner, configMap) + setManifestIdentifierAnnotation(manifestObj, configMap) if err := r.spokeClient.Create(ctx, configMap); err != nil { + if apierrors.IsAlreadyExists(err) { + // delete config map annotation from manifest + annotations := manifestObj.GetAnnotations() + delete(annotations, configMapName) + if _, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Update(ctx, manifestObj, metav1.UpdateOptions{FieldManager: workFieldManagerName}); err != nil { + klog.ErrorS(err, "failed to delete config map name annotation on manifest", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) + return err, nil + } + } klog.ErrorS(err, "config map create failed", "configMap") - return err + return err, nil } klog.V(2).InfoS("config map was created", "configMap", configMap.GetName()) - return nil + return nil, configMap } -func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, obj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, error) { +func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash string) (*v1.ConfigMap, error) { var configMap v1.ConfigMap annotations := obj.GetAnnotations() configMapName, ok := annotations[configMapNameAnnotation] if !ok { return nil, errors.New("manifest doesn't have a config map") } + var lastModifiedConfig string if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { if apierrors.IsNotFound(err) { - // create config map since it's not found - manifestHash, err := computeManifestHash(obj) + err, migrateObj, existingManifestHash, existingLastModifiedConfig := migrateToConfigMap(obj) if err != nil { - klog.ErrorS(err, "failed to compute manifest hash", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } - lastModifiedConfig, err := computeModifiedConfiguration(obj) - if err != nil { - klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - return nil, err + // check if object needs to be migrated + if existingManifestHash != "" && existingLastModifiedConfig != "" { + configMapName = getRandomConfigMapName() + setConfigMapNameAnnotation(configMapName, migrateObj) + manifestHash, lastModifiedConfig = existingManifestHash, existingLastModifiedConfig + } else { + lastModifiedConfig, err = computeModifiedConfiguration(obj) + if err != nil { + klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return nil, err + } } - if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { + if err := r.createConfigMap(ctx, gvr, obj, owner, configMapName, manifestHash, lastModifiedConfig); err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } - // get newly created config map recursively, doesn't look clean - newConfigMap, err := r.getConfigMap(ctx, obj, owner) - if err != nil { - return nil, err - } - return newConfigMap, nil } else { klog.ErrorS(err, "failed to retrieve config map for object", "manifest", obj.GetName(), "config map", configMapName) return nil, err @@ -753,52 +744,28 @@ func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1 } // We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. -func (r *ApplyWorkReconciler) migrateToConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference) error { - // we can also use obj.getAnnotations(). - annots, err := metadataAccessor.Annotations(obj) - if err != nil { - return fmt.Errorf("cannot access metadata.annotations: %w", err) - } - if annots == nil { - return errors.New("manifest does not have any annotations") +func migrateToConfigMap(obj *unstructured.Unstructured) (error, *unstructured.Unstructured, string, string) { + annotations := obj.GetAnnotations() + if annotations == nil { + return errors.New("manifest doesn't have any annotations"), obj, "", "" } // Assuming that manifest has both annotations on it - manifestHash, ok1 := annots[manifestHashAnnotation] - lastModifiedConfig, ok2 := annots[lastAppliedConfigAnnotation] - if ok1 || ok2 { - delete(annots, manifestHashAnnotation) - delete(annots, lastAppliedConfigAnnotation) - configMapName, err := r.getRandomConfigMapName(ctx) - if err != nil { - return err - } - setConfigMapNameAnnotation(configMapName, obj) - // update manifest - if _, err := r.spokeDynamicClient.Resource(gvr).Namespace(obj.GetNamespace()).Update(ctx, obj, metav1.UpdateOptions{}); err != nil { - klog.ErrorS(err, "failed to update manifest with config map name annotation") - return err - } - if err := r.createConfigMap(ctx, configMapName, owner, manifestHash, lastModifiedConfig); err != nil { - return err - } - } - return nil + manifestHash, ok1 := annotations[manifestHashAnnotation] + lastModifiedConfig, ok2 := annotations[lastAppliedConfigAnnotation] + if ok1 && ok2 { + delete(annotations, manifestHashAnnotation) + delete(annotations, lastAppliedConfigAnnotation) + obj.SetAnnotations(annotations) + return nil, obj, manifestHash, lastModifiedConfig + } else if !ok1 && ok2 || ok1 && !ok2 { + return errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())), obj, "", "" + } + return nil, obj, manifestHash, lastModifiedConfig } -func (r *ApplyWorkReconciler) getRandomConfigMapName(ctx context.Context) (string, error) { +func getRandomConfigMapName() string { generator := names.SimpleNameGenerator - var configMapName string - var configMap v1.ConfigMap - for { - configMapName = generator.GenerateName(configMapNamePrefix) - if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { - if apierrors.IsNotFound(err) { - break - } - return "", err - } - } - return configMapName, nil + return generator.GenerateName(configMapNamePrefix) } func setConfigMapNameAnnotation(configMapName string, obj *unstructured.Unstructured) { @@ -810,6 +777,27 @@ func setConfigMapNameAnnotation(configMapName string, obj *unstructured.Unstruct obj.SetAnnotations(annotations) } +func setManifestIdentifierAnnotation(obj *unstructured.Unstructured, configMap *v1.ConfigMap) { + manifestId := getManifestIdentifier(obj) + annotations := configMap.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[manifestIdentifierAnnotation] = manifestId + obj.SetAnnotations(annotations) +} + +func getManifestIdentifier(obj *unstructured.Unstructured) string { + var manifestId string + isNamespaced := len(obj.GetNamespace()) > 0 + if isNamespaced { + manifestId = obj.GetName() + "-" + obj.GetNamespace() + } else { + manifestId = obj.GetName() + } + return manifestId +} + // generateWorkAppliedCondition generate applied status condition for work. // If one of the manifests is applied failed on the spoke, the applied status condition of the work is false. func generateWorkAppliedCondition(manifestConditions []workv1alpha1.ManifestCondition, observedGeneration int64) metav1.Condition { diff --git a/pkg/controllers/work/manager.go b/pkg/controllers/work/manager.go index a48b537cc..9e47f2589 100644 --- a/pkg/controllers/work/manager.go +++ b/pkg/controllers/work/manager.go @@ -32,13 +32,13 @@ import ( const ( workFinalizer = "fleet.azure.com/work-cleanup" - configMapNameAnnotation = "fleet.azure.com/config-map-name" + configMapNameAnnotation = "fleet.azure.com/config-map-name" + manifestIdentifierAnnotation = "fleet.azure.com/manifest-identifier" + manifestHashAnnotation = "fleet.azure.com/spec-hash" + lastAppliedConfigAnnotation = "fleet.azure.com/last-applied-configuration" - manifestHashAnnotation = "fleet.azure.com/spec-hash" - manifestHashKey = "fleet-spec-hash" - - lastAppliedConfigAnnotation = "fleet.azure.com/last-applied-configuration" - lastAppliedConfigKey = "fleet-last-applied-configuration" + manifestHashKey = "fleet-spec-hash" + lastAppliedConfigKey = "fleet-last-applied-configuration" ConditionTypeApplied = "Applied" ConditionTypeAvailable = "Available" From 68ee8f17fbdcf608ac4f0786e7254e45626bb2bb Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 1 Dec 2022 14:31:02 -0800 Subject: [PATCH 19/32] Changes --- pkg/controllers/work/apply_controller.go | 47 ++++++++++++------------ 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index a867dfa0d..1d0a2d122 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -363,7 +363,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. if err != nil { return nil, ManifestNoChangeAction, err } - if err, _ := r.createConfigMap(ctx, gvr, actual, owner, configMapName, manifestHash, lastModifiedConfig); err != nil { + if _, err := r.createConfigMap(ctx, gvr, actual, owner, configMapName, manifestHash, lastModifiedConfig); err != nil { return nil, ManifestNoChangeAction, err } return actual, ManifestCreatedAction, nil @@ -448,7 +448,8 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche } // calculate the values based on obj from hub cluster, only track revisions from hub cluster changes - if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj); err != nil { + // passing in manifest hash calculated earlier as ownerRef field might have changed with union + if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj, manifestHash); err != nil { return nil, ManifestNoChangeAction, err } klog.V(2).InfoS("manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) @@ -653,7 +654,7 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } -func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured, owner metav1.OwnerReference, configMapName, manifestHash, lastModifiedConfig string) (error, *v1.ConfigMap) { +func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured, owner metav1.OwnerReference, configMapName, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: configMapName, @@ -673,16 +674,17 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr delete(annotations, configMapName) if _, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Update(ctx, manifestObj, metav1.UpdateOptions{FieldManager: workFieldManagerName}); err != nil { klog.ErrorS(err, "failed to delete config map name annotation on manifest", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) - return err, nil + return nil, err } } - klog.ErrorS(err, "config map create failed", "configMap") - return err, nil + klog.ErrorS(err, "config map create failed", "configMap", configMap.GetName()) + return nil, err } klog.V(2).InfoS("config map was created", "configMap", configMap.GetName()) - return nil, configMap + return configMap, nil } +// Need to handle case where manifest obj and configmap annotations func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash string) (*v1.ConfigMap, error) { var configMap v1.ConfigMap annotations := obj.GetAnnotations() @@ -693,7 +695,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group var lastModifiedConfig string if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { if apierrors.IsNotFound(err) { - err, migrateObj, existingManifestHash, existingLastModifiedConfig := migrateToConfigMap(obj) + migrateObj, existingManifestHash, existingLastModifiedConfig, err := migrateToConfigMap(obj) if err != nil { return nil, err } @@ -703,38 +705,37 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group setConfigMapNameAnnotation(configMapName, migrateObj) manifestHash, lastModifiedConfig = existingManifestHash, existingLastModifiedConfig } else { + // using this lastModifiedCofig is incorrect, as it's from the member cluster lastModifiedConfig, err = computeModifiedConfiguration(obj) if err != nil { klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } } - if err := r.createConfigMap(ctx, gvr, obj, owner, configMapName, manifestHash, lastModifiedConfig); err != nil { + newConfigMap, err := r.createConfigMap(ctx, gvr, obj, owner, configMapName, manifestHash, lastModifiedConfig) + if err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return nil, err } + return newConfigMap, nil + } else { klog.ErrorS(err, "failed to retrieve config map for object", "manifest", obj.GetName(), "config map", configMapName) return nil, err } } + // need to handle special case where manifest was created with config map name but controller failed, turns out config map belonged to another manifest. return &configMap, nil } -func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured) error { - updatedManifestHash, err := computeManifestHash(obj) - if err != nil { - // namespace for manifest can be empty - klog.ErrorS(err, "failed to compute new manifest hash", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - return err - } +func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured, manifestHash string) error { updatedLastModifiedConfig, err := computeModifiedConfiguration(obj) if err != nil { // namespace for manifest can be empty klog.ErrorS(err, "failed to compute new last modified configuration", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return err } - configMap.Data[manifestHashKey] = updatedManifestHash + configMap.Data[manifestHashKey] = manifestHash configMap.Data[lastAppliedConfigKey] = updatedLastModifiedConfig if err := r.spokeClient.Update(ctx, configMap); err != nil { klog.ErrorS(err, "failed to update config map", "configMap", configMap.Name) @@ -744,10 +745,10 @@ func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1 } // We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. -func migrateToConfigMap(obj *unstructured.Unstructured) (error, *unstructured.Unstructured, string, string) { +func migrateToConfigMap(obj *unstructured.Unstructured) (*unstructured.Unstructured, string, string, error) { annotations := obj.GetAnnotations() if annotations == nil { - return errors.New("manifest doesn't have any annotations"), obj, "", "" + return obj, "", "", errors.New("manifest doesn't have any annotations") } // Assuming that manifest has both annotations on it manifestHash, ok1 := annotations[manifestHashAnnotation] @@ -756,11 +757,11 @@ func migrateToConfigMap(obj *unstructured.Unstructured) (error, *unstructured.Un delete(annotations, manifestHashAnnotation) delete(annotations, lastAppliedConfigAnnotation) obj.SetAnnotations(annotations) - return nil, obj, manifestHash, lastModifiedConfig + return obj, manifestHash, lastModifiedConfig, nil } else if !ok1 && ok2 || ok1 && !ok2 { - return errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())), obj, "", "" + return obj, "", "", errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())) } - return nil, obj, manifestHash, lastModifiedConfig + return obj, manifestHash, lastModifiedConfig, nil } func getRandomConfigMapName() string { @@ -784,7 +785,7 @@ func setManifestIdentifierAnnotation(obj *unstructured.Unstructured, configMap * annotations = map[string]string{} } annotations[manifestIdentifierAnnotation] = manifestId - obj.SetAnnotations(annotations) + configMap.SetAnnotations(annotations) } func getManifestIdentifier(obj *unstructured.Unstructured) string { From 24a9538a5f7f4b9701be6f53033f328e9bfe1bc4 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 1 Dec 2022 18:17:42 -0800 Subject: [PATCH 20/32] Handle configmap deletion --- pkg/controllers/work/apply_controller.go | 38 ++++++++++++------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 1d0a2d122..a9c63dedb 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -396,7 +396,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // get config map or create it if it doesn't exist - curObjConfigMap, err := r.getConfigMap(ctx, gvr, curObj, owner, manifestHash) + curObjConfigMap, isRecreated, err := r.getConfigMap(ctx, gvr, manifestObj, curObj, owner, manifestHash) if err != nil { return nil, ManifestNoChangeAction, err } @@ -406,8 +406,8 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // set config map name annotation on manifest object to prevent deletion during patch. setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) - // We only try to update the object if its spec hash value has changed. - if manifestHash != curObjConfigMap.Data[manifestHashKey] { + // We update the object if its spec hash value has changed or if config map got deleted or never created + if manifestHash != curObjConfigMap.Data[manifestHashKey] || isRecreated { return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, curObjConfigMap) } @@ -685,19 +685,19 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr } // Need to handle case where manifest obj and configmap annotations -func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash string) (*v1.ConfigMap, error) { +func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash string) (*v1.ConfigMap, bool, error) { var configMap v1.ConfigMap - annotations := obj.GetAnnotations() + annotations := currentObj.GetAnnotations() configMapName, ok := annotations[configMapNameAnnotation] if !ok { - return nil, errors.New("manifest doesn't have a config map") + return nil, false, errors.New("manifest doesn't have a config map") } var lastModifiedConfig string if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { if apierrors.IsNotFound(err) { - migrateObj, existingManifestHash, existingLastModifiedConfig, err := migrateToConfigMap(obj) + migrateObj, existingManifestHash, existingLastModifiedConfig, err := migrateToConfigMap(currentObj) if err != nil { - return nil, err + return nil, false, err } // check if object needs to be migrated if existingManifestHash != "" && existingLastModifiedConfig != "" { @@ -705,27 +705,27 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group setConfigMapNameAnnotation(configMapName, migrateObj) manifestHash, lastModifiedConfig = existingManifestHash, existingLastModifiedConfig } else { - // using this lastModifiedCofig is incorrect, as it's from the member cluster - lastModifiedConfig, err = computeModifiedConfiguration(obj) + // using this lastModifiedConfig is incorrect, as it's from the member cluster + lastModifiedConfig, err = computeModifiedConfiguration(manifestObj) if err != nil { - klog.ErrorS(err, "failed compute last applied config", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - return nil, err + klog.ErrorS(err, "failed compute last applied config", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) + return nil, false, err } } - newConfigMap, err := r.createConfigMap(ctx, gvr, obj, owner, configMapName, manifestHash, lastModifiedConfig) + newConfigMap, err := r.createConfigMap(ctx, gvr, currentObj, owner, configMapName, manifestHash, lastModifiedConfig) if err != nil { - klog.ErrorS(err, "cannot create config map for object", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - return nil, err + klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) + return nil, false, err } - return newConfigMap, nil + return newConfigMap, true, nil } else { - klog.ErrorS(err, "failed to retrieve config map for object", "manifest", obj.GetName(), "config map", configMapName) - return nil, err + klog.ErrorS(err, "failed to retrieve config map for object", "manifest", currentObj.GetName(), "config map", configMapName) + return nil, false, err } } // need to handle special case where manifest was created with config map name but controller failed, turns out config map belonged to another manifest. - return &configMap, nil + return &configMap, false, nil } func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured, manifestHash string) error { From d7d4949152266c54225d7f6d6ee46a4f939c50fe Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 00:18:02 -0800 Subject: [PATCH 21/32] refactor getConfigMap --- pkg/controllers/work/apply_controller.go | 121 ++++++++++++++--------- 1 file changed, 76 insertions(+), 45 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index a9c63dedb..b7f60d306 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -344,10 +344,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), } - manifestHash, err := computeManifestHash(manifestObj) - if err != nil { - return nil, ManifestNoChangeAction, err - } // extract the common create procedure to reuse var createFunc = func() (*unstructured.Unstructured, applyAction, error) { @@ -358,12 +354,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) - // record the raw manifest with config map name annotation - lastModifiedConfig, err := computeModifiedConfiguration(manifestObj) - if err != nil { - return nil, ManifestNoChangeAction, err - } - if _, err := r.createConfigMap(ctx, gvr, actual, owner, configMapName, manifestHash, lastModifiedConfig); err != nil { + if _, err := r.createConfigMap(ctx, gvr, actual, owner, configMapName); err != nil { return nil, ManifestNoChangeAction, err } return actual, ManifestCreatedAction, nil @@ -396,7 +387,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // get config map or create it if it doesn't exist - curObjConfigMap, isRecreated, err := r.getConfigMap(ctx, gvr, manifestObj, curObj, owner, manifestHash) + curObjConfigMap, isRecreated, err := r.getConfigMap(ctx, gvr, manifestObj, curObj, owner) if err != nil { return nil, ManifestNoChangeAction, err } @@ -406,6 +397,11 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // set config map name annotation on manifest object to prevent deletion during patch. setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) + manifestHash, err := computeManifestHash(manifestObj) + if err != nil { + return nil, ManifestNoChangeAction, err + } + // We update the object if its spec hash value has changed or if config map got deleted or never created if manifestHash != curObjConfigMap.Data[manifestHashKey] || isRecreated { return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, curObjConfigMap) @@ -654,6 +650,10 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } +//func (r *ApplyWorkReconciler) migrateCreateConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured, owner metav1.OwnerReference, configMapName, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { +// +//} + func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured, owner metav1.OwnerReference, configMapName, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -662,6 +662,19 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr }, Data: map[string]string{}, } + var err error + if manifestHash == "" { + manifestHash, err = computeManifestHash(manifestObj) + if err != nil { + return nil, err + } + } + if lastModifiedConfig == "" { + lastModifiedConfig, err = computeModifiedConfiguration(manifestObj) + if err != nil { + return nil, err + } + } configMap.Data[manifestHashKey] = manifestHash configMap.Data[lastAppliedConfigKey] = lastModifiedConfig // add applied work owner reference @@ -685,46 +698,68 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr } // Need to handle case where manifest obj and configmap annotations -func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash string) (*v1.ConfigMap, bool, error) { +// code reuse +func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, bool, error) { var configMap v1.ConfigMap - annotations := currentObj.GetAnnotations() - configMapName, ok := annotations[configMapNameAnnotation] - if !ok { - return nil, false, errors.New("manifest doesn't have a config map") + objAnnotations := currentObj.GetAnnotations() + // add check to see if objAnnotations is not nil + configMapName, ok := objAnnotations[configMapNameAnnotation] + // migrate check + existingManifestHash, existingLastModifiedConfig, err := migrateCheck(objAnnotations, currentObj) + if err != nil { + return nil, false, err + } + var manifestHash, lastModifiedConfig string + if existingManifestHash != "" && existingLastModifiedConfig != "" { + manifestHash = existingManifestHash + lastModifiedConfig = existingLastModifiedConfig + } + // This will only evaluate to true if object doesn't need to be migrated, it doesn't have config name annotation + if !ok && (manifestHash == "" && lastModifiedConfig == "") { + // Need to handle case where manifest has config map annotation removed due to conflict, it needs a new configmap name + objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() + if err := r.spokeClient.Update(ctx, currentObj); err != nil { + return nil, false, err + } + newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, "", "") + if err != nil { + klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) + return nil, false, err + } + return newConfigMap, true, nil } - var lastModifiedConfig string if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { if apierrors.IsNotFound(err) { - migrateObj, existingManifestHash, existingLastModifiedConfig, err := migrateToConfigMap(currentObj) - if err != nil { + objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() + if err := r.spokeClient.Update(ctx, currentObj); err != nil { return nil, false, err } - // check if object needs to be migrated - if existingManifestHash != "" && existingLastModifiedConfig != "" { - configMapName = getRandomConfigMapName() - setConfigMapNameAnnotation(configMapName, migrateObj) - manifestHash, lastModifiedConfig = existingManifestHash, existingLastModifiedConfig - } else { - // using this lastModifiedConfig is incorrect, as it's from the member cluster - lastModifiedConfig, err = computeModifiedConfiguration(manifestObj) - if err != nil { - klog.ErrorS(err, "failed compute last applied config", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) - return nil, false, err - } - } - newConfigMap, err := r.createConfigMap(ctx, gvr, currentObj, owner, configMapName, manifestHash, lastModifiedConfig) + newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, manifestHash, lastModifiedConfig) if err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) return nil, false, err } return newConfigMap, true, nil - - } else { - klog.ErrorS(err, "failed to retrieve config map for object", "manifest", currentObj.GetName(), "config map", configMapName) - return nil, false, err } + klog.ErrorS(err, "failed to retrieve config map for object", "manifest", currentObj.GetName(), "config map", configMapName) + return nil, false, err } // need to handle special case where manifest was created with config map name but controller failed, turns out config map belonged to another manifest. + configMapAnnotations := configMap.GetAnnotations() + annotatedManifestIdentifier, ok := configMapAnnotations[manifestIdentifierAnnotation] + manifestIdentifier := getManifestIdentifier(manifestObj) + if annotatedManifestIdentifier != manifestIdentifier { + objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() + if err := r.spokeClient.Update(ctx, currentObj); err != nil { + return nil, false, err + } + newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, "", "") + if err != nil { + klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) + return nil, false, err + } + return newConfigMap, true, nil + } return &configMap, false, nil } @@ -745,11 +780,7 @@ func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1 } // We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. -func migrateToConfigMap(obj *unstructured.Unstructured) (*unstructured.Unstructured, string, string, error) { - annotations := obj.GetAnnotations() - if annotations == nil { - return obj, "", "", errors.New("manifest doesn't have any annotations") - } +func migrateCheck(annotations map[string]string, obj *unstructured.Unstructured) (string, string, error) { // Assuming that manifest has both annotations on it manifestHash, ok1 := annotations[manifestHashAnnotation] lastModifiedConfig, ok2 := annotations[lastAppliedConfigAnnotation] @@ -757,11 +788,11 @@ func migrateToConfigMap(obj *unstructured.Unstructured) (*unstructured.Unstructu delete(annotations, manifestHashAnnotation) delete(annotations, lastAppliedConfigAnnotation) obj.SetAnnotations(annotations) - return obj, manifestHash, lastModifiedConfig, nil + return manifestHash, lastModifiedConfig, nil } else if !ok1 && ok2 || ok1 && !ok2 { - return obj, "", "", errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())) + return "", "", errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())) } - return obj, manifestHash, lastModifiedConfig, nil + return manifestHash, lastModifiedConfig, nil } func getRandomConfigMapName() string { From 8e750a0de5a85b61c69a970b3fa75bd1ac3676bb Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 00:35:37 -0800 Subject: [PATCH 22/32] fix getConfigMap --- pkg/controllers/work/apply_controller.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index b7f60d306..9b9421d7e 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -714,14 +714,14 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group manifestHash = existingManifestHash lastModifiedConfig = existingLastModifiedConfig } - // This will only evaluate to true if object doesn't need to be migrated, it doesn't have config name annotation - if !ok && (manifestHash == "" && lastModifiedConfig == "") { + // if obj doesn't have config name annotation + if !ok { // Need to handle case where manifest has config map annotation removed due to conflict, it needs a new configmap name objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() if err := r.spokeClient.Update(ctx, currentObj); err != nil { return nil, false, err } - newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, "", "") + newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, manifestHash, lastModifiedConfig) if err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) return nil, false, err @@ -730,7 +730,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group } if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { if apierrors.IsNotFound(err) { - objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() + //objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() if err := r.spokeClient.Update(ctx, currentObj); err != nil { return nil, false, err } From 338e7839c4041b18dc39a81ccfb28022d5d7ba05 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 09:59:10 -0800 Subject: [PATCH 23/32] clean up --- pkg/controllers/work/apply_controller.go | 53 ++++++++++-------------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 9b9421d7e..6fec77c9b 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -354,7 +354,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) - if _, err := r.createConfigMap(ctx, gvr, actual, owner, configMapName); err != nil { + if _, err := r.createConfigMap(ctx, gvr, actual, owner, configMapName, "", ""); err != nil { return nil, ManifestNoChangeAction, err } return actual, ManifestCreatedAction, nil @@ -698,7 +698,6 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr } // Need to handle case where manifest obj and configmap annotations -// code reuse func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, bool, error) { var configMap v1.ConfigMap objAnnotations := currentObj.GetAnnotations() @@ -717,29 +716,16 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group // if obj doesn't have config name annotation if !ok { // Need to handle case where manifest has config map annotation removed due to conflict, it needs a new configmap name - objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() - if err := r.spokeClient.Update(ctx, currentObj); err != nil { - return nil, false, err + setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) + if configMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, objAnnotations, manifestHash, lastModifiedConfig); err != nil { + return configMap, true, err } - newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, manifestHash, lastModifiedConfig) - if err != nil { - klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) - return nil, false, err - } - return newConfigMap, true, nil } if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { if apierrors.IsNotFound(err) { - //objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() - if err := r.spokeClient.Update(ctx, currentObj); err != nil { - return nil, false, err - } - newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, manifestHash, lastModifiedConfig) - if err != nil { - klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) - return nil, false, err + if configMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, objAnnotations, manifestHash, lastModifiedConfig); err != nil { + return configMap, true, err } - return newConfigMap, true, nil } klog.ErrorS(err, "failed to retrieve config map for object", "manifest", currentObj.GetName(), "config map", configMapName) return nil, false, err @@ -749,20 +735,27 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group annotatedManifestIdentifier, ok := configMapAnnotations[manifestIdentifierAnnotation] manifestIdentifier := getManifestIdentifier(manifestObj) if annotatedManifestIdentifier != manifestIdentifier { - objAnnotations[configMapNameAnnotation] = getRandomConfigMapName() - if err := r.spokeClient.Update(ctx, currentObj); err != nil { - return nil, false, err + setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) + if configMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, objAnnotations, manifestHash, lastModifiedConfig); err != nil { + return configMap, true, err } - newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, "", "") - if err != nil { - klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) - return nil, false, err - } - return newConfigMap, true, nil } return &configMap, false, nil } +func (r *ApplyWorkReconciler) updateCurrentObjectAndCreateConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, + owner metav1.OwnerReference, curObjAnnotations map[string]string, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { + if err := r.spokeClient.Update(ctx, currentObj); err != nil { + return nil, err + } + newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, curObjAnnotations[configMapNameAnnotation], manifestHash, lastModifiedConfig) + if err != nil { + klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) + return nil, err + } + return newConfigMap, nil +} + func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1.ConfigMap, obj *unstructured.Unstructured, manifestHash string) error { updatedLastModifiedConfig, err := computeModifiedConfiguration(obj) if err != nil { @@ -792,7 +785,7 @@ func migrateCheck(annotations map[string]string, obj *unstructured.Unstructured) } else if !ok1 && ok2 || ok1 && !ok2 { return "", "", errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())) } - return manifestHash, lastModifiedConfig, nil + return "", "", nil } func getRandomConfigMapName() string { From 70f93133fd3df89f2e486b58213e6e5615fcd96b Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 10:29:41 -0800 Subject: [PATCH 24/32] check for annotations is nil --- pkg/controllers/work/apply_controller.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 6fec77c9b..4ea9763c0 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -650,10 +650,6 @@ func buildManifestAppliedCondition(err error, action applyAction, observedGenera } } -//func (r *ApplyWorkReconciler) migrateCreateConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured, owner metav1.OwnerReference, configMapName, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { -// -//} - func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured, owner metav1.OwnerReference, configMapName, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { configMap := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -702,6 +698,9 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group var configMap v1.ConfigMap objAnnotations := currentObj.GetAnnotations() // add check to see if objAnnotations is not nil + if objAnnotations != nil { + objAnnotations = map[string]string{} + } configMapName, ok := objAnnotations[configMapNameAnnotation] // migrate check existingManifestHash, existingLastModifiedConfig, err := migrateCheck(objAnnotations, currentObj) From ec366cfa9395aea7ee24ea39570eddb007e62812 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 10:34:48 -0800 Subject: [PATCH 25/32] fix comment --- pkg/controllers/work/patch_util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index 5e97cb783..0bc7fc119 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -107,7 +107,7 @@ func setModifiedConfigurationAnnotation(obj runtime.Object) error { return metadataAccessor.SetAnnotations(obj, annotations) } -// setModifiedConfigurationAnnotation serializes the object into byte stream and returns it. +// computeModifiedConfiguration computes the json marshal and returns it. func computeModifiedConfiguration(obj runtime.Object) (string, error) { modified, err := json.Marshal(obj) if err != nil { From 35b467cf7945ada6f7d0dc702e98a7a0c0f22f87 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 11:25:24 -0800 Subject: [PATCH 26/32] fix manifestIdentifier --- pkg/controllers/work/apply_controller.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 4ea9763c0..957002440 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -675,7 +675,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr configMap.Data[lastAppliedConfigKey] = lastModifiedConfig // add applied work owner reference addOwnerRef(owner, configMap) - setManifestIdentifierAnnotation(manifestObj, configMap) + setManifestIdentifierAnnotation(gvr, manifestObj, configMap) if err := r.spokeClient.Create(ctx, configMap); err != nil { if apierrors.IsAlreadyExists(err) { // delete config map annotation from manifest @@ -732,7 +732,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group // need to handle special case where manifest was created with config map name but controller failed, turns out config map belonged to another manifest. configMapAnnotations := configMap.GetAnnotations() annotatedManifestIdentifier, ok := configMapAnnotations[manifestIdentifierAnnotation] - manifestIdentifier := getManifestIdentifier(manifestObj) + manifestIdentifier := getManifestIdentifier(gvr, manifestObj) if annotatedManifestIdentifier != manifestIdentifier { setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) if configMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, objAnnotations, manifestHash, lastModifiedConfig); err != nil { @@ -801,8 +801,8 @@ func setConfigMapNameAnnotation(configMapName string, obj *unstructured.Unstruct obj.SetAnnotations(annotations) } -func setManifestIdentifierAnnotation(obj *unstructured.Unstructured, configMap *v1.ConfigMap) { - manifestId := getManifestIdentifier(obj) +func setManifestIdentifierAnnotation(gvr schema.GroupVersionResource, obj *unstructured.Unstructured, configMap *v1.ConfigMap) { + manifestId := getManifestIdentifier(gvr, obj) annotations := configMap.GetAnnotations() if annotations == nil { annotations = map[string]string{} @@ -811,13 +811,13 @@ func setManifestIdentifierAnnotation(obj *unstructured.Unstructured, configMap * configMap.SetAnnotations(annotations) } -func getManifestIdentifier(obj *unstructured.Unstructured) string { +func getManifestIdentifier(gvr schema.GroupVersionResource, obj *unstructured.Unstructured) string { var manifestId string isNamespaced := len(obj.GetNamespace()) > 0 if isNamespaced { - manifestId = obj.GetName() + "-" + obj.GetNamespace() + manifestId = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() + "-" + obj.GetNamespace() } else { - manifestId = obj.GetName() + manifestId = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() } return manifestId } From ad0aa4037dae856fd2ff9a11a530283b7d37e23f Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 11:56:49 -0800 Subject: [PATCH 27/32] fix check --- pkg/controllers/work/apply_controller.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 957002440..267353309 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -697,8 +697,8 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, bool, error) { var configMap v1.ConfigMap objAnnotations := currentObj.GetAnnotations() - // add check to see if objAnnotations is not nil - if objAnnotations != nil { + if objAnnotations == nil { + klog.InfoS("manifest's annotation is empty") objAnnotations = map[string]string{} } configMapName, ok := objAnnotations[configMapNameAnnotation] @@ -777,6 +777,7 @@ func migrateCheck(annotations map[string]string, obj *unstructured.Unstructured) manifestHash, ok1 := annotations[manifestHashAnnotation] lastModifiedConfig, ok2 := annotations[lastAppliedConfigAnnotation] if ok1 && ok2 { + klog.InfoS("manifest needs to be migrated", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) delete(annotations, manifestHashAnnotation) delete(annotations, lastAppliedConfigAnnotation) obj.SetAnnotations(annotations) @@ -784,6 +785,7 @@ func migrateCheck(annotations map[string]string, obj *unstructured.Unstructured) } else if !ok1 && ok2 || ok1 && !ok2 { return "", "", errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())) } + klog.InfoS("manifest doesn't need to be migrated", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return "", "", nil } From 504fc4cd3aaaa0ef0c71187c9af81f7795b8a0ba Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 13:08:11 -0800 Subject: [PATCH 28/32] fix check --- pkg/controllers/work/apply_controller.go | 25 +++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 267353309..b6666ca24 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -716,15 +716,23 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group if !ok { // Need to handle case where manifest has config map annotation removed due to conflict, it needs a new configmap name setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) - if configMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, objAnnotations, manifestHash, lastModifiedConfig); err != nil { - return configMap, true, err + newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, manifestHash, lastModifiedConfig) + if err != nil { + return nil, false, err } + return newConfigMap, true, nil } + // if configmap name annotation exists if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { + // config map doesn't exist if apierrors.IsNotFound(err) { - if configMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, objAnnotations, manifestHash, lastModifiedConfig); err != nil { - return configMap, true, err + // use existing config map name annotation, since the config map doesn't exist + newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, manifestHash, lastModifiedConfig) + if err != nil { + klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) + return nil, false, err } + return newConfigMap, true, err } klog.ErrorS(err, "failed to retrieve config map for object", "manifest", currentObj.GetName(), "config map", configMapName) return nil, false, err @@ -735,18 +743,21 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group manifestIdentifier := getManifestIdentifier(gvr, manifestObj) if annotatedManifestIdentifier != manifestIdentifier { setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) - if configMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, objAnnotations, manifestHash, lastModifiedConfig); err != nil { - return configMap, true, err + newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, manifestHash, lastModifiedConfig) + if err != nil { + return nil, false, err } + return newConfigMap, true, nil } return &configMap, false, nil } func (r *ApplyWorkReconciler) updateCurrentObjectAndCreateConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, - owner metav1.OwnerReference, curObjAnnotations map[string]string, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { + owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { if err := r.spokeClient.Update(ctx, currentObj); err != nil { return nil, err } + curObjAnnotations := currentObj.GetAnnotations() newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, curObjAnnotations[configMapNameAnnotation], manifestHash, lastModifiedConfig) if err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) From f9f3e33a99f1d7543c83b6312a583b7c24723ef7 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Dec 2022 16:53:19 -0800 Subject: [PATCH 29/32] changes --- pkg/controllers/work/apply_controller.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index b6666ca24..1da36a33d 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -377,8 +377,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } - klog.InfoS("generation for curObj", "generation", curObj.GetGeneration()) - // check if the existing manifest is managed by the work if !isManifestManagedByWork(curObj.GetOwnerReferences()) { err = fmt.Errorf("resource is not managed by the work controller") @@ -392,8 +390,6 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, ManifestNoChangeAction, err } - klog.InfoS("generation for configmap", "generation", curObjConfigMap.GetGeneration()) - // set config map name annotation on manifest object to prevent deletion during patch. setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) @@ -825,12 +821,17 @@ func setManifestIdentifierAnnotation(gvr schema.GroupVersionResource, obj *unstr } func getManifestIdentifier(gvr schema.GroupVersionResource, obj *unstructured.Unstructured) string { - var manifestId string + var manifestId, groupVersion string isNamespaced := len(obj.GetNamespace()) > 0 + if gvr.Group != "" { + groupVersion = gvr.Group + "-" + gvr.Version + } else { + groupVersion = gvr.Version + } if isNamespaced { - manifestId = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() + "-" + obj.GetNamespace() + manifestId = groupVersion + "-" + gvr.Resource + "-" + obj.GetName() + "-" + obj.GetNamespace() } else { - manifestId = gvr.Version + "-" + gvr.Resource + "-" + obj.GetName() + manifestId = groupVersion + "-" + gvr.Resource + "-" + obj.GetName() } return manifestId } From 514e4754085f41c16fce904c86f3c84ced7b7511 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Sun, 18 Dec 2022 15:44:52 -0800 Subject: [PATCH 30/32] Changes --- go.mod | 6 +- go.sum | 12 +++- pkg/controllers/work/apply_controller.go | 83 ++++++++++++++---------- 3 files changed, 60 insertions(+), 41 deletions(-) diff --git a/go.mod b/go.mod index c595f0608..0fe3553dd 100644 --- a/go.mod +++ b/go.mod @@ -17,14 +17,14 @@ require ( github.com/stretchr/testify v1.7.0 go.uber.org/atomic v1.9.0 golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e - golang.org/x/sync v0.0.0-20210220032951-036812b2e83c + golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4 golang.org/x/time v0.0.0-20220210224613-90d013bbcef8 k8s.io/api v0.25.2 k8s.io/apiextensions-apiserver v0.24.2 k8s.io/apimachinery v0.25.2 k8s.io/apiserver v0.24.2 k8s.io/client-go v0.25.2 - k8s.io/component-base v0.24.2 + k8s.io/component-base v0.25.2 k8s.io/klog/v2 v2.70.1 k8s.io/metrics v0.25.2 k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed @@ -43,7 +43,7 @@ require ( github.com/emicklei/go-restful/v3 v3.8.0 // indirect github.com/evanphx/json-patch v4.12.0+incompatible // indirect github.com/fsnotify/fsnotify v1.5.1 // indirect - github.com/go-logr/zapr v1.2.0 // indirect + github.com/go-logr/zapr v1.2.3 // indirect github.com/go-openapi/jsonpointer v0.19.5 // indirect github.com/go-openapi/jsonreference v0.19.6 // indirect github.com/go-openapi/swag v0.21.1 // indirect diff --git a/go.sum b/go.sum index 82fba03f2..2d11a2022 100644 --- a/go.sum +++ b/go.sum @@ -165,10 +165,12 @@ github.com/go-logfmt/logfmt v0.5.0/go.mod h1:wCYkCAKZfumFQihp8CzCvQ3paCTfi41vtzG github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas= github.com/go-logr/logr v0.2.0/go.mod h1:z6/tIYblkpsD+a4lm/fGIIU9mZ+XfAiaFtq7xTgseGU= github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= +github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.2.3 h1:2DntVwHkVopvECVRSlL5PSo9eG+cAkDCuckLubN+rq0= github.com/go-logr/logr v1.2.3/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= -github.com/go-logr/zapr v1.2.0 h1:n4JnPI1T3Qq1SFEi/F8rwLrZERp2bso19PJZDB9dayk= github.com/go-logr/zapr v1.2.0/go.mod h1:Qa4Bsj2Vb+FAVeAKsLD8RLQ+YRJB8YDmOAKxaBQf7Ro= +github.com/go-logr/zapr v1.2.3 h1:a9vnzlIBPQBBkeaR9IuMUfmVOrQlkoC4YfPoFkX3T7A= +github.com/go-logr/zapr v1.2.3/go.mod h1:eIauM6P8qSvTw5o2ez6UEAfGjQKrxQTl5EoK+Qa2oG4= github.com/go-openapi/jsonpointer v0.19.3/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg= github.com/go-openapi/jsonpointer v0.19.5 h1:gZr+CIYByUqjcgeLXnQu2gHYQC9o73G2XUeOFYEICuY= github.com/go-openapi/jsonpointer v0.19.5/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg= @@ -647,8 +649,9 @@ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ= golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4 h1:uVc8UZUe6tr40fFVnUP5Oj+veunVezqYl9z7DYw9xzw= +golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180823144017-11551d06cbcc/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -963,12 +966,15 @@ k8s.io/apimachinery v0.25.2 h1:WbxfAjCx+AeN8Ilp9joWnyJ6xu9OMeS/fsfjK/5zaQs= k8s.io/apimachinery v0.25.2/go.mod h1:hqqA1X0bsgsxI6dXsJ4HnNTBOmJNxyPp8dw3u2fSHwA= k8s.io/apiserver v0.24.2 h1:orxipm5elPJSkkFNlwH9ClqaKEDJJA3yR2cAAlCnyj4= k8s.io/apiserver v0.24.2/go.mod h1:pSuKzr3zV+L+MWqsEo0kHHYwCo77AT5qXbFXP2jbvFI= +k8s.io/apiserver v0.25.2 h1:YePimobk187IMIdnmsMxsfIbC5p4eX3WSOrS9x6FEYw= +k8s.io/apiserver v0.25.2/go.mod h1:30r7xyQTREWCkG2uSjgjhQcKVvAAlqoD+YyrqR6Cn+I= k8s.io/client-go v0.24.2/go.mod h1:zg4Xaoo+umDsfCWr4fCnmLEtQXyCNXCvJuSsglNcV30= k8s.io/client-go v0.25.2 h1:SUPp9p5CwM0yXGQrwYurw9LWz+YtMwhWd0GqOsSiefo= k8s.io/client-go v0.25.2/go.mod h1:i7cNU7N+yGQmJkewcRD2+Vuj4iz7b30kI8OcL3horQ4= k8s.io/code-generator v0.24.2/go.mod h1:dpVhs00hTuTdTY6jvVxvTFCk6gSMrtfRydbhZwHI15w= -k8s.io/component-base v0.24.2 h1:kwpQdoSfbcH+8MPN4tALtajLDfSfYxBDYlXobNWI6OU= k8s.io/component-base v0.24.2/go.mod h1:ucHwW76dajvQ9B7+zecZAP3BVqvrHoOxm8olHEg0nmM= +k8s.io/component-base v0.25.2 h1:Nve/ZyHLUBHz1rqwkjXm/Re6IniNa5k7KgzxZpTfSQY= +k8s.io/component-base v0.25.2/go.mod h1:90W21YMr+Yjg7MX+DohmZLzjsBtaxQDDwaX4YxDkl60= k8s.io/gengo v0.0.0-20210813121822-485abfe95c7c/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E= k8s.io/gengo v0.0.0-20211129171323-c02415ce4185/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E= k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE= diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 1da36a33d..7b5137bef 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -385,7 +385,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // get config map or create it if it doesn't exist - curObjConfigMap, isRecreated, err := r.getConfigMap(ctx, gvr, manifestObj, curObj, owner) + curObjConfigMap, err := r.getConfigMap(ctx, gvr, manifestObj, curObj, owner) if err != nil { return nil, ManifestNoChangeAction, err } @@ -399,7 +399,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // We update the object if its spec hash value has changed or if config map got deleted or never created - if manifestHash != curObjConfigMap.Data[manifestHashKey] || isRecreated { + if manifestHash != curObjConfigMap.Data[manifestHashKey] { return r.patchCurrentResource(ctx, gvr, manifestObj, curObj, manifestHash, curObjConfigMap) } @@ -439,8 +439,10 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche return nil, ManifestNoChangeAction, patchErr } + //TODO: Need to handle case where configmap doesn't get updated + // calculate the values based on obj from hub cluster, only track revisions from hub cluster changes - // passing in manifest hash calculated earlier as ownerRef field might have changed with union + // passing in manifest hash calculated earlier as ownerRef field might have changed with mergeOwnerReference if err := r.updateConfigMap(ctx, curObjConfigMap, manifestObj, manifestHash); err != nil { return nil, ManifestNoChangeAction, err } @@ -671,6 +673,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr configMap.Data[lastAppliedConfigKey] = lastModifiedConfig // add applied work owner reference addOwnerRef(owner, configMap) + // Annotating config map with manifest identifier to create two-way link. setManifestIdentifierAnnotation(gvr, manifestObj, configMap) if err := r.spokeClient.Create(ctx, configMap); err != nil { if apierrors.IsAlreadyExists(err) { @@ -681,6 +684,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr klog.ErrorS(err, "failed to delete config map name annotation on manifest", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace()) return nil, err } + klog.InfoS("config map annotation was removed from manifest since config map with same name already exists", "manifest", manifestObj.GetName(), "namespace", manifestObj.GetNamespace(), "configMap", configMapName) } klog.ErrorS(err, "config map create failed", "configMap", configMap.GetName()) return nil, err @@ -690,34 +694,37 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr } // Need to handle case where manifest obj and configmap annotations -func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, bool, error) { +func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, owner metav1.OwnerReference) (*v1.ConfigMap, error) { var configMap v1.ConfigMap + var manifestHash, lastModifiedConfig string + objAnnotations := currentObj.GetAnnotations() if objAnnotations == nil { klog.InfoS("manifest's annotation is empty") objAnnotations = map[string]string{} + } else { + // Need to check and migrate annotations present on manifests to configmaps + existingManifestHash, existingLastModifiedConfig, err := getExistingAnnotationAndRemove(currentObj) + if err != nil { + return nil, err + } + if existingManifestHash != "" && existingLastModifiedConfig != "" { + manifestHash = existingManifestHash + lastModifiedConfig = existingLastModifiedConfig + } } + configMapName, ok := objAnnotations[configMapNameAnnotation] - // migrate check - existingManifestHash, existingLastModifiedConfig, err := migrateCheck(objAnnotations, currentObj) - if err != nil { - return nil, false, err - } - var manifestHash, lastModifiedConfig string - if existingManifestHash != "" && existingLastModifiedConfig != "" { - manifestHash = existingManifestHash - lastModifiedConfig = existingLastModifiedConfig - } - // if obj doesn't have config name annotation if !ok { - // Need to handle case where manifest has config map annotation removed due to conflict, it needs a new configmap name + // Need to handle case where manifest has config map annotation removed due to conflict, it needs a new configmap name or old manifest objects won't have a config map associated with them setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, manifestHash, lastModifiedConfig) if err != nil { - return nil, false, err + return nil, err } - return newConfigMap, true, nil + return newConfigMap, nil } + // if configmap name annotation exists if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: configMapName, Namespace: fleetSystemNamespace}, &configMap); err != nil { // config map doesn't exist @@ -725,15 +732,16 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group // use existing config map name annotation, since the config map doesn't exist newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, manifestHash, lastModifiedConfig) if err != nil { - klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) - return nil, false, err + klog.ErrorS(err, "cannot create config map for manifest", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) + return nil, err } - return newConfigMap, true, err + return newConfigMap, err } - klog.ErrorS(err, "failed to retrieve config map for object", "manifest", currentObj.GetName(), "config map", configMapName) - return nil, false, err + klog.ErrorS(err, "failed to retrieve config map for manifest", "manifest", currentObj.GetName(), "config map", configMapName) + return nil, err } - // need to handle special case where manifest was created with config map name but controller failed, turns out config map belonged to another manifest. + + // need to handle special case where manifest was created with config map name but controller failed without checking if config map already exists, turns out config map belonged to another manifest. configMapAnnotations := configMap.GetAnnotations() annotatedManifestIdentifier, ok := configMapAnnotations[manifestIdentifierAnnotation] manifestIdentifier := getManifestIdentifier(gvr, manifestObj) @@ -741,11 +749,11 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, manifestHash, lastModifiedConfig) if err != nil { - return nil, false, err + return nil, err } - return newConfigMap, true, nil + return newConfigMap, nil } - return &configMap, false, nil + return &configMap, nil } func (r *ApplyWorkReconciler) updateCurrentObjectAndCreateConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, @@ -779,20 +787,25 @@ func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1 } // We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. -func migrateCheck(annotations map[string]string, obj *unstructured.Unstructured) (string, string, error) { +func getExistingAnnotationAndRemove(obj *unstructured.Unstructured) (string, string, error) { + objAnnotations := obj.GetAnnotations() + if objAnnotations == nil { + klog.InfoS("annotations is nil, no existing annotations to migrate", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + return "", "", nil + } // Assuming that manifest has both annotations on it - manifestHash, ok1 := annotations[manifestHashAnnotation] - lastModifiedConfig, ok2 := annotations[lastAppliedConfigAnnotation] + manifestHash, ok1 := objAnnotations[manifestHashAnnotation] + lastModifiedConfig, ok2 := objAnnotations[lastAppliedConfigAnnotation] if ok1 && ok2 { - klog.InfoS("manifest needs to be migrated", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) - delete(annotations, manifestHashAnnotation) - delete(annotations, lastAppliedConfigAnnotation) - obj.SetAnnotations(annotations) + klog.InfoS("annotation on manifest needs to be migrated", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + delete(objAnnotations, manifestHashAnnotation) + delete(objAnnotations, lastAppliedConfigAnnotation) + obj.SetAnnotations(objAnnotations) return manifestHash, lastModifiedConfig, nil } else if !ok1 && ok2 || ok1 && !ok2 { return "", "", errors.New(fmt.Sprintf("manifest (%s/%s)cannot be migrated because it doesn't have manifestHash/lastModified Annotation", obj.GetName(), obj.GetNamespace())) } - klog.InfoS("manifest doesn't need to be migrated", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) + klog.InfoS("manifest doesn't contain annotations that need to be migrated", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) return "", "", nil } From a0f41d78cb7cc57bd90f92e6d057ecc0573887e8 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 19 Dec 2022 11:41:22 -0800 Subject: [PATCH 31/32] Version check for config map --- pkg/controllers/work/apply_controller.go | 29 +++++++++++++++++------- pkg/controllers/work/manager.go | 1 + 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 7b5137bef..87a8603ac 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -391,7 +391,18 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // set config map name annotation on manifest object to prevent deletion during patch. - setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) + // setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) + + if curObj.GetResourceVersion() != curObjConfigMap.Data[versionKey] { + // Need to update the config map, with current Object because the controller failed to update config map after patch + manifestHash, err := computeManifestHash(curObj) + if err != nil { + return nil, ManifestNoChangeAction, err + } + if err := r.updateConfigMap(ctx, curObjConfigMap, curObj, manifestHash); err != nil { + return nil, ManifestNoChangeAction, err + } + } manifestHash, err := computeManifestHash(manifestObj) if err != nil { @@ -671,6 +682,7 @@ func (r *ApplyWorkReconciler) createConfigMap(ctx context.Context, gvr schema.Gr } configMap.Data[manifestHashKey] = manifestHash configMap.Data[lastAppliedConfigKey] = lastModifiedConfig + configMap.Data[versionKey] = manifestObj.GetResourceVersion() // add applied work owner reference addOwnerRef(owner, configMap) // Annotating config map with manifest identifier to create two-way link. @@ -704,7 +716,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group objAnnotations = map[string]string{} } else { // Need to check and migrate annotations present on manifests to configmaps - existingManifestHash, existingLastModifiedConfig, err := getExistingAnnotationAndRemove(currentObj) + existingManifestHash, existingLastModifiedConfig, err := getAndRemoveExistingAnnotation(currentObj) if err != nil { return nil, err } @@ -718,7 +730,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group if !ok { // Need to handle case where manifest has config map annotation removed due to conflict, it needs a new configmap name or old manifest objects won't have a config map associated with them setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) - newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, manifestHash, lastModifiedConfig) + newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, currentObj, owner, manifestHash, lastModifiedConfig) if err != nil { return nil, err } @@ -730,7 +742,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group // config map doesn't exist if apierrors.IsNotFound(err) { // use existing config map name annotation, since the config map doesn't exist - newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, configMapName, manifestHash, lastModifiedConfig) + newConfigMap, err := r.createConfigMap(ctx, gvr, currentObj, owner, configMapName, manifestHash, lastModifiedConfig) if err != nil { klog.ErrorS(err, "cannot create config map for manifest", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) return nil, err @@ -747,7 +759,7 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group manifestIdentifier := getManifestIdentifier(gvr, manifestObj) if annotatedManifestIdentifier != manifestIdentifier { setConfigMapNameAnnotation(getRandomConfigMapName(), currentObj) - newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, manifestObj, currentObj, owner, manifestHash, lastModifiedConfig) + newConfigMap, err := r.updateCurrentObjectAndCreateConfigMap(ctx, gvr, currentObj, owner, manifestHash, lastModifiedConfig) if err != nil { return nil, err } @@ -756,13 +768,14 @@ func (r *ApplyWorkReconciler) getConfigMap(ctx context.Context, gvr schema.Group return &configMap, nil } -func (r *ApplyWorkReconciler) updateCurrentObjectAndCreateConfigMap(ctx context.Context, gvr schema.GroupVersionResource, manifestObj, currentObj *unstructured.Unstructured, +func (r *ApplyWorkReconciler) updateCurrentObjectAndCreateConfigMap(ctx context.Context, gvr schema.GroupVersionResource, currentObj *unstructured.Unstructured, owner metav1.OwnerReference, manifestHash, lastModifiedConfig string) (*v1.ConfigMap, error) { if err := r.spokeClient.Update(ctx, currentObj); err != nil { return nil, err } curObjAnnotations := currentObj.GetAnnotations() - newConfigMap, err := r.createConfigMap(ctx, gvr, manifestObj, owner, curObjAnnotations[configMapNameAnnotation], manifestHash, lastModifiedConfig) + // we need to use current Object here since manifest Object is from hub cluster + newConfigMap, err := r.createConfigMap(ctx, gvr, currentObj, owner, curObjAnnotations[configMapNameAnnotation], manifestHash, lastModifiedConfig) if err != nil { klog.ErrorS(err, "cannot create config map for object", "manifest", currentObj.GetName(), "namespace", currentObj.GetNamespace()) return nil, err @@ -787,7 +800,7 @@ func (r *ApplyWorkReconciler) updateConfigMap(ctx context.Context, configMap *v1 } // We can calculate the manifestHash & lastAppliedConfig again wiping out previous state or make sure the annotations in the last applied config doesn't cause issue in patch. -func getExistingAnnotationAndRemove(obj *unstructured.Unstructured) (string, string, error) { +func getAndRemoveExistingAnnotation(obj *unstructured.Unstructured) (string, string, error) { objAnnotations := obj.GetAnnotations() if objAnnotations == nil { klog.InfoS("annotations is nil, no existing annotations to migrate", "manifest", obj.GetName(), "namespace", obj.GetNamespace()) diff --git a/pkg/controllers/work/manager.go b/pkg/controllers/work/manager.go index 9e47f2589..061a5bda4 100644 --- a/pkg/controllers/work/manager.go +++ b/pkg/controllers/work/manager.go @@ -39,6 +39,7 @@ const ( manifestHashKey = "fleet-spec-hash" lastAppliedConfigKey = "fleet-last-applied-configuration" + versionKey = "fleet-version-key" ConditionTypeApplied = "Applied" ConditionTypeAvailable = "Available" From 16f8d37f967991faa61f269cd1d02325ba807193 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 21 Dec 2022 13:16:24 -0800 Subject: [PATCH 32/32] change --- pkg/controllers/work/apply_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 87a8603ac..7e8ead482 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -391,7 +391,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. } // set config map name annotation on manifest object to prevent deletion during patch. - // setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) + setConfigMapNameAnnotation(curObjConfigMap.GetName(), manifestObj) if curObj.GetResourceVersion() != curObjConfigMap.Data[versionKey] { // Need to update the config map, with current Object because the controller failed to update config map after patch