From 47d92c189ae7a3807d1a85a1a06dc97cb697c241 Mon Sep 17 00:00:00 2001 From: jlandowner Date: Fri, 28 Jul 2023 14:08:52 +0900 Subject: [PATCH] fix gc --- api/v1alpha1/utils.go | 28 +++++++ internal/controllers/instance_controller.go | 83 +++++++++++---------- internal/controllers/user_controller.go | 29 +++---- pkg/transformer/metadata.go | 11 +-- pkg/transformer/metadata_test.go | 51 +++++++++++++ 5 files changed, 138 insertions(+), 64 deletions(-) diff --git a/api/v1alpha1/utils.go b/api/v1alpha1/utils.go index d2adcf89..c0152073 100644 --- a/api/v1alpha1/utils.go +++ b/api/v1alpha1/utils.go @@ -1,5 +1,7 @@ package v1alpha1 +import "strconv" + // LabelControllerManaged is a label on all resources managed by the controllers const LabelControllerManaged = "cosmo-workspace.github.io/controller-managed" @@ -17,3 +19,29 @@ func SetControllerManaged(obj LabelHolder) { labels[LabelControllerManaged] = "1" obj.SetLabels(labels) } + +// +kubebuilder:object:generate=false +type AnnotationHolder interface { + GetAnnotations() map[string]string + SetAnnotations(map[string]string) +} + +// AnnotationPruneDisabled is a bool annotation for each child resources not to be deleted in GC +const AnnotationPruneDisabled = "cosmo-workspace.github.io/prune-disabled" + +func IsPruneDisabled(obj AnnotationHolder) bool { + ann := obj.GetAnnotations() + if ann == nil { + return false + } + v, ok := ann[AnnotationPruneDisabled] + if !ok { + return false + } + isDisabled, err := strconv.ParseBool(v) + if err != nil { + // invalid bool value might be accidentally set while trying to be true + return true + } + return isDisabled +} diff --git a/internal/controllers/instance_controller.go b/internal/controllers/instance_controller.go index a4fb0213..390e3923 100644 --- a/internal/controllers/instance_controller.go +++ b/internal/controllers/instance_controller.go @@ -115,14 +115,13 @@ func (r *instanceReconciler) reconcileObjects(ctx context.Context, inst cosmov1a log := clog.FromContext(ctx).WithCaller() errs := make([]error, 0) - lastAppliedMap := sliceToObjectMap(inst.GetStatus().LastApplied) - lastApplied := make([]cosmov1alpha1.ObjectRef, len(inst.GetStatus().LastApplied)) copy(lastApplied, inst.GetStatus().LastApplied) currAppliedMap := make(map[types.UID]cosmov1alpha1.ObjectRef) + + // check dry-run apply on first reconciliation if len(inst.GetStatus().LastApplied) == 0 { - // first reconcile for _, built := range objects { if _, err := r.dryrunApply(ctx, &built, r.FieldManager); err != nil { // ignore NotFound in case the template contains a dependency resource that was not found. @@ -161,12 +160,11 @@ func (r *instanceReconciler) reconcileObjects(ctx context.Context, inst cosmov1a created, err := r.apply(ctx, &built, r.FieldManager) if err != nil { errs = append(errs, fmt.Errorf("failed to create resource: kind = %s name = %s: %w", built.GetKind(), built.GetName(), err)) - continue + } else { + r.Recorder.Eventf(inst, corev1.EventTypeNormal, "Synced", "%s %s created", built.GetKind(), built.GetName()) } - - r.Recorder.Eventf(inst, corev1.EventTypeNormal, "Synced", "%s %s created", built.GetKind(), built.GetName()) - currAppliedMap[created.GetUID()] = unstToObjectRef(created) + } else { errs = append(errs, fmt.Errorf("failed to get resource: kind = %s name = %s: %w", built.GetKind(), built.GetName(), err)) continue @@ -179,11 +177,7 @@ func (r *instanceReconciler) reconcileObjects(ctx context.Context, inst cosmov1a errs = append(errs, fmt.Errorf("dryrun failed: kind=%s name=%s: %w", built.GetKind(), built.GetName(), err)) continue } - if l, ok := lastAppliedMap[desired.GetUID()]; !ok { - currAppliedMap[desired.GetUID()] = l - } else { - currAppliedMap[desired.GetUID()] = unstToObjectRef(desired) - } + currAppliedMap[desired.GetUID()] = unstToObjectRef(desired) // compare current with the desired state if !kubeutil.LooseDeepEqual(current, desired) { @@ -194,44 +188,29 @@ func (r *instanceReconciler) reconcileObjects(ctx context.Context, inst cosmov1a log.DumpObject(r.Scheme, &built, "applying object") if _, err := r.apply(ctx, &built, r.FieldManager); err != nil { errs = append(errs, fmt.Errorf("failed to apply resource %s %s: %w", built.GetKind(), built.GetName(), err)) - continue + } else { + r.Recorder.Eventf(inst, corev1.EventTypeNormal, "Synced", "%s %s is not desired state, synced", built.GetKind(), built.GetName()) } - - r.Recorder.Eventf(inst, corev1.EventTypeNormal, "Synced", "%s %s is not desired state, synced", built.GetKind(), built.GetName()) - - currAppliedMap[desired.GetUID()] = unstToObjectRef(desired) } } } - for k, v := range currAppliedMap { - lastAppliedMap[k] = v - } - // garbage collection - shouldDeletes := objectRefNotExistsInMap(lastApplied, currAppliedMap) - for _, d := range shouldDeletes { - log.Info("start garbage collection", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name) - delete(lastAppliedMap, d.UID) - - var obj unstructured.Unstructured - obj.SetAPIVersion(d.APIVersion) - obj.SetKind(d.Kind) - err := r.Get(ctx, types.NamespacedName{Name: d.GetName(), Namespace: d.Namespace}, &obj) - if err != nil { - if !apierrs.IsNotFound(err) { - log.Error(err, "failed to get object to be deleted", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name) + if len(errs) == 0 && !cosmov1alpha1.IsPruneDisabled(inst) { + log.Info("start garbage collection") + shouldDeletes := objectRefNotExistsInMap(lastApplied, currAppliedMap) + for _, d := range shouldDeletes { + if skip, err := prune(ctx, r.Client, d); err != nil { + log.Error(err, "failed to delete unused obj", "pruneAPIVersion", d.APIVersion, "pruneKind", d.Kind, "pruneName", d.Name, "pruneNamespace", d.Namespace) + r.Recorder.Eventf(inst, corev1.EventTypeWarning, "GCFailed", "failed to delete unused obj: kind=%s name=%s namespace=%s", d.Kind, d.Name, d.Namespace) + } else if !skip { + log.Info("deleted unmanaged object", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name, "namespace", d.Namespace) + r.Recorder.Eventf(inst, corev1.EventTypeNormal, "GC", "deleted unmanaged object: kind=%s name=%s namespace=%s", d.Kind, d.Name, d.Namespace) } - continue - } - - if err := r.Delete(ctx, &obj); err != nil { - r.Recorder.Eventf(inst, corev1.EventTypeWarning, "GCFailed", "failed to delete unused obj: %s %s", obj.GetKind(), obj.GetName()) } - r.Recorder.Eventf(inst, corev1.EventTypeNormal, "GC", "deleted unmanaged object: %s %s", obj.GetKind(), obj.GetName()) } - inst.GetStatus().LastApplied = objectRefMapToSlice(lastAppliedMap) + inst.GetStatus().LastApplied = objectRefMapToSlice(currAppliedMap) inst.GetStatus().LastAppliedObjectsCount = len(inst.GetStatus().LastApplied) return errs @@ -287,3 +266,27 @@ func objectRefNotExistsInMap(s []cosmov1alpha1.ObjectRef, m map[types.UID]cosmov } return notExists } + +func prune(ctx context.Context, r client.Client, d cosmov1alpha1.ObjectRef) (skip bool, err error) { + log := clog.FromContext(ctx).WithValues("pruneAPIVersion", d.APIVersion, "pruneKind", d.Kind, "pruneName", d.Name, "pruneNamespace", d.Namespace) + + var obj unstructured.Unstructured + obj.SetAPIVersion(d.APIVersion) + obj.SetKind(d.Kind) + err = r.Get(ctx, types.NamespacedName{Name: d.Name, Namespace: d.Namespace}, &obj) + if err != nil { + log.Error(err, "failed to get object to be deleted") + return true, nil + } + + if obj.GetUID() != d.UID { + log.Error(err, "target object UID is changed. skip pruning", "desiredUID", d.UID, "currentUID", obj.GetUID()) + return true, nil + } + if cosmov1alpha1.IsPruneDisabled(&obj) { + log.Debug().Info("skip pruning by annotation", "apiVersion") + return true, nil + } + + return false, r.Delete(ctx, &obj) +} diff --git a/internal/controllers/user_controller.go b/internal/controllers/user_controller.go index 7d37d8a0..dd9fbeee 100644 --- a/internal/controllers/user_controller.go +++ b/internal/controllers/user_controller.go @@ -8,7 +8,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -157,26 +156,18 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. log.Debug().Info("status updated") } - // garbage collection - shouldDeletes := objectRefNotExistsInMap(lastAddons, currAddonsMap) - for _, d := range shouldDeletes { - log.Info("start garbage collection", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name) - - var obj unstructured.Unstructured - obj.SetAPIVersion(d.APIVersion) - obj.SetKind(d.Kind) - err := r.Get(ctx, types.NamespacedName{Name: d.GetName(), Namespace: d.Namespace}, &obj) - if err != nil { - if !apierrs.IsNotFound(err) { - log.Error(err, "failed to get object to be deleted", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name) + if user.Status.Phase != "AddonFailed" && !cosmov1alpha1.IsPruneDisabled(&user) { + log.Info("start garbage collection") + shouldDeletes := objectRefNotExistsInMap(lastAddons, currAddonsMap) + for _, d := range shouldDeletes { + if skip, err := prune(ctx, r.Client, d); err != nil { + log.Error(err, "failed to delete unused addon", "pruneAPIVersion", d.APIVersion, "pruneKind", d.Kind, "pruneName", d.Name, "pruneNamespace", d.Namespace) + r.Recorder.Eventf(&user, corev1.EventTypeWarning, "GCFailed", "failed to delete unused addon: kind=%s name=%s namespace=%s", d.Kind, d.Name, d.Namespace) + } else if !skip { + log.Info("deleted unmanaged addon", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name, "namespace", d.Namespace) + r.Recorder.Eventf(&user, corev1.EventTypeNormal, "GC", "deleted unmanaged addon: kind=%s name=%s namespace=%s", d.Kind, d.Name, d.Namespace) } - continue - } - - if err := r.Delete(ctx, &obj); err != nil { - r.Recorder.Eventf(&user, corev1.EventTypeWarning, "GCFailed", "failed to delete unused addon: %s %s", obj.GetKind(), obj.GetName()) } - r.Recorder.Eventf(&user, corev1.EventTypeNormal, "GC", "deleted unmanaged addon: %s %s", obj.GetKind(), obj.GetName()) } log.Debug().Info("finish reconcile") diff --git a/pkg/transformer/metadata.go b/pkg/transformer/metadata.go index 51595070..a7f1fde0 100644 --- a/pkg/transformer/metadata.go +++ b/pkg/transformer/metadata.go @@ -45,11 +45,12 @@ func (t *MetadataTransformer) Transform(src *unstructured.Unstructured) (*unstru labels[cosmov1alpha1.LabelKeyTemplateName] = t.tmplName obj.SetLabels(labels) - // Set owner reference - err := ctrl.SetControllerReference(t.inst, obj, t.scheme) - if err != nil { - return nil, fmt.Errorf("failed to set owner reference on %s: %w", obj.GetObjectKind().GroupVersionKind(), err) + if !cosmov1alpha1.IsPruneDisabled(t.inst) && !cosmov1alpha1.IsPruneDisabled(obj) { + // Set owner reference + err := ctrl.SetControllerReference(t.inst, obj, t.scheme) + if err != nil { + return nil, fmt.Errorf("failed to set owner reference on %s: %w", obj.GetObjectKind().GroupVersionKind(), err) + } } - return obj, nil } diff --git a/pkg/transformer/metadata_test.go b/pkg/transformer/metadata_test.go index 26176298..eed4a441 100644 --- a/pkg/transformer/metadata_test.go +++ b/pkg/transformer/metadata_test.go @@ -270,6 +270,57 @@ metadata: }, wantErr: true, }, + { + name: "not put owner ref when prune-disabled annotated", + fields: fields{ + inst: &cosmov1alpha1.Instance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cs1", + Namespace: "cosmo-user-tom", + Annotations: map[string]string{ + cosmov1alpha1.AnnotationPruneDisabled: "1", + }, + }, + Spec: cosmov1alpha1.InstanceSpec{ + Template: cosmov1alpha1.TemplateRef{ + Name: "code-server", + }, + Override: cosmov1alpha1.OverrideSpec{}, + Vars: map[string]string{"{{TEST}}": "OK"}, + }, + }, + disableNamePrefix: false, + scheme: scheme, + }, + args: args{ + src: `apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + annotations: + cosmo/ingress-patch-enable: "true" + kubernetes.io/ingress.class: alb + name: test + namespace: cosmo-user-tom +spec: + host: example.com +`, + }, + want: `apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + annotations: + cosmo/ingress-patch-enable: "true" + kubernetes.io/ingress.class: alb + labels: + cosmo-workspace.github.io/instance: cs1 + cosmo-workspace.github.io/template: code-server + name: cs1-test + namespace: cosmo-user-tom +spec: + host: example.com +`, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {