From 05e42961d5158fc72c1a92ca443ff27345c50b3b Mon Sep 17 00:00:00 2001 From: rthalho Date: Wed, 2 Apr 2025 11:20:24 +0200 Subject: [PATCH 01/27] feat: initialize conditions to unknown and move status updates to defer func --- api/v1beta1/enterprise_types.go | 18 +++++++ api/v1beta1/organization_types.go | 18 +++++++ api/v1beta1/repository_types.go | 18 +++++++ internal/controller/enterprise_controller.go | 50 +++++++------------ .../controller/organization_controller.go | 48 +++++++----------- internal/controller/repository_controller.go | 48 +++++++----------- pkg/conditions/conditions.go | 1 + 7 files changed, 106 insertions(+), 95 deletions(-) diff --git a/api/v1beta1/enterprise_types.go b/api/v1beta1/enterprise_types.go index 6a9c2e32..6952cfe5 100644 --- a/api/v1beta1/enterprise_types.go +++ b/api/v1beta1/enterprise_types.go @@ -44,6 +44,24 @@ type Enterprise struct { Status EnterpriseStatus `json:"status,omitempty"` } +func (e *Enterprise) InitializeConditions() { + if conditions.Get(e, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(e, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(e, conditions.PoolManager) == nil { + conditions.MarkUnknown(e, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(e, conditions.SecretReference) == nil { + conditions.MarkUnknown(e, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(e, conditions.CredentialsReference) == nil { + conditions.MarkUnknown(e, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + func (e *Enterprise) SetConditions(conditions []metav1.Condition) { e.Status.Conditions = conditions } diff --git a/api/v1beta1/organization_types.go b/api/v1beta1/organization_types.go index 55b962f2..0cf9d897 100644 --- a/api/v1beta1/organization_types.go +++ b/api/v1beta1/organization_types.go @@ -44,6 +44,24 @@ type Organization struct { Status OrganizationStatus `json:"status,omitempty"` } +func (o *Organization) InitializeConditions() { + if conditions.Get(o, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(o, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(o, conditions.PoolManager) == nil { + conditions.MarkUnknown(o, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(o, conditions.SecretReference) == nil { + conditions.MarkUnknown(o, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(o, conditions.CredentialsReference) == nil { + conditions.MarkUnknown(o, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + func (o *Organization) SetConditions(conditions []metav1.Condition) { o.Status.Conditions = conditions } diff --git a/api/v1beta1/repository_types.go b/api/v1beta1/repository_types.go index adfb9574..1006290a 100644 --- a/api/v1beta1/repository_types.go +++ b/api/v1beta1/repository_types.go @@ -45,6 +45,24 @@ type Repository struct { Status RepositoryStatus `json:"status,omitempty"` } +func (r *Repository) InitializeConditions() { + if conditions.Get(r, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(r, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(r, conditions.PoolManager) == nil { + conditions.MarkUnknown(r, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(r, conditions.SecretReference) == nil { + conditions.MarkUnknown(r, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(r, conditions.CredentialsReference) == nil { + conditions.MarkUnknown(r, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + func (r *Repository) SetConditions(conditions []metav1.Condition) { r.Status.Conditions = conditions } diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go index 4dccb2fa..a32eb369 100644 --- a/internal/controller/enterprise_controller.go +++ b/internal/controller/enterprise_controller.go @@ -47,7 +47,7 @@ type EnterpriseReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=enterprises/finalizers,verbs=update // +kubebuilder:rbac:groups="",namespace=xxxxx,resources=secrets,verbs=get;list;watch; -func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) enterprise := &garmoperatorv1beta1.Enterprise{} @@ -60,6 +60,8 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } + orig := enterprise.DeepCopy() + // Ignore objects that are paused if annotations.IsPaused(enterprise) { log.Info("Reconciliation is paused for this object") @@ -73,6 +75,20 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) enterpriseClient := garmClient.NewEnterpriseClient() + // Initialize conditions to unknown if not set already + enterprise.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(enterprise.Status, orig.Status) { + if err := r.Status().Update(ctx, enterprise); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } + }() + // Handle deleted enterprises if !enterprise.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, enterpriseClient, enterprise) @@ -89,13 +105,6 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(enterprise, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkUnknown(enterprise, conditions.CredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) - if conditions.Get(enterprise, conditions.PoolManager) == nil { - conditions.MarkUnknown(enterprise, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) - } - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(enterprise, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") @@ -104,12 +113,6 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) conditions.MarkFalse(enterprise, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) - if conditions.Get(enterprise, conditions.PoolManager) == nil { - conditions.MarkUnknown(enterprise, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) - } - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(enterprise, conditions.CredentialsReference, conditions.FetchingCredentialsRefSuccessReason, "") @@ -118,9 +121,6 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { event.Error(r.Recorder, enterprise, err.Error()) conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -130,9 +130,6 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { event.Error(r.Recorder, enterprise, err.Error()) conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } } @@ -146,9 +143,6 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { event.Error(r.Recorder, enterprise, err.Error()) conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -162,10 +156,6 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC conditions.MarkFalse(enterprise, conditions.PoolManager, conditions.PoolManagerFailureReason, garmEnterprise.PoolManagerStatus.FailureReason) } - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } - log.Info("reconciling enterprise successfully done") return ctrl.Result{}, nil } @@ -243,9 +233,6 @@ func (r *EnterpriseReconciler) reconcileDelete(ctx context.Context, client garmC log.Info("starting enterprise deletion") event.Deleting(r.Recorder, enterprise, "starting enterprise deletion") conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.DeletingReason, conditions.DeletingEnterpriseMsg) - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } err := client.DeleteEnterprise( enterprises.NewDeleteEnterpriseParams(). @@ -255,9 +242,6 @@ func (r *EnterpriseReconciler) reconcileDelete(ctx context.Context, client garmC log.V(1).Info(fmt.Sprintf("client.DeleteEnterprise error: %s", err)) event.Error(r.Recorder, enterprise, err.Error()) conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, enterprise); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index e11c64f9..94013e8b 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -46,7 +46,7 @@ type OrganizationReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=organizations/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=organizations/finalizers,verbs=update -func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) organization := &garmoperatorv1beta1.Organization{} @@ -59,6 +59,8 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } + orig := organization.DeepCopy() + // Ignore objects that are paused if annotations.IsPaused(organization) { log.Info("Reconciliation is paused for this object") @@ -72,6 +74,20 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request organizationClient := garmClient.NewOrganizationClient() + // Initialize conditions to unknown if not set already + organization.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(organization.Status, orig.Status) { + if err := r.Status().Update(ctx, organization); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } + }() + // Handle deleted organizations if !organization.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, organizationClient, organization) @@ -88,11 +104,6 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar if err != nil { conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(organization, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkUnknown(organization, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) - conditions.MarkUnknown(organization, conditions.CredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(organization, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") @@ -101,12 +112,6 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar if err != nil { conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) conditions.MarkFalse(organization, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) - if conditions.Get(organization, conditions.PoolManager) == nil { - conditions.MarkUnknown(organization, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) - } - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(organization, conditions.CredentialsReference, conditions.FetchingCredentialsRefSuccessReason, "") @@ -115,9 +120,6 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar if err != nil { event.Error(r.Recorder, organization, err.Error()) conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -127,9 +129,6 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar if err != nil { event.Error(r.Recorder, organization, err.Error()) conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } } @@ -143,9 +142,6 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar if err != nil { event.Error(r.Recorder, organization, err.Error()) conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -159,10 +155,6 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar conditions.MarkFalse(organization, conditions.PoolManager, conditions.PoolManagerFailureReason, garmOrganization.PoolManagerStatus.FailureReason) } - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } - log.Info("reconciling organization successfully done") return ctrl.Result{}, nil @@ -241,9 +233,6 @@ func (r *OrganizationReconciler) reconcileDelete(ctx context.Context, client gar log.Info("starting organization deletion") event.Deleting(r.Recorder, organization, "starting organization deletion") conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.DeletingReason, conditions.DeletingOrgMsg) - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } err := client.DeleteOrganization( organizations.NewDeleteOrgParams(). @@ -253,9 +242,6 @@ func (r *OrganizationReconciler) reconcileDelete(ctx context.Context, client gar log.V(1).Info(fmt.Sprintf("client.DeleteOrganization error: %s", err)) event.Error(r.Recorder, organization, err.Error()) conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, organization); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go index 784c32b8..a1c1e40e 100644 --- a/internal/controller/repository_controller.go +++ b/internal/controller/repository_controller.go @@ -46,7 +46,7 @@ type RepositoryReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=repositories/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=repositories/finalizers,verbs=update -func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) repository := &garmoperatorv1beta1.Repository{} @@ -59,6 +59,8 @@ func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } + orig := repository.DeepCopy() + // Ignore objects that are paused if annotations.IsPaused(repository) { log.Info("Reconciliation is paused for this object") @@ -72,6 +74,20 @@ func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) repositoryClient := garmClient.NewRepositoryClient() + // initialize conditions + repository.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(repository.Status, orig.Status) { + if err := r.Status().Update(ctx, repository); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } + }() + // Handle deleted repositories if !repository.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, repositoryClient, repository) @@ -88,11 +104,6 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(repository, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkUnknown(repository, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) - conditions.MarkUnknown(repository, conditions.CredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(repository, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") @@ -101,12 +112,6 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) conditions.MarkFalse(repository, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) - if conditions.Get(repository, conditions.PoolManager) == nil { - conditions.MarkUnknown(repository, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) - } - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(repository, conditions.CredentialsReference, conditions.FetchingCredentialsRefSuccessReason, "") @@ -115,9 +120,6 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { event.Error(r.Recorder, repository, err.Error()) conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -127,9 +129,6 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { event.Error(r.Recorder, repository, err.Error()) conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } } @@ -143,9 +142,6 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC if err != nil { event.Error(r.Recorder, repository, err.Error()) conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -159,10 +155,6 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC conditions.MarkFalse(repository, conditions.PoolManager, conditions.PoolManagerFailureReason, garmRepository.PoolManagerStatus.FailureReason) } - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } - log.Info("reconciling repository successfully done") return ctrl.Result{}, nil @@ -242,9 +234,6 @@ func (r *RepositoryReconciler) reconcileDelete(ctx context.Context, client garmC log.Info("starting repository deletion") event.Deleting(r.Recorder, repository, "starting repository deletion") conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.DeletingReason, conditions.DeletingRepoMsg) - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } err := client.DeleteRepository( repositories.NewDeleteRepoParams(). @@ -254,9 +243,6 @@ func (r *RepositoryReconciler) reconcileDelete(ctx context.Context, client garmC log.V(1).Info(fmt.Sprintf("client.DeleteRepository error: %s", err)) event.Error(r.Recorder, repository, err.Error()) conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, repository); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } diff --git a/pkg/conditions/conditions.go b/pkg/conditions/conditions.go index 39034f3f..1e8ae42e 100644 --- a/pkg/conditions/conditions.go +++ b/pkg/conditions/conditions.go @@ -11,6 +11,7 @@ import ( ) type ConditionStatusObject interface { + InitializeConditions() SetConditions(conditions []metav1.Condition) GetConditions() []metav1.Condition } From fcb58c5d682da7e355daf1fca0572c1cbd8dfe98 Mon Sep 17 00:00:00 2001 From: rthalho Date: Wed, 2 Apr 2025 11:30:21 +0200 Subject: [PATCH 02/27] fix: add missing error events --- internal/controller/enterprise_controller.go | 4 +++- internal/controller/organization_controller.go | 2 ++ internal/controller/repository_controller.go | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go index a32eb369..94e5a2ef 100644 --- a/internal/controller/enterprise_controller.go +++ b/internal/controller/enterprise_controller.go @@ -77,7 +77,7 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) // Initialize conditions to unknown if not set already enterprise.InitializeConditions() - + // always update the status defer func() { if !reflect.DeepEqual(enterprise.Status, orig.Status) { @@ -103,6 +103,7 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC webhookSecret, err := secret.FetchRef(ctx, r.Client, &enterprise.Spec.WebhookSecretRef, enterprise.Namespace) if err != nil { + event.Error(r.Recorder, enterprise, err.Error()) conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(enterprise, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) return ctrl.Result{}, err @@ -111,6 +112,7 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC credentials, err := r.getCredentialsRef(ctx, enterprise) if err != nil { + event.Error(r.Recorder, enterprise, err.Error()) conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) conditions.MarkFalse(enterprise, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) return ctrl.Result{}, err diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index 94013e8b..68f7d682 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -102,6 +102,7 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar webhookSecret, err := secret.FetchRef(ctx, r.Client, &organization.Spec.WebhookSecretRef, organization.Namespace) if err != nil { + event.Error(r.Recorder, organization, err.Error()) conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(organization, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) return ctrl.Result{}, err @@ -110,6 +111,7 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar credentials, err := r.getCredentialsRef(ctx, organization) if err != nil { + event.Error(r.Recorder, organization, err.Error()) conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) conditions.MarkFalse(organization, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) return ctrl.Result{}, err diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go index a1c1e40e..d5139e97 100644 --- a/internal/controller/repository_controller.go +++ b/internal/controller/repository_controller.go @@ -102,6 +102,7 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC webhookSecret, err := secret.FetchRef(ctx, r.Client, &repository.Spec.WebhookSecretRef, repository.Namespace) if err != nil { + event.Error(r.Recorder, repository, err.Error()) conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(repository, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) return ctrl.Result{}, err @@ -110,6 +111,7 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC credentials, err := r.getCredentialsRef(ctx, repository) if err != nil { + event.Error(r.Recorder, repository, err.Error()) conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) conditions.MarkFalse(repository, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) return ctrl.Result{}, err From 46d41c1008ec2697175337d382391ef79079566d Mon Sep 17 00:00:00 2001 From: rthalho Date: Wed, 2 Apr 2025 19:19:02 +0200 Subject: [PATCH 03/27] fix: implement ConditionStatusObject interface on all API objects --- DEVELOPMENT.md | 2 +- api/v1alpha1/enterprise_types.go | 18 ++++++++++++++++++ api/v1alpha1/organization_types.go | 18 ++++++++++++++++++ api/v1alpha1/repository_types.go | 18 ++++++++++++++++++ api/v1beta1/garmserverconfig_types.go | 2 ++ api/v1beta1/githubcredentials_types.go | 2 ++ api/v1beta1/githubendpoint_types.go | 2 ++ api/v1beta1/pool_types.go | 7 +++++++ 8 files changed, 68 insertions(+), 1 deletion(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index e26abc47..a2503c68 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -93,6 +93,6 @@ Clone the `garm-provider-k8s` repo: ``` And follow this [guide](https://github.com/mercedes-benz/garm-provider-k8s/blob/main/DEVELOPMENT.md). But `instead` of the `make tilt-up` in the `garm-provider-k8s` repo, execute the folling command. Make sure you are in your `kind-garm-operator` kubernetes context: ```bash - $ RUNNER_IMAGE="localhost:5000/runner:linux-ubuntu-22.04-x86_64" make build copy docker-build docker-build-summerwind-runner && kubectl apply -k hack/local-development/kubernetes/ + $ make build copy docker-build docker-build-summerwind-runner && kubectl apply -k hack/local-development/kubernetes/ ``` Essentially this does the same as the `make tilt-up` target in `garm-provider-k8s`, but in your local garm-operator cluster. Otherwise, a separate cluster will be spawned with the latest garm-operator release. diff --git a/api/v1alpha1/enterprise_types.go b/api/v1alpha1/enterprise_types.go index 15ead20f..dfe01562 100644 --- a/api/v1alpha1/enterprise_types.go +++ b/api/v1alpha1/enterprise_types.go @@ -41,6 +41,24 @@ type Enterprise struct { Status EnterpriseStatus `json:"status,omitempty"` } +func (e *Enterprise) InitializeConditions() { + if conditions.Get(e, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(e, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(e, conditions.PoolManager) == nil { + conditions.MarkUnknown(e, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(e, conditions.SecretReference) == nil { + conditions.MarkUnknown(e, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(e, conditions.CredentialsReference) == nil { + conditions.MarkUnknown(e, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + func (e *Enterprise) SetConditions(conditions []metav1.Condition) { e.Status.Conditions = conditions } diff --git a/api/v1alpha1/organization_types.go b/api/v1alpha1/organization_types.go index 28ac60ec..f3a9d1ee 100644 --- a/api/v1alpha1/organization_types.go +++ b/api/v1alpha1/organization_types.go @@ -41,6 +41,24 @@ type Organization struct { Status OrganizationStatus `json:"status,omitempty"` } +func (o *Organization) InitializeConditions() { + if conditions.Get(o, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(o, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(o, conditions.PoolManager) == nil { + conditions.MarkUnknown(o, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(o, conditions.SecretReference) == nil { + conditions.MarkUnknown(o, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(o, conditions.CredentialsReference) == nil { + conditions.MarkUnknown(o, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + func (o *Organization) SetConditions(conditions []metav1.Condition) { o.Status.Conditions = conditions } diff --git a/api/v1alpha1/repository_types.go b/api/v1alpha1/repository_types.go index 8310acbf..7d30392b 100644 --- a/api/v1alpha1/repository_types.go +++ b/api/v1alpha1/repository_types.go @@ -42,6 +42,24 @@ type Repository struct { Status RepositoryStatus `json:"status,omitempty"` } +func (r *Repository) InitializeConditions() { + if conditions.Get(r, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(r, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(r, conditions.PoolManager) == nil { + conditions.MarkUnknown(r, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(r, conditions.SecretReference) == nil { + conditions.MarkUnknown(r, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(r, conditions.CredentialsReference) == nil { + conditions.MarkUnknown(r, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + func (r *Repository) SetConditions(conditions []metav1.Condition) { r.Status.Conditions = conditions } diff --git a/api/v1beta1/garmserverconfig_types.go b/api/v1beta1/garmserverconfig_types.go index acd024c9..1d365fbf 100644 --- a/api/v1beta1/garmserverconfig_types.go +++ b/api/v1beta1/garmserverconfig_types.go @@ -47,6 +47,8 @@ type GarmServerConfig struct { Status GarmServerConfigStatus `json:"status,omitempty"` } +func (g *GarmServerConfig) InitializeConditions() {} + func (g *GarmServerConfig) SetConditions(conditions []metav1.Condition) { g.Status.Conditions = conditions } diff --git a/api/v1beta1/githubcredentials_types.go b/api/v1beta1/githubcredentials_types.go index a00e2de7..9df36c7e 100644 --- a/api/v1beta1/githubcredentials_types.go +++ b/api/v1beta1/githubcredentials_types.go @@ -60,6 +60,8 @@ type GitHubCredential struct { Status GitHubCredentialStatus `json:"status,omitempty"` } +func (g *GitHubCredential) InitializeConditions() {} + func (g *GitHubCredential) SetConditions(conditions []metav1.Condition) { g.Status.Conditions = conditions } diff --git a/api/v1beta1/githubendpoint_types.go b/api/v1beta1/githubendpoint_types.go index d79636c0..34491578 100644 --- a/api/v1beta1/githubendpoint_types.go +++ b/api/v1beta1/githubendpoint_types.go @@ -38,6 +38,8 @@ type GitHubEndpoint struct { Status GitHubEndpointStatus `json:"status,omitempty"` } +func (e *GitHubEndpoint) InitializeConditions() {} + func (e *GitHubEndpoint) SetConditions(conditions []metav1.Condition) { e.Status.Conditions = conditions } diff --git a/api/v1beta1/pool_types.go b/api/v1beta1/pool_types.go index b38fe36e..4f6feeea 100644 --- a/api/v1beta1/pool_types.go +++ b/api/v1beta1/pool_types.go @@ -4,6 +4,7 @@ package v1beta1 import ( commonParams "github.com/cloudbase/garm-provider-common/params" + "github.com/mercedes-benz/garm-operator/pkg/conditions" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -52,6 +53,12 @@ type PoolStatus struct { Conditions []metav1.Condition `json:"conditions,omitempty"` } +func (p *Pool) InitializeConditions() { + if conditions.Get(p, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(p, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + func (p *Pool) SetConditions(conditions []metav1.Condition) { p.Status.Conditions = conditions } From 9f5d864edc5d806c2bfa33ba98a9d7c3737f15fd Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 22 Apr 2025 09:42:01 +0200 Subject: [PATCH 04/27] feat: improve garmserverconif controller status updates --- api/v1beta1/garmserverconfig_types.go | 7 +- .../controller/garmserverconfig_controller.go | 79 ++++++++----------- 2 files changed, 37 insertions(+), 49 deletions(-) diff --git a/api/v1beta1/garmserverconfig_types.go b/api/v1beta1/garmserverconfig_types.go index 1d365fbf..6cd6174b 100644 --- a/api/v1beta1/garmserverconfig_types.go +++ b/api/v1beta1/garmserverconfig_types.go @@ -3,6 +3,7 @@ package v1beta1 import ( + "github.com/mercedes-benz/garm-operator/pkg/conditions" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -47,7 +48,11 @@ type GarmServerConfig struct { Status GarmServerConfigStatus `json:"status,omitempty"` } -func (g *GarmServerConfig) InitializeConditions() {} +func (g *GarmServerConfig) InitializeConditions() { + if conditions.Get(g, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(g, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} func (g *GarmServerConfig) SetConditions(conditions []metav1.Condition) { g.Status.Conditions = conditions diff --git a/internal/controller/garmserverconfig_controller.go b/internal/controller/garmserverconfig_controller.go index bd02a0af..e5d89d89 100644 --- a/internal/controller/garmserverconfig_controller.go +++ b/internal/controller/garmserverconfig_controller.go @@ -5,6 +5,8 @@ package controller import ( "context" "errors" + "github.com/mercedes-benz/garm-operator/pkg/conditions" + "github.com/mercedes-benz/garm-operator/pkg/event" "reflect" garmapiserverparams "github.com/cloudbase/garm/apiserver/params" @@ -37,7 +39,7 @@ type GarmServerConfigReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=garmserverconfigs/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=garmserverconfigs/finalizers,verbs=update -func (r *GarmServerConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *GarmServerConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) log.Info("Reconciling GarmServerConfig") @@ -52,12 +54,27 @@ func (r *GarmServerConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } + orig := garmServerConfig.DeepCopy() + // Ignore objects that are paused if annotations.IsPaused(garmServerConfig) { log.Info("Reconciliation is paused for GarmServerConfig") return ctrl.Result{}, nil } + garmServerConfig.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(garmServerConfig.Status, orig.Status) { + if err := r.Status().Update(ctx, garmServerConfig); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } + }() + return r.reconcileNormal(ctx, controllerClient, garmServerConfig) } @@ -73,13 +90,23 @@ func (r *GarmServerConfigReconciler) reconcileNormal(ctx context.Context, contro // sync applied spec with controller info in garm newControllerInfo, err := r.updateControllerInfo(ctx, controllerClient, garmServerConfig, &controllerInfo) if err != nil { + log.Error(err, "Failed to update controller info") + event.Error(r.Recorder, garmServerConfig, err.Error()) + conditions.MarkFalse(garmServerConfig, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) return ctrl.Result{}, err } // update CR with new state from garm - if err := r.updateGarmServerConfigStatus(ctx, newControllerInfo, garmServerConfig); err != nil { - return ctrl.Result{}, err - } + garmServerConfig.Status.ControllerID = newControllerInfo.ControllerID.String() + garmServerConfig.Status.Hostname = newControllerInfo.Hostname + garmServerConfig.Status.MetadataURL = newControllerInfo.MetadataURL + garmServerConfig.Status.CallbackURL = newControllerInfo.CallbackURL + garmServerConfig.Status.WebhookURL = newControllerInfo.WebhookURL + garmServerConfig.Status.ControllerWebhookURL = newControllerInfo.ControllerWebhookURL + garmServerConfig.Status.MinimumJobAgeBackoff = newControllerInfo.MinimumJobAgeBackoff + garmServerConfig.Status.Version = newControllerInfo.Version + + conditions.MarkTrue(garmServerConfig, conditions.ReadyCondition, conditions.SuccessfulReconcileReason, "") return ctrl.Result{}, nil } @@ -109,50 +136,6 @@ func (r *GarmServerConfigReconciler) updateControllerInfo(ctx context.Context, c return &response.Payload, nil } -func (r *GarmServerConfigReconciler) updateGarmServerConfigStatus(ctx context.Context, controllerInfo *params.ControllerInfo, garmServerConfigCR *garmoperatorv1beta1.GarmServerConfig) error { - log := log.FromContext(ctx) - - if !r.needsStatusUpdate(controllerInfo, garmServerConfigCR) { - log.Info("GarmServerConfig CR up to date") - return nil - } - - log.Info("Updating GarmServerConfig CR") - garmServerConfigStatus := garmoperatorv1beta1.GarmServerConfigStatus{ - ControllerID: controllerInfo.ControllerID.String(), - Hostname: controllerInfo.Hostname, - MetadataURL: controllerInfo.MetadataURL, - CallbackURL: controllerInfo.CallbackURL, - WebhookURL: controllerInfo.WebhookURL, - ControllerWebhookURL: controllerInfo.ControllerWebhookURL, - MinimumJobAgeBackoff: controllerInfo.MinimumJobAgeBackoff, - Version: controllerInfo.Version, - } - - garmServerConfigCR.Status = garmServerConfigStatus - err := r.Status().Update(ctx, garmServerConfigCR) - if err != nil { - log.Error(err, "Failed to update GarmServerConfig CR") - return err - } - return nil -} - -func (r *GarmServerConfigReconciler) needsStatusUpdate(controllerInfo *params.ControllerInfo, garmServerConfigCR *garmoperatorv1beta1.GarmServerConfig) bool { - tempStatus := garmoperatorv1beta1.GarmServerConfigStatus{ - ControllerID: controllerInfo.ControllerID.String(), - Hostname: controllerInfo.Hostname, - MetadataURL: controllerInfo.MetadataURL, - CallbackURL: controllerInfo.CallbackURL, - WebhookURL: controllerInfo.WebhookURL, - ControllerWebhookURL: controllerInfo.ControllerWebhookURL, - MinimumJobAgeBackoff: controllerInfo.MinimumJobAgeBackoff, - Version: controllerInfo.Version, - } - - return !reflect.DeepEqual(garmServerConfigCR.Status, tempStatus) -} - func (r *GarmServerConfigReconciler) getControllerInfo(client garmclient.ControllerClient) (params.ControllerInfo, error) { controllerInfo, err := client.GetControllerInfo() if err == nil { From d9d7f6a46c5e087e9d2b7c9e0779db071d76c4be Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 22 Apr 2025 09:43:00 +0200 Subject: [PATCH 05/27] feat: improve githubcredentials controller status updates --- api/v1beta1/githubcredentials_types.go | 15 ++++++- .../githubcredentials_controller.go | 39 ++++++++----------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/api/v1beta1/githubcredentials_types.go b/api/v1beta1/githubcredentials_types.go index 9df36c7e..af2faf02 100644 --- a/api/v1beta1/githubcredentials_types.go +++ b/api/v1beta1/githubcredentials_types.go @@ -4,6 +4,7 @@ package v1beta1 import ( "github.com/cloudbase/garm/params" + "github.com/mercedes-benz/garm-operator/pkg/conditions" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -60,7 +61,19 @@ type GitHubCredential struct { Status GitHubCredentialStatus `json:"status,omitempty"` } -func (g *GitHubCredential) InitializeConditions() {} +func (g *GitHubCredential) InitializeConditions() { + if conditions.Get(g, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(g, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(g, conditions.EndpointReference) == nil { + conditions.MarkUnknown(g, conditions.EndpointReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(g, conditions.SecretReference) == nil { + conditions.MarkUnknown(g, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} func (g *GitHubCredential) SetConditions(conditions []metav1.Condition) { g.Status.Conditions = conditions diff --git a/internal/controller/githubcredentials_controller.go b/internal/controller/githubcredentials_controller.go index 762bf708..95d3d27e 100644 --- a/internal/controller/githubcredentials_controller.go +++ b/internal/controller/githubcredentials_controller.go @@ -48,7 +48,7 @@ type GitHubCredentialReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=githubcredentials/finalizers,verbs=update //+kubebuilder:rbac:groups="",namespace=xxxxx,resources=secrets,verbs=get;list;watch; -func (r *GitHubCredentialReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *GitHubCredentialReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) credentials := &garmoperatorv1beta1.GitHubCredential{} @@ -60,6 +60,8 @@ func (r *GitHubCredentialReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } + orig := credentials.DeepCopy() + // Ignore objects that are paused if annotations.IsPaused(credentials) { log.Info("Reconciliation is paused for this object") @@ -73,6 +75,20 @@ func (r *GitHubCredentialReconciler) Reconcile(ctx context.Context, req ctrl.Req credentialsClient := garmClient.NewCredentialsClient() + // Initialize conditions to unknown if not set already + credentials.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(credentials.Status, orig.Status) { + if err := r.Status().Update(ctx, credentials); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } + }() + // Handle deleted credentials if !credentials.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, credentialsClient, credentials) @@ -90,9 +106,6 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client if err != nil { event.Error(r.Recorder, credentials, err.Error()) conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, credentials); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -101,9 +114,6 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client if err != nil { conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.FetchingEndpointRefFailedReason, err.Error()) conditions.MarkFalse(credentials, conditions.EndpointReference, conditions.FetchingEndpointRefFailedReason, err.Error()) - if err := r.Status().Update(ctx, credentials); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(credentials, conditions.EndpointReference, conditions.FetchingEndpointRefSuccessReason, "Successfully fetched GitHubEndpoint CR Ref") @@ -113,9 +123,6 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client if err != nil { conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(credentials, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) - if err := r.Status().Update(ctx, credentials); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } conditions.MarkTrue(credentials, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") @@ -126,9 +133,6 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client if err != nil { event.Error(r.Recorder, credentials, err.Error()) conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, credentials); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } } @@ -138,9 +142,6 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client if err != nil { event.Error(r.Recorder, credentials, err.Error()) conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, credentials); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -149,9 +150,6 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client if err != nil { event.Error(r.Recorder, credentials, err.Error()) conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, credentials); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } garmGitHubCreds = res.Payload @@ -168,9 +166,6 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client credentials.Status.Enterprises = enterprises conditions.MarkTrue(credentials, conditions.ReadyCondition, conditions.SuccessfulReconcileReason, "") - if err := r.Status().Update(ctx, credentials); err != nil { - return ctrl.Result{}, err - } log.Info("reconciling credentials successfully done") return ctrl.Result{}, nil From 05cc4db4d8000b7405c751e58c26d8259c1d7415 Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 22 Apr 2025 09:43:22 +0200 Subject: [PATCH 06/27] feat: improve githubendpoint controller status updates --- api/v1beta1/githubendpoint_types.go | 11 ++++- .../controller/githubendpoint_controller.go | 40 ++++++++----------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/api/v1beta1/githubendpoint_types.go b/api/v1beta1/githubendpoint_types.go index 34491578..8904e493 100644 --- a/api/v1beta1/githubendpoint_types.go +++ b/api/v1beta1/githubendpoint_types.go @@ -3,6 +3,7 @@ package v1beta1 import ( + "github.com/mercedes-benz/garm-operator/pkg/conditions" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -38,7 +39,15 @@ type GitHubEndpoint struct { Status GitHubEndpointStatus `json:"status,omitempty"` } -func (e *GitHubEndpoint) InitializeConditions() {} +func (e *GitHubEndpoint) InitializeConditions() { + if conditions.Get(e, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(e, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } + + if conditions.Get(e, conditions.SecretReference) == nil { + conditions.MarkUnknown(e, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} func (e *GitHubEndpoint) SetConditions(conditions []metav1.Condition) { e.Status.Conditions = conditions diff --git a/internal/controller/githubendpoint_controller.go b/internal/controller/githubendpoint_controller.go index bf9dbc87..59bdaa59 100644 --- a/internal/controller/githubendpoint_controller.go +++ b/internal/controller/githubendpoint_controller.go @@ -47,7 +47,7 @@ type GitHubEndpointReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=githubendpoints/finalizers,verbs=update // +kubebuilder:rbac:groups="",namespace=xxxxx,resources=secrets,verbs=get;list;watch; -func (r *GitHubEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *GitHubEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) endpoint := &garmoperatorv1beta1.GitHubEndpoint{} @@ -59,6 +59,8 @@ func (r *GitHubEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } + orig := endpoint.DeepCopy() + // Ignore objects that are paused if annotations.IsPaused(endpoint) { log.Info("Reconciliation is paused for this object") @@ -72,6 +74,20 @@ func (r *GitHubEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Reque endpointClient := garmClient.NewEndpointClient() + // Initialize conditions to unknown if not set already + endpoint.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(endpoint.Status, orig.Status) { + if err := r.Status().Update(ctx, endpoint); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } + }() + // Handle deleted endpoints if !endpoint.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, endpointClient, endpoint) @@ -95,9 +111,6 @@ func (r *GitHubEndpointReconciler) reconcileNormal(ctx context.Context, client g if err != nil { event.Error(r.Recorder, endpoint, err.Error()) conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, endpoint); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -107,9 +120,6 @@ func (r *GitHubEndpointReconciler) reconcileNormal(ctx context.Context, client g if err != nil { event.Error(r.Recorder, endpoint, err.Error()) conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, endpoint); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } } @@ -119,20 +129,13 @@ func (r *GitHubEndpointReconciler) reconcileNormal(ctx context.Context, client g if err != nil { event.Error(r.Recorder, endpoint, err.Error()) conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, endpoint); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } // set and update endpoint status conditions.MarkTrue(endpoint, conditions.ReadyCondition, conditions.SuccessfulReconcileReason, "") log.Info("reconciling endpoint successfully done", "endpoint", garmEndpoint.Name) - if err := r.Status().Update(ctx, endpoint); err != nil { - return ctrl.Result{}, err - } - log.Info("reconciling endpoint successfully done") return ctrl.Result{}, nil } @@ -206,9 +209,6 @@ func (r *GitHubEndpointReconciler) reconcileDelete(ctx context.Context, client g log.Info("starting endpoint deletion") event.Deleting(r.Recorder, endpoint, "starting endpoint deletion") conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.DeletingReason, conditions.DeletingEndpointMsg) - if err := r.Status().Update(ctx, endpoint); err != nil { - return ctrl.Result{}, err - } err := client.DeleteEndpoint( endpoints.NewDeleteGithubEndpointParams(). @@ -218,9 +218,6 @@ func (r *GitHubEndpointReconciler) reconcileDelete(ctx context.Context, client g log.V(1).Info(fmt.Sprintf("client.DeleteEndpoint error: %s", err)) event.Error(r.Recorder, endpoint, err.Error()) conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.GarmAPIErrorReason, err.Error()) - if err := r.Status().Update(ctx, endpoint); err != nil { - return ctrl.Result{}, err - } return ctrl.Result{}, err } @@ -246,9 +243,6 @@ func (r *GitHubEndpointReconciler) handleCaCertBundleSecret(ctx context.Context, if err != nil { conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) conditions.MarkFalse(endpoint, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) - if err := r.Status().Update(ctx, endpoint); err != nil { - return "", err - } return "", err } conditions.MarkTrue(endpoint, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") From c5c5687d023326094b5b31fadc27c4f1b7c79d56 Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 22 Apr 2025 09:44:03 +0200 Subject: [PATCH 07/27] fix: cleanup image_types.go --- api/v1beta1/image_types.go | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/api/v1beta1/image_types.go b/api/v1beta1/image_types.go index 5064817a..236307dc 100644 --- a/api/v1beta1/image_types.go +++ b/api/v1beta1/image_types.go @@ -6,14 +6,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! -// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. - // ImageSpec defines the desired state of Image type ImageSpec struct { - // INSERT ADDITIONAL SPEC FIELDS - desired state of cluster - // Important: Run "make" to regenerate code after modifying this file - // Tag is the Name of the image in its registry // e.g. // - in openstack it can be the image name or id @@ -22,10 +16,7 @@ type ImageSpec struct { } // ImageStatus defines the observed state of Image -type ImageStatus struct { - // INSERT ADDITIONAL STATUS FIELD - define observed state of cluster - // Important: Run "make" to regenerate code after modifying this file -} +type ImageStatus struct{} //+kubebuilder:object:root=true //+kubebuilder:storageversion From 98fd1ebc18366d8cddb90b389c85aaa1d2eb4a38 Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 22 Apr 2025 09:45:01 +0200 Subject: [PATCH 08/27] feat: improve pool controller status updates --- internal/controller/pool_controller.go | 114 ++++++++++++++----------- 1 file changed, 64 insertions(+), 50 deletions(-) diff --git a/internal/controller/pool_controller.go b/internal/controller/pool_controller.go index 7cef7eca..9690fee1 100644 --- a/internal/controller/pool_controller.go +++ b/internal/controller/pool_controller.go @@ -56,7 +56,7 @@ const ( //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=images,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=pools/status,verbs=get;update;patch -func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) pool := &garmoperatorv1beta1.Pool{} @@ -65,9 +65,12 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. return ctrl.Result{}, nil } log.Error(err, "cannot fetch Pool") - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + event.Error(r.Recorder, pool, err.Error()) + return ctrl.Result{}, err } + orig := pool.DeepCopy() + // Ignore objects that are paused if annotations.IsPaused(pool) { log.Info("Reconciliation is paused for this object") @@ -83,6 +86,20 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. instanceClient := garmClient.NewInstanceClient() + // Initialize conditions to unknown if not set already + pool.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(pool.Status, orig.Status) { + if err := r.Status().Update(ctx, pool); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } + }() + // handle deletion if !pool.ObjectMeta.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, poolClient, pool, instanceClient) @@ -94,14 +111,18 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. func (r *PoolReconciler) reconcileNormal(ctx context.Context, poolClient garmClient.PoolClient, pool *garmoperatorv1beta1.Pool, instanceClient garmClient.InstanceClient) (ctrl.Result, error) { gitHubScopeRef, err := r.fetchGitHubScopeCRD(ctx, pool) if err != nil { + r.errorLog(ctx, pool, err) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) conditions.MarkFalse(pool, conditions.ScopeReference, conditions.FetchingScopeRefFailedReason, err.Error()) - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + return ctrl.Result{}, err } if gitHubScopeRef.GetID() == "" { err := fmt.Errorf("referenced GitHubScopeRef %s/%s not ready yet", pool.Spec.GitHubScopeRef.Kind, pool.Spec.GitHubScopeRef.Name) + r.errorLog(ctx, pool, err) conditions.MarkFalse(pool, conditions.ScopeReference, conditions.ScopeRefNotReadyReason, err.Error()) - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) + return ctrl.Result{}, err } conditions.MarkTrue(pool, conditions.ScopeReference, conditions.FetchingScopeRefSuccessReason, fmt.Sprintf("Successfully fetched %s CR Ref", pool.Spec.GitHubScopeRef.Kind)) @@ -122,14 +143,18 @@ func (r *PoolReconciler) reconcileCreate(ctx context.Context, garmClient garmCli image, err := pool.GetImageCR(ctx, r.Client) if err != nil { conditions.MarkFalse(pool, conditions.ImageReference, conditions.FetchingImageRefFailedReason, err.Error()) - return r.handleUpdateError(ctx, pool, err, conditions.FetchingImageRefFailedReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.FetchingImageRefFailedReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } conditions.MarkTrue(pool, conditions.ImageReference, conditions.FetchingImageRefSuccessReason, "Successfully fetched Image CR Ref") // always create new pool in garm garmPool, err := poolUtil.CreatePool(ctx, garmClient, pool, image, gitHubScopeRef) if err != nil { - return r.handleUpdateError(ctx, pool, fmt.Errorf("failed creating pool %s: %s", pool.Name, err.Error()), conditions.ReconcileErrorReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) + r.errorLog(ctx, pool, fmt.Errorf("failed creating pool %s: %s", pool.Name, err.Error())) + return ctrl.Result{}, err } log.Info("creating pool in garm succeeded") @@ -137,7 +162,10 @@ func (r *PoolReconciler) reconcileCreate(ctx context.Context, garmClient garmCli pool.Status.ID = garmPool.ID pool.Status.LongRunningIdleRunners = garmPool.MinIdleRunners - return r.handleSuccessfulUpdate(ctx, pool) + + conditions.MarkTrue(pool, conditions.ReadyCondition, conditions.SuccessfulReconcileReason, "") + + return ctrl.Result{}, nil } func (r *PoolReconciler) reconcileUpdate(ctx context.Context, garmClient garmClient.PoolClient, pool *garmoperatorv1beta1.Pool, instanceClient garmClient.InstanceClient) (ctrl.Result, error) { @@ -148,14 +176,18 @@ func (r *PoolReconciler) reconcileUpdate(ctx context.Context, garmClient garmCli image, err := pool.GetImageCR(ctx, r.Client) if err != nil { conditions.MarkFalse(pool, conditions.ImageReference, conditions.FetchingImageRefFailedReason, err.Error()) - return r.handleUpdateError(ctx, pool, err, conditions.FetchingImageRefFailedReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.FetchingImageRefFailedReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } conditions.MarkTrue(pool, conditions.ImageReference, conditions.FetchingImageRefSuccessReason, "Successfully fetched Image CR Ref") poolCRdiffersFromGarmPool, idleRunners, err := r.comparePoolSpecs(ctx, pool, image.Spec.Tag, garmClient) if err != nil { - log.Error(err, "error comparing pool specs") - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + err := fmt.Errorf("error comparing pool specs: %s", err.Error()) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } if !poolCRdiffersFromGarmPool { @@ -163,7 +195,9 @@ func (r *PoolReconciler) reconcileUpdate(ctx context.Context, garmClient garmCli if err = poolUtil.UpdatePool(ctx, garmClient, pool, image); err != nil { log.Error(err, "error updating pool") - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } } @@ -212,7 +246,8 @@ func (r *PoolReconciler) reconcileUpdate(ctx context.Context, garmClient garmCli pool.Status.LongRunningIdleRunners = uint(longRunningIdleRunnersCount) } - return r.handleSuccessfulUpdate(ctx, pool) + conditions.MarkTrue(pool, conditions.ReadyCondition, conditions.SuccessfulReconcileReason, "") + return ctrl.Result{}, nil } func (r *PoolReconciler) reconcileDelete(ctx context.Context, garmClient garmClient.PoolClient, pool *garmoperatorv1beta1.Pool, instanceClient garmClient.InstanceClient) (ctrl.Result, error) { @@ -229,7 +264,9 @@ func (r *PoolReconciler) reconcileDelete(ctx context.Context, garmClient garmCli if pool.Status.ID == "" && controllerutil.ContainsFinalizer(pool, key.PoolFinalizerName) { controllerutil.RemoveFinalizer(pool, key.PoolFinalizerName) if err := r.Update(ctx, pool); err != nil { - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } log.Info("Successfully deleted pool", "pool", pool.Name) @@ -239,12 +276,15 @@ func (r *PoolReconciler) reconcileDelete(ctx context.Context, garmClient garmCli pool.Spec.MinIdleRunners = 0 pool.Spec.Enabled = false if err := r.Update(ctx, pool); err != nil { - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } if err := poolUtil.UpdatePool(ctx, garmClient, pool, nil); err != nil { - log.Error(err, "error updating pool") - return r.handleUpdateError(ctx, pool, err, conditions.ReconcileErrorReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } // get all runners @@ -274,54 +314,28 @@ func (r *PoolReconciler) reconcileDelete(ctx context.Context, garmClient garmCli // delete pool in garm if err = garmClient.DeletePool(pools.NewDeletePoolParams().WithPoolID(pool.Status.ID)); err != nil { - return r.handleUpdateError(ctx, pool, fmt.Errorf("error deleting pool %s: %w", pool.Name, err), conditions.DeletionFailedReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.DeletionFailedReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } // remove finalizer so k8s can delete resource controllerutil.RemoveFinalizer(pool, key.PoolFinalizerName) if err := r.Update(ctx, pool); err != nil { - return r.handleUpdateError(ctx, pool, fmt.Errorf("error deleting pool %s: %w", pool.Name, err), conditions.DeletionFailedReason) + conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.DeletionFailedReason, err.Error()) + r.errorLog(ctx, pool, err) + return ctrl.Result{}, err } log.Info("Successfully deleted pool", "pool", pool.Name) return ctrl.Result{}, nil } -func (r *PoolReconciler) updatePoolCRStatus(ctx context.Context, pool *garmoperatorv1beta1.Pool) error { - log := log.FromContext(ctx) - if err := r.Status().Update(ctx, pool); err != nil { - log.Error(err, "unable to update Pool status") - return err - } - return nil -} - -func (r *PoolReconciler) handleUpdateError(ctx context.Context, pool *garmoperatorv1beta1.Pool, err error, conditionReason conditions.ConditionReason) (ctrl.Result, error) { +func (r *PoolReconciler) errorLog(ctx context.Context, obj client.Object, err error) { log := log.FromContext(ctx) log.Error(err, "error") - event.Error(r.Recorder, pool, err.Error()) - - conditions.MarkFalse(pool, conditions.ReadyCondition, conditions.ReconcileErrorReason, err.Error()) - if conditionReason != "" { - conditions.MarkFalse(pool, conditions.ReadyCondition, conditionReason, err.Error()) - } - - if updateErr := r.updatePoolCRStatus(ctx, pool); updateErr != nil { - return ctrl.Result{}, updateErr - } - - return ctrl.Result{}, err -} - -func (r *PoolReconciler) handleSuccessfulUpdate(ctx context.Context, pool *garmoperatorv1beta1.Pool) (ctrl.Result, error) { - conditions.MarkTrue(pool, conditions.ReadyCondition, conditions.SuccessfulReconcileReason, "") - - if err := r.updatePoolCRStatus(ctx, pool); err != nil { - return ctrl.Result{}, err - } - - return ctrl.Result{}, nil + event.Error(r.Recorder, obj, err.Error()) } func (r *PoolReconciler) comparePoolSpecs(ctx context.Context, pool *garmoperatorv1beta1.Pool, imageTag string, poolClient garmClient.PoolClient) (bool, []params.Instance, error) { From 484076432f802037aca16fd06f583371169f1a81 Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 22 Apr 2025 09:46:26 +0200 Subject: [PATCH 09/27] feat: improve runner controller sync --- DEVELOPMENT.md | 6 + api/v1alpha1/runner_conversion.go | 22 ++ api/v1alpha1/runner_types.go | 8 +- api/v1alpha1/zz_generated.conversion.go | 48 ++-- api/v1beta1/runner_types.go | 28 ++- api/v1beta1/zz_generated.deepcopy.go | 7 + ...rm-operator.mercedes-benz.com_runners.yaml | 85 ++++++- internal/controller/runner_controller.go | 214 ++++++++---------- pkg/conditions/condition_types.go | 13 +- 9 files changed, 276 insertions(+), 155 deletions(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index a2503c68..1d6fcd5b 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -92,7 +92,13 @@ Clone the `garm-provider-k8s` repo: $ git clone https://github.com/mercedes-benz/garm-provider-k8s && cd ./garm-provider-k8s ``` And follow this [guide](https://github.com/mercedes-benz/garm-provider-k8s/blob/main/DEVELOPMENT.md). But `instead` of the `make tilt-up` in the `garm-provider-k8s` repo, execute the folling command. Make sure you are in your `kind-garm-operator` kubernetes context: +This will build a GitHub actions runner image and a garm-server image and bootstrap the garm-server in your local `kind` cluster: ```bash $ make build copy docker-build docker-build-summerwind-runner && kubectl apply -k hack/local-development/kubernetes/ ``` + +Deploy the `garm-operator` CRs to your local `kind` cluster: + ```bash + $ kubectl apply -k hack/local-development/kubernetes/garm-operator-crs.yaml + ``` Essentially this does the same as the `make tilt-up` target in `garm-provider-k8s`, but in your local garm-operator cluster. Otherwise, a separate cluster will be spawned with the latest garm-operator release. diff --git a/api/v1alpha1/runner_conversion.go b/api/v1alpha1/runner_conversion.go index 3809d8b4..8bab0daf 100644 --- a/api/v1alpha1/runner_conversion.go +++ b/api/v1alpha1/runner_conversion.go @@ -2,6 +2,7 @@ package v1alpha1 import ( + apiconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/controller-runtime/pkg/conversion" "github.com/mercedes-benz/garm-operator/api/v1beta1" @@ -16,3 +17,24 @@ func (r *Runner) ConvertTo(dstRaw conversion.Hub) error { func (r *Runner) ConvertFrom(dstRaw conversion.Hub) error { return Convert_v1beta1_Runner_To_v1alpha1_Runner(dstRaw.(*v1beta1.Runner), r, nil) } + +func Convert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(in *v1beta1.RunnerStatus, out *RunnerStatus, s apiconversion.Scope) error { + out = &RunnerStatus{ + ID: in.ID, + ProviderID: in.ProviderID, + AgentID: in.AgentID, + Name: in.Name, + OSType: in.OSType, + OSName: in.OSName, + OSVersion: in.OSVersion, + OSArch: in.OSArch, + Addresses: nil, + Status: in.Status, + InstanceStatus: in.InstanceStatus, + PoolID: in.PoolID, + ProviderFault: in.ProviderFault, + GitHubRunnerGroup: in.GitHubRunnerGroup, + } + + return autoConvert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(in, out, s) +} diff --git a/api/v1alpha1/runner_types.go b/api/v1alpha1/runner_types.go index 64523e4e..7e4b5e50 100644 --- a/api/v1alpha1/runner_types.go +++ b/api/v1alpha1/runner_types.go @@ -50,11 +50,11 @@ type RunnerStatus struct { // for this instance. Addresses []commonParams.Address `json:"addresses,omitempty"` - // Status is the status of the instance inside the provider (eg: running, stopped, etc) - Status commonParams.InstanceStatus `json:"status,omitempty"` + // Status is the runner status as it appears on GitHub. (idle, pending, ...) + Status params.RunnerStatus `json:"status,omitempty"` - // RunnerStatus is the github runner status as it appears on GitHub. - InstanceStatus params.RunnerStatus `json:"instanceStatus,omitempty"` + // InstanceStatus of the instance inside the respective cloud provider of the physical instance (eg: running, stopped, ...) + InstanceStatus commonParams.InstanceStatus `json:"instanceStatus,omitempty"` // PoolID is the ID of the garm pool to which a runner belongs. PoolID string `json:"poolId,omitempty"` diff --git a/api/v1alpha1/zz_generated.conversion.go b/api/v1alpha1/zz_generated.conversion.go index 5d140450..a557a1fd 100644 --- a/api/v1alpha1/zz_generated.conversion.go +++ b/api/v1alpha1/zz_generated.conversion.go @@ -230,11 +230,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.RunnerStatus)(nil), (*RunnerStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(a.(*v1beta1.RunnerStatus), b.(*RunnerStatus), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*SecretRef)(nil), (*v1beta1.SecretRef)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha1_SecretRef_To_v1beta1_SecretRef(a.(*SecretRef), b.(*v1beta1.SecretRef), scope) }); err != nil { @@ -275,6 +270,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.RunnerStatus)(nil), (*RunnerStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(a.(*v1beta1.RunnerStatus), b.(*RunnerStatus), scope) + }); err != nil { + return err + } return nil } @@ -871,7 +871,17 @@ func Convert_v1beta1_Runner_To_v1alpha1_Runner(in *v1beta1.Runner, out *Runner, func autoConvert_v1alpha1_RunnerList_To_v1beta1_RunnerList(in *RunnerList, out *v1beta1.RunnerList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]v1beta1.Runner)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]v1beta1.Runner, len(*in)) + for i := range *in { + if err := Convert_v1alpha1_Runner_To_v1beta1_Runner(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -882,7 +892,17 @@ func Convert_v1alpha1_RunnerList_To_v1beta1_RunnerList(in *RunnerList, out *v1be func autoConvert_v1beta1_RunnerList_To_v1alpha1_RunnerList(in *v1beta1.RunnerList, out *RunnerList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]Runner)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]Runner, len(*in)) + for i := range *in { + if err := Convert_v1beta1_Runner_To_v1alpha1_Runner(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -919,8 +939,8 @@ func autoConvert_v1alpha1_RunnerStatus_To_v1beta1_RunnerStatus(in *RunnerStatus, out.OSVersion = in.OSVersion out.OSArch = params.OSArch(in.OSArch) out.Addresses = *(*[]params.Address)(unsafe.Pointer(&in.Addresses)) - out.Status = params.InstanceStatus(in.Status) - out.InstanceStatus = garmparams.RunnerStatus(in.InstanceStatus) + out.Status = garmparams.RunnerStatus(in.Status) + out.InstanceStatus = params.InstanceStatus(in.InstanceStatus) out.PoolID = in.PoolID out.ProviderFault = in.ProviderFault out.GitHubRunnerGroup = in.GitHubRunnerGroup @@ -942,19 +962,15 @@ func autoConvert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(in *v1beta1.Runne out.OSVersion = in.OSVersion out.OSArch = params.OSArch(in.OSArch) out.Addresses = *(*[]params.Address)(unsafe.Pointer(&in.Addresses)) - out.Status = params.InstanceStatus(in.Status) - out.InstanceStatus = garmparams.RunnerStatus(in.InstanceStatus) + out.Status = garmparams.RunnerStatus(in.Status) + out.InstanceStatus = params.InstanceStatus(in.InstanceStatus) out.PoolID = in.PoolID out.ProviderFault = in.ProviderFault out.GitHubRunnerGroup = in.GitHubRunnerGroup + // WARNING: in.Conditions requires manual conversion: does not exist in peer-type return nil } -// Convert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus is an autogenerated conversion function. -func Convert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(in *v1beta1.RunnerStatus, out *RunnerStatus, s conversion.Scope) error { - return autoConvert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(in, out, s) -} - func autoConvert_v1alpha1_SecretRef_To_v1beta1_SecretRef(in *SecretRef, out *v1beta1.SecretRef, s conversion.Scope) error { out.Name = in.Name out.Key = in.Key diff --git a/api/v1beta1/runner_types.go b/api/v1beta1/runner_types.go index 55288156..3fe463fe 100644 --- a/api/v1beta1/runner_types.go +++ b/api/v1beta1/runner_types.go @@ -5,12 +5,10 @@ package v1beta1 import ( commonParams "github.com/cloudbase/garm-provider-common/params" "github.com/cloudbase/garm/params" + "github.com/mercedes-benz/garm-operator/pkg/conditions" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! -// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. - // RunnerSpec defines the desired state of Runner type RunnerSpec struct{} @@ -50,11 +48,11 @@ type RunnerStatus struct { // for this instance. Addresses []commonParams.Address `json:"addresses,omitempty"` - // Status is the status of the instance inside the provider (eg: running, stopped, etc) - Status commonParams.InstanceStatus `json:"status,omitempty"` + // Status is the runner status as it appears on GitHub. (idle, pending, ...) + Status params.RunnerStatus `json:"status,omitempty"` - // RunnerStatus is the github runner status as it appears on GitHub. - InstanceStatus params.RunnerStatus `json:"instanceStatus,omitempty"` + // InstanceStatus of the instance inside the respective cloud provider of the physical instance (eg: running, stopped, ...) + InstanceStatus commonParams.InstanceStatus `json:"instanceStatus,omitempty"` // PoolID is the ID of the garm pool to which a runner belongs. PoolID string `json:"poolId,omitempty"` @@ -72,6 +70,8 @@ type RunnerStatus struct { // GithubRunnerGroup is the github runner group to which the runner belongs. // The runner group must be created by someone with access to the enterprise. GitHubRunnerGroup string `json:"githubRunnerGroup"` + + Conditions []metav1.Condition `json:"conditions,omitempty"` } //+kubebuilder:object:root=true @@ -95,6 +95,20 @@ type Runner struct { Status RunnerStatus `json:"status,omitempty"` } +func (r *Runner) InitializeConditions() { + if conditions.Get(r, conditions.ReadyCondition) == nil { + conditions.MarkUnknown(r, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + } +} + +func (r *Runner) SetConditions(conditions []metav1.Condition) { + r.Status.Conditions = conditions +} + +func (r *Runner) GetConditions() []metav1.Condition { + return r.Status.Conditions +} + //+kubebuilder:object:root=true // RunnerList contains a list of Runner diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 711083c5..40402b8b 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -930,6 +930,13 @@ func (in *RunnerStatus) DeepCopyInto(out *RunnerStatus) { *out = make([]params.Address, len(*in)) copy(*out, *in) } + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]v1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RunnerStatus. diff --git a/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml b/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml index ec56ed32..78607bea 100644 --- a/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml +++ b/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml @@ -106,8 +106,8 @@ spec: description: ID is the database ID of this instance. type: string instanceStatus: - description: RunnerStatus is the github runner status as it appears - on GitHub. + description: 'InstanceStatus of the instance inside the respective + cloud provider of the physical instance (eg: running, stopped, ...)' type: string name: description: |- @@ -145,8 +145,8 @@ spec: instance in the provider. type: string status: - description: 'Status is the status of the instance inside the provider - (eg: running, stopped, etc)' + description: Status is the runner status as it appears on GitHub. + (idle, pending, ...) type: string required: - agentId @@ -234,6 +234,75 @@ spec: description: AgentID is the github runner agent ID. format: int64 type: integer + conditions: + items: + description: "Condition contains details for one aspect of the current + state of this API Resource.\n---\nThis struct is intended for + direct use as an array at the field path .status.conditions. For + example,\n\n\n\ttype FooStatus struct{\n\t // Represents the + observations of a foo's current state.\n\t // Known .status.conditions.type + are: \"Available\", \"Progressing\", and \"Degraded\"\n\t // + +patchMergeKey=type\n\t // +patchStrategy=merge\n\t // +listType=map\n\t + \ // +listMapKey=type\n\t Conditions []metav1.Condition `json:\"conditions,omitempty\" + patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`\n\n\n\t + \ // other fields\n\t}" + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: |- + type of condition in CamelCase or in foo.example.com/CamelCase. + --- + Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be + useful (see .node.status.conditions), the ability to deconflict is important. + The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array githubRunnerGroup: description: |- GithubRunnerGroup is the github runner group to which the runner belongs. @@ -243,8 +312,8 @@ spec: description: ID is the database ID of this instance. type: string instanceStatus: - description: RunnerStatus is the github runner status as it appears - on GitHub. + description: 'InstanceStatus of the instance inside the respective + cloud provider of the physical instance (eg: running, stopped, ...)' type: string name: description: |- @@ -282,8 +351,8 @@ spec: instance in the provider. type: string status: - description: 'Status is the status of the instance inside the provider - (eg: running, stopped, etc)' + description: Status is the runner status as it appears on GitHub. + (idle, pending, ...) type: string required: - agentId diff --git a/internal/controller/runner_controller.go b/internal/controller/runner_controller.go index 1ca0f762..3ab959ce 100644 --- a/internal/controller/runner_controller.go +++ b/internal/controller/runner_controller.go @@ -4,6 +4,9 @@ package controller import ( "context" + "fmt" + commonParams "github.com/cloudbase/garm-provider-common/params" + "github.com/mercedes-benz/garm-operator/pkg/conditions" "reflect" "strings" "time" @@ -30,6 +33,8 @@ import ( "github.com/mercedes-benz/garm-operator/pkg/config" "github.com/mercedes-benz/garm-operator/pkg/filter" runnerUtil "github.com/mercedes-benz/garm-operator/pkg/runners" + + "github.com/google/go-cmp/cmp" ) // RunnerReconciler reconciles a Runner object @@ -44,12 +49,11 @@ type RunnerReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=runners/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=runners/finalizers,verbs=update -func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { + log := log.FromContext(ctx) + instanceClient := garmClient.NewInstanceClient() - return r.reconcile(ctx, req, instanceClient) -} -func (r *RunnerReconciler) reconcile(ctx context.Context, req ctrl.Request, instanceClient garmClient.InstanceClient) (ctrl.Result, error) { // try fetch runner instance in garm db with events coming from reconcile loop events of RunnerCR or from manually enqueued events of garm api. garmRunner, err := r.getGarmRunnerInstanceByName(instanceClient, req.Name) if err != nil { @@ -58,38 +62,74 @@ func (r *RunnerReconciler) reconcile(ctx context.Context, req ctrl.Request, inst // only create RunnerCR if it does not yet exist runner := &garmoperatorv1beta1.Runner{} - if err := r.Get(ctx, types.NamespacedName{Namespace: req.Namespace, Name: strings.ToLower(req.Name)}, runner); err != nil { - return r.handleCreateRunnerCR(ctx, req, err, garmRunner) + err = r.Get(ctx, types.NamespacedName{Namespace: req.Namespace, Name: strings.ToLower(req.Name)}, runner) + + switch { + // found Runner CR and matching garm runner or RunnerCR is already in deleting state, continue with reconcile + case err == nil && garmRunner != nil || !runner.ObjectMeta.DeletionTimestamp.IsZero(): + log.Info("Found Runner CR and matching garm runner, continue with reconcile", "runner", runner.Name) + + // Found RunnerCR but no matching garm runner, delete the RunnerCR + case err == nil && garmRunner == nil: + log.Info("Found RunnerCR for event but no matching garm runner, deleting RunnerCR", "runner", runner.Name) + if err := r.Delete(ctx, runner); err != nil { + return ctrl.Result{}, err + } + + // Did not find RunnerCR and found garm runner, create the RunnerCR + case apierrors.IsNotFound(err) && garmRunner != nil: + log.Info("Did not find RunnerCR and found garm runner, creating RunnerCR", "runner", garmRunner.Name) + return r.createRunnerCR(ctx, runner, garmRunner, req.Namespace) + + // Did not find RunnerCR and no matching garm runner, do nothing + case apierrors.IsNotFound(err) && garmRunner == nil: + log.Info("No RunnerCR and no garm runner was found for event", "request", req) + return ctrl.Result{}, nil + + // Reconcile error + default: + log.Error(err, "Error reconciling runner", "request", req) + return ctrl.Result{}, err } + orig := runner.DeepCopy() + + // Initialize conditions to unknown if not set already + runner.InitializeConditions() + + // always update the status + defer func() { + if !reflect.DeepEqual(runner.Status, orig.Status) { + log.Info("Update runner status...") + diff := cmp.Diff(orig.Status, runner.Status) + fmt.Println("Differences found:") + fmt.Println(diff) + if err := r.Status().Update(ctx, runner); err != nil { + log.Error(err, "failed to update status") + res = ctrl.Result{Requeue: true} + retErr = err + } + } else { + log.Info("Nothing changed in runner status") + } + }() + // delete runner in garm db if !runner.ObjectMeta.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, instanceClient, garmRunner) + return r.reconcileDelete(ctx, instanceClient, runner, garmRunner) } // sync garm runner status back to RunnerCR - return r.updateRunnerStatus(ctx, runner, garmRunner) -} - -func (r *RunnerReconciler) handleCreateRunnerCR(ctx context.Context, req ctrl.Request, fetchErr error, garmRunner *params.Instance) (ctrl.Result, error) { - log := log.FromContext(ctx) - if apierrors.IsNotFound(fetchErr) && garmRunner != nil { - return r.createRunnerCR(ctx, garmRunner, req.Namespace) - } + err = r.updateRunnerStatus(ctx, runner, garmRunner) - if apierrors.IsNotFound(fetchErr) { - log.Info("object was not found") - return ctrl.Result{}, nil - } - - return ctrl.Result{}, fetchErr + return ctrl.Result{RequeueAfter: time.Second * 5}, err } -func (r *RunnerReconciler) createRunnerCR(ctx context.Context, garmRunner *params.Instance, namespace string) (ctrl.Result, error) { +func (r *RunnerReconciler) createRunnerCR(ctx context.Context, runnerCR *garmoperatorv1beta1.Runner, garmRunner *params.Instance, namespace string) (ctrl.Result, error) { log := log.FromContext(ctx) - log.Info("Creating Runner", "Runner", garmRunner.Name) + log.Info("Creating RunnerCR", "Runner", garmRunner.Name) - runnerObj := &garmoperatorv1beta1.Runner{ + runnerCR = &garmoperatorv1beta1.Runner{ ObjectMeta: metav1.ObjectMeta{ Name: strings.ToLower(garmRunner.Name), Namespace: namespace, @@ -97,22 +137,22 @@ func (r *RunnerReconciler) createRunnerCR(ctx context.Context, garmRunner *param Spec: garmoperatorv1beta1.RunnerSpec{}, } - if err := r.Create(ctx, runnerObj); err != nil { + if err := r.Create(ctx, runnerCR); err != nil { return ctrl.Result{}, err } - if err := r.ensureFinalizer(ctx, runnerObj); err != nil { + if err := r.ensureFinalizer(ctx, runnerCR); err != nil { return ctrl.Result{}, err } - if _, err := r.updateRunnerStatus(ctx, runnerObj, garmRunner); err != nil { + if err := r.updateRunnerStatus(ctx, runnerCR, garmRunner); err != nil { return ctrl.Result{}, err } return ctrl.Result{}, nil } -func (r *RunnerReconciler) reconcileDelete(ctx context.Context, runnerClient garmClient.InstanceClient, garmRunner *params.Instance) (ctrl.Result, error) { +func (r *RunnerReconciler) reconcileDelete(ctx context.Context, runnerClient garmClient.InstanceClient, runnerCR *garmoperatorv1beta1.Runner, garmRunner *params.Instance) (ctrl.Result, error) { log := log.FromContext(ctx) if garmRunner != nil { log.Info("Deleting Runner in Garm", "Runner Name", garmRunner.Name) @@ -121,6 +161,12 @@ func (r *RunnerReconciler) reconcileDelete(ctx context.Context, runnerClient gar return ctrl.Result{}, err } } + if controllerutil.ContainsFinalizer(runnerCR, key.RunnerFinalizerName) { + controllerutil.RemoveFinalizer(runnerCR, key.RunnerFinalizerName) + if err := r.Update(ctx, runnerCR); err != nil { + return ctrl.Result{}, err + } + } return ctrl.Result{}, nil } @@ -146,22 +192,14 @@ func (r *RunnerReconciler) ensureFinalizer(ctx context.Context, runner *garmoper return nil } -func (r *RunnerReconciler) updateRunnerStatus(ctx context.Context, runner *garmoperatorv1beta1.Runner, garmRunner *params.Instance) (ctrl.Result, error) { +func (r *RunnerReconciler) updateRunnerStatus(ctx context.Context, runner *garmoperatorv1beta1.Runner, garmRunner *params.Instance) error { if garmRunner == nil { - return ctrl.Result{}, nil - } - - if r.runnerSpecsEqual(*runner, garmRunner) { - return ctrl.Result{}, nil + return nil } - log := log.FromContext(ctx) - log.Info("Update runner status...") - poolName := garmRunner.PoolID pools := &garmoperatorv1beta1.PoolList{} - err := r.List(ctx, pools) - if err == nil { + if err := r.List(ctx, pools); err == nil { filteredPools := filter.Match(pools.Items, garmoperatorv1beta1.MatchesID(garmRunner.PoolID)) if len(filteredPools) > 0 { @@ -178,18 +216,28 @@ func (r *RunnerReconciler) updateRunnerStatus(ctx context.Context, runner *garmo runner.Status.OSVersion = garmRunner.OSVersion runner.Status.OSArch = garmRunner.OSArch runner.Status.Addresses = garmRunner.Addresses - runner.Status.Status = garmRunner.Status - runner.Status.InstanceStatus = garmRunner.RunnerStatus + runner.Status.Status = garmRunner.RunnerStatus + runner.Status.InstanceStatus = garmRunner.Status runner.Status.PoolID = poolName runner.Status.ProviderFault = string(garmRunner.ProviderFault) runner.Status.GitHubRunnerGroup = garmRunner.GitHubRunnerGroup - if err := r.Status().Update(ctx, runner); err != nil { - log.Error(err, "unable to update Runner status") - return ctrl.Result{}, err + if runner.Status.InstanceStatus == commonParams.InstancePendingCreate || + runner.Status.InstanceStatus == commonParams.InstanceCreating || + runner.Status.Status == params.RunnerInstalling || + runner.Status.Status == params.RunnerPending { + conditions.MarkFalse(runner, conditions.ReadyCondition, conditions.RunnerNotReadyReason, conditions.RunnerNotReadyYetMsg) } - return ctrl.Result{}, nil + if runner.Status.InstanceStatus == commonParams.InstanceError || runner.Status.Status == params.RunnerFailed { + conditions.MarkFalse(runner, conditions.ReadyCondition, conditions.RunnerErrorReason, conditions.RunnerProvisioningFailedMsg) + } + + if runner.Status.InstanceStatus == commonParams.InstanceRunning && runner.Status.Status == params.RunnerIdle { + conditions.MarkTrue(runner, conditions.ReadyCondition, conditions.RunnerReadyReason, conditions.RunnerIdleAndRunningMsg) + } + + return nil } // SetupWithManager sets up the controller with the Manager. @@ -232,12 +280,6 @@ func (r *RunnerReconciler) EnqueueRunnerInstances(ctx context.Context, instanceC return err } - // compares garm db with RunnerCRs and deletes RunnerCRs not present in garm db - err = r.cleanUpNotMatchingRunnerCRs(ctx, garmRunnerInstances) - if err != nil { - return err - } - // triggers controller to reconcile based on instances in garm db r.enqeueRunnerEvents(garmRunnerInstances) return nil @@ -260,56 +302,6 @@ func (r *RunnerReconciler) enqeueRunnerEvents(garmRunnerInstances params.Instanc } } -func (r *RunnerReconciler) cleanUpNotMatchingRunnerCRs(ctx context.Context, garmRunnerInstances params.Instances) error { - runnerCRList := &garmoperatorv1beta1.RunnerList{} - err := r.List(ctx, runnerCRList) - if err != nil { - return err - } - - var runnerCRNameList []string - for _, runner := range runnerCRList.Items { - runnerCRNameList = append(runnerCRNameList, runner.Name) - } - - var runnerInstanceNameList []string - for _, runner := range garmRunnerInstances { - runnerInstanceNameList = append(runnerInstanceNameList, strings.ToLower(runner.Name)) - } - - runnersToDelete := getRunnerDiff(runnerCRNameList, runnerInstanceNameList) - log.Log.V(1).Info("Deleting runners: ", "Runners", runnersToDelete) - - for _, runnerName := range runnersToDelete { - runner := &garmoperatorv1beta1.Runner{} - err := r.Get(ctx, types.NamespacedName{Namespace: config.Config.Operator.WatchNamespace, Name: runnerName}, runner) - if err != nil { - return err - } - - if runner.DeletionTimestamp.IsZero() { - err = r.Delete(ctx, runner) - if err != nil { - return err - } - } - - // getting RunnerCR from cache again before removing finalizer, as in the meantime object has changed - err = r.Get(ctx, types.NamespacedName{Namespace: config.Config.Operator.WatchNamespace, Name: runnerName}, runner) - if err != nil { - return err - } - - if controllerutil.ContainsFinalizer(runner, key.RunnerFinalizerName) { - controllerutil.RemoveFinalizer(runner, key.RunnerFinalizerName) - if err := r.Update(ctx, runner); err != nil { - return err - } - } - } - return nil -} - func (r *RunnerReconciler) fetchPools(ctx context.Context) (*garmoperatorv1beta1.PoolList, error) { pools := &garmoperatorv1beta1.PoolList{} err := r.List(ctx, pools) @@ -346,8 +338,8 @@ func (r *RunnerReconciler) runnerSpecsEqual(runner garmoperatorv1beta1.Runner, g OSVersion: garmRunner.OSVersion, OSArch: garmRunner.OSArch, Addresses: garmRunner.Addresses, - Status: garmRunner.Status, - InstanceStatus: garmRunner.RunnerStatus, + Status: garmRunner.RunnerStatus, + InstanceStatus: garmRunner.Status, ProviderFault: string(garmRunner.ProviderFault), GitHubRunnerGroup: garmRunner.GitHubRunnerGroup, PoolID: "", @@ -355,19 +347,3 @@ func (r *RunnerReconciler) runnerSpecsEqual(runner garmoperatorv1beta1.Runner, g return reflect.DeepEqual(runner.Status, tmpRunnerStatus) } - -func getRunnerDiff(runnerCRs, garmRunners []string) []string { - cache := make(map[string]struct{}) - var diff []string - - for _, runner := range garmRunners { - cache[runner] = struct{}{} - } - - for _, runnerCR := range runnerCRs { - if _, found := cache[runnerCR]; !found { - diff = append(diff, runnerCR) - } - } - return diff -} diff --git a/pkg/conditions/condition_types.go b/pkg/conditions/condition_types.go index 097ae173..1c986f46 100644 --- a/pkg/conditions/condition_types.go +++ b/pkg/conditions/condition_types.go @@ -53,6 +53,13 @@ const ( FetchingEndpointRefFailedReason ConditionReason = "FetchingEndpointRefFailed" ) +// Runner Conditions +const ( + RunnerReadyReason ConditionReason = "RunnerIdleAndRunning" + RunnerNotReadyReason ConditionReason = "RunnerNotReadyYet" + RunnerErrorReason ConditionReason = "RunnerProvisioningError" +) + const ( GarmServerNotReconciledYetMsg string = "GARM server not reconciled yet" CredentialsNotReconciledYetMsg string = "credentials not reconciled yet" @@ -61,5 +68,9 @@ const ( DeletingRepoMsg string = "Deleting repository" DeletingPoolMsg string = "Deleting pool" DeletingEndpointMsg string = "Deleting endpoint" - DeletingCredentialsMsg string = "Deleting credentials" // #nosec G101 + DeletingCredentialsMsg string = "Deleting credentials" // #nosec G101 + DeletingRunnerMsg string = "Deleting runner" // #nosec G101 + RunnerIdleAndRunningMsg string = "Runner is idle and running" // #nosec G101 + RunnerProvisioningFailedMsg string = "Provisioning runner failed" // #nosec G101 + RunnerNotReadyYetMsg string = "Runner is still being provisioned" // #nosec G101 ) From 2a9d4c9429cc1db69475b6d4e1a1bb0dabd4c47c Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 24 Jun 2025 13:47:38 +0200 Subject: [PATCH 10/27] fix: improve runner controller performance --- config/manager/manager.yaml | 3 ++ hack/install-prometheus.sh | 0 internal/controller/runner_controller.go | 47 +++++++++++++++++++++--- 3 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 hack/install-prometheus.sh diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 9a0d7b65..2e39e43a 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -37,6 +37,9 @@ spec: metadata: annotations: kubectl.kubernetes.io/default-container: manager + prometheus.io/scrape: "true" + prometheus.io/path: "/metrics" + prometheus.io/port: "8080" labels: control-plane: controller-manager spec: diff --git a/hack/install-prometheus.sh b/hack/install-prometheus.sh new file mode 100644 index 00000000..e69de29b diff --git a/internal/controller/runner_controller.go b/internal/controller/runner_controller.go index 3ab959ce..070c9e1b 100644 --- a/internal/controller/runner_controller.go +++ b/internal/controller/runner_controller.go @@ -122,7 +122,7 @@ func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res // sync garm runner status back to RunnerCR err = r.updateRunnerStatus(ctx, runner, garmRunner) - return ctrl.Result{RequeueAfter: time.Second * 5}, err + return ctrl.Result{}, err } func (r *RunnerReconciler) createRunnerCR(ctx context.Context, runnerCR *garmoperatorv1beta1.Runner, garmRunner *params.Instance, namespace string) (ctrl.Result, error) { @@ -280,16 +280,35 @@ func (r *RunnerReconciler) EnqueueRunnerInstances(ctx context.Context, instanceC return err } - // triggers controller to reconcile based on instances in garm db - r.enqeueRunnerEvents(garmRunnerInstances) + runnerCRList := &garmoperatorv1beta1.RunnerList{} + err = r.List(ctx, runnerCRList) + if err != nil { + return err + } + + var runnerCRNameList []string + for _, runner := range runnerCRList.Items { + runnerCRNameList = append(runnerCRNameList, runner.Name) + } + + var runnerInstanceNameList []string + for _, runner := range garmRunnerInstances { + runnerInstanceNameList = append(runnerInstanceNameList, strings.ToLower(runner.Name)) + } + + runnersToDelete := getRunnerDiff(runnerCRNameList, runnerInstanceNameList) + + runnersToEnqueue := append(runnerInstanceNameList, runnersToDelete...) + + r.enqeueRunnerEvents(runnersToEnqueue) return nil } -func (r *RunnerReconciler) enqeueRunnerEvents(garmRunnerInstances params.Instances) { - for _, runner := range garmRunnerInstances { +func (r *RunnerReconciler) enqeueRunnerEvents(runnerNames []string) { + for _, runner := range runnerNames { runnerObj := garmoperatorv1beta1.Runner{ ObjectMeta: metav1.ObjectMeta{ - Name: strings.ToLower(runner.Name), + Name: strings.ToLower(runner), Namespace: config.Config.Operator.WatchNamespace, }, } @@ -347,3 +366,19 @@ func (r *RunnerReconciler) runnerSpecsEqual(runner garmoperatorv1beta1.Runner, g return reflect.DeepEqual(runner.Status, tmpRunnerStatus) } + +func getRunnerDiff(runnerCRs, garmRunners []string) []string { + cache := make(map[string]struct{}) + var diff []string + + for _, runner := range garmRunners { + cache[runner] = struct{}{} + } + + for _, runnerCR := range runnerCRs { + if _, found := cache[runnerCR]; !found { + diff = append(diff, runnerCR) + } + } + return diff +} From eb2fdefb2c26388123a203405a06256a8df06e79 Mon Sep 17 00:00:00 2001 From: rthalho Date: Thu, 28 Aug 2025 11:27:49 +0200 Subject: [PATCH 11/27] fix: rename conditions and add status messages --- api/v1alpha1/enterprise_types.go | 8 +-- api/v1alpha1/organization_types.go | 8 +-- api/v1alpha1/repository_types.go | 8 +-- api/v1beta1/enterprise_types.go | 8 +-- api/v1beta1/githubcredentials_types.go | 8 +-- api/v1beta1/githubendpoint_types.go | 4 +- api/v1beta1/organization_types.go | 8 +-- api/v1beta1/repository_types.go | 8 +-- internal/controller/enterprise_controller.go | 12 ++-- .../controller/enterprise_controller_test.go | 56 +++++++++---------- .../githubcredentials_controller.go | 12 ++-- .../githubcredentials_controller_test.go | 48 ++++++++-------- .../controller/githubendpoint_controller.go | 6 +- .../githubendpoint_controller_test.go | 26 ++++----- .../controller/organization_controller.go | 12 ++-- .../organization_controller_test.go | 56 +++++++++---------- internal/controller/repository_controller.go | 12 ++-- .../controller/repository_controller_test.go | 56 +++++++++---------- pkg/conditions/condition_types.go | 44 ++++++++------- 19 files changed, 201 insertions(+), 199 deletions(-) diff --git a/api/v1alpha1/enterprise_types.go b/api/v1alpha1/enterprise_types.go index dfe01562..86fe8aa5 100644 --- a/api/v1alpha1/enterprise_types.go +++ b/api/v1alpha1/enterprise_types.go @@ -50,12 +50,12 @@ func (e *Enterprise) InitializeConditions() { conditions.MarkUnknown(e, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(e, conditions.SecretReference) == nil { - conditions.MarkUnknown(e, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(e, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(e, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } - if conditions.Get(e, conditions.CredentialsReference) == nil { - conditions.MarkUnknown(e, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(e, conditions.GithubCredentialsReference) == nil { + conditions.MarkUnknown(e, conditions.GithubCredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) } } diff --git a/api/v1alpha1/organization_types.go b/api/v1alpha1/organization_types.go index f3a9d1ee..104a1956 100644 --- a/api/v1alpha1/organization_types.go +++ b/api/v1alpha1/organization_types.go @@ -50,12 +50,12 @@ func (o *Organization) InitializeConditions() { conditions.MarkUnknown(o, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(o, conditions.SecretReference) == nil { - conditions.MarkUnknown(o, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(o, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(o, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } - if conditions.Get(o, conditions.CredentialsReference) == nil { - conditions.MarkUnknown(o, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(o, conditions.GithubCredentialsReference) == nil { + conditions.MarkUnknown(o, conditions.GithubCredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) } } diff --git a/api/v1alpha1/repository_types.go b/api/v1alpha1/repository_types.go index 7d30392b..50adf55d 100644 --- a/api/v1alpha1/repository_types.go +++ b/api/v1alpha1/repository_types.go @@ -51,12 +51,12 @@ func (r *Repository) InitializeConditions() { conditions.MarkUnknown(r, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(r, conditions.SecretReference) == nil { - conditions.MarkUnknown(r, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(r, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(r, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } - if conditions.Get(r, conditions.CredentialsReference) == nil { - conditions.MarkUnknown(r, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(r, conditions.GithubCredentialsReference) == nil { + conditions.MarkUnknown(r, conditions.GithubCredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) } } diff --git a/api/v1beta1/enterprise_types.go b/api/v1beta1/enterprise_types.go index 6952cfe5..fdcc80aa 100644 --- a/api/v1beta1/enterprise_types.go +++ b/api/v1beta1/enterprise_types.go @@ -53,12 +53,12 @@ func (e *Enterprise) InitializeConditions() { conditions.MarkUnknown(e, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(e, conditions.SecretReference) == nil { - conditions.MarkUnknown(e, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(e, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(e, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } - if conditions.Get(e, conditions.CredentialsReference) == nil { - conditions.MarkUnknown(e, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(e, conditions.GithubCredentialsReference) == nil { + conditions.MarkUnknown(e, conditions.GithubCredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) } } diff --git a/api/v1beta1/githubcredentials_types.go b/api/v1beta1/githubcredentials_types.go index af2faf02..c2de0bc5 100644 --- a/api/v1beta1/githubcredentials_types.go +++ b/api/v1beta1/githubcredentials_types.go @@ -66,12 +66,12 @@ func (g *GitHubCredential) InitializeConditions() { conditions.MarkUnknown(g, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(g, conditions.EndpointReference) == nil { - conditions.MarkUnknown(g, conditions.EndpointReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(g, conditions.GithubEndpointReference) == nil { + conditions.MarkUnknown(g, conditions.GithubEndpointReference, conditions.UnknownReason, conditions.GithubEndpointNotReconciledYetMsg) } - if conditions.Get(g, conditions.SecretReference) == nil { - conditions.MarkUnknown(g, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(g, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(g, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } } diff --git a/api/v1beta1/githubendpoint_types.go b/api/v1beta1/githubendpoint_types.go index 8904e493..6df6aabf 100644 --- a/api/v1beta1/githubendpoint_types.go +++ b/api/v1beta1/githubendpoint_types.go @@ -44,8 +44,8 @@ func (e *GitHubEndpoint) InitializeConditions() { conditions.MarkUnknown(e, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(e, conditions.SecretReference) == nil { - conditions.MarkUnknown(e, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(e, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(e, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } } diff --git a/api/v1beta1/organization_types.go b/api/v1beta1/organization_types.go index 0cf9d897..2d0f2a8d 100644 --- a/api/v1beta1/organization_types.go +++ b/api/v1beta1/organization_types.go @@ -53,12 +53,12 @@ func (o *Organization) InitializeConditions() { conditions.MarkUnknown(o, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(o, conditions.SecretReference) == nil { - conditions.MarkUnknown(o, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(o, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(o, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } - if conditions.Get(o, conditions.CredentialsReference) == nil { - conditions.MarkUnknown(o, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(o, conditions.GithubCredentialsReference) == nil { + conditions.MarkUnknown(o, conditions.GithubCredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) } } diff --git a/api/v1beta1/repository_types.go b/api/v1beta1/repository_types.go index 1006290a..8dd9f35a 100644 --- a/api/v1beta1/repository_types.go +++ b/api/v1beta1/repository_types.go @@ -54,12 +54,12 @@ func (r *Repository) InitializeConditions() { conditions.MarkUnknown(r, conditions.PoolManager, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) } - if conditions.Get(r, conditions.SecretReference) == nil { - conditions.MarkUnknown(r, conditions.SecretReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(r, conditions.WebhookSecretReference) == nil { + conditions.MarkUnknown(r, conditions.WebhookSecretReference, conditions.UnknownReason, conditions.WebhookSecretNotReconciledYetMsg) } - if conditions.Get(r, conditions.CredentialsReference) == nil { - conditions.MarkUnknown(r, conditions.CredentialsReference, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) + if conditions.Get(r, conditions.GithubCredentialsReference) == nil { + conditions.MarkUnknown(r, conditions.GithubCredentialsReference, conditions.UnknownReason, conditions.CredentialsNotReconciledYetMsg) } } diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go index 94e5a2ef..1a928d80 100644 --- a/internal/controller/enterprise_controller.go +++ b/internal/controller/enterprise_controller.go @@ -104,20 +104,20 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC webhookSecret, err := secret.FetchRef(ctx, r.Client, &enterprise.Spec.WebhookSecretRef, enterprise.Namespace) if err != nil { event.Error(r.Recorder, enterprise, err.Error()) - conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkFalse(enterprise, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) + conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) + conditions.MarkFalse(enterprise, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(enterprise, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") + conditions.MarkTrue(enterprise, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefSuccessReason, "") credentials, err := r.getCredentialsRef(ctx, enterprise) if err != nil { event.Error(r.Recorder, enterprise, err.Error()) - conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) - conditions.MarkFalse(enterprise, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) + conditions.MarkFalse(enterprise, conditions.ReadyCondition, conditions.FetchingGithubCredentialsRefFailedReason, err.Error()) + conditions.MarkFalse(enterprise, conditions.GithubCredentialsReference, conditions.FetchingGithubCredentialsRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(enterprise, conditions.CredentialsReference, conditions.FetchingCredentialsRefSuccessReason, "") + conditions.MarkTrue(enterprise, conditions.GithubCredentialsReference, conditions.FetchingGithubCredentialsRefSuccessReason, "") garmEnterprise, err := r.getExistingGarmEnterprise(ctx, client, enterprise) if err != nil { diff --git a/internal/controller/enterprise_controller_test.go b/internal/controller/enterprise_controller_test.go index 5746842d..70192964 100644 --- a/internal/controller/enterprise_controller_test.go +++ b/internal/controller/enterprise_controller_test.go @@ -118,8 +118,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -132,8 +132,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -246,8 +246,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -260,10 +260,10 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), + Type: string(conditions.WebhookSecretReference), Status: metav1.ConditionTrue, Message: "", - Reason: string(conditions.FetchingSecretRefSuccessReason), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), LastTransitionTime: metav1.NewTime(time.Now()), }, }, @@ -348,8 +348,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -362,10 +362,10 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), + Type: string(conditions.WebhookSecretReference), Status: metav1.ConditionTrue, Message: "", - Reason: string(conditions.FetchingSecretRefSuccessReason), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), LastTransitionTime: metav1.NewTime(time.Now()), }, }, @@ -477,8 +477,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -491,10 +491,10 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), + Type: string(conditions.WebhookSecretReference), Status: metav1.ConditionTrue, Message: "", - Reason: string(conditions.FetchingSecretRefSuccessReason), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), LastTransitionTime: metav1.NewTime(time.Now()), }, }, @@ -615,8 +615,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -629,10 +629,10 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), + Type: string(conditions.WebhookSecretReference), Status: metav1.ConditionTrue, Message: "", - Reason: string(conditions.FetchingSecretRefSuccessReason), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), LastTransitionTime: metav1.NewTime(time.Now()), }, }, @@ -768,8 +768,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -782,8 +782,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -870,13 +870,13 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(conditions.ReadyCondition), - Reason: string(conditions.FetchingSecretRefFailedReason), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"my-webhook-secret\" not found", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), + Type: string(conditions.GithubCredentialsReference), Reason: string(conditions.UnknownReason), Status: metav1.ConditionUnknown, Message: conditions.CredentialsNotReconciledYetMsg, @@ -890,8 +890,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefFailedReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"my-webhook-secret\" not found", LastTransitionTime: metav1.NewTime(time.Now()), diff --git a/internal/controller/githubcredentials_controller.go b/internal/controller/githubcredentials_controller.go index 95d3d27e..2e465e02 100644 --- a/internal/controller/githubcredentials_controller.go +++ b/internal/controller/githubcredentials_controller.go @@ -112,20 +112,20 @@ func (r *GitHubCredentialReconciler) reconcileNormal(ctx context.Context, client // fetch endpoint resource endpoint, err := r.getEndpointRef(ctx, credentials) if err != nil { - conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.FetchingEndpointRefFailedReason, err.Error()) - conditions.MarkFalse(credentials, conditions.EndpointReference, conditions.FetchingEndpointRefFailedReason, err.Error()) + conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.FetchingGithubEndpointRefFailedReason, err.Error()) + conditions.MarkFalse(credentials, conditions.GithubEndpointReference, conditions.FetchingGithubEndpointRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(credentials, conditions.EndpointReference, conditions.FetchingEndpointRefSuccessReason, "Successfully fetched GitHubEndpoint CR Ref") + conditions.MarkTrue(credentials, conditions.GithubEndpointReference, conditions.FetchingGithubEndpointRefSuccessReason, "Successfully fetched GitHubEndpoint CR Ref") // fetch secret githubSecret, err := secret.FetchRef(ctx, r.Client, &credentials.Spec.SecretRef, credentials.Namespace) if err != nil { - conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkFalse(credentials, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) + conditions.MarkFalse(credentials, conditions.ReadyCondition, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) + conditions.MarkFalse(credentials, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(credentials, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") + conditions.MarkTrue(credentials, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefSuccessReason, "") // if not found, create credentials in garm db if reflect.ValueOf(garmGitHubCreds).IsZero() { diff --git a/internal/controller/githubcredentials_controller_test.go b/internal/controller/githubcredentials_controller_test.go index b3890649..4fe09f13 100644 --- a/internal/controller/githubcredentials_controller_test.go +++ b/internal/controller/githubcredentials_controller_test.go @@ -128,15 +128,15 @@ func TestGitHubCredentialReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.EndpointReference), - Reason: string(conditions.FetchingEndpointRefSuccessReason), + Type: string(conditions.GithubEndpointReference), + Reason: string(conditions.FetchingGithubEndpointRefSuccessReason), Status: metav1.ConditionTrue, Message: "Successfully fetched GitHubEndpoint CR Ref", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -298,15 +298,15 @@ func TestGitHubCredentialReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.EndpointReference), - Reason: string(conditions.FetchingEndpointRefSuccessReason), + Type: string(conditions.GithubEndpointReference), + Reason: string(conditions.FetchingGithubEndpointRefSuccessReason), Status: metav1.ConditionTrue, Message: "Successfully fetched GitHubEndpoint CR Ref", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -443,15 +443,15 @@ func TestGitHubCredentialReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.EndpointReference), - Reason: string(conditions.FetchingEndpointRefSuccessReason), + Type: string(conditions.GithubEndpointReference), + Reason: string(conditions.FetchingGithubEndpointRefSuccessReason), Status: metav1.ConditionTrue, Message: "Successfully fetched GitHubEndpoint CR Ref", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -619,15 +619,15 @@ func TestGitHubCredentialReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.EndpointReference), - Reason: string(conditions.FetchingEndpointRefSuccessReason), + Type: string(conditions.GithubEndpointReference), + Reason: string(conditions.FetchingGithubEndpointRefSuccessReason), Status: metav1.ConditionTrue, Message: "Successfully fetched GitHubEndpoint CR Ref", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -673,15 +673,15 @@ func TestGitHubCredentialReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.EndpointReference), - Reason: string(conditions.FetchingEndpointRefSuccessReason), + Type: string(conditions.GithubEndpointReference), + Reason: string(conditions.FetchingGithubEndpointRefSuccessReason), Status: metav1.ConditionTrue, Message: "Successfully fetched GitHubEndpoint CR Ref", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -850,15 +850,15 @@ func TestGitHubCredentialReconciler_reconcileDelete(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.EndpointReference), - Reason: string(conditions.FetchingEndpointRefSuccessReason), + Type: string(conditions.GithubEndpointReference), + Reason: string(conditions.FetchingGithubEndpointRefSuccessReason), Status: metav1.ConditionTrue, Message: "Successfully fetched GitHubEndpoint CR Ref", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), diff --git a/internal/controller/githubendpoint_controller.go b/internal/controller/githubendpoint_controller.go index 59bdaa59..a50d8c1c 100644 --- a/internal/controller/githubendpoint_controller.go +++ b/internal/controller/githubendpoint_controller.go @@ -241,11 +241,11 @@ func (r *GitHubEndpointReconciler) handleCaCertBundleSecret(ctx context.Context, caCertBundleSecret, err := secret.FetchRef(ctx, r.Client, &endpoint.Spec.CACertBundleSecretRef, endpoint.Namespace) if err != nil { - conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkFalse(endpoint, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) + conditions.MarkFalse(endpoint, conditions.ReadyCondition, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) + conditions.MarkFalse(endpoint, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) return "", err } - conditions.MarkTrue(endpoint, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") + conditions.MarkTrue(endpoint, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefSuccessReason, "") return caCertBundleSecret, nil } diff --git a/internal/controller/githubendpoint_controller_test.go b/internal/controller/githubendpoint_controller_test.go index 18891e04..394095ea 100644 --- a/internal/controller/githubendpoint_controller_test.go +++ b/internal/controller/githubendpoint_controller_test.go @@ -98,8 +98,8 @@ func TestGitHubEndpointReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -169,8 +169,8 @@ func TestGitHubEndpointReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -217,8 +217,8 @@ func TestGitHubEndpointReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -309,8 +309,8 @@ func TestGitHubEndpointReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -479,14 +479,14 @@ func TestGitHubEndpointReconciler_reconcileNormal(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(conditions.ReadyCondition), - Reason: string(conditions.FetchingSecretRefFailedReason), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"ca-cert-bundle\" not found", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefFailedReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"ca-cert-bundle\" not found", LastTransitionTime: metav1.NewTime(time.Now()), @@ -667,8 +667,8 @@ func TestGitHubEndpointReconciler_reconcileDelete(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index 68f7d682..9a314110 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -103,20 +103,20 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar webhookSecret, err := secret.FetchRef(ctx, r.Client, &organization.Spec.WebhookSecretRef, organization.Namespace) if err != nil { event.Error(r.Recorder, organization, err.Error()) - conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkFalse(organization, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) + conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) + conditions.MarkFalse(organization, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(organization, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") + conditions.MarkTrue(organization, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefSuccessReason, "") credentials, err := r.getCredentialsRef(ctx, organization) if err != nil { event.Error(r.Recorder, organization, err.Error()) - conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) - conditions.MarkFalse(organization, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) + conditions.MarkFalse(organization, conditions.ReadyCondition, conditions.FetchingGithubCredentialsRefFailedReason, err.Error()) + conditions.MarkFalse(organization, conditions.GithubCredentialsReference, conditions.FetchingGithubCredentialsRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(organization, conditions.CredentialsReference, conditions.FetchingCredentialsRefSuccessReason, "") + conditions.MarkTrue(organization, conditions.GithubCredentialsReference, conditions.FetchingGithubCredentialsRefSuccessReason, "") garmOrganization, err := r.getExistingGarmOrg(ctx, client, organization) if err != nil { diff --git a/internal/controller/organization_controller_test.go b/internal/controller/organization_controller_test.go index f7657c9b..9db988ac 100644 --- a/internal/controller/organization_controller_test.go +++ b/internal/controller/organization_controller_test.go @@ -118,8 +118,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -132,8 +132,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -246,8 +246,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -260,10 +260,10 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), + Type: string(conditions.WebhookSecretReference), Status: metav1.ConditionTrue, Message: "", - Reason: string(conditions.FetchingSecretRefSuccessReason), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), LastTransitionTime: metav1.NewTime(time.Now()), }, }, @@ -374,8 +374,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -388,10 +388,10 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), + Type: string(conditions.WebhookSecretReference), Status: metav1.ConditionTrue, Message: "", - Reason: string(conditions.FetchingSecretRefSuccessReason), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), LastTransitionTime: metav1.NewTime(time.Now()), }, }, @@ -503,8 +503,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -517,8 +517,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -641,8 +641,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -655,8 +655,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -768,8 +768,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -782,8 +782,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -870,13 +870,13 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(conditions.ReadyCondition), - Reason: string(conditions.FetchingSecretRefFailedReason), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"my-webhook-secret\" not found", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), + Type: string(conditions.GithubCredentialsReference), Reason: string(conditions.UnknownReason), Status: metav1.ConditionUnknown, Message: "credentials not reconciled yet", @@ -890,8 +890,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefFailedReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"my-webhook-secret\" not found", LastTransitionTime: metav1.NewTime(time.Now()), diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go index d5139e97..5208e44f 100644 --- a/internal/controller/repository_controller.go +++ b/internal/controller/repository_controller.go @@ -103,20 +103,20 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC webhookSecret, err := secret.FetchRef(ctx, r.Client, &repository.Spec.WebhookSecretRef, repository.Namespace) if err != nil { event.Error(r.Recorder, repository, err.Error()) - conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingSecretRefFailedReason, err.Error()) - conditions.MarkFalse(repository, conditions.SecretReference, conditions.FetchingSecretRefFailedReason, err.Error()) + conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) + conditions.MarkFalse(repository, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(repository, conditions.SecretReference, conditions.FetchingSecretRefSuccessReason, "") + conditions.MarkTrue(repository, conditions.WebhookSecretReference, conditions.FetchingWebhookSecretRefSuccessReason, "") credentials, err := r.getCredentialsRef(ctx, repository) if err != nil { event.Error(r.Recorder, repository, err.Error()) - conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingCredentialsRefFailedReason, err.Error()) - conditions.MarkFalse(repository, conditions.CredentialsReference, conditions.FetchingCredentialsRefFailedReason, err.Error()) + conditions.MarkFalse(repository, conditions.ReadyCondition, conditions.FetchingGithubCredentialsRefFailedReason, err.Error()) + conditions.MarkFalse(repository, conditions.GithubCredentialsReference, conditions.FetchingGithubCredentialsRefFailedReason, err.Error()) return ctrl.Result{}, err } - conditions.MarkTrue(repository, conditions.CredentialsReference, conditions.FetchingCredentialsRefSuccessReason, "") + conditions.MarkTrue(repository, conditions.GithubCredentialsReference, conditions.FetchingGithubCredentialsRefSuccessReason, "") garmRepository, err := r.getExistingGarmRepo(ctx, client, repository) if err != nil { diff --git a/internal/controller/repository_controller_test.go b/internal/controller/repository_controller_test.go index 9a9add77..aea3839f 100644 --- a/internal/controller/repository_controller_test.go +++ b/internal/controller/repository_controller_test.go @@ -120,8 +120,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -134,8 +134,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -252,8 +252,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -266,8 +266,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -384,8 +384,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -398,8 +398,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -517,8 +517,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -531,8 +531,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -661,8 +661,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -675,8 +675,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefSuccessReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -792,8 +792,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { Message: "Pool Manager is not running", }, { - Type: string(conditions.CredentialsReference), - Reason: string(conditions.FetchingCredentialsRefSuccessReason), + Type: string(conditions.GithubCredentialsReference), + Reason: string(conditions.FetchingGithubCredentialsRefSuccessReason), Status: metav1.ConditionTrue, Message: "", LastTransitionTime: metav1.NewTime(time.Now()), @@ -806,10 +806,10 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), + Type: string(conditions.WebhookSecretReference), Status: metav1.ConditionTrue, Message: "", - Reason: string(conditions.FetchingSecretRefSuccessReason), + Reason: string(conditions.FetchingWebhookSecretRefSuccessReason), LastTransitionTime: metav1.NewTime(time.Now()), }, }, @@ -897,13 +897,13 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(conditions.ReadyCondition), - Reason: string(conditions.FetchingSecretRefFailedReason), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"my-webhook-secret\" not found", LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.CredentialsReference), + Type: string(conditions.GithubCredentialsReference), Reason: string(conditions.UnknownReason), Status: metav1.ConditionUnknown, Message: conditions.CredentialsNotReconciledYetMsg, @@ -917,8 +917,8 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { LastTransitionTime: metav1.NewTime(time.Now()), }, { - Type: string(conditions.SecretReference), - Reason: string(conditions.FetchingSecretRefFailedReason), + Type: string(conditions.WebhookSecretReference), + Reason: string(conditions.FetchingWebhookSecretRefFailedReason), Status: metav1.ConditionFalse, Message: "secrets \"my-webhook-secret\" not found", LastTransitionTime: metav1.NewTime(time.Now()), diff --git a/pkg/conditions/condition_types.go b/pkg/conditions/condition_types.go index 1c986f46..297a673d 100644 --- a/pkg/conditions/condition_types.go +++ b/pkg/conditions/condition_types.go @@ -37,20 +37,20 @@ const ( PoolManagerRunningReason ConditionReason = "PoolManagerRunning" PoolManagerFailureReason ConditionReason = "PoolManagerFailure" - SecretReference ConditionType = "SecretReference" - FetchingSecretRefSuccessReason ConditionReason = "FetchingSecretRefSuccess" - FetchingSecretRefFailedReason ConditionReason = "FetchingSecretRefFailed" + WebhookSecretReference ConditionType = "WebhookSecretReference" + FetchingWebhookSecretRefSuccessReason ConditionReason = "FetchingWebhookSecretRefSuccess" + FetchingWebhookSecretRefFailedReason ConditionReason = "FetchingWebhookSecretRefFailed" - CredentialsReference ConditionType = "CredentialsReference" - FetchingCredentialsRefSuccessReason ConditionReason = "CredentialsRefSuccess" - FetchingCredentialsRefFailedReason ConditionReason = "CredentialsRefFailed" + GithubCredentialsReference ConditionType = "GithubCredentialsReference" + FetchingGithubCredentialsRefSuccessReason ConditionReason = "GithubCredentialsRefSuccess" + FetchingGithubCredentialsRefFailedReason ConditionReason = "GithubCredentialsRefFailed" ) // Credential Conditions const ( - EndpointReference ConditionType = "EndpointReference" - FetchingEndpointRefSuccessReason ConditionReason = "FetchingEndpointRefSuccess" - FetchingEndpointRefFailedReason ConditionReason = "FetchingEndpointRefFailed" + GithubEndpointReference ConditionType = "GithubEndpointReference" + FetchingGithubEndpointRefSuccessReason ConditionReason = "FetchingGithubEndpointRefSuccess" + FetchingGithubEndpointRefFailedReason ConditionReason = "FetchingGithubEndpointRefFailed" ) // Runner Conditions @@ -61,16 +61,18 @@ const ( ) const ( - GarmServerNotReconciledYetMsg string = "GARM server not reconciled yet" - CredentialsNotReconciledYetMsg string = "credentials not reconciled yet" - DeletingEnterpriseMsg string = "Deleting enterprise" - DeletingOrgMsg string = "Deleting organization" - DeletingRepoMsg string = "Deleting repository" - DeletingPoolMsg string = "Deleting pool" - DeletingEndpointMsg string = "Deleting endpoint" - DeletingCredentialsMsg string = "Deleting credentials" // #nosec G101 - DeletingRunnerMsg string = "Deleting runner" // #nosec G101 - RunnerIdleAndRunningMsg string = "Runner is idle and running" // #nosec G101 - RunnerProvisioningFailedMsg string = "Provisioning runner failed" // #nosec G101 - RunnerNotReadyYetMsg string = "Runner is still being provisioned" // #nosec G101 + GarmServerNotReconciledYetMsg string = "GARM server not reconciled yet" + CredentialsNotReconciledYetMsg string = "Github Credentials Ref not reconciled yet" + GithubEndpointNotReconciledYetMsg string = "Github Endpoint Ref not reconciled yet" + WebhookSecretNotReconciledYetMsg string = "Webhook Secret Ref not reconciled yet" + DeletingEnterpriseMsg string = "Deleting enterprise" + DeletingOrgMsg string = "Deleting organization" + DeletingRepoMsg string = "Deleting repository" + DeletingPoolMsg string = "Deleting pool" + DeletingEndpointMsg string = "Deleting endpoint" + DeletingCredentialsMsg string = "Deleting credentials" // #nosec G101 + DeletingRunnerMsg string = "Deleting runner" // #nosec G101 + RunnerIdleAndRunningMsg string = "Runner is idle and running" // #nosec G101 + RunnerProvisioningFailedMsg string = "Provisioning runner failed" // #nosec G101 + RunnerNotReadyYetMsg string = "Runner is still being provisioned" // #nosec G101 ) From 2827af4d1eae94a0c15cfbcc9153bb01eeac3159 Mon Sep 17 00:00:00 2001 From: rthalho Date: Thu, 28 Aug 2025 11:37:15 +0200 Subject: [PATCH 12/27] fix: runner controller status conditions --- ...rm-operator.mercedes-benz.com_runners.yaml | 19 +++------------ internal/controller/runner_controller.go | 13 ++++++---- internal/controller/runner_controller_test.go | 24 +++++++++---------- 3 files changed, 23 insertions(+), 33 deletions(-) diff --git a/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml b/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml index 78607bea..239d41df 100644 --- a/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml +++ b/config/crd/bases/garm-operator.mercedes-benz.com_runners.yaml @@ -236,16 +236,8 @@ spec: type: integer conditions: items: - description: "Condition contains details for one aspect of the current - state of this API Resource.\n---\nThis struct is intended for - direct use as an array at the field path .status.conditions. For - example,\n\n\n\ttype FooStatus struct{\n\t // Represents the - observations of a foo's current state.\n\t // Known .status.conditions.type - are: \"Available\", \"Progressing\", and \"Degraded\"\n\t // - +patchMergeKey=type\n\t // +patchStrategy=merge\n\t // +listType=map\n\t - \ // +listMapKey=type\n\t Conditions []metav1.Condition `json:\"conditions,omitempty\" - patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`\n\n\n\t - \ // other fields\n\t}" + description: Condition contains details for one aspect of the current + state of this API Resource. properties: lastTransitionTime: description: |- @@ -286,12 +278,7 @@ spec: - Unknown type: string type: - description: |- - type of condition in CamelCase or in foo.example.com/CamelCase. - --- - Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be - useful (see .node.status.conditions), the ability to deconflict is important. - The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + description: type of condition in CamelCase or in foo.example.com/CamelCase. maxLength: 316 pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ type: string diff --git a/internal/controller/runner_controller.go b/internal/controller/runner_controller.go index 070c9e1b..59efea14 100644 --- a/internal/controller/runner_controller.go +++ b/internal/controller/runner_controller.go @@ -49,10 +49,13 @@ type RunnerReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=runners/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=runners/finalizers,verbs=update -func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { - log := log.FromContext(ctx) - +func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { instanceClient := garmClient.NewInstanceClient() + return r.reconcileNormal(ctx, req, instanceClient) +} + +func (r *RunnerReconciler) reconcileNormal(ctx context.Context, req ctrl.Request, instanceClient garmClient.InstanceClient) (res ctrl.Result, retErr error) { + log := log.FromContext(ctx) // try fetch runner instance in garm db with events coming from reconcile loop events of RunnerCR or from manually enqueued events of garm api. garmRunner, err := r.getGarmRunnerInstanceByName(instanceClient, req.Name) @@ -304,8 +307,8 @@ func (r *RunnerReconciler) EnqueueRunnerInstances(ctx context.Context, instanceC return nil } -func (r *RunnerReconciler) enqeueRunnerEvents(runnerNames []string) { - for _, runner := range runnerNames { +func (r *RunnerReconciler) enqeueRunnerEvents(runners []string) { + for _, runner := range runners { runnerObj := garmoperatorv1beta1.Runner{ ObjectMeta: metav1.ObjectMeta{ Name: strings.ToLower(runner), diff --git a/internal/controller/runner_controller_test.go b/internal/controller/runner_controller_test.go index d9c77a75..605cf810 100644 --- a/internal/controller/runner_controller_test.go +++ b/internal/controller/runner_controller_test.go @@ -86,8 +86,8 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { OSType: "linux", PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", ProviderID: "kubernetes_external", - Status: commonParams.InstanceRunning, - InstanceStatus: params.RunnerIdle, + InstanceStatus: commonParams.InstanceRunning, + Status: params.RunnerIdle, }, }, }, @@ -117,7 +117,7 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { mockInstanceClient := mock.NewMockInstanceClient(mockCtrl) tt.expectGarmRequest(mockInstanceClient.EXPECT()) - _, err = reconciler.reconcile(context.Background(), tt.req, mockInstanceClient) + _, err = reconciler.reconcileNormal(context.Background(), tt.req, mockInstanceClient) if (err != nil) != tt.wantErr { t.Errorf("RunnerReconciler.reconcile() error = %v, wantErr %v", err, tt.wantErr) return @@ -187,8 +187,8 @@ func TestRunnerReconciler_reconcileDeleteGarmRunner(t *testing.T) { OSType: "linux", PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", ProviderID: "kubernetes_external", - Status: commonParams.InstanceRunning, - InstanceStatus: params.RunnerIdle, + InstanceStatus: commonParams.InstanceRunning, + Status: params.RunnerIdle, }, }, }, @@ -230,8 +230,8 @@ func TestRunnerReconciler_reconcileDeleteGarmRunner(t *testing.T) { OSType: "linux", PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", ProviderID: "kubernetes_external", - Status: commonParams.InstanceRunning, - InstanceStatus: params.RunnerIdle, + InstanceStatus: commonParams.InstanceRunning, + Status: params.RunnerIdle, }, }, }, @@ -261,7 +261,7 @@ func TestRunnerReconciler_reconcileDeleteGarmRunner(t *testing.T) { mockInstanceClient := mock.NewMockInstanceClient(mockCtrl) tt.expectGarmRequest(mockInstanceClient.EXPECT()) - _, err = reconciler.reconcile(context.Background(), tt.req, mockInstanceClient) + _, err = reconciler.reconcileNormal(context.Background(), tt.req, mockInstanceClient) if (err != nil) != tt.wantErr { t.Errorf("RunnerReconciler.reconcile() error = %v, wantErr %v", err, tt.wantErr) return @@ -321,8 +321,8 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { OSType: "linux", PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", ProviderID: "road-runner-k8s-fy5snjcv5dzn", - Status: commonParams.InstanceRunning, - InstanceStatus: params.RunnerIdle, + InstanceStatus: commonParams.InstanceRunning, + Status: params.RunnerIdle, }, }, &garmoperatorv1beta1.Runner{ @@ -346,8 +346,8 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { OSType: "linux", PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", ProviderID: "road-runner-k8s-n6kq2mt3k4qr", - Status: commonParams.InstanceRunning, - InstanceStatus: params.RunnerIdle, + InstanceStatus: commonParams.InstanceRunning, + Status: params.RunnerIdle, }, }, &garmoperatorv1beta1.Pool{ From 4d356153e845d91c254280e7a2a3298c67e304da Mon Sep 17 00:00:00 2001 From: rthalho Date: Thu, 28 Aug 2025 11:38:08 +0200 Subject: [PATCH 13/27] fix: enterprise controller tests --- internal/controller/enterprise_controller.go | 14 ++++++++++---- internal/controller/enterprise_controller_test.go | 12 ++++++------ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go index 1a928d80..91dd901b 100644 --- a/internal/controller/enterprise_controller.go +++ b/internal/controller/enterprise_controller.go @@ -50,16 +50,24 @@ type EnterpriseReconciler struct { func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) + enterpriseClient := garmClient.NewEnterpriseClient() + enterprise := &garmoperatorv1beta1.Enterprise{} err := r.Get(ctx, req.NamespacedName, enterprise) if err != nil { if apierrors.IsNotFound(err) { - log.Info("object was not found") + log.Info("object was not found", "name", req.Name, "namespace", req.Namespace, "kind", "Enterprise") return ctrl.Result{}, nil } return ctrl.Result{}, err } + return r.reconcile(ctx, enterpriseClient, enterprise) +} + +func (r *EnterpriseReconciler) reconcile(ctx context.Context, enterpriseClient garmClient.EnterpriseClient, enterprise *garmoperatorv1beta1.Enterprise) (res ctrl.Result, retErr error) { + log := log.FromContext(ctx) + orig := enterprise.DeepCopy() // Ignore objects that are paused @@ -73,8 +81,6 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - enterpriseClient := garmClient.NewEnterpriseClient() - // Initialize conditions to unknown if not set already enterprise.InitializeConditions() @@ -83,7 +89,7 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) if !reflect.DeepEqual(enterprise.Status, orig.Status) { if err := r.Status().Update(ctx, enterprise); err != nil { log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + res = ctrl.Result{} retErr = err } } diff --git a/internal/controller/enterprise_controller_test.go b/internal/controller/enterprise_controller_test.go index 70192964..89d7f638 100644 --- a/internal/controller/enterprise_controller_test.go +++ b/internal/controller/enterprise_controller_test.go @@ -925,10 +925,10 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { enterprise := tt.object.DeepCopyObject().(*garmoperatorv1beta1.Enterprise) - mockEnterprise := mock.NewMockEnterpriseClient(mockCtrl) - tt.expectGarmRequest(mockEnterprise.EXPECT()) + mockEnterpriseClient := mock.NewMockEnterpriseClient(mockCtrl) + tt.expectGarmRequest(mockEnterpriseClient.EXPECT()) - _, err = reconciler.reconcileNormal(context.Background(), mockEnterprise, enterprise) + _, err = reconciler.reconcile(context.Background(), mockEnterpriseClient, enterprise) if (err != nil) != tt.wantErr { t.Errorf("EnterpriseReconciler.reconcileNormal() error = %v, wantErr %v", err, tt.wantErr) return @@ -1045,10 +1045,10 @@ func TestEnterpriseReconciler_reconcileDelete(t *testing.T) { enterprise := tt.object.DeepCopyObject().(*garmoperatorv1beta1.Enterprise) - mockEnterprise := mock.NewMockEnterpriseClient(mockCtrl) - tt.expectGarmRequest(mockEnterprise.EXPECT()) + mockEnterpriseClient := mock.NewMockEnterpriseClient(mockCtrl) + tt.expectGarmRequest(mockEnterpriseClient.EXPECT()) - _, err = reconciler.reconcileDelete(context.Background(), mockEnterprise, enterprise) + _, err = reconciler.reconcileDelete(context.Background(), mockEnterpriseClient, enterprise) if (err != nil) != tt.wantErr { t.Errorf("EnterpriseReconciler.reconcileDelete() error = %v, wantErr %v", err, tt.wantErr) return From 5976f747f4bc3c42845de56ab06f8b19856b5301 Mon Sep 17 00:00:00 2001 From: rthalho Date: Fri, 29 Aug 2025 08:00:21 +0200 Subject: [PATCH 14/27] fix: garmserverconfig_controller tests --- .../controller/garmserverconfig_controller_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/controller/garmserverconfig_controller_test.go b/internal/controller/garmserverconfig_controller_test.go index 428f5504..e36d1337 100644 --- a/internal/controller/garmserverconfig_controller_test.go +++ b/internal/controller/garmserverconfig_controller_test.go @@ -5,6 +5,7 @@ import ( "context" "reflect" "testing" + "time" "github.com/cloudbase/garm/client/controller_info" "github.com/cloudbase/garm/params" @@ -67,6 +68,15 @@ func TestGarmServerConfig_reconcile(t *testing.T) { ControllerWebhookURL: " http://garm-server.garm-server.svc:9997/api/v1/webhook/BE4B3620-D424-43AC-8EDD-5760DBD516BF", MinimumJobAgeBackoff: 30, Version: "v0.1.5", + Conditions: []metav1.Condition{ + { + Type: string(conditions.ReadyCondition), + Reason: string(conditions.SuccessfulReconcileReason), + Status: metav1.ConditionTrue, + Message: "", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, }, }, runtimeObjects: []runtime.Object{}, From 23a85495997fb1e430e9ebdedb545c69e631d245 Mon Sep 17 00:00:00 2001 From: rthalho Date: Fri, 29 Aug 2025 08:38:34 +0200 Subject: [PATCH 15/27] fix: organization_controller tests --- internal/controller/organization_controller.go | 10 ++++++++-- internal/controller/organization_controller_test.go | 10 +++++----- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index 9a314110..e2c406d9 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -49,6 +49,8 @@ type OrganizationReconciler struct { func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) + organizationClient := garmClient.NewOrganizationClient() + organization := &garmoperatorv1beta1.Organization{} err := r.Get(ctx, req.NamespacedName, organization) if err != nil { @@ -59,6 +61,12 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } + return r.reconcile(ctx, organizationClient, organization) +} + +func (r *OrganizationReconciler) reconcile(ctx context.Context, organizationClient garmClient.OrganizationClient, organization *garmoperatorv1beta1.Organization) (res ctrl.Result, retErr error) { + log := log.FromContext(ctx) + orig := organization.DeepCopy() // Ignore objects that are paused @@ -72,8 +80,6 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } - organizationClient := garmClient.NewOrganizationClient() - // Initialize conditions to unknown if not set already organization.InitializeConditions() diff --git a/internal/controller/organization_controller_test.go b/internal/controller/organization_controller_test.go index 9db988ac..d0c4a828 100644 --- a/internal/controller/organization_controller_test.go +++ b/internal/controller/organization_controller_test.go @@ -879,14 +879,14 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { Type: string(conditions.GithubCredentialsReference), Reason: string(conditions.UnknownReason), Status: metav1.ConditionUnknown, - Message: "credentials not reconciled yet", + Message: conditions.CredentialsNotReconciledYetMsg, LastTransitionTime: metav1.NewTime(time.Now()), }, { Type: string(conditions.PoolManager), Reason: string(conditions.UnknownReason), Status: metav1.ConditionUnknown, - Message: "GARM server not reconciled yet", + Message: conditions.GarmServerNotReconciledYetMsg, LastTransitionTime: metav1.NewTime(time.Now()), }, { @@ -925,10 +925,10 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { organization := tt.object.DeepCopyObject().(*garmoperatorv1beta1.Organization) - mockOrganization := mock.NewMockOrganizationClient(mockCtrl) - tt.expectGarmRequest(mockOrganization.EXPECT()) + mockOrganizationClient := mock.NewMockOrganizationClient(mockCtrl) + tt.expectGarmRequest(mockOrganizationClient.EXPECT()) - _, err = reconciler.reconcileNormal(context.Background(), mockOrganization, organization) + _, err = reconciler.reconcile(context.Background(), mockOrganizationClient, organization) if (err != nil) != tt.wantErr { t.Errorf("OrganizationReconciler.reconcileNormal() error = %v, wantErr %v", err, tt.wantErr) return From b3e21cb5a643f3ff68bf951d6db9ff1c682ea441 Mon Sep 17 00:00:00 2001 From: rthalho Date: Fri, 29 Aug 2025 08:38:53 +0200 Subject: [PATCH 16/27] fix: repository_controller tests --- internal/controller/repository_controller.go | 10 ++++++++-- internal/controller/repository_controller_test.go | 6 +++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go index 5208e44f..025963e0 100644 --- a/internal/controller/repository_controller.go +++ b/internal/controller/repository_controller.go @@ -49,6 +49,8 @@ type RepositoryReconciler struct { func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) + repositoryClient := garmClient.NewRepositoryClient() + repository := &garmoperatorv1beta1.Repository{} err := r.Get(ctx, req.NamespacedName, repository) if err != nil { @@ -59,6 +61,12 @@ func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } + return r.reconcile(ctx, repositoryClient, repository) +} + +func (r *RepositoryReconciler) reconcile(ctx context.Context, repositoryClient garmClient.RepositoryClient, repository *garmoperatorv1beta1.Repository) (res ctrl.Result, retErr error) { + log := log.FromContext(ctx) + orig := repository.DeepCopy() // Ignore objects that are paused @@ -72,8 +80,6 @@ func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - repositoryClient := garmClient.NewRepositoryClient() - // initialize conditions repository.InitializeConditions() diff --git a/internal/controller/repository_controller_test.go b/internal/controller/repository_controller_test.go index aea3839f..2ed781c6 100644 --- a/internal/controller/repository_controller_test.go +++ b/internal/controller/repository_controller_test.go @@ -952,10 +952,10 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { repository := tt.object.DeepCopyObject().(*garmoperatorv1beta1.Repository) - mockRepository := mock.NewMockRepositoryClient(mockCtrl) - tt.expectGarmRequest(mockRepository.EXPECT()) + mockRepositoryClient := mock.NewMockRepositoryClient(mockCtrl) + tt.expectGarmRequest(mockRepositoryClient.EXPECT()) - _, err = reconciler.reconcileNormal(context.Background(), mockRepository, repository) + _, err = reconciler.reconcile(context.Background(), mockRepositoryClient, repository) if (err != nil) != tt.wantErr { t.Errorf("RepositoryReconciler.reconcileNormal() error = %v, wantErr %v", err, tt.wantErr) return From b1415a356e13930d110efed0f9dfcfb5eab4a1f3 Mon Sep 17 00:00:00 2001 From: rthalho Date: Fri, 29 Aug 2025 15:46:16 +0200 Subject: [PATCH 17/27] fix: runner_controller tests --- internal/controller/runner_controller.go | 13 +- internal/controller/runner_controller_test.go | 179 ++++++++++++++---- 2 files changed, 150 insertions(+), 42 deletions(-) diff --git a/internal/controller/runner_controller.go b/internal/controller/runner_controller.go index 59efea14..7464ce80 100644 --- a/internal/controller/runner_controller.go +++ b/internal/controller/runner_controller.go @@ -4,7 +4,6 @@ package controller import ( "context" - "fmt" commonParams "github.com/cloudbase/garm-provider-common/params" "github.com/mercedes-benz/garm-operator/pkg/conditions" "reflect" @@ -105,15 +104,12 @@ func (r *RunnerReconciler) reconcileNormal(ctx context.Context, req ctrl.Request if !reflect.DeepEqual(runner.Status, orig.Status) { log.Info("Update runner status...") diff := cmp.Diff(orig.Status, runner.Status) - fmt.Println("Differences found:") - fmt.Println(diff) + log.V(1).Info("Runner status changed", "diff", diff) if err := r.Status().Update(ctx, runner); err != nil { - log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + log.Error(err, "failed to update status", "runner", runner.Name) + res = ctrl.Result{} retErr = err } - } else { - log.Info("Nothing changed in runner status") } }() @@ -124,6 +120,9 @@ func (r *RunnerReconciler) reconcileNormal(ctx context.Context, req ctrl.Request // sync garm runner status back to RunnerCR err = r.updateRunnerStatus(ctx, runner, garmRunner) + if err != nil { + log.Error(err, "Failed to update runner status", "runner", runner.Name) + } return ctrl.Result{}, err } diff --git a/internal/controller/runner_controller_test.go b/internal/controller/runner_controller_test.go index 605cf810..9fefcab1 100644 --- a/internal/controller/runner_controller_test.go +++ b/internal/controller/runner_controller_test.go @@ -3,8 +3,9 @@ package controller import ( "context" + "github.com/mercedes-benz/garm-operator/pkg/conditions" + apierrors "k8s.io/apimachinery/pkg/api/errors" "reflect" - "slices" "strings" "testing" "time" @@ -50,7 +51,40 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { Namespace: "runner", }, }, - runtimeObjects: []runtime.Object{}, + runtimeObjects: []runtime.Object{ + &garmoperatorv1beta1.Pool{ + TypeMeta: metav1.TypeMeta{ + Kind: "Pool", + APIVersion: garmoperatorv1beta1.GroupVersion.Group + "/" + garmoperatorv1beta1.GroupVersion.Version, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-enterprise-pool", + Namespace: "test-namespace", + }, + Spec: garmoperatorv1beta1.PoolSpec{ + GitHubScopeRef: corev1.TypedLocalObjectReference{ + APIGroup: &garmoperatorv1beta1.GroupVersion.Group, + Kind: string(garmoperatorv1beta1.EnterpriseScope), + Name: "my-enterprise", + }, + ProviderName: "kubernetes_external", + MaxRunners: 5, + MinIdleRunners: 3, + ImageName: "ubuntu-image", + Flavor: "medium", + OSType: "linux", + OSArch: "arm64", + Tags: []string{"kubernetes", "linux", "arm64", "ubuntu"}, + Enabled: true, + RunnerBootstrapTimeout: 20, + ExtraSpecs: "", + GitHubRunnerGroup: "", + }, + Status: garmoperatorv1beta1.PoolStatus{ + ID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", + }, + }, + }, expectGarmRequest: func(m *mock.MockInstanceClientMockRecorder) { response := params.Instances{ params.Instance{ @@ -67,6 +101,7 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { } m.ListInstances(instances.NewListInstancesParams()).Return(&instances.ListInstancesOK{Payload: response}, nil) + m.ListInstances(instances.NewListInstancesParams()).Return(&instances.ListInstancesOK{Payload: response}, nil) }, wantErr: false, expectedObject: &garmoperatorv1beta1.Runner{ @@ -84,10 +119,19 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { ID: "8215f6c6-486e-4893-84df-3231b185a148", OSArch: "amd64", OSType: "linux", - PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", + PoolID: "my-enterprise-pool", ProviderID: "kubernetes_external", InstanceStatus: commonParams.InstanceRunning, Status: params.RunnerIdle, + Conditions: []metav1.Condition{ + { + Type: string(conditions.ReadyCondition), + Reason: string(conditions.RunnerReadyReason), + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now()), + Message: conditions.RunnerIdleAndRunningMsg, + }, + }, }, }, }, @@ -123,6 +167,12 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { return } + _, err = reconciler.reconcileNormal(context.Background(), tt.req, mockInstanceClient) + if (err != nil) != tt.wantErr { + t.Errorf("RunnerReconciler.reconcile() error = %v, wantErr %v", err, tt.wantErr) + return + } + runner := &garmoperatorv1beta1.Runner{} err = client.Get(context.Background(), types.NamespacedName{Namespace: tt.req.Namespace, Name: strings.ToLower(tt.req.Name)}, runner) if (err != nil) != tt.wantErr { @@ -135,8 +185,12 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { // empty resource version to avoid comparison errors runner.ObjectMeta.ResourceVersion = "" + + conditions.NilLastTransitionTime(tt.expectedObject) + conditions.NilLastTransitionTime(runner) + if !reflect.DeepEqual(runner, tt.expectedObject) { - t.Errorf("RunnerReconciler.reconcile() got = %#v, want %#v", runner, tt.expectedObject) + t.Errorf("RunnerReconciler.reconcile() \ngot = %#v\n want %#v", runner, tt.expectedObject) } }) } @@ -269,12 +323,7 @@ func TestRunnerReconciler_reconcileDeleteGarmRunner(t *testing.T) { runner := &garmoperatorv1beta1.Runner{} err = client.Get(context.Background(), types.NamespacedName{Namespace: tt.req.Namespace, Name: strings.ToLower(tt.req.Name)}, runner) - if (err != nil) != tt.wantErr { - t.Errorf("RunnerReconciler.reconcile() error = %v, wantErr %v", err, tt.wantErr) - return - } - - assert.NotZero(t, runner.ObjectMeta.DeletionTimestamp) + assert.Equal(t, true, apierrors.IsNotFound(err)) }) } } @@ -283,22 +332,18 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() + now := metav1.NewTime(time.Now().UTC()) + tests := []struct { name string - req ctrl.Request runtimeObjects []runtime.Object expectGarmRequest func(m *mock.MockInstanceClientMockRecorder) wantErr bool - expectedEvents []event.GenericEvent + expectedObject *garmoperatorv1beta1.Runner + expectedLength int }{ { name: "Delete Runner CR with no matching entry in Garm DB", - req: ctrl.Request{ - NamespacedName: types.NamespacedName{ - Name: "road-runner-k8s-fy5snjcv5dzn", - Namespace: "runner", - }, - }, runtimeObjects: []runtime.Object{ &garmoperatorv1beta1.Runner{ TypeMeta: metav1.TypeMeta{ @@ -323,6 +368,15 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { ProviderID: "road-runner-k8s-fy5snjcv5dzn", InstanceStatus: commonParams.InstanceRunning, Status: params.RunnerIdle, + Conditions: []metav1.Condition{ + { + Type: string(conditions.ReadyCondition), + Reason: string(conditions.RunnerReadyReason), + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now()), + Message: conditions.RunnerIdleAndRunningMsg, + }, + }, }, }, &garmoperatorv1beta1.Runner{ @@ -348,6 +402,15 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { ProviderID: "road-runner-k8s-n6kq2mt3k4qr", InstanceStatus: commonParams.InstanceRunning, Status: params.RunnerIdle, + Conditions: []metav1.Condition{ + { + Type: string(conditions.ReadyCondition), + Reason: string(conditions.RunnerReadyReason), + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now()), + Message: conditions.RunnerIdleAndRunningMsg, + }, + }, }, }, &garmoperatorv1beta1.Pool{ @@ -384,7 +447,23 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { }, }, expectGarmRequest: func(m *mock.MockInstanceClientMockRecorder) { - response := &instances.ListPoolInstancesOK{Payload: params.Instances{ + allInstancesResponse := &instances.ListInstancesOK{Payload: params.Instances{ + params.Instance{ + Name: "road-runner-k8s-FY5snJcv5dzn", + AgentID: 120, + ID: "8215f6c6-486e-4893-84df-3231b185a148", + OSArch: "amd64", + OSType: "linux", + PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", + ProviderID: "road-runner-k8s-fy5snjcv5dzn", + Status: commonParams.InstanceRunning, + RunnerStatus: params.RunnerIdle, + }, + }} + m.ListInstances(instances.NewListInstancesParams()).Return(allInstancesResponse, nil) + m.ListInstances(instances.NewListInstancesParams()).Return(allInstancesResponse, nil) + + poolInstancesResponse := &instances.ListPoolInstancesOK{Payload: params.Instances{ params.Instance{ Name: "road-runner-k8s-FY5snJcv5dzn", AgentID: 120, @@ -397,15 +476,41 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { RunnerStatus: params.RunnerIdle, }, }} - m.ListPoolInstances(instances.NewListPoolInstancesParams().WithPoolID("a46553c6-ad87-454b-b5f5-a1c468d78c1e")).Return(response, nil) + m.ListPoolInstances(instances.NewListPoolInstancesParams().WithPoolID("a46553c6-ad87-454b-b5f5-a1c468d78c1e")).Return(poolInstancesResponse, nil) + }, wantErr: false, - expectedEvents: []event.GenericEvent{ - { - Object: &garmoperatorv1beta1.Runner{ - ObjectMeta: metav1.ObjectMeta{ - Name: "road-runner-k8s-fy5snjcv5dzn", - Namespace: "test-namespace", + expectedObject: &garmoperatorv1beta1.Runner{ + TypeMeta: metav1.TypeMeta{ + Kind: "Runner", + APIVersion: garmoperatorv1beta1.GroupVersion.Group + "/" + garmoperatorv1beta1.GroupVersion.Version, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "road-runner-k8s-n6kq2mt3k4qr", + Namespace: "test-namespace", + DeletionTimestamp: &now, + Finalizers: []string{ + key.RunnerFinalizerName, + }, + }, + Spec: garmoperatorv1beta1.RunnerSpec{}, + Status: garmoperatorv1beta1.RunnerStatus{ + Name: "road-runner-k8s-n6KQ2Mt3k4qr", + AgentID: 130, + ID: "13d31cad-588b-4ea8-8015-052a76ad3dd3", + OSArch: "amd64", + OSType: "linux", + PoolID: "a46553c6-ad87-454b-b5f5-a1c468d78c1e", + ProviderID: "road-runner-k8s-n6kq2mt3k4qr", + InstanceStatus: commonParams.InstanceRunning, + Status: params.RunnerIdle, + Conditions: []metav1.Condition{ + { + Type: string(conditions.ReadyCondition), + Reason: string(conditions.RunnerReadyReason), + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now()), + Message: conditions.RunnerIdleAndRunningMsg, }, }, }, @@ -448,19 +553,23 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { close(reconciler.ReconcileChan) }() - // receive events in fake channel and compare if expected event occurs by filtering received events in fake channel by expected event - var eventCount int for obj := range reconciler.ReconcileChan { t.Logf("Received Event: %s", obj.Object.GetName()) - filtered := slices.CompactFunc(tt.expectedEvents, func(i, j event.GenericEvent) bool { - return i.Object.GetName() == j.Object.GetName() && i.Object.GetNamespace() == j.Object.GetNamespace() - }) - - eventCount++ - assert.Equal(t, 1, len(filtered)) + _, err = reconciler.reconcileNormal(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: obj.Object.GetName(), Namespace: obj.Object.GetNamespace()}}, mockInstanceClient) + if (err != nil) != tt.wantErr { + t.Errorf("RunnerReconciler.reconcile() error = %v, wantErr %v", err, tt.wantErr) + return + } + } + runner := &garmoperatorv1beta1.Runner{} + err = client.Get(context.Background(), types.NamespacedName{Namespace: tt.expectedObject.Namespace, Name: tt.expectedObject.Name}, runner) + if (err != nil) != tt.wantErr { + t.Errorf("RunnerReconciler.EnqueueRunnerInstances() error = %v, wantErr %v", err, tt.wantErr) + return } - assert.Equal(t, len(tt.expectedEvents), eventCount) + + assert.Equal(t, true, !runner.DeletionTimestamp.IsZero()) }) } } From 0dc473e127e97c59f84dcef6ac14d6361ab1199e Mon Sep 17 00:00:00 2001 From: rthalho Date: Fri, 29 Aug 2025 15:55:12 +0200 Subject: [PATCH 18/27] chore: fix go lint --- api/v1alpha1/runner_conversion.go | 30 +++++++------- api/v1beta1/garmserverconfig_types.go | 3 +- api/v1beta1/githubcredentials_types.go | 3 +- api/v1beta1/githubendpoint_types.go | 3 +- api/v1beta1/pool_types.go | 3 +- api/v1beta1/runner_types.go | 3 +- .../controller/garmserverconfig_controller.go | 4 +- internal/controller/runner_controller.go | 39 ++++--------------- internal/controller/runner_controller_test.go | 5 +-- pkg/conditions/condition_types.go | 12 +++--- 10 files changed, 42 insertions(+), 63 deletions(-) diff --git a/api/v1alpha1/runner_conversion.go b/api/v1alpha1/runner_conversion.go index 8bab0daf..69b5facb 100644 --- a/api/v1alpha1/runner_conversion.go +++ b/api/v1alpha1/runner_conversion.go @@ -19,22 +19,20 @@ func (r *Runner) ConvertFrom(dstRaw conversion.Hub) error { } func Convert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(in *v1beta1.RunnerStatus, out *RunnerStatus, s apiconversion.Scope) error { - out = &RunnerStatus{ - ID: in.ID, - ProviderID: in.ProviderID, - AgentID: in.AgentID, - Name: in.Name, - OSType: in.OSType, - OSName: in.OSName, - OSVersion: in.OSVersion, - OSArch: in.OSArch, - Addresses: nil, - Status: in.Status, - InstanceStatus: in.InstanceStatus, - PoolID: in.PoolID, - ProviderFault: in.ProviderFault, - GitHubRunnerGroup: in.GitHubRunnerGroup, - } + out.ID = in.ID + out.ProviderID = in.ProviderID + out.AgentID = in.AgentID + out.Name = in.Name + out.OSType = in.OSType + out.OSName = in.OSName + out.OSVersion = in.OSVersion + out.OSArch = in.OSArch + out.Addresses = nil + out.Status = in.Status + out.InstanceStatus = in.InstanceStatus + out.PoolID = in.PoolID + out.ProviderFault = in.ProviderFault + out.GitHubRunnerGroup = in.GitHubRunnerGroup return autoConvert_v1beta1_RunnerStatus_To_v1alpha1_RunnerStatus(in, out, s) } diff --git a/api/v1beta1/garmserverconfig_types.go b/api/v1beta1/garmserverconfig_types.go index 6cd6174b..3ce26bac 100644 --- a/api/v1beta1/garmserverconfig_types.go +++ b/api/v1beta1/garmserverconfig_types.go @@ -3,8 +3,9 @@ package v1beta1 import ( - "github.com/mercedes-benz/garm-operator/pkg/conditions" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/mercedes-benz/garm-operator/pkg/conditions" ) // GarmServerConfigSpec defines the desired state of GarmServerConfig diff --git a/api/v1beta1/githubcredentials_types.go b/api/v1beta1/githubcredentials_types.go index c2de0bc5..29a4b6a7 100644 --- a/api/v1beta1/githubcredentials_types.go +++ b/api/v1beta1/githubcredentials_types.go @@ -4,9 +4,10 @@ package v1beta1 import ( "github.com/cloudbase/garm/params" - "github.com/mercedes-benz/garm-operator/pkg/conditions" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/mercedes-benz/garm-operator/pkg/conditions" ) // GitHubCredentialSpec defines the desired state of GitHubCredential diff --git a/api/v1beta1/githubendpoint_types.go b/api/v1beta1/githubendpoint_types.go index 6df6aabf..0d88a332 100644 --- a/api/v1beta1/githubendpoint_types.go +++ b/api/v1beta1/githubendpoint_types.go @@ -3,8 +3,9 @@ package v1beta1 import ( - "github.com/mercedes-benz/garm-operator/pkg/conditions" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/mercedes-benz/garm-operator/pkg/conditions" ) // GitHubEndpointSpec defines the desired state of GitHubEndpoint diff --git a/api/v1beta1/pool_types.go b/api/v1beta1/pool_types.go index 4f6feeea..501f4c2e 100644 --- a/api/v1beta1/pool_types.go +++ b/api/v1beta1/pool_types.go @@ -4,9 +4,10 @@ package v1beta1 import ( commonParams "github.com/cloudbase/garm-provider-common/params" - "github.com/mercedes-benz/garm-operator/pkg/conditions" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/mercedes-benz/garm-operator/pkg/conditions" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! diff --git a/api/v1beta1/runner_types.go b/api/v1beta1/runner_types.go index 3fe463fe..e7e69599 100644 --- a/api/v1beta1/runner_types.go +++ b/api/v1beta1/runner_types.go @@ -5,8 +5,9 @@ package v1beta1 import ( commonParams "github.com/cloudbase/garm-provider-common/params" "github.com/cloudbase/garm/params" - "github.com/mercedes-benz/garm-operator/pkg/conditions" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/mercedes-benz/garm-operator/pkg/conditions" ) // RunnerSpec defines the desired state of Runner diff --git a/internal/controller/garmserverconfig_controller.go b/internal/controller/garmserverconfig_controller.go index e5d89d89..e4257413 100644 --- a/internal/controller/garmserverconfig_controller.go +++ b/internal/controller/garmserverconfig_controller.go @@ -5,8 +5,6 @@ package controller import ( "context" "errors" - "github.com/mercedes-benz/garm-operator/pkg/conditions" - "github.com/mercedes-benz/garm-operator/pkg/event" "reflect" garmapiserverparams "github.com/cloudbase/garm/apiserver/params" @@ -24,6 +22,8 @@ import ( garmoperatorv1beta1 "github.com/mercedes-benz/garm-operator/api/v1beta1" "github.com/mercedes-benz/garm-operator/pkg/annotations" garmclient "github.com/mercedes-benz/garm-operator/pkg/client" + "github.com/mercedes-benz/garm-operator/pkg/conditions" + "github.com/mercedes-benz/garm-operator/pkg/event" "github.com/mercedes-benz/garm-operator/pkg/util" ) diff --git a/internal/controller/runner_controller.go b/internal/controller/runner_controller.go index 7464ce80..69bef0eb 100644 --- a/internal/controller/runner_controller.go +++ b/internal/controller/runner_controller.go @@ -4,14 +4,14 @@ package controller import ( "context" - commonParams "github.com/cloudbase/garm-provider-common/params" - "github.com/mercedes-benz/garm-operator/pkg/conditions" "reflect" "strings" "time" + commonParams "github.com/cloudbase/garm-provider-common/params" "github.com/cloudbase/garm/client/instances" "github.com/cloudbase/garm/params" + "github.com/google/go-cmp/cmp" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -29,11 +29,10 @@ import ( garmoperatorv1beta1 "github.com/mercedes-benz/garm-operator/api/v1beta1" garmClient "github.com/mercedes-benz/garm-operator/pkg/client" "github.com/mercedes-benz/garm-operator/pkg/client/key" + "github.com/mercedes-benz/garm-operator/pkg/conditions" "github.com/mercedes-benz/garm-operator/pkg/config" "github.com/mercedes-benz/garm-operator/pkg/filter" runnerUtil "github.com/mercedes-benz/garm-operator/pkg/runners" - - "github.com/google/go-cmp/cmp" ) // RunnerReconciler reconciles a Runner object @@ -81,7 +80,7 @@ func (r *RunnerReconciler) reconcileNormal(ctx context.Context, req ctrl.Request // Did not find RunnerCR and found garm runner, create the RunnerCR case apierrors.IsNotFound(err) && garmRunner != nil: log.Info("Did not find RunnerCR and found garm runner, creating RunnerCR", "runner", garmRunner.Name) - return r.createRunnerCR(ctx, runner, garmRunner, req.Namespace) + return r.createRunnerCR(ctx, garmRunner, req.Namespace) // Did not find RunnerCR and no matching garm runner, do nothing case apierrors.IsNotFound(err) && garmRunner == nil: @@ -127,11 +126,11 @@ func (r *RunnerReconciler) reconcileNormal(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } -func (r *RunnerReconciler) createRunnerCR(ctx context.Context, runnerCR *garmoperatorv1beta1.Runner, garmRunner *params.Instance, namespace string) (ctrl.Result, error) { +func (r *RunnerReconciler) createRunnerCR(ctx context.Context, garmRunner *params.Instance, namespace string) (ctrl.Result, error) { log := log.FromContext(ctx) log.Info("Creating RunnerCR", "Runner", garmRunner.Name) - runnerCR = &garmoperatorv1beta1.Runner{ + runnerCR := &garmoperatorv1beta1.Runner{ ObjectMeta: metav1.ObjectMeta{ Name: strings.ToLower(garmRunner.Name), Namespace: namespace, @@ -300,9 +299,9 @@ func (r *RunnerReconciler) EnqueueRunnerInstances(ctx context.Context, instanceC runnersToDelete := getRunnerDiff(runnerCRNameList, runnerInstanceNameList) - runnersToEnqueue := append(runnerInstanceNameList, runnersToDelete...) + runnerInstanceNameList = append(runnerInstanceNameList, runnersToDelete...) - r.enqeueRunnerEvents(runnersToEnqueue) + r.enqeueRunnerEvents(runnerInstanceNameList) return nil } @@ -347,28 +346,6 @@ func (r *RunnerReconciler) fetchRunnerInstancesByNamespacedPools(instanceClient return garmRunnerInstances, nil } -func (r *RunnerReconciler) runnerSpecsEqual(runner garmoperatorv1beta1.Runner, garmRunner *params.Instance) bool { - runner.Status.PoolID = "" - tmpRunnerStatus := garmoperatorv1beta1.RunnerStatus{ - ID: garmRunner.ID, - ProviderID: garmRunner.ProviderID, - AgentID: garmRunner.AgentID, - Name: garmRunner.Name, - OSType: garmRunner.OSType, - OSName: garmRunner.OSName, - OSVersion: garmRunner.OSVersion, - OSArch: garmRunner.OSArch, - Addresses: garmRunner.Addresses, - Status: garmRunner.RunnerStatus, - InstanceStatus: garmRunner.Status, - ProviderFault: string(garmRunner.ProviderFault), - GitHubRunnerGroup: garmRunner.GitHubRunnerGroup, - PoolID: "", - } - - return reflect.DeepEqual(runner.Status, tmpRunnerStatus) -} - func getRunnerDiff(runnerCRs, garmRunners []string) []string { cache := make(map[string]struct{}) var diff []string diff --git a/internal/controller/runner_controller_test.go b/internal/controller/runner_controller_test.go index 9fefcab1..6f252a19 100644 --- a/internal/controller/runner_controller_test.go +++ b/internal/controller/runner_controller_test.go @@ -3,8 +3,6 @@ package controller import ( "context" - "github.com/mercedes-benz/garm-operator/pkg/conditions" - apierrors "k8s.io/apimachinery/pkg/api/errors" "reflect" "strings" "testing" @@ -16,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "go.uber.org/mock/gomock" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -28,6 +27,7 @@ import ( garmoperatorv1beta1 "github.com/mercedes-benz/garm-operator/api/v1beta1" "github.com/mercedes-benz/garm-operator/pkg/client/key" "github.com/mercedes-benz/garm-operator/pkg/client/mock" + "github.com/mercedes-benz/garm-operator/pkg/conditions" "github.com/mercedes-benz/garm-operator/pkg/config" ) @@ -477,7 +477,6 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { }, }} m.ListPoolInstances(instances.NewListPoolInstancesParams().WithPoolID("a46553c6-ad87-454b-b5f5-a1c468d78c1e")).Return(poolInstancesResponse, nil) - }, wantErr: false, expectedObject: &garmoperatorv1beta1.Runner{ diff --git a/pkg/conditions/condition_types.go b/pkg/conditions/condition_types.go index 297a673d..7c1d5ec4 100644 --- a/pkg/conditions/condition_types.go +++ b/pkg/conditions/condition_types.go @@ -41,9 +41,9 @@ const ( FetchingWebhookSecretRefSuccessReason ConditionReason = "FetchingWebhookSecretRefSuccess" FetchingWebhookSecretRefFailedReason ConditionReason = "FetchingWebhookSecretRefFailed" - GithubCredentialsReference ConditionType = "GithubCredentialsReference" - FetchingGithubCredentialsRefSuccessReason ConditionReason = "GithubCredentialsRefSuccess" - FetchingGithubCredentialsRefFailedReason ConditionReason = "GithubCredentialsRefFailed" + GithubCredentialsReference ConditionType = "GithubCredentialsReference" // #nosec G101 + FetchingGithubCredentialsRefSuccessReason ConditionReason = "GithubCredentialsRefSuccess" // #nosec G101 + FetchingGithubCredentialsRefFailedReason ConditionReason = "GithubCredentialsRefFailed" // #nosec G101 ) // Credential Conditions @@ -62,9 +62,9 @@ const ( const ( GarmServerNotReconciledYetMsg string = "GARM server not reconciled yet" - CredentialsNotReconciledYetMsg string = "Github Credentials Ref not reconciled yet" - GithubEndpointNotReconciledYetMsg string = "Github Endpoint Ref not reconciled yet" - WebhookSecretNotReconciledYetMsg string = "Webhook Secret Ref not reconciled yet" + CredentialsNotReconciledYetMsg string = "Github Credentials Ref not reconciled yet" // #nosec G101 + GithubEndpointNotReconciledYetMsg string = "Github Endpoint Ref not reconciled yet" // #nosec G101 + WebhookSecretNotReconciledYetMsg string = "Webhook Secret Ref not reconciled yet" // #nosec G101 DeletingEnterpriseMsg string = "Deleting enterprise" DeletingOrgMsg string = "Deleting organization" DeletingRepoMsg string = "Deleting repository" From 800be3c2f671b232fa4ac00339aa277e61de6793 Mon Sep 17 00:00:00 2001 From: rthalho Date: Wed, 8 Oct 2025 10:48:55 +0200 Subject: [PATCH 19/27] fix(runner): remove conditions for now --- api/v1beta1/runner_types.go | 16 -------- internal/controller/runner_controller.go | 20 ---------- internal/controller/runner_controller_test.go | 40 ------------------- pkg/conditions/condition_types.go | 7 ---- 4 files changed, 83 deletions(-) diff --git a/api/v1beta1/runner_types.go b/api/v1beta1/runner_types.go index e7e69599..4f022ebc 100644 --- a/api/v1beta1/runner_types.go +++ b/api/v1beta1/runner_types.go @@ -6,8 +6,6 @@ import ( commonParams "github.com/cloudbase/garm-provider-common/params" "github.com/cloudbase/garm/params" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "github.com/mercedes-benz/garm-operator/pkg/conditions" ) // RunnerSpec defines the desired state of Runner @@ -96,20 +94,6 @@ type Runner struct { Status RunnerStatus `json:"status,omitempty"` } -func (r *Runner) InitializeConditions() { - if conditions.Get(r, conditions.ReadyCondition) == nil { - conditions.MarkUnknown(r, conditions.ReadyCondition, conditions.UnknownReason, conditions.GarmServerNotReconciledYetMsg) - } -} - -func (r *Runner) SetConditions(conditions []metav1.Condition) { - r.Status.Conditions = conditions -} - -func (r *Runner) GetConditions() []metav1.Condition { - return r.Status.Conditions -} - //+kubebuilder:object:root=true // RunnerList contains a list of Runner diff --git a/internal/controller/runner_controller.go b/internal/controller/runner_controller.go index 69bef0eb..7ebd7fe3 100644 --- a/internal/controller/runner_controller.go +++ b/internal/controller/runner_controller.go @@ -8,7 +8,6 @@ import ( "strings" "time" - commonParams "github.com/cloudbase/garm-provider-common/params" "github.com/cloudbase/garm/client/instances" "github.com/cloudbase/garm/params" "github.com/google/go-cmp/cmp" @@ -29,7 +28,6 @@ import ( garmoperatorv1beta1 "github.com/mercedes-benz/garm-operator/api/v1beta1" garmClient "github.com/mercedes-benz/garm-operator/pkg/client" "github.com/mercedes-benz/garm-operator/pkg/client/key" - "github.com/mercedes-benz/garm-operator/pkg/conditions" "github.com/mercedes-benz/garm-operator/pkg/config" "github.com/mercedes-benz/garm-operator/pkg/filter" runnerUtil "github.com/mercedes-benz/garm-operator/pkg/runners" @@ -95,9 +93,6 @@ func (r *RunnerReconciler) reconcileNormal(ctx context.Context, req ctrl.Request orig := runner.DeepCopy() - // Initialize conditions to unknown if not set already - runner.InitializeConditions() - // always update the status defer func() { if !reflect.DeepEqual(runner.Status, orig.Status) { @@ -223,21 +218,6 @@ func (r *RunnerReconciler) updateRunnerStatus(ctx context.Context, runner *garmo runner.Status.ProviderFault = string(garmRunner.ProviderFault) runner.Status.GitHubRunnerGroup = garmRunner.GitHubRunnerGroup - if runner.Status.InstanceStatus == commonParams.InstancePendingCreate || - runner.Status.InstanceStatus == commonParams.InstanceCreating || - runner.Status.Status == params.RunnerInstalling || - runner.Status.Status == params.RunnerPending { - conditions.MarkFalse(runner, conditions.ReadyCondition, conditions.RunnerNotReadyReason, conditions.RunnerNotReadyYetMsg) - } - - if runner.Status.InstanceStatus == commonParams.InstanceError || runner.Status.Status == params.RunnerFailed { - conditions.MarkFalse(runner, conditions.ReadyCondition, conditions.RunnerErrorReason, conditions.RunnerProvisioningFailedMsg) - } - - if runner.Status.InstanceStatus == commonParams.InstanceRunning && runner.Status.Status == params.RunnerIdle { - conditions.MarkTrue(runner, conditions.ReadyCondition, conditions.RunnerReadyReason, conditions.RunnerIdleAndRunningMsg) - } - return nil } diff --git a/internal/controller/runner_controller_test.go b/internal/controller/runner_controller_test.go index 6f252a19..c5860bd3 100644 --- a/internal/controller/runner_controller_test.go +++ b/internal/controller/runner_controller_test.go @@ -27,7 +27,6 @@ import ( garmoperatorv1beta1 "github.com/mercedes-benz/garm-operator/api/v1beta1" "github.com/mercedes-benz/garm-operator/pkg/client/key" "github.com/mercedes-benz/garm-operator/pkg/client/mock" - "github.com/mercedes-benz/garm-operator/pkg/conditions" "github.com/mercedes-benz/garm-operator/pkg/config" ) @@ -123,15 +122,6 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { ProviderID: "kubernetes_external", InstanceStatus: commonParams.InstanceRunning, Status: params.RunnerIdle, - Conditions: []metav1.Condition{ - { - Type: string(conditions.ReadyCondition), - Reason: string(conditions.RunnerReadyReason), - Status: metav1.ConditionTrue, - LastTransitionTime: metav1.NewTime(time.Now()), - Message: conditions.RunnerIdleAndRunningMsg, - }, - }, }, }, }, @@ -186,9 +176,6 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) { // empty resource version to avoid comparison errors runner.ObjectMeta.ResourceVersion = "" - conditions.NilLastTransitionTime(tt.expectedObject) - conditions.NilLastTransitionTime(runner) - if !reflect.DeepEqual(runner, tt.expectedObject) { t.Errorf("RunnerReconciler.reconcile() \ngot = %#v\n want %#v", runner, tt.expectedObject) } @@ -368,15 +355,6 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { ProviderID: "road-runner-k8s-fy5snjcv5dzn", InstanceStatus: commonParams.InstanceRunning, Status: params.RunnerIdle, - Conditions: []metav1.Condition{ - { - Type: string(conditions.ReadyCondition), - Reason: string(conditions.RunnerReadyReason), - Status: metav1.ConditionTrue, - LastTransitionTime: metav1.NewTime(time.Now()), - Message: conditions.RunnerIdleAndRunningMsg, - }, - }, }, }, &garmoperatorv1beta1.Runner{ @@ -402,15 +380,6 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { ProviderID: "road-runner-k8s-n6kq2mt3k4qr", InstanceStatus: commonParams.InstanceRunning, Status: params.RunnerIdle, - Conditions: []metav1.Condition{ - { - Type: string(conditions.ReadyCondition), - Reason: string(conditions.RunnerReadyReason), - Status: metav1.ConditionTrue, - LastTransitionTime: metav1.NewTime(time.Now()), - Message: conditions.RunnerIdleAndRunningMsg, - }, - }, }, }, &garmoperatorv1beta1.Pool{ @@ -503,15 +472,6 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) { ProviderID: "road-runner-k8s-n6kq2mt3k4qr", InstanceStatus: commonParams.InstanceRunning, Status: params.RunnerIdle, - Conditions: []metav1.Condition{ - { - Type: string(conditions.ReadyCondition), - Reason: string(conditions.RunnerReadyReason), - Status: metav1.ConditionTrue, - LastTransitionTime: metav1.NewTime(time.Now()), - Message: conditions.RunnerIdleAndRunningMsg, - }, - }, }, }, }, diff --git a/pkg/conditions/condition_types.go b/pkg/conditions/condition_types.go index 7c1d5ec4..e4319727 100644 --- a/pkg/conditions/condition_types.go +++ b/pkg/conditions/condition_types.go @@ -53,13 +53,6 @@ const ( FetchingGithubEndpointRefFailedReason ConditionReason = "FetchingGithubEndpointRefFailed" ) -// Runner Conditions -const ( - RunnerReadyReason ConditionReason = "RunnerIdleAndRunning" - RunnerNotReadyReason ConditionReason = "RunnerNotReadyYet" - RunnerErrorReason ConditionReason = "RunnerProvisioningError" -) - const ( GarmServerNotReconciledYetMsg string = "GARM server not reconciled yet" CredentialsNotReconciledYetMsg string = "Github Credentials Ref not reconciled yet" // #nosec G101 From df30b7e69a6a4011b114da10dbdb78688dc49357 Mon Sep 17 00:00:00 2001 From: rthalho Date: Wed, 8 Oct 2025 11:00:49 +0200 Subject: [PATCH 20/27] fix(controller): remove depricated ctrl.Result{Requeue: true} and unsused named return vars --- internal/controller/enterprise_controller.go | 2 +- internal/controller/garmserverconfig_controller.go | 2 +- internal/controller/githubcredentials_controller.go | 2 +- internal/controller/githubendpoint_controller.go | 2 +- internal/controller/organization_controller.go | 4 ++-- internal/controller/pool_controller.go | 6 +++--- internal/controller/repository_controller.go | 4 ++-- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go index 91dd901b..616c9861 100644 --- a/internal/controller/enterprise_controller.go +++ b/internal/controller/enterprise_controller.go @@ -47,7 +47,7 @@ type EnterpriseReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=enterprises/finalizers,verbs=update // +kubebuilder:rbac:groups="",namespace=xxxxx,resources=secrets,verbs=get;list;watch; -func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { +func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := log.FromContext(ctx) enterpriseClient := garmClient.NewEnterpriseClient() diff --git a/internal/controller/garmserverconfig_controller.go b/internal/controller/garmserverconfig_controller.go index e4257413..e5928595 100644 --- a/internal/controller/garmserverconfig_controller.go +++ b/internal/controller/garmserverconfig_controller.go @@ -69,7 +69,7 @@ func (r *GarmServerConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req if !reflect.DeepEqual(garmServerConfig.Status, orig.Status) { if err := r.Status().Update(ctx, garmServerConfig); err != nil { log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + res = ctrl.Result{} retErr = err } } diff --git a/internal/controller/githubcredentials_controller.go b/internal/controller/githubcredentials_controller.go index 2e465e02..da25602e 100644 --- a/internal/controller/githubcredentials_controller.go +++ b/internal/controller/githubcredentials_controller.go @@ -83,7 +83,7 @@ func (r *GitHubCredentialReconciler) Reconcile(ctx context.Context, req ctrl.Req if !reflect.DeepEqual(credentials.Status, orig.Status) { if err := r.Status().Update(ctx, credentials); err != nil { log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + res = ctrl.Result{} retErr = err } } diff --git a/internal/controller/githubendpoint_controller.go b/internal/controller/githubendpoint_controller.go index a50d8c1c..66fa5d67 100644 --- a/internal/controller/githubendpoint_controller.go +++ b/internal/controller/githubendpoint_controller.go @@ -82,7 +82,7 @@ func (r *GitHubEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Reque if !reflect.DeepEqual(endpoint.Status, orig.Status) { if err := r.Status().Update(ctx, endpoint); err != nil { log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + res = ctrl.Result{} retErr = err } } diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index e2c406d9..a62c698c 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -46,7 +46,7 @@ type OrganizationReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=organizations/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=organizations/finalizers,verbs=update -func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { +func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := log.FromContext(ctx) organizationClient := garmClient.NewOrganizationClient() @@ -88,7 +88,7 @@ func (r *OrganizationReconciler) reconcile(ctx context.Context, organizationClie if !reflect.DeepEqual(organization.Status, orig.Status) { if err := r.Status().Update(ctx, organization); err != nil { log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + res = ctrl.Result{} retErr = err } } diff --git a/internal/controller/pool_controller.go b/internal/controller/pool_controller.go index 9690fee1..6e01d694 100644 --- a/internal/controller/pool_controller.go +++ b/internal/controller/pool_controller.go @@ -94,7 +94,7 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res c if !reflect.DeepEqual(pool.Status, orig.Status) { if err := r.Status().Update(ctx, pool); err != nil { log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + res = ctrl.Result{} retErr = err } } @@ -290,13 +290,13 @@ func (r *PoolReconciler) reconcileDelete(ctx context.Context, garmClient garmCli // get all runners runners, err := runnerUtil.GetRunnersByPoolID(ctx, pool, instanceClient) if err != nil { - return ctrl.Result{Requeue: true, RequeueAfter: 1 * time.Minute}, err + return ctrl.Result{RequeueAfter: 1 * time.Minute}, err } // get a list of all idle runners to trigger deletion deletableRunners := runnerUtil.DeletableRunners(ctx, runners) if err != nil { - return ctrl.Result{Requeue: true, RequeueAfter: 1 * time.Minute}, err + return ctrl.Result{RequeueAfter: 1 * time.Minute}, err } // set current idle runners count in status diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go index 025963e0..9a34e73f 100644 --- a/internal/controller/repository_controller.go +++ b/internal/controller/repository_controller.go @@ -46,7 +46,7 @@ type RepositoryReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=repositories/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=repositories/finalizers,verbs=update -func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { +func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := log.FromContext(ctx) repositoryClient := garmClient.NewRepositoryClient() @@ -88,7 +88,7 @@ func (r *RepositoryReconciler) reconcile(ctx context.Context, repositoryClient g if !reflect.DeepEqual(repository.Status, orig.Status) { if err := r.Status().Update(ctx, repository); err != nil { log.Error(err, "failed to update status") - res = ctrl.Result{Requeue: true} + res = ctrl.Result{} retErr = err } } From 68fbe6367deb6b82a77321126d06e4f967de7a59 Mon Sep 17 00:00:00 2001 From: rthalho Date: Wed, 8 Oct 2025 11:11:12 +0200 Subject: [PATCH 21/27] fix(controller): rename initial CR var --- internal/controller/enterprise_controller.go | 4 ++-- internal/controller/garmserverconfig_controller.go | 4 ++-- internal/controller/githubcredentials_controller.go | 4 ++-- internal/controller/githubendpoint_controller.go | 4 ++-- internal/controller/organization_controller.go | 4 ++-- internal/controller/pool_controller.go | 4 ++-- internal/controller/repository_controller.go | 4 ++-- internal/controller/runner_controller.go | 6 +++--- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go index 616c9861..e3379b6f 100644 --- a/internal/controller/enterprise_controller.go +++ b/internal/controller/enterprise_controller.go @@ -68,7 +68,7 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) func (r *EnterpriseReconciler) reconcile(ctx context.Context, enterpriseClient garmClient.EnterpriseClient, enterprise *garmoperatorv1beta1.Enterprise) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) - orig := enterprise.DeepCopy() + initialEnterprise := enterprise.DeepCopy() // Ignore objects that are paused if annotations.IsPaused(enterprise) { @@ -86,7 +86,7 @@ func (r *EnterpriseReconciler) reconcile(ctx context.Context, enterpriseClient g // always update the status defer func() { - if !reflect.DeepEqual(enterprise.Status, orig.Status) { + if !reflect.DeepEqual(enterprise.Status, initialEnterprise.Status) { if err := r.Status().Update(ctx, enterprise); err != nil { log.Error(err, "failed to update status") res = ctrl.Result{} diff --git a/internal/controller/garmserverconfig_controller.go b/internal/controller/garmserverconfig_controller.go index e5928595..bdd18353 100644 --- a/internal/controller/garmserverconfig_controller.go +++ b/internal/controller/garmserverconfig_controller.go @@ -54,7 +54,7 @@ func (r *GarmServerConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } - orig := garmServerConfig.DeepCopy() + initialGarmServerConfig := garmServerConfig.DeepCopy() // Ignore objects that are paused if annotations.IsPaused(garmServerConfig) { @@ -66,7 +66,7 @@ func (r *GarmServerConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req // always update the status defer func() { - if !reflect.DeepEqual(garmServerConfig.Status, orig.Status) { + if !reflect.DeepEqual(garmServerConfig.Status, initialGarmServerConfig.Status) { if err := r.Status().Update(ctx, garmServerConfig); err != nil { log.Error(err, "failed to update status") res = ctrl.Result{} diff --git a/internal/controller/githubcredentials_controller.go b/internal/controller/githubcredentials_controller.go index da25602e..75a7d32f 100644 --- a/internal/controller/githubcredentials_controller.go +++ b/internal/controller/githubcredentials_controller.go @@ -60,7 +60,7 @@ func (r *GitHubCredentialReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } - orig := credentials.DeepCopy() + initialCredentials := credentials.DeepCopy() // Ignore objects that are paused if annotations.IsPaused(credentials) { @@ -80,7 +80,7 @@ func (r *GitHubCredentialReconciler) Reconcile(ctx context.Context, req ctrl.Req // always update the status defer func() { - if !reflect.DeepEqual(credentials.Status, orig.Status) { + if !reflect.DeepEqual(credentials.Status, initialCredentials.Status) { if err := r.Status().Update(ctx, credentials); err != nil { log.Error(err, "failed to update status") res = ctrl.Result{} diff --git a/internal/controller/githubendpoint_controller.go b/internal/controller/githubendpoint_controller.go index 66fa5d67..fcd83f2a 100644 --- a/internal/controller/githubendpoint_controller.go +++ b/internal/controller/githubendpoint_controller.go @@ -59,7 +59,7 @@ func (r *GitHubEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - orig := endpoint.DeepCopy() + initialEndpoint := endpoint.DeepCopy() // Ignore objects that are paused if annotations.IsPaused(endpoint) { @@ -79,7 +79,7 @@ func (r *GitHubEndpointReconciler) Reconcile(ctx context.Context, req ctrl.Reque // always update the status defer func() { - if !reflect.DeepEqual(endpoint.Status, orig.Status) { + if !reflect.DeepEqual(endpoint.Status, initialEndpoint.Status) { if err := r.Status().Update(ctx, endpoint); err != nil { log.Error(err, "failed to update status") res = ctrl.Result{} diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index a62c698c..d4cee044 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -67,7 +67,7 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request func (r *OrganizationReconciler) reconcile(ctx context.Context, organizationClient garmClient.OrganizationClient, organization *garmoperatorv1beta1.Organization) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) - orig := organization.DeepCopy() + initialOrganization := organization.DeepCopy() // Ignore objects that are paused if annotations.IsPaused(organization) { @@ -85,7 +85,7 @@ func (r *OrganizationReconciler) reconcile(ctx context.Context, organizationClie // always update the status defer func() { - if !reflect.DeepEqual(organization.Status, orig.Status) { + if !reflect.DeepEqual(organization.Status, initialOrganization.Status) { if err := r.Status().Update(ctx, organization); err != nil { log.Error(err, "failed to update status") res = ctrl.Result{} diff --git a/internal/controller/pool_controller.go b/internal/controller/pool_controller.go index 6e01d694..1b73100a 100644 --- a/internal/controller/pool_controller.go +++ b/internal/controller/pool_controller.go @@ -69,7 +69,7 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res c return ctrl.Result{}, err } - orig := pool.DeepCopy() + initialPool := pool.DeepCopy() // Ignore objects that are paused if annotations.IsPaused(pool) { @@ -91,7 +91,7 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res c // always update the status defer func() { - if !reflect.DeepEqual(pool.Status, orig.Status) { + if !reflect.DeepEqual(pool.Status, initialPool.Status) { if err := r.Status().Update(ctx, pool); err != nil { log.Error(err, "failed to update status") res = ctrl.Result{} diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go index 9a34e73f..2cd12565 100644 --- a/internal/controller/repository_controller.go +++ b/internal/controller/repository_controller.go @@ -67,7 +67,7 @@ func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) func (r *RepositoryReconciler) reconcile(ctx context.Context, repositoryClient garmClient.RepositoryClient, repository *garmoperatorv1beta1.Repository) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) - orig := repository.DeepCopy() + initialRepository := repository.DeepCopy() // Ignore objects that are paused if annotations.IsPaused(repository) { @@ -85,7 +85,7 @@ func (r *RepositoryReconciler) reconcile(ctx context.Context, repositoryClient g // always update the status defer func() { - if !reflect.DeepEqual(repository.Status, orig.Status) { + if !reflect.DeepEqual(repository.Status, initialRepository.Status) { if err := r.Status().Update(ctx, repository); err != nil { log.Error(err, "failed to update status") res = ctrl.Result{} diff --git a/internal/controller/runner_controller.go b/internal/controller/runner_controller.go index 7ebd7fe3..8c543383 100644 --- a/internal/controller/runner_controller.go +++ b/internal/controller/runner_controller.go @@ -91,13 +91,13 @@ func (r *RunnerReconciler) reconcileNormal(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } - orig := runner.DeepCopy() + initialRunner := runner.DeepCopy() // always update the status defer func() { - if !reflect.DeepEqual(runner.Status, orig.Status) { + if !reflect.DeepEqual(runner.Status, initialRunner.Status) { log.Info("Update runner status...") - diff := cmp.Diff(orig.Status, runner.Status) + diff := cmp.Diff(initialRunner.Status, runner.Status) log.V(1).Info("Runner status changed", "diff", diff) if err := r.Status().Update(ctx, runner); err != nil { log.Error(err, "failed to update status", "runner", runner.Name) From 771f533a35e708882f248f94b0db587c5253c04e Mon Sep 17 00:00:00 2001 From: rthalho Date: Wed, 8 Oct 2025 13:52:56 +0200 Subject: [PATCH 22/27] fix(conditions): cleanup --- pkg/conditions/condition_types.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/conditions/condition_types.go b/pkg/conditions/condition_types.go index e4319727..12b17b38 100644 --- a/pkg/conditions/condition_types.go +++ b/pkg/conditions/condition_types.go @@ -55,17 +55,13 @@ const ( const ( GarmServerNotReconciledYetMsg string = "GARM server not reconciled yet" - CredentialsNotReconciledYetMsg string = "Github Credentials Ref not reconciled yet" // #nosec G101 - GithubEndpointNotReconciledYetMsg string = "Github Endpoint Ref not reconciled yet" // #nosec G101 - WebhookSecretNotReconciledYetMsg string = "Webhook Secret Ref not reconciled yet" // #nosec G101 + CredentialsNotReconciledYetMsg string = "GithubCredentialsRef not reconciled yet" // #nosec G101 + GithubEndpointNotReconciledYetMsg string = "GithubEndpointRef not reconciled yet" // #nosec G101 + WebhookSecretNotReconciledYetMsg string = "WebhookSecretRef not reconciled yet" // #nosec G101 DeletingEnterpriseMsg string = "Deleting enterprise" DeletingOrgMsg string = "Deleting organization" DeletingRepoMsg string = "Deleting repository" DeletingPoolMsg string = "Deleting pool" DeletingEndpointMsg string = "Deleting endpoint" - DeletingCredentialsMsg string = "Deleting credentials" // #nosec G101 - DeletingRunnerMsg string = "Deleting runner" // #nosec G101 - RunnerIdleAndRunningMsg string = "Runner is idle and running" // #nosec G101 - RunnerProvisioningFailedMsg string = "Provisioning runner failed" // #nosec G101 - RunnerNotReadyYetMsg string = "Runner is still being provisioned" // #nosec G101 + DeletingCredentialsMsg string = "Deleting credentials" // #nosec G101 ) From e284bd7a88538e9467c9c43144666a996c75c1ae Mon Sep 17 00:00:00 2001 From: rthalho Date: Mon, 27 Oct 2025 10:08:23 +0100 Subject: [PATCH 23/27] fix(enterprise): simplify Reconcile method --- internal/controller/enterprise_controller.go | 8 +------- internal/controller/enterprise_controller_test.go | 4 +++- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go index e3379b6f..7ee57476 100644 --- a/internal/controller/enterprise_controller.go +++ b/internal/controller/enterprise_controller.go @@ -47,7 +47,7 @@ type EnterpriseReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=enterprises/finalizers,verbs=update // +kubebuilder:rbac:groups="",namespace=xxxxx,resources=secrets,verbs=get;list;watch; -func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) enterpriseClient := garmClient.NewEnterpriseClient() @@ -62,12 +62,6 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - return r.reconcile(ctx, enterpriseClient, enterprise) -} - -func (r *EnterpriseReconciler) reconcile(ctx context.Context, enterpriseClient garmClient.EnterpriseClient, enterprise *garmoperatorv1beta1.Enterprise) (res ctrl.Result, retErr error) { - log := log.FromContext(ctx) - initialEnterprise := enterprise.DeepCopy() // Ignore objects that are paused diff --git a/internal/controller/enterprise_controller_test.go b/internal/controller/enterprise_controller_test.go index 89d7f638..d48c610c 100644 --- a/internal/controller/enterprise_controller_test.go +++ b/internal/controller/enterprise_controller_test.go @@ -928,7 +928,9 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) { mockEnterpriseClient := mock.NewMockEnterpriseClient(mockCtrl) tt.expectGarmRequest(mockEnterpriseClient.EXPECT()) - _, err = reconciler.reconcile(context.Background(), mockEnterpriseClient, enterprise) + enterprise.InitializeConditions() + + _, err = reconciler.reconcileNormal(context.Background(), mockEnterpriseClient, enterprise) if (err != nil) != tt.wantErr { t.Errorf("EnterpriseReconciler.reconcileNormal() error = %v, wantErr %v", err, tt.wantErr) return From 76a919944319edce8cde1755490ff9ea8c6f9436 Mon Sep 17 00:00:00 2001 From: rthalho Date: Mon, 27 Oct 2025 10:11:51 +0100 Subject: [PATCH 24/27] fix(organization): simplify Reconcile method --- internal/controller/organization_controller.go | 8 +------- internal/controller/organization_controller_test.go | 4 +++- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index d4cee044..2cbec823 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -46,7 +46,7 @@ type OrganizationReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=organizations/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=organizations/finalizers,verbs=update -func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) organizationClient := garmClient.NewOrganizationClient() @@ -61,12 +61,6 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } - return r.reconcile(ctx, organizationClient, organization) -} - -func (r *OrganizationReconciler) reconcile(ctx context.Context, organizationClient garmClient.OrganizationClient, organization *garmoperatorv1beta1.Organization) (res ctrl.Result, retErr error) { - log := log.FromContext(ctx) - initialOrganization := organization.DeepCopy() // Ignore objects that are paused diff --git a/internal/controller/organization_controller_test.go b/internal/controller/organization_controller_test.go index d0c4a828..9f7acf5a 100644 --- a/internal/controller/organization_controller_test.go +++ b/internal/controller/organization_controller_test.go @@ -928,7 +928,9 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) { mockOrganizationClient := mock.NewMockOrganizationClient(mockCtrl) tt.expectGarmRequest(mockOrganizationClient.EXPECT()) - _, err = reconciler.reconcile(context.Background(), mockOrganizationClient, organization) + organization.InitializeConditions() + + _, err = reconciler.reconcileNormal(context.Background(), mockOrganizationClient, organization) if (err != nil) != tt.wantErr { t.Errorf("OrganizationReconciler.reconcileNormal() error = %v, wantErr %v", err, tt.wantErr) return From 9099287d81c59a48dbb4b04202ea81a1ca77ad61 Mon Sep 17 00:00:00 2001 From: rthalho Date: Mon, 27 Oct 2025 10:15:59 +0100 Subject: [PATCH 25/27] fix(repository): simplify Reconcile method --- hack/install-prometheus.sh | 0 internal/controller/repository_controller.go | 8 +------- internal/controller/repository_controller_test.go | 4 +++- 3 files changed, 4 insertions(+), 8 deletions(-) delete mode 100644 hack/install-prometheus.sh diff --git a/hack/install-prometheus.sh b/hack/install-prometheus.sh deleted file mode 100644 index e69de29b..00000000 diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go index 2cd12565..168258f1 100644 --- a/internal/controller/repository_controller.go +++ b/internal/controller/repository_controller.go @@ -46,7 +46,7 @@ type RepositoryReconciler struct { //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=repositories/status,verbs=get;update;patch //+kubebuilder:rbac:groups=garm-operator.mercedes-benz.com,namespace=xxxxx,resources=repositories/finalizers,verbs=update -func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, retErr error) { log := log.FromContext(ctx) repositoryClient := garmClient.NewRepositoryClient() @@ -61,12 +61,6 @@ func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - return r.reconcile(ctx, repositoryClient, repository) -} - -func (r *RepositoryReconciler) reconcile(ctx context.Context, repositoryClient garmClient.RepositoryClient, repository *garmoperatorv1beta1.Repository) (res ctrl.Result, retErr error) { - log := log.FromContext(ctx) - initialRepository := repository.DeepCopy() // Ignore objects that are paused diff --git a/internal/controller/repository_controller_test.go b/internal/controller/repository_controller_test.go index 2ed781c6..7fc05cdd 100644 --- a/internal/controller/repository_controller_test.go +++ b/internal/controller/repository_controller_test.go @@ -955,7 +955,9 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) { mockRepositoryClient := mock.NewMockRepositoryClient(mockCtrl) tt.expectGarmRequest(mockRepositoryClient.EXPECT()) - _, err = reconciler.reconcile(context.Background(), mockRepositoryClient, repository) + repository.InitializeConditions() + + _, err = reconciler.reconcileNormal(context.Background(), mockRepositoryClient, repository) if (err != nil) != tt.wantErr { t.Errorf("RepositoryReconciler.reconcileNormal() error = %v, wantErr %v", err, tt.wantErr) return From 1fc532c5460ecb89b85281c0804802fbba7786d5 Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 4 Nov 2025 14:14:42 +0100 Subject: [PATCH 26/27] chore: bump go to 1.24.9 --- go.mod | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index ebfe57ec..d725ebaa 100644 --- a/go.mod +++ b/go.mod @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT module github.com/mercedes-benz/garm-operator -go 1.24.4 +go 1.24.9 require ( github.com/cloudbase/garm v0.1.5 @@ -9,6 +9,7 @@ require ( github.com/go-openapi/runtime v0.28.0 github.com/go-playground/validator/v10 v10.27.0 github.com/golang-jwt/jwt/v4 v4.5.2 + github.com/google/go-cmp v0.7.0 github.com/google/uuid v1.6.0 github.com/iancoleman/strcase v0.3.0 github.com/knadh/koanf/parsers/yaml v1.1.0 @@ -62,7 +63,6 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/google/btree v1.1.3 // indirect github.com/google/gnostic-models v0.6.9 // indirect - github.com/google/go-cmp v0.7.0 // indirect github.com/google/go-github/v57 v57.0.0 // indirect github.com/google/go-github/v60 v60.0.0 // indirect github.com/google/go-querystring v1.1.0 // indirect From 271ba3e3e9ec9c1b47e0f46d3b726b0d67db6e71 Mon Sep 17 00:00:00 2001 From: rthalho Date: Tue, 4 Nov 2025 14:20:11 +0100 Subject: [PATCH 27/27] chore: remove nancy scan --- Dockerfile | 2 +- Makefile | 13 +------------ 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/Dockerfile b/Dockerfile index 8fe98e5e..dd47f87a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: MIT # Build the manager binary -FROM golang:1.24.4 as builder +FROM golang:1.24.9 as builder ARG TARGETOS ARG TARGETARCH diff --git a/Makefile b/Makefile index ab4758d9..fff9cff3 100644 --- a/Makefile +++ b/Makefile @@ -173,7 +173,6 @@ MOCKGEN ?= $(LOCALBIN)/mockgen GORELEASER ?= $(LOCALBIN)/goreleaser MDTOC ?= $(LOCALBIN)/mdtoc SLICE ?= $(LOCALBIN)/kubectl-slice -NANCY ?= $(LOCALBIN)/nancy GOVULNCHECK ?= $(LOCALBIN)/govulncheck KBOM ?= $(LOCALBIN)/bom KIND ?= $(LOCALBIN)/kind @@ -247,12 +246,6 @@ $(SLICE): $(LOCALBIN) test -s $(LOCALBIN)/kubectl-slice && $(LOCALBIN)/kubectl-slice --version | grep -q $(SLICE_VERSION) || \ GOBIN=$(LOCALBIN) go install github.com/patrickdappollonio/kubectl-slice@$(SLICE_VERSION) -.PHONY: nancy -nancy: $(NANCY) ## Download nancy locally if necessary. If wrong version is installed, it will be overwritten. -$(NANCY): $(LOCALBIN) - test -s $(LOCALBIN)/nancy && $(LOCALBIN)/nancy --version | grep -q $(NANCY_VERSION) || \ - GOBIN=$(LOCALBIN) go install github.com/sonatype-nexus-community/nancy@$(NANCY_VERSION) - .PHONY: govulncheck govulncheck: $(GOVULNCHECK) ## Download govulncheck locally if necessary. If wrong version is installed, it will be overwritten. $(GOVULNCHECK): $(LOCALBIN) @@ -311,17 +304,13 @@ verify-doctoc: generate-doctoc fi .PHONY: verify-security -verify-security: govulncheck-scan nancy-scan ## Verify security by running govulncheck and nancy +verify-security: govulncheck-scan ## Verify security by running govulncheck @echo "Security checks passed" .PHONY: govulncheck-scan govulncheck-scan: govulncheck ## Perform govulncheck scan $(GOVULNCHECK) ./... -.PHONY: nancy-scan -nancy-scan: nancy ## Perform nancy scan - go list -json -deps ./... | $(NANCY) sleuth - ##@ Documentation .PHONY: generate-doctoc generate-doctoc: mdtoc ## Generate documentation table of contents