Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved HelmRelease deletion #242

Merged
merged 1 commit into from
Sep 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,6 @@ test/e2e/*.log
*.swp
*.swo
*~

# Vendoring directory
vendor
3 changes: 3 additions & 0 deletions api/v1alpha1/managedcluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,14 @@ import (
)

const (
BlockingFinalizer = "hmc.mirantis.com/cleanup"
ManagedClusterFinalizer = "hmc.mirantis.com/managed-cluster"

FluxHelmChartNameKey = "helm.toolkit.fluxcd.io/name"
HMCManagedLabelKey = "hmc.mirantis.com/managed"
HMCManagedLabelValue = "true"

ClusterNameLabelKey = "cluster.x-k8s.io/cluster-name"
)

const (
Expand Down
124 changes: 122 additions & 2 deletions internal/controller/managedcluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import (
"fmt"
"time"

"k8s.io/apimachinery/pkg/labels"

hcv2 "github.com/fluxcd/helm-controller/api/v2"
fluxmeta "github.com/fluxcd/pkg/apis/meta"
fluxconditions "github.com/fluxcd/pkg/runtime/conditions"
Expand All @@ -33,6 +31,7 @@ import (
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -59,6 +58,30 @@ type ManagedClusterReconciler struct {
SystemNamespace string
}

type providerSchema struct {
machine, cluster schema.GroupVersionKind
}

var (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use const as globals

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

​In Go, constants can only be fixed, literal values.

gvkAWSCluster = schema.GroupVersionKind{
Group: "infrastructure.cluster.x-k8s.io",
Version: "v1beta2",
Kind: "awscluster",
}

gvkAzureCluster = schema.GroupVersionKind{
Group: "infrastructure.cluster.x-k8s.io",
Version: "v1beta1",
Kind: "azurecluster",
}

gvkMachine = schema.GroupVersionKind{
Group: "cluster.x-k8s.io",
Version: "v1beta1",
Kind: "machine",
}
)

// Reconcile is part of the main kubernetes reconciliation loop which aims to
// move the current state of the cluster closer to the desired state.
func (r *ManagedClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
Expand Down Expand Up @@ -385,14 +408,111 @@ func (r *ManagedClusterReconciler) Delete(ctx context.Context, l logr.Logger, ma
}
return ctrl.Result{}, err
}

err = helm.DeleteHelmRelease(ctx, r.Client, managedCluster.Name, managedCluster.Namespace)
if err != nil {
return ctrl.Result{}, err
}

err = r.releaseCluster(ctx, managedCluster.Namespace, managedCluster.Name, managedCluster.Spec.Template)
if err != nil {
return ctrl.Result{}, err
}

l.Info("HelmRelease still exists, retrying")
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}

func (r *ManagedClusterReconciler) releaseCluster(ctx context.Context, namespace, name, templateName string) error {
providers, err := r.getProviders(ctx, templateName)
if err != nil {
return err
}

providerGVKs := map[string]providerSchema{
"aws": {machine: gvkMachine, cluster: gvkAWSCluster},
"azure": {machine: gvkMachine, cluster: gvkAzureCluster},
}

// Associate the provider with it's GVK
for _, provider := range providers {
gvk, ok := providerGVKs[provider]
if !ok {
continue
}

cluster, err := r.getCluster(ctx, namespace, name, gvk.cluster)
if err != nil {
return err
}

found, err := r.machinesAvailable(ctx, namespace, cluster.Name, gvk.machine)
if err != nil {
return err
}

if !found {
return r.removeClusterFinalizer(ctx, cluster)
}
}

return nil
}

func (r *ManagedClusterReconciler) getProviders(ctx context.Context, templateName string) ([]string, error) {
template := &hmc.ClusterTemplate{}
templateRef := types.NamespacedName{Name: templateName, Namespace: r.SystemNamespace}
if err := r.Get(ctx, templateRef, template); err != nil {
log.FromContext(ctx).Error(err, "Failed to get Template", "templateName", templateName)
return nil, err
}
return template.Status.Providers.InfrastructureProviders, nil
}

func (r *ManagedClusterReconciler) getCluster(ctx context.Context, namespace, name string, gvk schema.GroupVersionKind) (*metav1.PartialObjectMetadata, error) {
opts := &client.ListOptions{
LabelSelector: labels.SelectorFromSet(map[string]string{hmc.FluxHelmChartNameKey: name}),
Namespace: namespace,
}
itemsList := &metav1.PartialObjectMetadataList{}
itemsList.SetGroupVersionKind(gvk)
if err := r.Client.List(ctx, itemsList, opts); err != nil {
return nil, err
}
if len(itemsList.Items) == 0 {
return nil, fmt.Errorf("%s with name %s was not found", gvk.Kind, name)
}

return &itemsList.Items[0], nil
}

func (r *ManagedClusterReconciler) removeClusterFinalizer(ctx context.Context, cluster *metav1.PartialObjectMetadata) error {
originalCluster := *cluster
finalizersUpdated := controllerutil.RemoveFinalizer(cluster, hmc.BlockingFinalizer)
if finalizersUpdated {
log.FromContext(ctx).Info("Allow to stop cluster", "finalizer", hmc.BlockingFinalizer)
if err := r.Client.Patch(ctx, cluster, client.MergeFrom(&originalCluster)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use Client.Update here. I don't see need for Client.Patch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch was recommended here: #242 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't use Update with PartialObjectMetadata

return fmt.Errorf("failed to patch cluster %s/%s: %w", cluster.Namespace, cluster.Name, err)
}
}

return nil
}

func (r *ManagedClusterReconciler) machinesAvailable(ctx context.Context, namespace, clusterName string, gvk schema.GroupVersionKind) (bool, error) {
opts := &client.ListOptions{
LabelSelector: labels.SelectorFromSet(map[string]string{hmc.ClusterNameLabelKey: clusterName}),
Namespace: namespace,
Limit: 1,
}
itemsList := &metav1.PartialObjectMetadataList{}
itemsList.SetGroupVersionKind(gvk)
if err := r.Client.List(ctx, itemsList, opts); err != nil {
return false, err
}
return len(itemsList.Items) != 0, nil
}

// SetupWithManager sets up the controller with the Manager.
func (r *ManagedClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
return ctrl.NewControllerManagedBy(mgr).
Expand Down
2 changes: 2 additions & 0 deletions templates/cluster/aws-hosted-cp/templates/awscluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ metadata:
name: {{ include "cluster.name" . }}
annotations:
cluster.x-k8s.io/managed-by: k0smotron
finalizers:
- hmc.mirantis.com/cleanup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure if this will persist after release installation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@squizzi tested this functionality as well

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test it, I just thought this would fix the behavior since I encountered it on standalone in test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suspect some controller to remove it accidentally? I briefly checked capa, and they seem to add/remove only awscluster.infrastructure.cluster.x-k8s.io without affecting anything else

spec:
region: {{ .Values.region }}
# identityRef:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ metadata:
name: {{ include "cluster.name" . }}
annotations:
cluster.x-k8s.io/managed-by: k0smotron
finalizers:
- hmc.mirantis.com/cleanup
spec:
identityRef:
kind: AzureClusterIdentity
Expand Down
18 changes: 18 additions & 0 deletions templates/provider/hmc/templates/rbac/roles.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,24 @@ rules:
- certificates
verbs:
- create
- apiGroups:
- infrastructure.cluster.x-k8s.io
resources:
- awsclusters
- azureclusters
verbs:
- get
- list
- patch
- watch
- apiGroups:
- cluster.x-k8s.io
resources:
- machines
verbs:
- get
- list
- watch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
Expand Down