diff --git a/apis/actions.github.com/v1alpha1/version.go b/apis/actions.github.com/v1alpha1/version.go index 731c6011..4a3dc332 100644 --- a/apis/actions.github.com/v1alpha1/version.go +++ b/apis/actions.github.com/v1alpha1/version.go @@ -3,7 +3,7 @@ package v1alpha1 import "strings" func IsVersionAllowed(resourceVersion, buildVersion string) bool { - if buildVersion == "dev" || resourceVersion == buildVersion || strings.HasPrefix(buildVersion, "canary-") { + if resourceVersion == buildVersion || buildVersion == "dev" || strings.HasPrefix(buildVersion, "canary-") { return true } diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 266d9d60..471d237a 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -78,7 +78,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err := r.Get(ctx, req.NamespacedName, &autoscalingListener); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingListener.DeepCopy() + var original once[*v1alpha1.AutoscalingListener] if !autoscalingListener.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { @@ -96,9 +96,14 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{Requeue: true, RequeueAfter: time.Second}, nil } - log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + removeFinalizer := controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) + if removeFinalizer { + original.Do(autoscalingListener.DeepCopy) + controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) + } + if removeFinalizer { + log.Info("Removing finalizer") + if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original.Get())); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to remove finalizer") return ctrl.Result{}, err } @@ -109,8 +114,13 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil { + addFinalizer := !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) + if addFinalizer { + original.Do(autoscalingListener.DeepCopy) + controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) + } + if addFinalizer { + if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to add finalizer") return ctrl.Result{}, err } @@ -168,17 +178,19 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. labelsModified := !maps.Equal(serviceAccount.Labels, desiredLabels) desiredAnnotations := r.mergeAnnotations(serviceAccount.Annotations, desiredServiceAccount.Annotations) annotationsModified := !maps.Equal(serviceAccount.Annotations, desiredAnnotations) + var original once[*corev1.ServiceAccount] + if labelsModified { + original.Do(serviceAccount.DeepCopy) + serviceAccount.Labels = desiredLabels + } + if annotationsModified { + original.Do(serviceAccount.DeepCopy) + serviceAccount.Annotations = desiredAnnotations + } if labelsModified || annotationsModified { - updatedServiceAccount := serviceAccount.DeepCopy() - if labelsModified { - updatedServiceAccount.Labels = desiredLabels - } - if annotationsModified { - updatedServiceAccount.Annotations = desiredAnnotations - } log.Info("Updating listener service account") - if err := r.Patch(ctx, updatedServiceAccount, client.MergeFrom(&serviceAccount)); err != nil { + if err := r.Patch(ctx, &serviceAccount, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener service account") return ctrl.Result{}, err } @@ -212,19 +224,22 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. desiredAnnotations := r.mergeAnnotations(listenerRole.Annotations, desiredRole.Annotations) annotationsModified := !maps.Equal(listenerRole.Annotations, desiredAnnotations) rulesModified := !reflect.DeepEqual(listenerRole.Rules, desiredRole.Rules) + var original once[*rbacv1.Role] + if labelsModified { + original.Do(listenerRole.DeepCopy) + listenerRole.Labels = desiredLabels + } + if annotationsModified { + original.Do(listenerRole.DeepCopy) + listenerRole.Annotations = desiredAnnotations + } + if rulesModified { + original.Do(listenerRole.DeepCopy) + listenerRole.Rules = desiredRole.Rules + } if labelsModified || annotationsModified || rulesModified { - updatedRole := listenerRole.DeepCopy() - if labelsModified { - updatedRole.Labels = desiredLabels - } - if annotationsModified { - updatedRole.Annotations = desiredAnnotations - } - if rulesModified { - updatedRole.Rules = desiredRole.Rules - } log.Info("Updating listener role") - if err := r.Patch(ctx, updatedRole, client.MergeFrom(&listenerRole)); err != nil { + if err := r.Patch(ctx, &listenerRole, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener role") return ctrl.Result{}, err } @@ -253,16 +268,18 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. labelsModified := !maps.Equal(listenerRoleBinding.Labels, desiredLabels) desiredAnnotations := r.mergeAnnotations(listenerRoleBinding.Annotations, desiredRoleBinding.Annotations) annotationsModified := !maps.Equal(listenerRoleBinding.Annotations, desiredAnnotations) + var original once[*rbacv1.RoleBinding] + if labelsModified { + original.Do(listenerRoleBinding.DeepCopy) + listenerRoleBinding.Labels = desiredLabels + } + if annotationsModified { + original.Do(listenerRoleBinding.DeepCopy) + listenerRoleBinding.Annotations = desiredAnnotations + } if labelsModified || annotationsModified { - updatedRoleBinding := listenerRoleBinding.DeepCopy() - if labelsModified { - updatedRoleBinding.Labels = desiredLabels - } - if annotationsModified { - updatedRoleBinding.Annotations = desiredAnnotations - } log.Info("Updating listener role binding") - if err := r.Patch(ctx, updatedRoleBinding, client.MergeFrom(&listenerRoleBinding)); err != nil { + if err := r.Patch(ctx, &listenerRoleBinding, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener role binding") return ctrl.Result{}, err } @@ -308,16 +325,18 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. labelsModified := !maps.Equal(proxySecret.Labels, desiredLabels) desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredListenerProxy.Annotations) annotationsModified := !maps.Equal(proxySecret.Annotations, desiredAnnotations) + var original once[*corev1.Secret] + if labelsModified { + original.Do(proxySecret.DeepCopy) + proxySecret.Labels = desiredLabels + } + if annotationsModified { + original.Do(proxySecret.DeepCopy) + proxySecret.Annotations = desiredAnnotations + } if labelsModified || annotationsModified { - updatedProxySecret := proxySecret.DeepCopy() - if labelsModified { - updatedProxySecret.Labels = desiredLabels - } - if annotationsModified { - updatedProxySecret.Annotations = desiredAnnotations - } log.Info("Updating listener proxy secret") - if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(&proxySecret)); err != nil { + if err := r.Patch(ctx, &proxySecret, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener proxy secret") return ctrl.Result{}, err } @@ -394,17 +413,19 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. labelsModified := !maps.Equal(listenerConfigSecret.Labels, desiredLabels) desiredAnnotations := r.mergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations) annotationsModified := !maps.Equal(listenerConfigSecret.Annotations, desiredAnnotations) + var original once[*corev1.Secret] + if labelsModified { + original.Do(listenerConfigSecret.DeepCopy) + listenerConfigSecret.Labels = desiredLabels + } + if annotationsModified { + original.Do(listenerConfigSecret.DeepCopy) + listenerConfigSecret.Annotations = desiredAnnotations + } if labelsModified || annotationsModified { - updatedSecret := listenerConfigSecret.DeepCopy() - if labelsModified { - updatedSecret.Labels = desiredLabels - } - if annotationsModified { - updatedSecret.Annotations = desiredAnnotations - } - log.Info("Updating listener config secret", "namespace", updatedSecret.Namespace, "name", updatedSecret.Name) - if err := r.Patch(ctx, updatedSecret, client.MergeFrom(&listenerConfigSecret)); err != nil { + log.Info("Updating listener config secret", "namespace", listenerConfigSecret.Namespace, "name", listenerConfigSecret.Name) + if err := r.Patch(ctx, &listenerConfigSecret, client.MergeFrom(original.Get())); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update listener config secret: %w", err) } return ctrl.Result{Requeue: true}, nil @@ -463,6 +484,20 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } + desiredLabels := r.filterAndMergeLabels(listenerPod.Labels, desiredPod.Labels) + labelsModified := !maps.Equal(listenerPod.Labels, desiredLabels) + desiredAnnotations := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) + annotationsModified := !maps.Equal(listenerPod.Annotations, desiredAnnotations) + var original once[*corev1.Pod] + if labelsModified { + original.Do(listenerPod.DeepCopy) + listenerPod.Labels = desiredLabels + } + if annotationsModified { + original.Do(listenerPod.DeepCopy) + listenerPod.Annotations = desiredAnnotations + } + shouldReCreate := listenerPodSpecRequiresRecreation(&listenerPod, desiredPod) if shouldReCreate { log.Info("Listener pod dependency changed, recreating listener pod") @@ -474,22 +509,10 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } - desiredLabels := r.filterAndMergeLabels(listenerPod.Labels, desiredPod.Labels) - labelsModified := !maps.Equal(listenerPod.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) - annotationsModified := !maps.Equal(listenerPod.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedPod := listenerPod.DeepCopy() - if labelsModified { - updatedPod.Labels = desiredLabels - } - if annotationsModified { - updatedPod.Annotations = desiredAnnotations - } - log.Info("Updating listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) - if err := r.Patch(ctx, updatedPod, client.MergeFrom(&listenerPod)); err != nil { - log.Error(err, "Unable to update listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) + log.Info("Updating listener pod", "namespace", listenerPod.Namespace, "name", listenerPod.Name) + if err := r.Patch(ctx, &listenerPod, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Unable to update listener pod", "namespace", listenerPod.Namespace, "name", listenerPod.Name) return ctrl.Result{}, err } return ctrl.Result{}, nil diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index a632c3f0..5b98e2c5 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -74,7 +74,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if err := r.Get(ctx, req.NamespacedName, &autoscalingRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingRunnerSet.DeepCopy() + var original once[*v1alpha1.AutoscalingRunnerSet] if !autoscalingRunnerSet.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { @@ -90,7 +90,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if !done { log.Info("Waiting for resources to be cleaned up before removing finalizer") return ctrl.Result{ - RequeueAfter: 5 * time.Second, + RequeueAfter: 2 * time.Second, }, nil } @@ -99,9 +99,14 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - if controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + removeFinalizer := controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + if removeFinalizer { + original.Do(autoscalingRunnerSet.DeepCopy) + controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + } + if removeFinalizer { log.Info("Removing finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original.Get())); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to update autoscaling runner set without finalizer") return ctrl.Result{}, err } @@ -130,10 +135,15 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + addFinalizer := !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + if addFinalizer { + original.Do(autoscalingRunnerSet.DeepCopy) + controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + } + if addFinalizer { log.Info("Adding finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update autoscaling runner set with finalizer") return ctrl.Result{}, err } @@ -195,12 +205,22 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Spec.Replicas = 0 - ephemeralRunnerSet.Spec.PatchID = 0 - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") - return ctrl.Result{}, err + var original once[*v1alpha1.EphemeralRunnerSet] + ephemeralRunnerReplicasModified := ephemeralRunnerSet.Spec.Replicas != 0 + if ephemeralRunnerReplicasModified { + original.Do(ephemeralRunnerSet.DeepCopy) + ephemeralRunnerSet.Spec.Replicas = 0 + } + ephemeralRunnerPatchIDModified := ephemeralRunnerSet.Spec.PatchID != 0 + if ephemeralRunnerPatchIDModified { + original.Do(ephemeralRunnerSet.DeepCopy) + ephemeralRunnerSet.Spec.PatchID = 0 + } + if ephemeralRunnerReplicasModified || ephemeralRunnerPatchIDModified { + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") + return ctrl.Result{}, err + } } return ctrl.Result{}, nil @@ -282,40 +302,44 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) { - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata + var original once[*v1alpha1.EphemeralRunnerSet] + + ephemeralRunnerActionableSpecModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerSpec, desired.Spec.EphemeralRunnerSpec) + if ephemeralRunnerActionableSpecModified { + original.Do(ephemeralRunnerSet.DeepCopy) ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec ephemeralRunnerSet.Spec.ActionableRevision = nextActionableRevision(&ephemeralRunnerSet) - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerSet.Annotations = desired.Annotations - - log.Info("Updating ephemeral runner set spec to match the desired spec") - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to patch ephemeral runner set to match the desired spec") - return ctrl.Result{}, err - } - - log.Info("Successfully patched ephemeral runner set spec") - return ctrl.Result{}, nil } ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata) - ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) - - if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerSet.Annotations = desired.Annotations + if ephemeralRunnerMetadataModified { + original.Do(ephemeralRunnerSet.DeepCopy) ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata - log.Info("Updating ephemeral runner set metadata to match desired labels and annotations") + } + ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) + if ephemeralRunnerLabelsModified { + original.Do(ephemeralRunnerSet.DeepCopy) + ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) + } + ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) + if ephemeralRunnerAnnotationsModified { + original.Do(ephemeralRunnerSet.DeepCopy) + ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) + } + + if ephemeralRunnerActionableSpecModified || ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { + original := original.Get() + if ephemeralRunnerActionableSpecModified { + log.Info("Updating ephemeral runner set spec to match the desired spec") + } else { + log.Info("Updating ephemeral runner set metadata to match desired labels and annotations") + } if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to patch ephemeral runner set metadata to match desired labels and annotations") + log.Error(err, "Failed to patch ephemeral runner set to match the desired state") return ctrl.Result{}, err } - log.Info("Successfully patched ephemeral runner set metadata") + log.Info("Successfully patched ephemeral runner set") return ctrl.Result{}, nil } } @@ -512,10 +536,11 @@ func (r *AutoscalingRunnerSetReconciler) removeFinalizersFromDependentResources( } func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (ctrl.Result, error) { - original := autoscalingRunnerSet.DeepCopy() + var original once[*v1alpha1.AutoscalingRunnerSet] logger.Info("Creating a new runner scale set") actionsClient, err := r.GetActionsService(ctx, autoscalingRunnerSet) if len(autoscalingRunnerSet.Spec.RunnerScaleSetName) == 0 { + original.Do(autoscalingRunnerSet.DeepCopy) autoscalingRunnerSet.Spec.RunnerScaleSetName = autoscalingRunnerSet.Name } if err != nil { @@ -590,6 +615,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex actionsClient.SetSystemInfo(info) logger.Info("Created/Reused a runner scale set", "id", runnerScaleSet.ID, "runnerGroupName", runnerScaleSet.RunnerGroupName) + original.Do(autoscalingRunnerSet.DeepCopy) if autoscalingRunnerSet.Annotations == nil { autoscalingRunnerSet.Annotations = map[string]string{} } @@ -606,7 +632,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex } logger.Info("Adding runner scale set ID, name and runner group name as an annotation and url labels") - if err = r.Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + if err = r.Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original.Get())); err != nil { logger.Error(err, "Failed to add runner scale set ID, name and runner group name as an annotation") return ctrl.Result{}, err } @@ -1093,6 +1119,11 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleBindin err := c.client.Get(ctx, types.NamespacedName{Name: managerRoleBindingName, Namespace: c.autoscalingRunnerSet.Namespace}, roleBinding) switch { case err == nil: + if !controllerutil.ContainsFinalizer(roleBinding, AutoscalingRunnerSetCleanupFinalizerName) { + c.logger.Info("Manager role binding finalizer has already been removed", "name", managerRoleBindingName) + return + } + original := roleBinding.DeepCopy() if controllerutil.RemoveFinalizer(roleBinding, AutoscalingRunnerSetCleanupFinalizerName) { if err = c.client.Patch(ctx, roleBinding, client.MergeFrom(original)); err != nil { @@ -1133,6 +1164,11 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleFinali err := c.client.Get(ctx, types.NamespacedName{Name: managerRoleName, Namespace: c.autoscalingRunnerSet.Namespace}, role) switch { case err == nil: + if !controllerutil.ContainsFinalizer(role, AutoscalingRunnerSetCleanupFinalizerName) { + c.logger.Info("Manager role finalizer has already been removed", "name", managerRoleName) + return + } + original := role.DeepCopy() if controllerutil.RemoveFinalizer(role, AutoscalingRunnerSetCleanupFinalizerName) { if err := c.client.Patch(ctx, role, client.MergeFrom(original)); err != nil { diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 256ed311..07dfe7de 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -94,7 +94,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.Get(ctx, req.NamespacedName, &ephemeralRunner); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunner.DeepCopy() + var original once[*v1alpha1.EphemeralRunner] if !ephemeralRunner.DeletionTimestamp.IsZero() { r.publishEphemeralRunnerPhaseMetric(&ephemeralRunner, "", log) @@ -103,7 +103,8 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - if controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { + removeActionsFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + if removeActionsFinalizer { log.Info("Trying to clean up runner from the service") ok, err := r.cleanupRunnerFromService(ctx, &ephemeralRunner, log) if err != nil { @@ -116,14 +117,16 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Runner is cleaned up from the service, removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { - log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update ephemeral runner after removing finalizer") - return ctrl.Result{}, err - } - } + original.Do(ephemeralRunner.DeepCopy) + controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + } + if removeActionsFinalizer { log.Info("Removed finalizer from ephemeral runner") + if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to update ephemeral runner after removing finalizer") + return ctrl.Result{}, err + } + return ctrl.Result{}, nil } log.Info("Finalizing ephemeral runner") @@ -142,10 +145,15 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } - log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) { + removeFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + if removeFinalizer { + original.Do(ephemeralRunner.DeepCopy) + controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + } + if removeFinalizer { + log.Info("Removing finalizer") log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); client.IgnoreNotFound(err) != nil { + if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original.Get())); client.IgnoreNotFound(err) != nil { log.Error(err, "Failed to update ephemeral runner after removing finalizer") return ctrl.Result{}, err } @@ -171,17 +179,21 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - addFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addFinalizers { + ephemeralRunnerFinalizerModified := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + if ephemeralRunnerFinalizerModified { + original.Do(ephemeralRunner.DeepCopy) + controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + } + ephemeralRunnerActionsFinalizerModified := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + if ephemeralRunnerActionsFinalizerModified { + original.Do(ephemeralRunner.DeepCopy) + controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + } + if ephemeralRunnerFinalizerModified || ephemeralRunnerActionsFinalizerModified { log.Info("Adding finalizers") - var addedFinalizers bool - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addedFinalizers { - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update with finalizer set") - return ctrl.Result{}, err - } + if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to update with finalizer set") + return ctrl.Result{}, err } log.Info("Successfully added finalizers") } diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index d9798137..0748f6ca 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -80,7 +80,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.Get(ctx, req.NamespacedName, &ephemeralRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunnerSet.DeepCopy() + var original once[*v1alpha1.EphemeralRunnerSet] // Requested deletion does not need reconciled. if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { @@ -109,9 +109,14 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{RequeueAfter: 1 * time.Second}, nil } - log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + removeFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + if removeFinalizer { + original.Do(ephemeralRunnerSet.DeepCopy) + controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + } + if removeFinalizer { + log.Info("Removing finalizer") + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update ephemeral runner set with removed finalizer") return ctrl.Result{}, err } @@ -123,9 +128,14 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } // Add finalizer if not present - if controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + addFinalizer := !controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + if addFinalizer { + original.Do(ephemeralRunnerSet.DeepCopy) + controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + } + if addFinalizer { log.Info("Adding finalizer") - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update ephemeral runner set with new finalizer") return ctrl.Result{}, err } @@ -264,8 +274,8 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx return err } - original := latest.DeepCopy() - latest.Status.AppliedActionableRevision = targetAppliedRevision + desiredStatus := latest.Status + desiredStatus.AppliedActionableRevision = targetAppliedRevision ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) if err := r.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace), client.MatchingFields{resourceOwnerKey: latest.Name}); err != nil { @@ -273,13 +283,15 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx } if len(newEphemeralRunnersByStates(ephemeralRunnerList).outdated) == 0 { - latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + desiredStatus.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning } - if original.Status == latest.Status { + if latest.Status == desiredStatus { return nil } + original := latest.DeepCopy() + latest.Status = desiredStatus return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) }) } @@ -303,7 +315,6 @@ func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(c } func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error { - original := ephemeralRunnerSet.DeepCopy() var phase v1alpha1.EphemeralRunnerSetPhase switch { case len(state.outdated) > 0: @@ -321,6 +332,7 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer // Update the status if needed. if ephemeralRunnerSet.Status != desiredStatus { + original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Status = desiredStatus if err := r.Status().Patch(ctx, ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update EphemeralRunnerSet status") @@ -530,22 +542,25 @@ func (r *EphemeralRunnerSetReconciler) reconcileEphemeralRunnerSetProxySecret(ct labelsModified := !maps.Equal(proxySecret.Labels, desiredLabels) desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredRunnerSetProxy.Annotations) annotationsModified := !maps.Equal(proxySecret.Annotations, desiredAnnotations) + var original once[*corev1.Secret] + if dataModified { + original.Do(proxySecret.DeepCopy) + proxySecret.Data = desiredRunnerSetProxy.Data + } + if labelsModified { + original.Do(proxySecret.DeepCopy) + proxySecret.Labels = desiredLabels + } + if annotationsModified { + original.Do(proxySecret.DeepCopy) + proxySecret.Annotations = desiredAnnotations + } if dataModified || labelsModified || annotationsModified { - updatedProxySecret := proxySecret.DeepCopy() - if dataModified { - updatedProxySecret.Data = desiredRunnerSetProxy.Data - } - if labelsModified { - updatedProxySecret.Labels = desiredLabels - } - if annotationsModified { - updatedProxySecret.Annotations = desiredAnnotations - } log.Info("Updating ephemeralRunnerSet proxy secret") - if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(&proxySecret)); err != nil { + if err := r.Patch(ctx, &proxySecret, client.MergeFrom(original.Get())); err != nil { return nil, false, fmt.Errorf("failed to update ephemeralRunnerSet proxy secret: %w", err) } - return updatedProxySecret, true, nil + return &proxySecret, true, nil } return &proxySecret, false, nil case kerrors.IsNotFound(err): diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index c660a94f..3657cfaf 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -1084,7 +1084,6 @@ func (b *ResourceBuilder) mergeAnnotations(base, overwrite map[string]string) ma if base == nil && overwrite == nil { return nil } - base = maps.Clone(base) maps.Copy(base, overwrite) return base } diff --git a/controllers/actions.github.com/utils.go b/controllers/actions.github.com/utils.go index a77b24ba..c17aba6d 100644 --- a/controllers/actions.github.com/utils.go +++ b/controllers/actions.github.com/utils.go @@ -1,8 +1,6 @@ package actionsgithubcom -import ( - "k8s.io/apimachinery/pkg/util/rand" -) +import "sigs.k8s.io/controller-runtime/pkg/client" func FilterLabels(labels map[string]string, filter string) map[string]string { filtered := map[string]string{} @@ -16,12 +14,23 @@ func FilterLabels(labels map[string]string, filter string) map[string]string { return filtered } -var letterRunes = []rune("abcdefghijklmnopqrstuvwxyz1234567890") +type once[T client.Object] struct { + value T + fn func(T) *T + done bool +} -func RandStringRunes(n int) string { - b := make([]rune, n) - for i := range b { - b[i] = letterRunes[rand.Intn(len(letterRunes))] +func (o *once[T]) Do(f func() T) T { + if !o.done { + o.value = f() + o.done = true } - return string(b) + return o.value +} + +func (o *once[T]) Get() T { + if !o.done { + panic("not done") + } + return o.value } diff --git a/controllers/actions.github.com/utils_test.go b/controllers/actions.github.com/utils_test.go index 9e98b981..d0c35d16 100644 --- a/controllers/actions.github.com/utils_test.go +++ b/controllers/actions.github.com/utils_test.go @@ -3,6 +3,8 @@ package actionsgithubcom import ( "reflect" "testing" + + "k8s.io/apimachinery/pkg/util/rand" ) func Test_filterLabels(t *testing.T) { @@ -32,3 +34,13 @@ func Test_filterLabels(t *testing.T) { }) } } + +var letterRunes = []rune("abcdefghijklmnopqrstuvwxyz1234567890") + +func RandStringRunes(n int) string { + b := make([]rune, n) + for i := range b { + b[i] = letterRunes[rand.Intn(len(letterRunes))] + } + return string(b) +}