From ae418ebc5a8ec890ae11345a3d2270b784feac8b Mon Sep 17 00:00:00 2001 From: Thomas Polkowski Date: Fri, 8 Dec 2023 15:41:34 -0500 Subject: [PATCH] Refactor; Address review comments --- config/rbac/role.yaml | 12 ------------ controllers/addon_controller.go | 22 ++++++++++++++-------- controllers/manifest_controller.go | 4 ++-- 3 files changed, 16 insertions(+), 22 deletions(-) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index d7a2fa22..374a5534 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -17,12 +17,8 @@ rules: resources: - daemonsets verbs: - - create - - delete - get - list - - patch - - update - watch - apiGroups: - apps @@ -35,12 +31,8 @@ rules: resources: - deployments verbs: - - create - - delete - get - list - - patch - - update - watch - apiGroups: - apps @@ -53,12 +45,8 @@ rules: resources: - jobs verbs: - - create - - delete - get - list - - patch - - update - watch - apiGroups: - batch diff --git a/controllers/addon_controller.go b/controllers/addon_controller.go index a708c6d5..ee5a714e 100644 --- a/controllers/addon_controller.go +++ b/controllers/addon_controller.go @@ -35,6 +35,7 @@ const ( addonHelmchartFinalizer = "boundless.mirantis.com/helmchart-finalizer" addonManifestFinalizer = "boundless.mirantis.com/manifest-finalizer" addonIndexName = "helmchartIndex" + helmJobNameTemplate = "helm-install-%s" ) // AddonReconciler reconciles a Addon object @@ -49,7 +50,7 @@ type AddonReconciler struct { //+kubebuilder:rbac:groups=boundless.mirantis.com,resources=addons/finalizers,verbs=update //+kubebuilder:rbac:groups=boundless.mirantis.com,resources=manifests,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=boundless.mirantis.com,resources=manifests/status,verbs=get;update;patch -//+kubebuilder:rbac:groups=batch,resources=jobs,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=batch,resources=jobs,verbs=get;list;watch //+kubebuilder:rbac:groups=batch,resources=jobs/status,verbs=get //+kubebuilder:rbac:groups="",resources=events,verbs=create;patch @@ -152,7 +153,7 @@ func (r *AddonReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl // unfortunately the HelmChart CR doesn't have any useful events or status we can monitor // each helmchart object creates a job that runs the helm install - update status from that instead - jobName := fmt.Sprintf("helm-install-%s", instance.Spec.Chart.Name) + jobName := fmt.Sprintf(helmJobNameTemplate, instance.Spec.Chart.Name) job := &batch.Job{} err = r.Get(ctx, types.NamespacedName{Namespace: instance.Spec.Namespace, Name: jobName}, job) if err != nil { @@ -284,9 +285,8 @@ func (r *AddonReconciler) SetupWithManager(mgr ctrl.Manager) error { // This is done so we can later easily find the addon associated with a particular job if err := mgr.GetFieldIndexer().IndexField(context.Background(), &boundlessv1alpha1.Addon{}, addonIndexName, func(rawObj client.Object) []string { addon := rawObj.(*boundlessv1alpha1.Addon) - if addon.Spec.Chart != nil && addon.Spec.Chart.Name != "" { - - jobName := fmt.Sprintf("helm-install-%s", addon.Spec.Chart.Name) + if isHelmChartAddon(addon) { + jobName := fmt.Sprintf(helmJobNameTemplate, addon.Spec.Chart.Name) return []string{fmt.Sprintf("%s-%s", addon.Spec.Namespace, jobName)} } // don't add this index for non helm-chart type addons @@ -299,13 +299,18 @@ func (r *AddonReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&boundlessv1alpha1.Addon{}). Owns(&boundlessv1alpha1.Manifest{}). Watches( - &source.Kind{Type: &batch.Job{}}, - handler.EnqueueRequestsFromMapFunc(r.findAddonForJob), - builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}), + &source.Kind{Type: &batch.Job{}}, // Watch all Job Objects in the cluster + handler.EnqueueRequestsFromMapFunc(r.findAddonForJob), // All jobs trigger this MapFunc, the MapFunc filters which jobs should trigger reconciles to which addons, if any + builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}), // By default, any Update to job will trigger a run of the MapFunc, limit it to only Resource version updates ). Complete(r) } +// isHelmChartAddon checks the provided addon's spec and determines whether this addon is a chart kind +func isHelmChartAddon(addon *boundlessv1alpha1.Addon) bool { + return addon.Spec.Chart != nil && addon.Spec.Chart.Name != "" +} + // findAddonForJob finds the addons associated with a particular job // This is done by looking for the addon that was previously indexed in the form jobNamespace-jobName func (r *AddonReconciler) findAddonForJob(job client.Object) []reconcile.Request { @@ -352,6 +357,7 @@ func (r *AddonReconciler) updateHelmchartAddonStatus(ctx context.Context, logger } // updateStatus queries for a fresh Addon with the provided namespacedName. +// This avoids some errors where we fail to update status because we have an older (stale) version of the object // It then updates the Addon's status fields with the provided type, reason, and optionally message. func (r *AddonReconciler) updateStatus(ctx context.Context, logger logr.Logger, namespacedName types.NamespacedName, typeToApply boundlessv1alpha1.StatusType, reasonToApply string, messageToApply ...string) error { logger.Info("Update status with type and reason", "TypeToApply", typeToApply, "ReasonToApply", reasonToApply) diff --git a/controllers/manifest_controller.go b/controllers/manifest_controller.go index 32f4e37d..fa6d0c89 100644 --- a/controllers/manifest_controller.go +++ b/controllers/manifest_controller.go @@ -56,9 +56,9 @@ type ManifestReconciler struct { //+kubebuilder:rbac:groups=boundless.mirantis.com,resources=manifests/status,verbs=get;update;patch //+kubebuilder:rbac:groups=boundless.mirantis.com,resources=manifests/finalizers,verbs=update //+kubebuilder:rbac:groups="",resources=events,verbs=create;patch -//+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch //+kubebuilder:rbac:groups=apps,resources=deployments/status,verbs=get -//+kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=get;list;watch //+kubebuilder:rbac:groups=apps,resources=daemonsets/status,verbs=get // Reconcile is part of the main kubernetes reconciliation loop which aims to