From ad039ca93ea62c13a504d09b753c9b00e3ab3be8 Mon Sep 17 00:00:00 2001 From: janiskemper <63146658+janiskemper@users.noreply.github.com> Date: Mon, 27 Nov 2023 12:48:38 +0100 Subject: [PATCH] :bug: Fix deleting resources from Helm charts (#31) There are two problems with deleting resources from Helm charts right now: The first one is that the objects stay in the status.resources list, even after they are deleted. The second is that instead of returning an error when a deletion failed, we returned an error on success. Instead, as we base deletion of objects based on the status.resources list (this can be changed in the future), we keep resources that are supposed to be deleted but failed to in the status as not-synced. Additionally, we requeue and try the deletion another time, just as we do with applying currently. As the current Update method is unused since we changed from create to apply, it is deleted. Mocks are generated freshly as well. Signed-off-by: janiskemper --- .../controller/clusteraddon_controller.go | 2 + pkg/kube/helpers.go | 10 +--- pkg/kube/kube.go | 57 +++++-------------- pkg/kube/mocks/Client.go | 33 ----------- 4 files changed, 19 insertions(+), 83 deletions(-) diff --git a/internal/controller/clusteraddon_controller.go b/internal/controller/clusteraddon_controller.go index d2bfe285b..7e13a7b34 100644 --- a/internal/controller/clusteraddon_controller.go +++ b/internal/controller/clusteraddon_controller.go @@ -200,6 +200,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in) if err != nil { + conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply") return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err) } if shouldRequeue { @@ -240,6 +241,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in) if err != nil { + conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply") return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err) } if shouldRequeue { diff --git a/pkg/kube/helpers.go b/pkg/kube/helpers.go index 7c1c1db33..e0fcfbdff 100644 --- a/pkg/kube/helpers.go +++ b/pkg/kube/helpers.go @@ -105,17 +105,13 @@ func parseK8sYaml(template []byte) ([]*unstructured.Unstructured, error) { return objs, nil } -func getResourceMap(resources []*csov1alpha1.Resource) (resourceMap map[types.NamespacedName]*csov1alpha1.Resource, notToApply []*csov1alpha1.Resource) { - resourceMap = make(map[types.NamespacedName]*csov1alpha1.Resource) - notToApply = make([]*csov1alpha1.Resource, 0, len(resources)) +func getResourceMap(resources []*csov1alpha1.Resource) map[types.NamespacedName]*csov1alpha1.Resource { + resourceMap := make(map[types.NamespacedName]*csov1alpha1.Resource) for i, resource := range resources { - if resource.Status == csov1alpha1.ResourceStatusSynced { - notToApply = append(notToApply, resources[i]) - } resourceMap[types.NamespacedName{Name: resource.Name, Namespace: resource.Namespace}] = resources[i] } - return resourceMap, notToApply + return resourceMap } func setLabel(target *unstructured.Unstructured, key, val string) error { diff --git a/pkg/kube/kube.go b/pkg/kube/kube.go index c69d8b827..5ee522232 100644 --- a/pkg/kube/kube.go +++ b/pkg/kube/kube.go @@ -22,6 +22,7 @@ import ( "fmt" csov1alpha1 "github.com/SovereignCloudStack/cluster-stack-operator/api/v1alpha1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" @@ -37,7 +38,6 @@ type kube struct { // Client has all the meathod for helm chart kube operation. type Client interface { Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) - Update(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) Delete(template []byte) error } @@ -72,12 +72,13 @@ func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1 return nil, false, fmt.Errorf("couldn't parse k8s yaml: %w", err) } - resourceMap, newResources := getResourceMap(oldResources) + resourceMap := getResourceMap(oldResources) for _, obj := range objs { oldResource, found := resourceMap[types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}] // do nothing if synced if found && oldResource.Status == csov1alpha1.ResourceStatusSynced { + newResources = append(newResources, oldResource) continue } @@ -103,45 +104,7 @@ func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1 reterr := fmt.Errorf("failed to apply object: %w", err) resource.Error = reterr.Error() resource.Status = csov1alpha1.ResourceStatusNotSynced - logger.Error(reterr, "failed to apply object", "obj", obj.GetObjectKind().GroupVersionKind()) - shouldRequeue = true - } else { - resource.Status = csov1alpha1.ResourceStatusSynced - } - - newResources = append(newResources, resource) - } - - return newResources, shouldRequeue, nil -} - -func (k *kube) Update(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) { - logger := log.FromContext(ctx) - - objs, err := parseK8sYaml(template) - if err != nil { - return nil, false, fmt.Errorf("couldn't parse k8s yaml: %w", err) - } - - for _, obj := range objs { - if err := setLabel(obj, ObjectLabelKeyOwned, ObjectLabelValueOwned); err != nil { - return nil, false, fmt.Errorf("error setting label: %w", err) - } - - // call the function and get dynamic.ResourceInterface - // getDynamicResourceInterface - dr, err := getDynamicResourceInterface(k.Namespace, k.RestConfig, obj.GroupVersionKind()) - if err != nil { - return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", err) - } - - resource := csov1alpha1.NewResourceFromUnstructured(obj) - - if _, err := dr.Apply(ctx, obj.GetName(), obj, metav1.ApplyOptions{FieldManager: "kubectl", Force: true}); err != nil { - reterr := fmt.Errorf("failed to apply object: %w", err) - resource.Error = reterr.Error() - resource.Status = csov1alpha1.ResourceStatusNotSynced - logger.Error(reterr, "failed to apply object", "obj", obj.GetObjectKind().GroupVersionKind()) + logger.Error(reterr, "failed to apply object", "obj", obj.GetObjectKind().GroupVersionKind(), "name", obj.GetName(), "namespace", obj.GetNamespace()) shouldRequeue = true } else { resource.Status = csov1alpha1.ResourceStatusSynced @@ -163,10 +126,18 @@ func (k *kube) Update(ctx context.Context, template []byte, oldResources []*csov return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", err) } - if err := dr.Delete(ctx, resource.Name, metav1.DeleteOptions{}); err == nil { - return nil, false, fmt.Errorf("failed to delete object %q of namespace %q: %w", resource.Name, resource.Namespace, err) + if err := dr.Delete(ctx, resource.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + reterr := fmt.Errorf("failed to delete object: %w", err) + logger.Error(reterr, "failed to delete object", "obj", resource.GroupVersionKind(), "namespacedName", resource.NamespacedName()) + + // append resource to status and requeue again to be able to retry deletion + resource.Status = csov1alpha1.ResourceStatusNotSynced + resource.Error = reterr.Error() + newResources = append(newResources, resource) + shouldRequeue = true } } + return newResources, shouldRequeue, nil } diff --git a/pkg/kube/mocks/Client.go b/pkg/kube/mocks/Client.go index 91d9d4636..798c1b823 100644 --- a/pkg/kube/mocks/Client.go +++ b/pkg/kube/mocks/Client.go @@ -62,39 +62,6 @@ func (_m *Client) Delete(template []byte) error { return r0 } -// Update provides a mock function with given fields: ctx, template, oldResources -func (_m *Client) Update(ctx context.Context, template []byte, oldResources []*v1alpha1.Resource) ([]*v1alpha1.Resource, bool, error) { - ret := _m.Called(ctx, template, oldResources) - - var r0 []*v1alpha1.Resource - var r1 bool - var r2 error - if rf, ok := ret.Get(0).(func(context.Context, []byte, []*v1alpha1.Resource) ([]*v1alpha1.Resource, bool, error)); ok { - return rf(ctx, template, oldResources) - } - if rf, ok := ret.Get(0).(func(context.Context, []byte, []*v1alpha1.Resource) []*v1alpha1.Resource); ok { - r0 = rf(ctx, template, oldResources) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).([]*v1alpha1.Resource) - } - } - - if rf, ok := ret.Get(1).(func(context.Context, []byte, []*v1alpha1.Resource) bool); ok { - r1 = rf(ctx, template, oldResources) - } else { - r1 = ret.Get(1).(bool) - } - - if rf, ok := ret.Get(2).(func(context.Context, []byte, []*v1alpha1.Resource) error); ok { - r2 = rf(ctx, template, oldResources) - } else { - r2 = ret.Error(2) - } - - return r0, r1, r2 -} - // NewClient creates a new instance of Client. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewClient(t interface {