From 9601698ec028c92b7ff62e690963748263ee28dc Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Tue, 31 Oct 2023 15:20:26 +0100 Subject: [PATCH 1/8] feat: add kstatus conforment status updates --- api/v1alpha1/componentversion_types.go | 15 +- ...livery.ocm.software_componentversions.yaml | 2 +- controllers/componentversion_controller.go | 175 +++++++++--------- controllers/identifiable_contract.go | 20 ++ controllers/register_status_defer.go | 43 +++++ 5 files changed, 163 insertions(+), 92 deletions(-) create mode 100644 controllers/identifiable_contract.go create mode 100644 controllers/register_status_defer.go diff --git a/api/v1alpha1/componentversion_types.go b/api/v1alpha1/componentversion_types.go index 2ef33860..642104ca 100644 --- a/api/v1alpha1/componentversion_types.go +++ b/api/v1alpha1/componentversion_types.go @@ -5,6 +5,7 @@ package v1alpha1 import ( + "fmt" "time" "github.com/fluxcd/pkg/apis/meta" @@ -145,11 +146,23 @@ type ComponentVersionStatus struct { // +optional ReconciledVersion string `json:"reconciledVersion,omitempty"` - // Verified is a boolean indicating whether all of the specified signatures have been verified and are valid. + // Verified is a boolean indicating whether all the specified signatures have been verified and are valid. // +optional Verified bool `json:"verified,omitempty"` } +func (in *ComponentVersion) GetVID() map[string]string { + vid := fmt.Sprintf("%s:%s", in.Status.ComponentDescriptor.Name, in.Status.ReconciledVersion) + metadata := make(map[string]string) + metadata[GroupVersion.Group+"/component_version"] = vid + + return metadata +} + +func (in *ComponentVersion) SetObservedGeneration(v int64) { + in.Status.ObservedGeneration = v +} + // GetComponentName returns the name of the component func (in *ComponentVersion) GetComponentName() string { return in.Spec.Component diff --git a/config/crd/bases/delivery.ocm.software_componentversions.yaml b/config/crd/bases/delivery.ocm.software_componentversions.yaml index a6a22b42..0e71233e 100644 --- a/config/crd/bases/delivery.ocm.software_componentversions.yaml +++ b/config/crd/bases/delivery.ocm.software_componentversions.yaml @@ -264,7 +264,7 @@ spec: of the latest reconciled ComponentVersion. type: string verified: - description: Verified is a boolean indicating whether all of the specified + description: Verified is a boolean indicating whether all the specified signatures have been verified and are valid. type: boolean type: object diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index 7edfd0d8..f51dbb8e 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -19,7 +19,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" kuberecorder "k8s.io/client-go/tools/record" - "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -84,51 +83,29 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req return } - patchHelper, err := patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to create patchhelper: %w", err) - } + patchHelper := patch.NewSerialPatcher(obj, r.Client) // Always attempt to patch the object and status after each reconciliation. defer func() { - // Patching has not been set up, or the controller errored earlier. - if patchHelper == nil { - return - } - - // If still reconciling then reconciliation did not succeed, set to ProgressingWithRetry to - // indicate that reconciliation will be retried. - if conditions.IsReconciling(obj) { - reconciling := conditions.Get(obj, meta.ReconcilingCondition) - reconciling.Reason = meta.ProgressingWithRetryReason - conditions.Set(obj, reconciling) - msg := fmt.Sprintf("Reconciliation did not succeed, retrying in %s", obj.GetRequeueAfter()) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, msg, nil) - } - - // Set status observed generation option if the component is ready. - if conditions.IsReady(obj) { - obj.Status.ObservedGeneration = obj.Generation - msg := fmt.Sprintf("Reconciliation finished, next run in %s", obj.GetRequeueAfter()) - vid := fmt.Sprintf("%s:%s", obj.Status.ComponentDescriptor.Name, obj.Status.ReconciledVersion) - metadata := make(map[string]string) - metadata[v1alpha1.GroupVersion.Group+"/component_version"] = vid - event.New(r.EventRecorder, obj, eventv1.EventSeverityInfo, msg, metadata) - } - - // Update the object. - if err := patchHelper.Patch(ctx, obj); err != nil { - retErr = errors.Join(retErr, err) + if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + retErr = errors.Join(retErr, derr) } }() - rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconcilation in progress for component: %s", obj.Spec.Component) + // Starts the progression by setting ReconcilingCondition. + // This will be checked in defer. + // Should only be deleted on a success. + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress for component: %s", obj.Spec.Component) octx, err := r.OCMClient.CreateAuthenticatedOCMContext(ctx, obj) if err != nil { - msg := fmt.Sprintf("authentication failed for repository: %s with error: %s", obj.Spec.Repository.URL, err) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.AuthenticatedContextCreationFailedReason, msg) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, msg, nil) + // we don't fail here, because all manifests might have been applied at once or the secret + // for authentication is being reconciled. + _ = r.markAsFailed( + obj, + v1alpha1.AuthenticatedContextCreationFailedReason, + fmt.Errorf("authentication failed for repository: %s with error: %s", obj.Spec.Repository.URL, err), + ) return ctrl.Result{ RequeueAfter: obj.GetRequeueAfter(), @@ -138,9 +115,12 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req // reconcile the version before calling reconcile func update, version, err := r.checkVersion(ctx, octx, obj) if err != nil { - msg := fmt.Sprintf("version check failed for %s %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.CheckVersionFailedReason, msg) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, msg, nil) + // The component might not be there yet. We don't fail but keep polling instead. + _ = r.markAsFailed( + obj, + v1alpha1.CheckVersionFailedReason, + fmt.Errorf("version check failed for %s %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err), + ) return ctrl.Result{ RequeueAfter: obj.GetRequeueAfter(), @@ -152,19 +132,23 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, - fmt.Sprintf("Applied version: %s", version)) + "Applied version: %s", + version) return ctrl.Result{ RequeueAfter: obj.GetRequeueAfter(), }, nil } + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "updating component to new version: %s: %s", obj.Spec.Component, version) + ok, err := r.OCMClient.VerifyComponent(ctx, octx, obj, version) if err != nil { - msg := fmt.Sprintf("failed to verify %s with constraint %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err) - conditions.Delete(obj, meta.ReconcilingCondition) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.VerificationFailedReason, msg) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, fmt.Sprintf("%s, retrying in %s", err.Error(), obj.GetRequeueAfter()), nil) + _ = r.markAsFailed( + obj, + v1alpha1.VerificationFailedReason, + fmt.Errorf("failed to verify %s with constraint %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err), + ) return ctrl.Result{ RequeueAfter: obj.GetRequeueAfter(), @@ -172,38 +156,33 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req } if !ok { - msg := "attempted to verify component, but the digest didn't match" - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.VerificationFailedReason, msg) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, fmt.Sprintf("%s, retrying in %s", msg, obj.GetRequeueAfter()), nil) + _ = r.markAsFailed( + obj, + v1alpha1.VerificationFailedReason, + errors.New("attempted to verify component, but the digest didn't match"), + ) return ctrl.Result{ RequeueAfter: obj.GetRequeueAfter(), }, nil } - // update the result for the defer call to have the latest information - rresult, err := r.reconcile(ctx, octx, obj, version) - if err != nil { - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, fmt.Sprintf("Reconciliation failed: %s, retrying in %s", err.Error(), obj.GetRequeueAfter()), nil) - } - - return rresult, err + return r.reconcile(ctx, octx, obj, version) } func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Context, obj *v1alpha1.ComponentVersion, version string) (ctrl.Result, error) { if obj.Generation != obj.Status.ObservedGeneration { - // don't have to patch here since we patch the object in the outer reconcile call. rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "processing object: new generation %d -> %d", obj.Status.ObservedGeneration, obj.Generation) } cv, err := r.OCMClient.GetComponentVersion(ctx, octx, obj, obj.Spec.Component, version) if err != nil { - err = fmt.Errorf("failed to get component version: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.ComponentVersionInvalidReason, err.Error()) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) - - return ctrl.Result{}, err + return ctrl.Result{}, r.markAsFailed( + obj, + v1alpha1.ComponentVersionInvalidReason, + fmt.Errorf("failed to get component version: %w", err), + ) } defer cv.Close() @@ -212,18 +191,25 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con dv := &compdesc.DescriptorVersion{} cd, err := dv.ConvertFrom(cv.GetDescriptor()) if err != nil { - err = fmt.Errorf("failed to convert component descriptor: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.ConvertComponentDescriptorFailedReason, err.Error()) - return ctrl.Result{}, err + return ctrl.Result{}, r.markAsFailed( + obj, + v1alpha1.ConvertComponentDescriptorFailedReason, + fmt.Errorf("failed to convert component descriptor: %w", err), + ) } + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "component fetched, creating descriptors") + // setup the component descriptor kubernetes resource componentName, err := component.ConstructUniqueName(cd.GetName(), cd.GetVersion(), nil) if err != nil { - err = fmt.Errorf("failed to generate name: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.NameGenerationFailedReason, err.Error()) - return ctrl.Result{}, err + return ctrl.Result{}, r.markAsFailed( + obj, + v1alpha1.NameGenerationFailedReason, + fmt.Errorf("failed to generate name: %w", err), + ) } + descriptor := &v1alpha1.ComponentDescriptor{ ObjectMeta: metav1.ObjectMeta{ Namespace: obj.GetNamespace(), @@ -251,10 +237,11 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con }) if err != nil { - err = fmt.Errorf("failed to create or update component descriptor: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.CreateOrUpdateComponentDescriptorFailedReason, err.Error()) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) - return ctrl.Result{}, err + return ctrl.Result{}, r.markAsFailed( + obj, + v1alpha1.CreateOrUpdateComponentDescriptorFailedReason, + fmt.Errorf("failed to create or update component descriptor: %w", err), + ) } componentDescriptor := v1alpha1.Reference{ @@ -269,10 +256,11 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con if obj.Spec.References.Expand { componentDescriptor.References, err = r.parseReferences(ctx, octx, obj, cv.GetDescriptor().References) if err != nil { - err = fmt.Errorf("failed to parse references: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.ParseReferencesFailedReason, err.Error()) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) - return ctrl.Result{}, err + return ctrl.Result{}, r.markAsFailed( + obj, + v1alpha1.ParseReferencesFailedReason, + fmt.Errorf("failed to parse references: %w", err), + ) } } @@ -283,7 +271,8 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, - fmt.Sprintf("Applied version: %s", version)) + "Applied version: %s", + version) conditions.Delete(obj, meta.ReconcilingCondition) @@ -291,13 +280,13 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con } func (r *ComponentVersionReconciler) checkVersion(ctx context.Context, octx ocm.Context, obj *v1alpha1.ComponentVersion) (bool, string, error) { - log := log.FromContext(ctx).WithName("ocm-component-version-reconcile") + logger := log.FromContext(ctx).WithName("ocm-component-version-reconcile") latest, err := r.OCMClient.GetLatestValidComponentVersion(ctx, octx, obj) if err != nil { return false, "", fmt.Errorf("failed to get latest component version: %w", err) } - log.V(4).Info("got latest version of component", "version", latest) + logger.V(4).Info("got latest version of component", "version", latest) latestSemver, err := semver.NewVersion(latest) if err != nil { @@ -312,11 +301,11 @@ func (r *ComponentVersionReconciler) checkVersion(ctx context.Context, octx ocm. if err != nil { return false, "", fmt.Errorf("failed to parse reconciled version: %w", err) } - log.V(4).Info("current reconciled version is", "reconciled", current.String()) + logger.V(4).Info("current reconciled version is", "reconciled", current.String()) if latestSemver.Equal(current) || current.GreaterThan(latestSemver) { - log.V(4).Info("Reconciled version equal to or greater than newest available version", "version", latestSemver) - return false, "", nil + logger.V(4).Info("Reconciled version equal to or greater than newest available version", "version", latestSemver) + return false, latest, nil } event.New(r.EventRecorder, obj, eventv1.EventSeverityInfo, fmt.Sprintf("Version check succeeded, found latest version: %s", latest), nil) @@ -380,7 +369,6 @@ func (r *ComponentVersionReconciler) createComponentDescriptor(ctx context.Conte return nil, fmt.Errorf("failed to convert component descriptor: %w", err) } - log := log.FromContext(ctx) // setup the component descriptor kubernetes resource componentName, err := component.ConstructUniqueName(ref.ComponentName, ref.Version, ref.GetMeta().ExtraIdentity) if err != nil { @@ -397,19 +385,26 @@ func (r *ComponentVersionReconciler) createComponentDescriptor(ctx context.Conte }, } - if err := controllerutil.SetOwnerReference(parent, descriptor, r.Scheme); err != nil { - return nil, fmt.Errorf("failed to set owner reference: %w", err) - } - // create or update the component descriptor kubernetes resource // we don't need to update it - op, err := controllerutil.CreateOrUpdate(ctx, r.Client, descriptor, func() error { + if _, err = controllerutil.CreateOrUpdate(ctx, r.Client, descriptor, func() error { + if descriptor.ObjectMeta.CreationTimestamp.IsZero() { + if err := controllerutil.SetOwnerReference(parent, descriptor, r.Scheme); err != nil { + return fmt.Errorf("failed to set owner reference: %w", err) + } + } + return nil - }) - if err != nil { + }); err != nil { return nil, fmt.Errorf("failed to create/update component descriptor: %w", err) } - log.V(4).Info(fmt.Sprintf("%s(ed) descriptor", op), "descriptor", klog.KObj(descriptor)) return descriptor, nil } + +func (r *ComponentVersionReconciler) markAsFailed(obj *v1alpha1.ComponentVersion, reason string, err error) error { + conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error()) + event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + + return err +} diff --git a/controllers/identifiable_contract.go b/controllers/identifiable_contract.go new file mode 100644 index 00000000..4ff832ff --- /dev/null +++ b/controllers/identifiable_contract.go @@ -0,0 +1,20 @@ +package controllers + +import ( + "github.com/fluxcd/pkg/runtime/conditions" +) + +// IdentifiableClientObject defines an object which can create an identity for itself. +type IdentifiableClientObject interface { + StatusMutator + conditions.Setter + + // GetVID constructs an identifier for an object. + GetVID() map[string]string +} + +// StatusMutator allows mutating specific status fields of an object. +type StatusMutator interface { + // SetObservedGeneration mutates the observed generation field of an object. + SetObservedGeneration(v int64) +} diff --git a/controllers/register_status_defer.go b/controllers/register_status_defer.go new file mode 100644 index 00000000..d9aa9814 --- /dev/null +++ b/controllers/register_status_defer.go @@ -0,0 +1,43 @@ +package controllers + +import ( + "context" + "fmt" + "time" + + eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" + "github.com/fluxcd/pkg/runtime/patch" + "github.com/open-component-model/ocm-controller/pkg/event" + kuberecorder "k8s.io/client-go/tools/record" +) + +// DeferredStatusUpdate takes an object which can identify itself and updates its status including ObservedGeneration. +func DeferredStatusUpdate( + ctx context.Context, + patchHelper *patch.SerialPatcher, + obj IdentifiableClientObject, + recorder kuberecorder.EventRecorder, + requeue time.Duration, +) error { + // If still reconciling then reconciliation did not succeed, set to ProgressingWithRetry to + // indicate that reconciliation will be retried. + if conditions.IsReconciling(obj) { + reconciling := conditions.Get(obj, meta.ReconcilingCondition) + reconciling.Reason = meta.ProgressingWithRetryReason + conditions.Set(obj, reconciling) + msg := fmt.Sprintf("Reconciliation did not succeed, retrying in %s", requeue) + event.New(recorder, obj, eventv1.EventSeverityError, msg, obj.GetVID()) + } + + // Set status observed generation option if the component is ready. + if conditions.IsReady(obj) { + obj.SetObservedGeneration(obj.GetGeneration()) + msg := fmt.Sprintf("Reconciliation finished, next run in %s", requeue) + event.New(recorder, obj, eventv1.EventSeverityInfo, msg, obj.GetVID()) + } + + // Update the object. + return patchHelper.Patch(ctx, obj) +} From 6066c9f5e829015adda4b436fa66338c658fb8f0 Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Thu, 2 Nov 2023 12:37:41 +0100 Subject: [PATCH 2/8] adding applying status update to configuration controller --- api/v1alpha1/configuration_types.go | 11 ++ controllers/componentversion_controller.go | 2 + controllers/configuration_controller.go | 149 ++++++--------------- 3 files changed, 53 insertions(+), 109 deletions(-) diff --git a/api/v1alpha1/configuration_types.go b/api/v1alpha1/configuration_types.go index 7b7803b4..9dd80180 100644 --- a/api/v1alpha1/configuration_types.go +++ b/api/v1alpha1/configuration_types.go @@ -29,6 +29,17 @@ type Configuration struct { Status MutationStatus `json:"status,omitempty"` } +func (in *Configuration) GetVID() map[string]string { + metadata := make(map[string]string) + metadata[GroupVersion.Group+"/configuration_digest"] = in.Status.LatestSnapshotDigest + + return metadata +} + +func (in *Configuration) SetObservedGeneration(v int64) { + in.Status.ObservedGeneration = v +} + // GetConditions returns the conditions of the Configuration. func (in *Configuration) GetConditions() []metav1.Condition { return in.Status.Conditions diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index f51dbb8e..a1548537 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -254,6 +254,8 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con } if obj.Spec.References.Expand { + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "descriptors created, expanding references") + componentDescriptor.References, err = r.parseReferences(ctx, octx, obj, cv.GetDescriptor().References) if err != nil { return ctrl.Result{}, r.markAsFailed( diff --git a/controllers/configuration_controller.go b/controllers/configuration_controller.go index d017054e..8cd1475a 100644 --- a/controllers/configuration_controller.go +++ b/controllers/configuration_controller.go @@ -16,6 +16,7 @@ import ( "github.com/fluxcd/pkg/runtime/patch" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" sourcev1 "github.com/fluxcd/source-controller/api/v1" + "github.com/open-component-model/ocm-controller/pkg/event" "golang.org/x/exp/slices" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -36,7 +37,6 @@ import ( "github.com/open-component-model/ocm-controller/api/v1alpha1" "github.com/open-component-model/ocm-controller/pkg/cache" - "github.com/open-component-model/ocm-controller/pkg/event" "github.com/open-component-model/ocm-controller/pkg/ocm" "github.com/open-component-model/ocm-controller/pkg/snapshot" ) @@ -170,113 +170,43 @@ func (r *ConfigurationReconciler) Reconcile( return result, nil } - var patchHelper *patch.Helper - patchHelper, err = patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to create patch helper: %w", err) - } + patchHelper := patch.NewSerialPatcher(obj, r.Client) // Always attempt to patch the object and status after each reconciliation. defer func() { - // Patching has not been set up, or the controller errored earlier. - if patchHelper == nil { - return - } - - if condition := conditions.Get(obj, meta.StalledCondition); condition != nil && - condition.Status == metav1.ConditionTrue { - conditions.Delete(obj, meta.ReconcilingCondition) - } - - // Check if it's a successful reconciliation. - // We don't set Requeue in case of error, so we can safely check for Requeue. - if result.RequeueAfter == obj.GetRequeueAfter() && !result.Requeue && err == nil { - // Remove the reconciling condition if it's set. - conditions.Delete(obj, meta.ReconcilingCondition) - - // Set the return err as the ready failure message is the resource is not ready, but also not reconciling or stalled. - if ready := conditions.Get(obj, meta.ReadyCondition); ready != nil && - ready.Status == metav1.ConditionFalse && - !conditions.IsStalled(obj) { - err = errors.New(conditions.GetMessage(obj, meta.ReadyCondition)) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) - } - } - - // If still reconciling then reconciliation did not succeed, set to ProgressingWithRetry to - // indicate that reconciliation will be retried. - if conditions.IsReconciling(obj) { - reconciling := conditions.Get(obj, meta.ReconcilingCondition) - reconciling.Reason = meta.ProgressingWithRetryReason - conditions.Set(obj, reconciling) - } - - // If not reconciling or stalled than mark Ready=True - if !conditions.IsReconciling(obj) && !conditions.IsStalled(obj) && - err == nil && result.RequeueAfter == obj.GetRequeueAfter() { - conditions.MarkTrue( - obj, - meta.ReadyCondition, - meta.SucceededReason, - "Reconciliation success", - ) - event.New( - r.EventRecorder, - obj, - eventv1.EventSeverityInfo, - "Reconciliation succeeded", - nil, - ) - } - - // Set status observed generation option if the object is stalled or ready. - if conditions.IsStalled(obj) || conditions.IsReady(obj) { - obj.Status.ObservedGeneration = obj.Generation - event.New( - r.EventRecorder, - obj, - eventv1.EventSeverityInfo, - fmt.Sprintf("Reconciliation finished, next run in %s", obj.GetRequeueAfter()), - map[string]string{ - v1alpha1.GroupVersion.Group + "/configuration_digest": obj.Status.LatestSnapshotDigest, - }, - ) - } - - if perr := patchHelper.Patch(ctx, obj); perr != nil { - err = errors.Join(err, perr) + if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + err = errors.Join(err, derr) } }() - logger.Info("reconciling configuration") + // Starts the progression by setting ReconcilingCondition. + // This will be checked in defer. + // Should only be deleted on a success. + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress for configuration: %s", obj.Name) // check dependencies are ready ready, err := r.checkReadiness(ctx, obj.GetNamespace(), &obj.Spec.SourceRef) if err != nil { - logger.Info("source ref object is not ready with error", "error", err) + r.markAsFailed(obj, "SourceRefNotReadyWithError", err.Error(), "source ref not yet ready with error: %s", obj.Spec.SourceRef.Name) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - logger.Info( - "source ref object is not ready", - "source", - obj.Spec.SourceRef.GetNamespacedName(), - ) + r.markAsFailed(obj, "SourceRefNotReady", "source not ready yet", "source ref not yet ready: %s", obj.Spec.SourceRef.Name) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if obj.Spec.ConfigRef != nil { ready, err := r.checkReadiness(ctx, obj.GetNamespace(), obj.Spec.ConfigRef) if err != nil { - logger.Info("config ref object is not ready with error", "error", err) + r.markAsFailed(obj, "ConfigRefNotReadyWithError", err.Error(), "config ref not yet ready with error: %s", obj.Spec.ConfigRef.Name) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - logger.Info( - "config ref object is not ready", - "source", - obj.Spec.SourceRef.GetNamespacedName(), - ) + r.markAsFailed(obj, "ConfigRefNotReady", "config ref not ready", "config ref not yet ready: %s", obj.Spec.ConfigRef.Name) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } } @@ -284,17 +214,14 @@ func (r *ConfigurationReconciler) Reconcile( if obj.Spec.PatchStrategicMerge != nil { ready, err := r.checkFluxSourceReadiness(ctx, obj.Spec.PatchStrategicMerge.Source.SourceRef) if err != nil { - logger.Info("source object is not ready with error", "error", err) + r.markAsFailed(obj, "PatchStrategicMergeSourceRefNotReadyWithError", err.Error(), "patch strategic merge source ref not yet ready with error: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - ref := obj.Spec.PatchStrategicMerge.Source.SourceRef - logger.Info( - "patch git repository object is not ready", - "gitrepository", - (types.NamespacedName{Namespace: ref.Namespace, Name: ref.Name}).String(), - ) + r.markAsFailed(obj, "PatchStrategicMergeSourceRefNotReady", "patch strategic merge source not ready yet", "patch strategic merge source ref not yet ready: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } } @@ -304,9 +231,13 @@ func (r *ConfigurationReconciler) Reconcile( if obj.GetSnapshotName() == "" { name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { + r.markAsFailed(obj, "GenerateSnapshotNameError", err.Error(), "failed to generate snapshot name for: %s", obj.GetName()) + return ctrl.Result{}, err } + obj.Status.SnapshotName = name + return ctrl.Result{Requeue: true}, nil } @@ -338,31 +269,25 @@ func (r *ConfigurationReconciler) reconcile( if errors.Is(err, errTar) { err = fmt.Errorf("source resource is not a tar archive: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.SourceReasonNotATarArchiveReason, - err.Error(), - ) + r.markAsFailed(obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error(), "source is not a tar archive") + return ctrl.Result{}, err } err = fmt.Errorf("failed to reconcile mutation object: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.ReconcileMutationObjectFailedReason, - err.Error(), - ) + r.markAsFailed(obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error(), "failed to reconcile mutation object") + return ctrl.Result{}, err } obj.Status.ObservedGeneration = obj.GetGeneration() - // Remove any stale Ready condition, most likely False, set above. Its value - // is derived from the overall result of the reconciliation in the deferred - // bcfgk at the very end. - conditions.Delete(obj, meta.ReadyCondition) + conditions.MarkTrue(obj, + meta.ReadyCondition, + meta.SucceededReason, + "Reconciliation success") + + conditions.Delete(obj, meta.ReconcilingCondition) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } @@ -527,3 +452,9 @@ func makeRequestsForConfigurations(ll ...v1alpha1.Configuration) []reconcile.Req return requests } + +func (r *ConfigurationReconciler) markAsFailed(obj *v1alpha1.Configuration, reason, msg, format string, messageArgs ...any) { + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, format, messageArgs...) + conditions.MarkFalse(obj, meta.ReadyCondition, reason, msg) + event.New(r.EventRecorder, obj, eventv1.EventSeverityError, msg, nil) +} From 78fad04eebcea0b31849124bee742b250fa8ab82 Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Thu, 2 Nov 2023 14:41:09 +0100 Subject: [PATCH 3/8] removed progressive status from error cases --- api/v1alpha1/condition_types.go | 8 +- api/v1alpha1/localization_types.go | 11 ++ api/v1alpha1/resource_types.go | 11 ++ api/v1alpha1/snapshot_types.go | 11 ++ controllers/componentversion_controller.go | 62 ++++--- controllers/configuration_controller.go | 60 +++--- controllers/localization_controller.go | 176 ++++++------------ controllers/mutate_condition_status.go | 16 ++ controllers/resource_controller.go | 202 +++++---------------- controllers/snapshot_controller.go | 46 ++--- go.mod | 2 +- pkg/ocm/ocm.go | 50 +---- 12 files changed, 253 insertions(+), 402 deletions(-) create mode 100644 controllers/mutate_condition_status.go diff --git a/api/v1alpha1/condition_types.go b/api/v1alpha1/condition_types.go index 6b6d9c6a..1af7bcb2 100644 --- a/api/v1alpha1/condition_types.go +++ b/api/v1alpha1/condition_types.go @@ -20,7 +20,7 @@ const ( // ConvertComponentDescriptorFailedReason is used when the Component Descriptor cannot be converted. ConvertComponentDescriptorFailedReason = "ConvertComponentDescriptorFailed" - // NameGenerationFailedReason is used when the componentn name could not be generated. + // NameGenerationFailedReason is used when the component name could not be generated. NameGenerationFailedReason = "NameGenerationFailed" // CreateOrUpdateComponentDescriptorFailedReason is used when the Component Descriptor cannot be created or updated on the resource. @@ -44,6 +44,12 @@ const ( // ComponentDescriptorNotFoundReason is used when the component descriptor cannot be found. ComponentDescriptorNotFoundReason = "ComponentDescriptorNotFound" + // ComponentVersionNotFoundReason is used when the component version cannot be found. + ComponentVersionNotFoundReason = "ComponentVersionNotFound" + + // ComponentVersionNotReadyReason is used when the component version is not ready. + ComponentVersionNotReadyReason = "ComponentVersionNotReady" + // CreateOrUpdateSnapshotFailedReason is used when the snapshot cannot be created or updated. CreateOrUpdateSnapshotFailedReason = "CreateOrUpdateSnapshotFailed" diff --git a/api/v1alpha1/localization_types.go b/api/v1alpha1/localization_types.go index 27b31915..2a5a550c 100644 --- a/api/v1alpha1/localization_types.go +++ b/api/v1alpha1/localization_types.go @@ -28,6 +28,17 @@ type Localization struct { Status MutationStatus `json:"status,omitempty"` } +func (in *Localization) GetVID() map[string]string { + metadata := make(map[string]string) + metadata[GroupVersion.Group+"/localization_digest"] = in.Status.LatestSnapshotDigest + + return metadata +} + +func (in *Localization) SetObservedGeneration(v int64) { + in.Status.ObservedGeneration = v +} + // GetConditions returns the conditions of the Localization. func (in *Localization) GetConditions() []metav1.Condition { return in.Status.Conditions diff --git a/api/v1alpha1/resource_types.go b/api/v1alpha1/resource_types.go index a87f5dc2..4152cbd7 100644 --- a/api/v1alpha1/resource_types.go +++ b/api/v1alpha1/resource_types.go @@ -74,6 +74,17 @@ type Resource struct { Status ResourceStatus `json:"status,omitempty"` } +func (in *Resource) GetVID() map[string]string { + metadata := make(map[string]string) + metadata[GroupVersion.Group+"/resource_version"] = in.Status.LastAppliedResourceVersion + + return metadata +} + +func (in *Resource) SetObservedGeneration(v int64) { + in.Status.ObservedGeneration = v +} + // GetConditions returns the conditions of the Resource. func (in *Resource) GetConditions() []metav1.Condition { return in.Status.Conditions diff --git a/api/v1alpha1/snapshot_types.go b/api/v1alpha1/snapshot_types.go index a102b343..d51d6fdf 100644 --- a/api/v1alpha1/snapshot_types.go +++ b/api/v1alpha1/snapshot_types.go @@ -53,6 +53,17 @@ type SnapshotStatus struct { ObservedGeneration int64 `json:"observedGeneration,omitempty"` } +func (in *Snapshot) GetVID() map[string]string { + metadata := make(map[string]string) + metadata[GroupVersion.Group+"/snapshot_digest"] = in.Status.LastReconciledDigest + + return metadata +} + +func (in *Snapshot) SetObservedGeneration(v int64) { + in.Status.ObservedGeneration = v +} + // GetComponentVersion returns the component version for the snapshot func (in Snapshot) GetComponentVersion() string { return in.Spec.Identity[ComponentVersionKey] diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index a1548537..8586c034 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -101,10 +101,11 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req if err != nil { // we don't fail here, because all manifests might have been applied at once or the secret // for authentication is being reconciled. - _ = r.markAsFailed( + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, - fmt.Errorf("authentication failed for repository: %s with error: %s", obj.Spec.Repository.URL, err), + fmt.Sprintf("authentication failed for repository: %s with error: %s", obj.Spec.Repository.URL, err), ) return ctrl.Result{ @@ -116,10 +117,11 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req update, version, err := r.checkVersion(ctx, octx, obj) if err != nil { // The component might not be there yet. We don't fail but keep polling instead. - _ = r.markAsFailed( + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.CheckVersionFailedReason, - fmt.Errorf("version check failed for %s %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err), + fmt.Sprintf("version check failed for %s %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err), ) return ctrl.Result{ @@ -144,10 +146,11 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req ok, err := r.OCMClient.VerifyComponent(ctx, octx, obj, version) if err != nil { - _ = r.markAsFailed( + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.VerificationFailedReason, - fmt.Errorf("failed to verify %s with constraint %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err), + fmt.Sprintf("failed to verify %s with constraint %s with error: %s", obj.Spec.Component, obj.Spec.Version.Semver, err), ) return ctrl.Result{ @@ -156,10 +159,11 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req } if !ok { - _ = r.markAsFailed( + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.VerificationFailedReason, - errors.New("attempted to verify component, but the digest didn't match"), + "attempted to verify component, but the digest didn't match", ) return ctrl.Result{ @@ -178,11 +182,14 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con cv, err := r.OCMClient.GetComponentVersion(ctx, octx, obj, obj.Spec.Component, version) if err != nil { - return ctrl.Result{}, r.markAsFailed( + err = fmt.Errorf("failed to get component version: %w", err) + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.ComponentVersionInvalidReason, - fmt.Errorf("failed to get component version: %w", err), + err.Error(), ) + return ctrl.Result{}, err } defer cv.Close() @@ -191,11 +198,14 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con dv := &compdesc.DescriptorVersion{} cd, err := dv.ConvertFrom(cv.GetDescriptor()) if err != nil { - return ctrl.Result{}, r.markAsFailed( + err = fmt.Errorf("failed to convert component descriptor: %w", err) + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.ConvertComponentDescriptorFailedReason, - fmt.Errorf("failed to convert component descriptor: %w", err), + err.Error(), ) + return ctrl.Result{}, err } rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "component fetched, creating descriptors") @@ -203,11 +213,14 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con // setup the component descriptor kubernetes resource componentName, err := component.ConstructUniqueName(cd.GetName(), cd.GetVersion(), nil) if err != nil { - return ctrl.Result{}, r.markAsFailed( + err = fmt.Errorf("failed to generate name: %w", err) + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, - fmt.Errorf("failed to generate name: %w", err), + err.Error(), ) + return ctrl.Result{}, err } descriptor := &v1alpha1.ComponentDescriptor{ @@ -237,11 +250,14 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con }) if err != nil { - return ctrl.Result{}, r.markAsFailed( + err = fmt.Errorf("failed to create or update component descriptor: %w", err) + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.CreateOrUpdateComponentDescriptorFailedReason, - fmt.Errorf("failed to create or update component descriptor: %w", err), + err.Error(), ) + return ctrl.Result{}, err } componentDescriptor := v1alpha1.Reference{ @@ -258,11 +274,14 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con componentDescriptor.References, err = r.parseReferences(ctx, octx, obj, cv.GetDescriptor().References) if err != nil { - return ctrl.Result{}, r.markAsFailed( + err = fmt.Errorf("failed to parse references: %w", err) + MarkAsFailed( + r.EventRecorder, obj, v1alpha1.ParseReferencesFailedReason, - fmt.Errorf("failed to parse references: %w", err), + err.Error(), ) + return ctrl.Result{}, err } } @@ -403,10 +422,3 @@ func (r *ComponentVersionReconciler) createComponentDescriptor(ctx context.Conte return descriptor, nil } - -func (r *ComponentVersionReconciler) markAsFailed(obj *v1alpha1.ComponentVersion, reason string, err error) error { - conditions.MarkFalse(obj, meta.ReadyCondition, reason, err.Error()) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) - - return err -} diff --git a/controllers/configuration_controller.go b/controllers/configuration_controller.go index 8cd1475a..1e7c0e2c 100644 --- a/controllers/configuration_controller.go +++ b/controllers/configuration_controller.go @@ -10,13 +10,11 @@ import ( "fmt" "time" - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" sourcev1 "github.com/fluxcd/source-controller/api/v1" - "github.com/open-component-model/ocm-controller/pkg/event" "golang.org/x/exp/slices" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,7 +28,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" @@ -153,8 +150,6 @@ func (r *ConfigurationReconciler) Reconcile( ctx context.Context, req ctrl.Request, ) (result ctrl.Result, err error) { - logger := log.FromContext(ctx).WithName("configuration-controller") - obj := &v1alpha1.Configuration{} if err = r.Client.Get(ctx, req.NamespacedName, obj); err != nil { if apierrors.IsNotFound(err) { @@ -166,8 +161,7 @@ func (r *ConfigurationReconciler) Reconcile( // return early if obj is suspended if obj.Spec.Suspend { - logger.Info("configuration object suspended") - return result, nil + return ctrl.Result{}, nil } patchHelper := patch.NewSerialPatcher(obj, r.Client) @@ -187,12 +181,17 @@ func (r *ConfigurationReconciler) Reconcile( // check dependencies are ready ready, err := r.checkReadiness(ctx, obj.GetNamespace(), &obj.Spec.SourceRef) if err != nil { - r.markAsFailed(obj, "SourceRefNotReadyWithError", err.Error(), "source ref not yet ready with error: %s", obj.Spec.SourceRef.Name) + MarkAsFailed(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - r.markAsFailed(obj, "SourceRefNotReady", "source not ready yet", "source ref not yet ready: %s", obj.Spec.SourceRef.Name) + MarkAsFailed( + r.EventRecorder, + obj, + "SourceRefNotReady", + fmt.Sprintf("source ref not yet ready: %s", obj.Spec.SourceRef.Name), + ) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } @@ -200,12 +199,22 @@ func (r *ConfigurationReconciler) Reconcile( if obj.Spec.ConfigRef != nil { ready, err := r.checkReadiness(ctx, obj.GetNamespace(), obj.Spec.ConfigRef) if err != nil { - r.markAsFailed(obj, "ConfigRefNotReadyWithError", err.Error(), "config ref not yet ready with error: %s", obj.Spec.ConfigRef.Name) + MarkAsFailed( + r.EventRecorder, + obj, + "ConfigRefNotReadyWithError", + fmt.Sprintf("config ref not yet ready with error: %s: %s", obj.Spec.ConfigRef.Name, err), + ) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - r.markAsFailed(obj, "ConfigRefNotReady", "config ref not ready", "config ref not yet ready: %s", obj.Spec.ConfigRef.Name) + MarkAsFailed( + r.EventRecorder, + obj, + "ConfigRefNotReady", + fmt.Sprintf("config ref not yet ready: %s", obj.Spec.ConfigRef.Name), + ) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } @@ -214,13 +223,23 @@ func (r *ConfigurationReconciler) Reconcile( if obj.Spec.PatchStrategicMerge != nil { ready, err := r.checkFluxSourceReadiness(ctx, obj.Spec.PatchStrategicMerge.Source.SourceRef) if err != nil { - r.markAsFailed(obj, "PatchStrategicMergeSourceRefNotReadyWithError", err.Error(), "patch strategic merge source ref not yet ready with error: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name) + MarkAsFailed( + r.EventRecorder, + obj, + "PatchStrategicMergeSourceRefNotReadyWithError", + fmt.Sprintf("patch strategic merge source ref not yet ready with error: %s: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name, err), + ) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - r.markAsFailed(obj, "PatchStrategicMergeSourceRefNotReady", "patch strategic merge source not ready yet", "patch strategic merge source ref not yet ready: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name) + MarkAsFailed( + r.EventRecorder, + obj, + "PatchStrategicMergeSourceRefNotReady", + fmt.Sprintf("patch strategic merge source ref not yet ready: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name), + ) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } @@ -231,7 +250,8 @@ func (r *ConfigurationReconciler) Reconcile( if obj.GetSnapshotName() == "" { name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { - r.markAsFailed(obj, "GenerateSnapshotNameError", err.Error(), "failed to generate snapshot name for: %s", obj.GetName()) + err := fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -248,8 +268,6 @@ func (r *ConfigurationReconciler) reconcile( ctx context.Context, obj *v1alpha1.Configuration, ) (ctrl.Result, error) { - rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") - if obj.Generation != obj.Status.ObservedGeneration { rreconcile.ProgressiveStatus( false, @@ -269,13 +287,13 @@ func (r *ConfigurationReconciler) reconcile( if errors.Is(err, errTar) { err = fmt.Errorf("source resource is not a tar archive: %w", err) - r.markAsFailed(obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error(), "source is not a tar archive") + MarkAsFailed(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) return ctrl.Result{}, err } err = fmt.Errorf("failed to reconcile mutation object: %w", err) - r.markAsFailed(obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error(), "failed to reconcile mutation object") + MarkAsFailed(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) return ctrl.Result{}, err } @@ -452,9 +470,3 @@ func makeRequestsForConfigurations(ll ...v1alpha1.Configuration) []reconcile.Req return requests } - -func (r *ConfigurationReconciler) markAsFailed(obj *v1alpha1.Configuration, reason, msg, format string, messageArgs ...any) { - rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, format, messageArgs...) - conditions.MarkFalse(obj, meta.ReadyCondition, reason, msg) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, msg, nil) -} diff --git a/controllers/localization_controller.go b/controllers/localization_controller.go index 127fe130..99f5650b 100644 --- a/controllers/localization_controller.go +++ b/controllers/localization_controller.go @@ -10,7 +10,6 @@ import ( "fmt" "time" - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" @@ -30,14 +29,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" "github.com/open-component-model/ocm-controller/api/v1alpha1" "github.com/open-component-model/ocm-controller/pkg/cache" - "github.com/open-component-model/ocm-controller/pkg/event" "github.com/open-component-model/ocm-controller/pkg/ocm" "github.com/open-component-model/ocm-controller/pkg/snapshot" ) @@ -136,12 +133,9 @@ func (r *LocalizationReconciler) Reconcile( ctx context.Context, req ctrl.Request, ) (result ctrl.Result, err error) { - logger := log.FromContext(ctx).WithName("localization-controller") - obj := &v1alpha1.Localization{} if err = r.Client.Get(ctx, req.NamespacedName, obj); err != nil { if apierrors.IsNotFound(err) { - logger.V(4).Info("localization object has been deleted, skipping reconcile") return ctrl.Result{}, nil } @@ -150,115 +144,61 @@ func (r *LocalizationReconciler) Reconcile( // return early if obj is suspended if obj.Spec.Suspend { - logger.Info("localization object suspended") return result, nil } - var patchHelper *patch.Helper - patchHelper, err = patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to create patch helper: %w", err) - } + patchHelper := patch.NewSerialPatcher(obj, r.Client) // Always attempt to patch the object and status after each reconciliation. defer func() { - // Patching has not been set up, or the controller errored earlier. - if patchHelper == nil { - return - } - - if condition := conditions.Get(obj, meta.StalledCondition); condition != nil && - condition.Status == metav1.ConditionTrue { - conditions.Delete(obj, meta.ReconcilingCondition) - } - - // Check if it's a successful reconciliation. - // We don't set Requeue in case of error, so we can safely check for Requeue. - if result.RequeueAfter == obj.GetRequeueAfter() && !result.Requeue && err == nil { - // Remove the reconciling condition if it's set. - conditions.Delete(obj, meta.ReconcilingCondition) - - // Set the return err as the ready failure message is the resource is not ready, but also not reconciling or stalled. - if ready := conditions.Get(obj, meta.ReadyCondition); ready != nil && - ready.Status == metav1.ConditionFalse && - !conditions.IsStalled(obj) { - err = errors.New(conditions.GetMessage(obj, meta.ReadyCondition)) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) - } - } - - // If still reconciling then reconciliation did not succeed, set to ProgressingWithRetry to - // indicate that reconciliation will be retried. - if conditions.IsReconciling(obj) { - reconciling := conditions.Get(obj, meta.ReconcilingCondition) - reconciling.Reason = meta.ProgressingWithRetryReason - conditions.Set(obj, reconciling) - } - - // If not reconciling or stalled than mark Ready=True - if !conditions.IsReconciling(obj) && !conditions.IsStalled(obj) && - err == nil && result.RequeueAfter == obj.GetRequeueAfter() { - conditions.MarkTrue( - obj, - meta.ReadyCondition, - meta.SucceededReason, - "Reconciliation success", - ) - event.New( - r.EventRecorder, - obj, - eventv1.EventSeverityInfo, - "Reconciliation succeeded", - nil, - ) - } - - // Set status observed generation option if the object is stalled or ready. - if conditions.IsStalled(obj) || conditions.IsReady(obj) { - obj.Status.ObservedGeneration = obj.Generation - event.New( - r.EventRecorder, - obj, - eventv1.EventSeverityInfo, - fmt.Sprintf("Reconciliation finished, next run in %s", obj.GetRequeueAfter()), - map[string]string{ - v1alpha1.GroupVersion.Group + "/localization_digest": obj.Status.LatestSnapshotDigest, - }, - ) - } - - if perr := patchHelper.Patch(ctx, obj); perr != nil { - err = errors.Join(err, perr) + if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + err = errors.Join(err, derr) } }() + // Starts the progression by setting ReconcilingCondition. + // This will be checked in defer. + // Should only be deleted on a success. + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress for localization: %s", obj.Name) + // check dependencies are ready ready, err := r.checkReadiness(ctx, obj.GetNamespace(), &obj.Spec.SourceRef) if err != nil { - logger.Info("source ref object is not ready with error", "error", err) + MarkAsFailed(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - logger.Info( - "source ref object is not ready", - "source", - obj.Spec.SourceRef.GetNamespacedName(), + MarkAsFailed( + r.EventRecorder, + obj, + "SourceRefNotReady", + fmt.Sprintf("source ref not yet ready: %s", obj.Spec.SourceRef.Name), ) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if obj.Spec.ConfigRef != nil { ready, err := r.checkReadiness(ctx, obj.GetNamespace(), obj.Spec.ConfigRef) if err != nil { - logger.Info("config ref object is not ready with error", "error", err) + MarkAsFailed( + r.EventRecorder, + obj, + "ConfigRefNotReadyWithError", + fmt.Sprintf("config ref not yet ready with error: %s: %s", obj.Spec.ConfigRef.Name, err), + ) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - logger.Info( - "config ref object is not ready", - "source", - obj.Spec.SourceRef.GetNamespacedName(), + MarkAsFailed( + r.EventRecorder, + obj, + "ConfigRefNotReady", + fmt.Sprintf("config ref not yet ready: %s", obj.Spec.ConfigRef.Name), ) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } } @@ -266,17 +206,24 @@ func (r *LocalizationReconciler) Reconcile( if obj.Spec.PatchStrategicMerge != nil { ready, err := r.checkFluxSourceReadiness(ctx, obj.Spec.PatchStrategicMerge.Source.SourceRef) if err != nil { - logger.Info("source object is not ready with error", "error", err) + MarkAsFailed( + r.EventRecorder, + obj, + "PatchStrategicMergeSourceRefNotReadyWithError", + fmt.Sprintf("patch strategic merge source ref not yet ready with error: %s: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name, err), + ) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - ref := obj.Spec.PatchStrategicMerge.Source.SourceRef - logger.Info( - "patch git repository object is not ready", - "gitrepository", - (types.NamespacedName{Namespace: ref.Namespace, Name: ref.Name}).String(), + MarkAsFailed( + r.EventRecorder, + obj, + "PatchStrategicMergeSourceRefNotReady", + fmt.Sprintf("patch strategic merge source ref not yet ready: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name), ) + return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } } @@ -286,14 +233,16 @@ func (r *LocalizationReconciler) Reconcile( if obj.GetSnapshotName() == "" { name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to get snapshot name: %w", err) + err = fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + + return ctrl.Result{}, err } + obj.Status.SnapshotName = name return ctrl.Result{Requeue: true}, nil } - logger.Info("reconciling localization") - return r.reconcile(ctx, obj) } @@ -301,9 +250,6 @@ func (r *LocalizationReconciler) reconcile( ctx context.Context, obj *v1alpha1.Localization, ) (ctrl.Result, error) { - logger := log.FromContext(ctx) - rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") - if obj.Generation != obj.Status.ObservedGeneration { rreconcile.ProgressiveStatus( false, @@ -317,40 +263,30 @@ func (r *LocalizationReconciler) reconcile( if err := r.MutationReconciler.ReconcileMutationObject(ctx, obj); err != nil { if apierrors.IsNotFound(err) { - logger.Error(err, "mutation object is not found, skipping reconcile loop") return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if errors.Is(err, errTar) { err = fmt.Errorf("source resource is not a tar archive: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.SourceReasonNotATarArchiveReason, - err.Error(), - ) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) + return ctrl.Result{}, err } err = fmt.Errorf("failed to reconcile mutation object: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.ReconcileMutationObjectFailedReason, - err.Error(), - ) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) return ctrl.Result{}, err } - logger.V(4).Info("finished updating localization object") obj.Status.ObservedGeneration = obj.GetGeneration() - // Remove any stale Ready condition, most likely False, set above. Its value - // is derived from the overall result of the reconciliation in the deferred - // block at the very end. - conditions.Delete(obj, meta.ReadyCondition) + conditions.MarkTrue(obj, + meta.ReadyCondition, + meta.SucceededReason, + "Reconciliation success") + + conditions.Delete(obj, meta.ReconcilingCondition) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } diff --git a/controllers/mutate_condition_status.go b/controllers/mutate_condition_status.go new file mode 100644 index 00000000..7c2f340b --- /dev/null +++ b/controllers/mutate_condition_status.go @@ -0,0 +1,16 @@ +package controllers + +import ( + eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" + "github.com/open-component-model/ocm-controller/pkg/event" + kuberecorder "k8s.io/client-go/tools/record" +) + +// MarkAsFailed sets the condition and progressive status of an Object to the set reason, msg, and format for the +// progressive status. +func MarkAsFailed(recorder kuberecorder.EventRecorder, obj conditions.Setter, reason, msg string) { + conditions.MarkFalse(obj, meta.ReadyCondition, reason, msg) + event.New(recorder, obj, eventv1.EventSeverityError, msg, nil) +} diff --git a/controllers/resource_controller.go b/controllers/resource_controller.go index e233a55c..fa2e0ff5 100644 --- a/controllers/resource_controller.go +++ b/controllers/resource_controller.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" @@ -17,7 +16,6 @@ import ( "github.com/open-component-model/ocm-controller/api/v1alpha1" "github.com/open-component-model/ocm-controller/pkg/cache" "github.com/open-component-model/ocm-controller/pkg/component" - "github.com/open-component-model/ocm-controller/pkg/event" "github.com/open-component-model/ocm-controller/pkg/ocm" "github.com/open-component-model/ocm-controller/pkg/snapshot" ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1" @@ -32,7 +30,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" @@ -86,8 +83,6 @@ func (r *ResourceReconciler) Reconcile( ctx context.Context, req ctrl.Request, ) (result ctrl.Result, err error) { - logger := log.FromContext(ctx).WithName("resource-controller") - obj := &v1alpha1.Resource{} if err = r.Client.Get(ctx, req.NamespacedName, obj); err != nil { if apierrors.IsNotFound(err) { @@ -98,94 +93,34 @@ func (r *ResourceReconciler) Reconcile( } if obj.Spec.Suspend { - logger.Info("resource object suspended") return result, nil } - var patchHelper *patch.Helper - patchHelper, err = patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to create patch helper: %w", err) - } + patchHelper := patch.NewSerialPatcher(obj, r.Client) // Always attempt to patch the object and status after each reconciliation. defer func() { - // Patching has not been set up, or the controller errored earlier. - if patchHelper == nil { - return - } - - if condition := conditions.Get(obj, meta.StalledCondition); condition != nil && - condition.Status == metav1.ConditionTrue { - conditions.Delete(obj, meta.ReconcilingCondition) - } - - // Check if it's a successful reconciliation. - // We don't set Requeue in case of error, so we can safely check for Requeue. - if result.RequeueAfter == obj.GetRequeueAfter() && !result.Requeue && err == nil { - // Remove the reconciling condition if it's set. - conditions.Delete(obj, meta.ReconcilingCondition) - - // Set the return err as the ready failure message is the resource is not ready, but also not reconciling or stalled. - if ready := conditions.Get(obj, meta.ReadyCondition); ready != nil && - ready.Status == metav1.ConditionFalse && - !conditions.IsStalled(obj) { - err = errors.New(conditions.GetMessage(obj, meta.ReadyCondition)) - } - } - - // If still reconciling then reconciliation did not succeed, set to ProgressingWithRetry to - // indicate that reconciliation will be retried. - if conditions.IsReconciling(obj) { - reconciling := conditions.Get(obj, meta.ReconcilingCondition) - reconciling.Reason = meta.ProgressingWithRetryReason - conditions.Set(obj, reconciling) - } - - // If not reconciling or stalled than mark Ready=True - if !conditions.IsReconciling(obj) && !conditions.IsStalled(obj) && - err == nil && result.RequeueAfter == obj.GetRequeueAfter() { - conditions.MarkTrue( - obj, - meta.ReadyCondition, - meta.SucceededReason, - "Reconciliation success", - ) - event.New( - r.EventRecorder, - obj, - eventv1.EventSeverityInfo, - "Reconciliation success", - nil, - ) - } - - // Set status observed generation option if the object is stalled or ready. - if conditions.IsStalled(obj) || conditions.IsReady(obj) { - obj.Status.ObservedGeneration = obj.Generation - event.New( - r.EventRecorder, - obj, - eventv1.EventSeverityInfo, - fmt.Sprintf("Reconciliation finished, next run in %s", obj.GetRequeueAfter()), - map[string]string{ - v1alpha1.GroupVersion.Group + "/resource_version": obj.Status.LastAppliedResourceVersion, - }, - ) - } - - if perr := patchHelper.Patch(ctx, obj); perr != nil { - err = errors.Join(err, perr) + if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + err = errors.Join(err, derr) } }() + // Starts the progression by setting ReconcilingCondition. + // This will be checked in defer. + // Should only be deleted on a success. + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress for resource: %s", obj.Name) + // if the snapshot name has not been generated then // generate, patch the status and requeue if obj.GetSnapshotName() == "" { name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { + err = fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + return ctrl.Result{}, err } + obj.Status.SnapshotName = name return ctrl.Result{Requeue: true}, nil } @@ -197,10 +132,6 @@ func (r *ResourceReconciler) reconcile( ctx context.Context, obj *v1alpha1.Resource, ) (ctrl.Result, error) { - logger := log.FromContext(ctx).WithName("resource-controller") - - rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress") - if obj.Generation != obj.Status.ObservedGeneration { rreconcile.ProgressiveStatus( false, @@ -216,60 +147,39 @@ func (r *ResourceReconciler) reconcile( obj.Spec.SourceRef.Namespace = obj.GetNamespace() } - conditions.Delete(obj, meta.StalledCondition) - var componentVersion v1alpha1.ComponentVersion if err := r.Get(ctx, obj.Spec.SourceRef.GetObjectKey(), &componentVersion); err != nil { if apierrors.IsNotFound(err) { - logger.Info("component version not found yet, waiting for it to exist...") - return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } err = fmt.Errorf("failed to get component version: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.GetResourceFailedReason, - err.Error(), - ) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.ComponentVersionNotFoundReason, err.Error()) return ctrl.Result{}, err } if !conditions.IsReady(&componentVersion) { - logger.Info("waiting for component version to be ready...") + MarkAsFailed(r.EventRecorder, obj, v1alpha1.ComponentVersionNotReadyReason, "component version not ready yet") return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "component version %s ready, processing ocm resource", componentVersion.Name) + octx, err := r.OCMClient.CreateAuthenticatedOCMContext(ctx, &componentVersion) if err != nil { err = fmt.Errorf("failed to create authenticated client: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.AuthenticatedContextCreationFailedReason, - err.Error(), - ) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error()) + + return ctrl.Result{}, err } - reader, digest, err := r.OCMClient.GetResource( - ctx, - octx, - &componentVersion, - obj.Spec.SourceRef.ResourceRef, - ) + reader, digest, err := r.OCMClient.GetResource(ctx, octx, &componentVersion, obj.Spec.SourceRef.ResourceRef) if err != nil { err = fmt.Errorf("failed to get resource: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.GetResourceFailedReason, - err.Error(), - ) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.GetResourceFailedReason, err.Error()) + return ctrl.Result{}, err } defer reader.Close() @@ -281,21 +191,11 @@ func (r *ResourceReconciler) reconcile( // This is important because THIS is the actual component for our resource. If we used ComponentVersion in the // below identity, that would be the top-level component instead of the component that this resource belongs to. - componentDescriptor, err := component.GetComponentDescriptor( - ctx, - r.Client, - obj.GetReferencePath(), - componentVersion.Status.ComponentDescriptor, - ) + componentDescriptor, err := component.GetComponentDescriptor(ctx, r.Client, obj.GetReferencePath(), componentVersion.Status.ComponentDescriptor) if err != nil { err = fmt.Errorf("failed to get component descriptor for resource: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.GetComponentDescriptorFailedReason, - err.Error(), - ) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.GetComponentDescriptorFailedReason, err.Error()) + return ctrl.Result{}, err } @@ -304,20 +204,19 @@ func (r *ResourceReconciler) reconcile( "couldn't find component descriptor for reference '%s' or any root components", obj.GetReferencePath(), ) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.ComponentDescriptorNotFoundReason, - err.Error(), - ) - // Mark stalled because we can't do anything until the component descriptor is available. Likely requires some sort of manual intervention. - conditions.MarkStalled(obj, v1alpha1.ComponentDescriptorNotFoundReason, err.Error()) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.ComponentDescriptorNotFoundReason, err.Error()) - return ctrl.Result{}, nil + return ctrl.Result{}, err } - conditions.Delete(obj, meta.StalledCondition) + if obj.GetSnapshotName() == "" { + err := errors.New("snapshot name should not be empty") + MarkAsFailed(r.EventRecorder, obj, "SnapshotNameEmpty", err.Error()) + + return ctrl.Result{}, err + } + + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "resource retrieve, constructing snapshot with name %s", obj.GetSnapshotName()) identity := ocmmetav1.Identity{ v1alpha1.ComponentNameKey: componentDescriptor.Name, @@ -329,10 +228,6 @@ func (r *ResourceReconciler) reconcile( identity[k] = v } - if obj.GetSnapshotName() == "" { - return ctrl.Result{}, fmt.Errorf("snapshot name should not be empty") - } - snapshotCR := &v1alpha1.Snapshot{ ObjectMeta: metav1.ObjectMeta{ Namespace: obj.GetNamespace(), @@ -355,28 +250,21 @@ func (r *ResourceReconciler) reconcile( }) if err != nil { err = fmt.Errorf("failed to create or update snapshot: %w", err) - conditions.MarkFalse( - obj, - meta.ReadyCondition, - v1alpha1.CreateOrUpdateSnapshotFailedReason, - err.Error(), - ) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.CreateOrUpdateSnapshotFailedReason, err.Error()) + return ctrl.Result{}, err } - logger.Info("successfully pushed snapshot for resource", "resource", obj.Spec.SourceRef.Name) - obj.Status.LastAppliedResourceVersion = obj.Spec.SourceRef.GetVersion() obj.Status.ObservedGeneration = obj.GetGeneration() obj.Status.LastAppliedComponentVersion = componentVersion.Status.ReconciledVersion - logger.Info("successfully reconciled resource", "name", obj.GetName()) + conditions.MarkTrue(obj, + meta.ReadyCondition, + meta.SucceededReason, + "Applied version: %s", obj.Status.LastAppliedComponentVersion) - // Remove any stale Ready condition, most likely False, set above. Its value - // is derived from the overall result of the reconciliation in the deferred - // block at the very end. - conditions.Delete(obj, meta.ReadyCondition) + conditions.Delete(obj, meta.ReconcilingCondition) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } @@ -394,9 +282,9 @@ func (r *ResourceReconciler) findObjects(key string) handler.MapFunc { requests := make([]reconcile.Request, len(resources.Items)) for i, item := range resources.Items { - // if the observedgeneration is -1 - // then the object has not been initialised yet - // so we should not trigger a reconcilation for sourceRef/configRefs + // if the observed generation is -1 + // then the object has not been initialised yet, + // so we should not trigger a reconciliation for sourceRef/configRefs if item.Status.ObservedGeneration != -1 { requests[i] = reconcile.Request{ NamespacedName: types.NamespacedName{ diff --git a/controllers/snapshot_controller.go b/controllers/snapshot_controller.go index 53fe76cc..424bba30 100644 --- a/controllers/snapshot_controller.go +++ b/controllers/snapshot_controller.go @@ -10,26 +10,23 @@ import ( "fmt" "net/http" - eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" + rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/google/go-containerregistry/pkg/v1/remote/transport" "github.com/open-component-model/ocm-controller/pkg/cache" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" kuberecorder "k8s.io/client-go/tools/record" - "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "github.com/open-component-model/ocm-controller/api/v1alpha1" deliveryv1alpha1 "github.com/open-component-model/ocm-controller/api/v1alpha1" - "github.com/open-component-model/ocm-controller/pkg/event" "github.com/open-component-model/ocm-controller/pkg/ocm" ) @@ -64,10 +61,6 @@ func (r *SnapshotReconciler) SetupWithManager(mgr ctrl.Manager) error { // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. func (r *SnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, err error) { - log := log.FromContext(ctx).WithName("snapshot-reconcile") - - log.Info("reconciling snapshot") - obj := &v1alpha1.Snapshot{} if err = r.Client.Get(ctx, req.NamespacedName, obj); err != nil { if apierrors.IsNotFound(err) { @@ -90,52 +83,41 @@ func (r *SnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r } if obj.Spec.Suspend { - log.Info("snapshot object suspended") return ctrl.Result{}, nil } - var patchHelper *patch.Helper - patchHelper, err = patch.NewHelper(obj, r.Client) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to create patch helper: %w", err) - } + patchHelper := patch.NewSerialPatcher(obj, r.Client) // AddFinalizer is not present already. controllerutil.AddFinalizer(obj, snapshotFinalizer) // Always attempt to patch the object and status after each reconciliation. defer func() { - // Patching has not been set up, or the controller errored earlier. - if patchHelper == nil { - return - } - - // Set status observed generation option if the object is stalled or ready. - if conditions.IsStalled(obj) || conditions.IsReady(obj) { - obj.Status.ObservedGeneration = obj.Generation - event.New(r.EventRecorder, obj, eventv1.EventSeverityInfo, "Reconciliation finished", - map[string]string{v1alpha1.GroupVersion.Group + "/snapshot_digest": obj.Status.LastReconciledDigest}) - } - - if perr := patchHelper.Patch(ctx, obj); perr != nil { - err = errors.Join(err, perr) + if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, 0); derr != nil { + err = errors.Join(err, derr) } }() + // Starts the progression by setting ReconcilingCondition. + // This will be checked in defer. + // Should only be deleted on a success. + rreconcile.ProgressiveStatus(false, obj, meta.ProgressingReason, "reconciliation in progress for snapshot: %s", obj.Name) + name, err := ocm.ConstructRepositoryName(obj.Spec.Identity) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, v1alpha1.CreateRepositoryNameReason, err.Error()) - event.New(r.EventRecorder, obj, eventv1.EventSeverityError, err.Error(), nil) + err = fmt.Errorf("failed to construct name: %w", err) + MarkAsFailed(r.EventRecorder, obj, v1alpha1.CreateRepositoryNameReason, err.Error()) - return ctrl.Result{}, fmt.Errorf("failed to construct name: %w", err) + return ctrl.Result{}, err } obj.Status.LastReconciledDigest = obj.Spec.Digest obj.Status.LastReconciledTag = obj.Spec.Tag obj.Status.RepositoryURL = fmt.Sprintf("%s://%s/%s", scheme, r.RegistryServiceName, name) + msg := fmt.Sprintf("Snapshot with name '%s' is ready", obj.Name) conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, msg) - log.Info("snapshot successfully reconciled", "snapshot", klog.KObj(obj)) + conditions.Delete(obj, meta.ReconcilingCondition) return ctrl.Result{}, nil } diff --git a/go.mod b/go.mod index f7041273..7549ba0a 100644 --- a/go.mod +++ b/go.mod @@ -365,7 +365,7 @@ require ( k8s.io/api v0.28.1 k8s.io/apiextensions-apiserver v0.28.0 k8s.io/component-base v0.28.1 // indirect - k8s.io/klog/v2 v2.100.1 + k8s.io/klog/v2 v2.100.1 // indirect k8s.io/kube-openapi v0.0.0-20230717233707-2695361300d9 // indirect k8s.io/utils v0.0.0-20230505201702-9f6742963106 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect diff --git a/pkg/ocm/ocm.go b/pkg/ocm/ocm.go index cd480da8..27c30b0e 100644 --- a/pkg/ocm/ocm.go +++ b/pkg/ocm/ocm.go @@ -42,38 +42,12 @@ const dockerConfigKey = ".dockerconfigjson" // Contract defines a subset of capabilities from the OCM library. type Contract interface { - CreateAuthenticatedOCMContext( - ctx context.Context, - obj *v1alpha1.ComponentVersion, - ) (ocm.Context, error) - GetResource( - ctx context.Context, - octx ocm.Context, - cv *v1alpha1.ComponentVersion, - resource *v1alpha1.ResourceReference, - ) (io.ReadCloser, string, error) - GetComponentVersion( - ctx context.Context, - octx ocm.Context, - obj *v1alpha1.ComponentVersion, - name, version string, - ) (ocm.ComponentVersionAccess, error) - GetLatestValidComponentVersion( - ctx context.Context, - octx ocm.Context, - obj *v1alpha1.ComponentVersion, - ) (string, error) - ListComponentVersions( - logger logr.Logger, - octx ocm.Context, - obj *v1alpha1.ComponentVersion, - ) ([]Version, error) - VerifyComponent( - ctx context.Context, - octx ocm.Context, - obj *v1alpha1.ComponentVersion, - version string, - ) (bool, error) + CreateAuthenticatedOCMContext(ctx context.Context, obj *v1alpha1.ComponentVersion) (ocm.Context, error) + GetResource(ctx context.Context, octx ocm.Context, cv *v1alpha1.ComponentVersion, resource *v1alpha1.ResourceReference) (io.ReadCloser, string, error) + GetComponentVersion(ctx context.Context, octx ocm.Context, obj *v1alpha1.ComponentVersion, name, version string) (ocm.ComponentVersionAccess, error) + GetLatestValidComponentVersion(ctx context.Context, octx ocm.Context, obj *v1alpha1.ComponentVersion) (string, error) + ListComponentVersions(logger logr.Logger, octx ocm.Context, obj *v1alpha1.ComponentVersion) ([]Version, error) + VerifyComponent(ctx context.Context, octx ocm.Context, obj *v1alpha1.ComponentVersion, version string) (bool, error) } // Client implements the OCM fetcher interface. @@ -92,10 +66,7 @@ func NewClient(client client.Client, cache cache.Cache) *Client { } } -func (c *Client) CreateAuthenticatedOCMContext( - ctx context.Context, - obj *v1alpha1.ComponentVersion, -) (ocm.Context, error) { +func (c *Client) CreateAuthenticatedOCMContext(ctx context.Context, obj *v1alpha1.ComponentVersion) (ocm.Context, error) { octx := ocm.New() if obj.Spec.ServiceAccountName != "" { @@ -193,12 +164,7 @@ func (c *Client) GetResource( version = resource.ElementMeta.Version } - cd, err := component.GetComponentDescriptor( - ctx, - c.client, - resource.ReferencePath, - cv.Status.ComponentDescriptor, - ) + cd, err := component.GetComponentDescriptor(ctx, c.client, resource.ReferencePath, cv.Status.ComponentDescriptor) if err != nil { return nil, "", fmt.Errorf("failed to find component descriptor for reference: %w", err) } From ef51cbb8209657621d12f27ace724ec8fc39d60c Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Mon, 6 Nov 2023 08:09:17 +0100 Subject: [PATCH 4/8] remove superflous object generation set --- controllers/componentversion_controller.go | 1 - controllers/configuration_controller.go | 2 -- controllers/localization_controller.go | 2 -- controllers/resource_controller.go | 1 - 4 files changed, 6 deletions(-) diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index 8586c034..9af4b344 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -287,7 +287,6 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con obj.Status.ComponentDescriptor = componentDescriptor obj.Status.ReconciledVersion = version - obj.Status.ObservedGeneration = obj.Generation conditions.MarkTrue(obj, meta.ReadyCondition, diff --git a/controllers/configuration_controller.go b/controllers/configuration_controller.go index 1e7c0e2c..b44a385d 100644 --- a/controllers/configuration_controller.go +++ b/controllers/configuration_controller.go @@ -298,8 +298,6 @@ func (r *ConfigurationReconciler) reconcile( return ctrl.Result{}, err } - obj.Status.ObservedGeneration = obj.GetGeneration() - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, diff --git a/controllers/localization_controller.go b/controllers/localization_controller.go index 99f5650b..5d94b4c0 100644 --- a/controllers/localization_controller.go +++ b/controllers/localization_controller.go @@ -279,8 +279,6 @@ func (r *LocalizationReconciler) reconcile( return ctrl.Result{}, err } - obj.Status.ObservedGeneration = obj.GetGeneration() - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, diff --git a/controllers/resource_controller.go b/controllers/resource_controller.go index fa2e0ff5..b9dc2d69 100644 --- a/controllers/resource_controller.go +++ b/controllers/resource_controller.go @@ -256,7 +256,6 @@ func (r *ResourceReconciler) reconcile( } obj.Status.LastAppliedResourceVersion = obj.Spec.SourceRef.GetVersion() - obj.Status.ObservedGeneration = obj.GetGeneration() obj.Status.LastAppliedComponentVersion = componentVersion.Status.ReconciledVersion conditions.MarkTrue(obj, From f83ea9802ea797173b8d89e8102da1fef5da1950 Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Mon, 6 Nov 2023 10:28:00 +0100 Subject: [PATCH 5/8] fix the e2e test and rename some functions --- controllers/componentversion_controller.go | 20 +++++++++--------- controllers/configuration_controller.go | 21 ++++++++++--------- controllers/fluxdeployer_controller.go | 2 +- controllers/localization_controller.go | 21 ++++++++++--------- controllers/mutate_condition_status.go | 4 ++-- controllers/register_status_defer.go | 4 ++-- controllers/resource_controller.go | 20 +++++++++--------- controllers/snapshot_controller.go | 4 ++-- .../component_version.yaml | 2 +- .../testHelmChartResource/deployer.yaml | 2 +- .../testHelmChartResource/resource.yaml | 2 +- 11 files changed, 52 insertions(+), 50 deletions(-) diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index 9af4b344..d87ac971 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -87,7 +87,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { retErr = errors.Join(retErr, derr) } }() @@ -101,7 +101,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req if err != nil { // we don't fail here, because all manifests might have been applied at once or the secret // for authentication is being reconciled. - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, @@ -117,7 +117,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req update, version, err := r.checkVersion(ctx, octx, obj) if err != nil { // The component might not be there yet. We don't fail but keep polling instead. - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.CheckVersionFailedReason, @@ -146,7 +146,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req ok, err := r.OCMClient.VerifyComponent(ctx, octx, obj, version) if err != nil { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.VerificationFailedReason, @@ -159,7 +159,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req } if !ok { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.VerificationFailedReason, @@ -183,7 +183,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con cv, err := r.OCMClient.GetComponentVersion(ctx, octx, obj, obj.Spec.Component, version) if err != nil { err = fmt.Errorf("failed to get component version: %w", err) - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.ComponentVersionInvalidReason, @@ -199,7 +199,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con cd, err := dv.ConvertFrom(cv.GetDescriptor()) if err != nil { err = fmt.Errorf("failed to convert component descriptor: %w", err) - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.ConvertComponentDescriptorFailedReason, @@ -214,7 +214,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con componentName, err := component.ConstructUniqueName(cd.GetName(), cd.GetVersion(), nil) if err != nil { err = fmt.Errorf("failed to generate name: %w", err) - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, @@ -251,7 +251,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con if err != nil { err = fmt.Errorf("failed to create or update component descriptor: %w", err) - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.CreateOrUpdateComponentDescriptorFailedReason, @@ -275,7 +275,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con componentDescriptor.References, err = r.parseReferences(ctx, octx, obj, cv.GetDescriptor().References) if err != nil { err = fmt.Errorf("failed to parse references: %w", err) - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, v1alpha1.ParseReferencesFailedReason, diff --git a/controllers/configuration_controller.go b/controllers/configuration_controller.go index b44a385d..13ce4165 100644 --- a/controllers/configuration_controller.go +++ b/controllers/configuration_controller.go @@ -168,7 +168,7 @@ func (r *ConfigurationReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -181,12 +181,13 @@ func (r *ConfigurationReconciler) Reconcile( // check dependencies are ready ready, err := r.checkReadiness(ctx, obj.GetNamespace(), &obj.Spec.SourceRef) if err != nil { - MarkAsFailed(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) + MarkNotReady(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } + if !ready { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "SourceRefNotReady", @@ -199,7 +200,7 @@ func (r *ConfigurationReconciler) Reconcile( if obj.Spec.ConfigRef != nil { ready, err := r.checkReadiness(ctx, obj.GetNamespace(), obj.Spec.ConfigRef) if err != nil { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "ConfigRefNotReadyWithError", @@ -209,7 +210,7 @@ func (r *ConfigurationReconciler) Reconcile( return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "ConfigRefNotReady", @@ -223,7 +224,7 @@ func (r *ConfigurationReconciler) Reconcile( if obj.Spec.PatchStrategicMerge != nil { ready, err := r.checkFluxSourceReadiness(ctx, obj.Spec.PatchStrategicMerge.Source.SourceRef) if err != nil { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "PatchStrategicMergeSourceRefNotReadyWithError", @@ -234,7 +235,7 @@ func (r *ConfigurationReconciler) Reconcile( } if !ready { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "PatchStrategicMergeSourceRefNotReady", @@ -251,7 +252,7 @@ func (r *ConfigurationReconciler) Reconcile( name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { err := fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -287,13 +288,13 @@ func (r *ConfigurationReconciler) reconcile( if errors.Is(err, errTar) { err = fmt.Errorf("source resource is not a tar archive: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) return ctrl.Result{}, err } err = fmt.Errorf("failed to reconcile mutation object: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) return ctrl.Result{}, err } diff --git a/controllers/fluxdeployer_controller.go b/controllers/fluxdeployer_controller.go index adab8259..015326e5 100644 --- a/controllers/fluxdeployer_controller.go +++ b/controllers/fluxdeployer_controller.go @@ -143,7 +143,7 @@ func (r *FluxDeployerReconciler) reconcile( // get snapshot snapshot, err := r.getSnapshot(ctx, obj) if err != nil { - logger.Info("could not find source ref", "name", obj.Spec.SourceRef.Name) + logger.Info("could not find source ref", "name", obj.Spec.SourceRef.Name, "err", err) return ctrl.Result{RequeueAfter: r.RetryInterval}, nil } diff --git a/controllers/localization_controller.go b/controllers/localization_controller.go index 5d94b4c0..c18ba329 100644 --- a/controllers/localization_controller.go +++ b/controllers/localization_controller.go @@ -151,7 +151,7 @@ func (r *LocalizationReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -164,12 +164,13 @@ func (r *LocalizationReconciler) Reconcile( // check dependencies are ready ready, err := r.checkReadiness(ctx, obj.GetNamespace(), &obj.Spec.SourceRef) if err != nil { - MarkAsFailed(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) + MarkNotReady(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } + if !ready { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "SourceRefNotReady", @@ -182,7 +183,7 @@ func (r *LocalizationReconciler) Reconcile( if obj.Spec.ConfigRef != nil { ready, err := r.checkReadiness(ctx, obj.GetNamespace(), obj.Spec.ConfigRef) if err != nil { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "ConfigRefNotReadyWithError", @@ -192,7 +193,7 @@ func (r *LocalizationReconciler) Reconcile( return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "ConfigRefNotReady", @@ -206,7 +207,7 @@ func (r *LocalizationReconciler) Reconcile( if obj.Spec.PatchStrategicMerge != nil { ready, err := r.checkFluxSourceReadiness(ctx, obj.Spec.PatchStrategicMerge.Source.SourceRef) if err != nil { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "PatchStrategicMergeSourceRefNotReadyWithError", @@ -217,7 +218,7 @@ func (r *LocalizationReconciler) Reconcile( } if !ready { - MarkAsFailed( + MarkNotReady( r.EventRecorder, obj, "PatchStrategicMergeSourceRefNotReady", @@ -234,7 +235,7 @@ func (r *LocalizationReconciler) Reconcile( name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { err = fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -268,13 +269,13 @@ func (r *LocalizationReconciler) reconcile( if errors.Is(err, errTar) { err = fmt.Errorf("source resource is not a tar archive: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) return ctrl.Result{}, err } err = fmt.Errorf("failed to reconcile mutation object: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) return ctrl.Result{}, err } diff --git a/controllers/mutate_condition_status.go b/controllers/mutate_condition_status.go index 7c2f340b..108da4f2 100644 --- a/controllers/mutate_condition_status.go +++ b/controllers/mutate_condition_status.go @@ -8,9 +8,9 @@ import ( kuberecorder "k8s.io/client-go/tools/record" ) -// MarkAsFailed sets the condition and progressive status of an Object to the set reason, msg, and format for the +// MarkNotReady sets the condition and progressive status of an Object to the set reason, msg, and format for the // progressive status. -func MarkAsFailed(recorder kuberecorder.EventRecorder, obj conditions.Setter, reason, msg string) { +func MarkNotReady(recorder kuberecorder.EventRecorder, obj conditions.Setter, reason, msg string) { conditions.MarkFalse(obj, meta.ReadyCondition, reason, msg) event.New(recorder, obj, eventv1.EventSeverityError, msg, nil) } diff --git a/controllers/register_status_defer.go b/controllers/register_status_defer.go index d9aa9814..714a3703 100644 --- a/controllers/register_status_defer.go +++ b/controllers/register_status_defer.go @@ -13,8 +13,8 @@ import ( kuberecorder "k8s.io/client-go/tools/record" ) -// DeferredStatusUpdate takes an object which can identify itself and updates its status including ObservedGeneration. -func DeferredStatusUpdate( +// UpdateStatus takes an object which can identify itself and updates its status including ObservedGeneration. +func UpdateStatus( ctx context.Context, patchHelper *patch.SerialPatcher, obj IdentifiableClientObject, diff --git a/controllers/resource_controller.go b/controllers/resource_controller.go index b9dc2d69..8fae751e 100644 --- a/controllers/resource_controller.go +++ b/controllers/resource_controller.go @@ -100,7 +100,7 @@ func (r *ResourceReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -116,7 +116,7 @@ func (r *ResourceReconciler) Reconcile( name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { err = fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -154,13 +154,13 @@ func (r *ResourceReconciler) reconcile( } err = fmt.Errorf("failed to get component version: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.ComponentVersionNotFoundReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentVersionNotFoundReason, err.Error()) return ctrl.Result{}, err } if !conditions.IsReady(&componentVersion) { - MarkAsFailed(r.EventRecorder, obj, v1alpha1.ComponentVersionNotReadyReason, "component version not ready yet") + MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentVersionNotReadyReason, "component version not ready yet") return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } @@ -170,7 +170,7 @@ func (r *ResourceReconciler) reconcile( octx, err := r.OCMClient.CreateAuthenticatedOCMContext(ctx, &componentVersion) if err != nil { err = fmt.Errorf("failed to create authenticated client: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -178,7 +178,7 @@ func (r *ResourceReconciler) reconcile( reader, digest, err := r.OCMClient.GetResource(ctx, octx, &componentVersion, obj.Spec.SourceRef.ResourceRef) if err != nil { err = fmt.Errorf("failed to get resource: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.GetResourceFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.GetResourceFailedReason, err.Error()) return ctrl.Result{}, err } @@ -194,7 +194,7 @@ func (r *ResourceReconciler) reconcile( componentDescriptor, err := component.GetComponentDescriptor(ctx, r.Client, obj.GetReferencePath(), componentVersion.Status.ComponentDescriptor) if err != nil { err = fmt.Errorf("failed to get component descriptor for resource: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.GetComponentDescriptorFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.GetComponentDescriptorFailedReason, err.Error()) return ctrl.Result{}, err } @@ -204,14 +204,14 @@ func (r *ResourceReconciler) reconcile( "couldn't find component descriptor for reference '%s' or any root components", obj.GetReferencePath(), ) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.ComponentDescriptorNotFoundReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentDescriptorNotFoundReason, err.Error()) return ctrl.Result{}, err } if obj.GetSnapshotName() == "" { err := errors.New("snapshot name should not be empty") - MarkAsFailed(r.EventRecorder, obj, "SnapshotNameEmpty", err.Error()) + MarkNotReady(r.EventRecorder, obj, "SnapshotNameEmpty", err.Error()) return ctrl.Result{}, err } @@ -250,7 +250,7 @@ func (r *ResourceReconciler) reconcile( }) if err != nil { err = fmt.Errorf("failed to create or update snapshot: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.CreateOrUpdateSnapshotFailedReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.CreateOrUpdateSnapshotFailedReason, err.Error()) return ctrl.Result{}, err } diff --git a/controllers/snapshot_controller.go b/controllers/snapshot_controller.go index 424bba30..527c67ae 100644 --- a/controllers/snapshot_controller.go +++ b/controllers/snapshot_controller.go @@ -93,7 +93,7 @@ func (r *SnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := DeferredStatusUpdate(ctx, patchHelper, obj, r.EventRecorder, 0); derr != nil { + if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, 0); derr != nil { err = errors.Join(err, derr) } }() @@ -106,7 +106,7 @@ func (r *SnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r name, err := ocm.ConstructRepositoryName(obj.Spec.Identity) if err != nil { err = fmt.Errorf("failed to construct name: %w", err) - MarkAsFailed(r.EventRecorder, obj, v1alpha1.CreateRepositoryNameReason, err.Error()) + MarkNotReady(r.EventRecorder, obj, v1alpha1.CreateRepositoryNameReason, err.Error()) return ctrl.Result{}, err } diff --git a/e2e/testdata/testHelmChartResource/component_version.yaml b/e2e/testdata/testHelmChartResource/component_version.yaml index a7520184..70a03e64 100644 --- a/e2e/testdata/testHelmChartResource/component_version.yaml +++ b/e2e/testdata/testHelmChartResource/component_version.yaml @@ -4,7 +4,7 @@ metadata: name: ocm-with-helm namespace: ocm-system spec: - interval: 10m0s + interval: 5s component: github.com/open-component-model/helm-test-component references: expand: true diff --git a/e2e/testdata/testHelmChartResource/deployer.yaml b/e2e/testdata/testHelmChartResource/deployer.yaml index c81a3128..f2c4a52a 100644 --- a/e2e/testdata/testHelmChartResource/deployer.yaml +++ b/e2e/testdata/testHelmChartResource/deployer.yaml @@ -4,7 +4,7 @@ metadata: name: fluxdeployer-podinfo-pipeline-backend namespace: ocm-system spec: - interval: 1m0s + interval: 5s sourceRef: kind: Resource name: ocm-with-helm-deployment diff --git a/e2e/testdata/testHelmChartResource/resource.yaml b/e2e/testdata/testHelmChartResource/resource.yaml index 78b2325d..d86b7f38 100644 --- a/e2e/testdata/testHelmChartResource/resource.yaml +++ b/e2e/testdata/testHelmChartResource/resource.yaml @@ -4,7 +4,7 @@ metadata: name: ocm-with-helm-deployment namespace: ocm-system spec: - interval: 10m + interval: 5s sourceRef: kind: ComponentVersion name: ocm-with-helm From 014c4b5ef0fc554d90f38f5890354357c441d619 Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Wed, 8 Nov 2023 15:20:32 +0100 Subject: [PATCH 6/8] adding reconciliation and stalling on missing authentication information --- api/v1alpha1/componentversion_types.go | 11 ++++++ api/v1alpha1/configuration_types.go | 11 ++++++ api/v1alpha1/localization_types.go | 11 ++++++ api/v1alpha1/resource_types.go | 11 ++++++ controllers/componentversion_controller.go | 33 ++++++++++++++--- controllers/configuration_controller.go | 4 +-- controllers/find_registry_secrets.go | 42 ++++++++++++++++++++++ controllers/localization_controller.go | 4 +-- controllers/mutate_condition_status.go | 10 ++++-- controllers/register_status_defer.go | 4 +-- controllers/resource_controller.go | 6 ++-- controllers/snapshot_controller.go | 2 +- 12 files changed, 132 insertions(+), 17 deletions(-) create mode 100644 controllers/find_registry_secrets.go diff --git a/api/v1alpha1/componentversion_types.go b/api/v1alpha1/componentversion_types.go index 642104ca..b451c2f1 100644 --- a/api/v1alpha1/componentversion_types.go +++ b/api/v1alpha1/componentversion_types.go @@ -11,6 +11,7 @@ import ( "github.com/fluxcd/pkg/apis/meta" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -189,6 +190,16 @@ func (in ComponentVersion) GetRequeueAfter() time.Duration { return in.Spec.Interval.Duration } +func (in *ComponentVersionList) List() []client.Object { + var result []client.Object + for _, o := range in.Items { + o := o + result = append(result, &o) + } + + return result +} + //+kubebuilder:object:root=true //+kubebuilder:resource:shortName=cv //+kubebuilder:subresource:status diff --git a/api/v1alpha1/configuration_types.go b/api/v1alpha1/configuration_types.go index 9dd80180..2afb39ea 100644 --- a/api/v1alpha1/configuration_types.go +++ b/api/v1alpha1/configuration_types.go @@ -8,6 +8,7 @@ import ( "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) //+kubebuilder:object:root=true @@ -76,6 +77,16 @@ func (in *Configuration) GetStatus() *MutationStatus { return &in.Status } +func (in *ConfigurationList) List() []client.Object { + var result []client.Object + for _, o := range in.Items { + o := o + result = append(result, &o) + } + + return result +} + //+kubebuilder:object:root=true // ConfigurationList contains a list of Configuration diff --git a/api/v1alpha1/localization_types.go b/api/v1alpha1/localization_types.go index 2a5a550c..b2ceca84 100644 --- a/api/v1alpha1/localization_types.go +++ b/api/v1alpha1/localization_types.go @@ -8,6 +8,7 @@ import ( "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) //+kubebuilder:object:root=true @@ -75,6 +76,16 @@ func (in *Localization) GetStatus() *MutationStatus { return &in.Status } +func (in *LocalizationList) List() []client.Object { + var result []client.Object + for _, o := range in.Items { + o := o + result = append(result, &o) + } + + return result +} + //+kubebuilder:object:root=true // LocalizationList contains a list of Localization diff --git a/api/v1alpha1/resource_types.go b/api/v1alpha1/resource_types.go index 4152cbd7..0a8bf861 100644 --- a/api/v1alpha1/resource_types.go +++ b/api/v1alpha1/resource_types.go @@ -9,6 +9,7 @@ import ( ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // ResourceSpec defines the desired state of Resource @@ -115,6 +116,16 @@ func (in Resource) GetSnapshotName() string { return in.Status.SnapshotName } +func (in *ResourceList) List() []client.Object { + var result []client.Object + for _, o := range in.Items { + o := o + result = append(result, &o) + } + + return result +} + //+kubebuilder:object:root=true // ResourceList contains a list of Resource diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index d87ac971..a2295f6f 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -15,6 +15,7 @@ import ( "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" + 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" @@ -23,8 +24,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/source" "github.com/open-component-model/ocm/pkg/contexts/ocm" ocmdesc "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" @@ -57,8 +60,30 @@ type ComponentVersionReconciler struct { // SetupWithManager sets up the controller with the Manager. func (r *ComponentVersionReconciler) SetupWithManager(mgr ctrl.Manager) error { + const ( + sourceKey = ".metadata.repository.secretRef" + ) + + if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &v1alpha1.ComponentVersion{}, sourceKey, func(rawObj client.Object) []string { + obj, ok := rawObj.(*v1alpha1.ComponentVersion) + if !ok { + return []string{} + } + if obj.Spec.Repository.SecretRef == nil { + return []string{} + } + + ns := obj.GetNamespace() + return []string{fmt.Sprintf("%s/%s", ns, obj.Spec.Repository.SecretRef.Name)} + }); err != nil { + return fmt.Errorf("failed setting index fields: %w", err) + } + return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.ComponentVersion{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + Watches( + &source.Kind{Type: &corev1.Secret{}}, + handler.EnqueueRequestsFromMapFunc(findRegistrySecrets(r.Client, sourceKey, &v1alpha1.ComponentVersionList{}))). Complete(r) } @@ -87,7 +112,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { retErr = errors.Join(retErr, derr) } }() @@ -101,16 +126,14 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req if err != nil { // we don't fail here, because all manifests might have been applied at once or the secret // for authentication is being reconciled. - MarkNotReady( + MarkAsStalled( r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, fmt.Sprintf("authentication failed for repository: %s with error: %s", obj.Spec.Repository.URL, err), ) - return ctrl.Result{ - RequeueAfter: obj.GetRequeueAfter(), - }, nil + return ctrl.Result{}, nil } // reconcile the version before calling reconcile func diff --git a/controllers/configuration_controller.go b/controllers/configuration_controller.go index 13ce4165..d936a097 100644 --- a/controllers/configuration_controller.go +++ b/controllers/configuration_controller.go @@ -168,7 +168,7 @@ func (r *ConfigurationReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -309,7 +309,7 @@ func (r *ConfigurationReconciler) reconcile( return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } -// The purpose of the findObjects function is to identify whether a given Kubernetes object +// The purpose of the findRegistrySecrets function is to identify whether a given Kubernetes object // is referenced by a Configuration. This is done by checking whether the object is a ComponentVersion // or a Snapshot. If it's a ComponentVersion, we look for all Configurations that reference // it by name. If it's a Snapshot, we first identify its owner and then look for Configurations diff --git a/controllers/find_registry_secrets.go b/controllers/find_registry_secrets.go new file mode 100644 index 00000000..0f1f0316 --- /dev/null +++ b/controllers/find_registry_secrets.go @@ -0,0 +1,42 @@ +package controllers + +import ( + "context" + + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +type Lister interface { + client.ObjectList + + List() []client.Object +} + +// findRegistrySecrets finds secrets that have a specific key for a given kind. +// Note, this is not working with ObjectList or Unstructured. ObjectList doesn't +// allow listing objects, and unstructured doesn't support fields. +func findRegistrySecrets(c client.Client, key string, list Lister) handler.MapFunc { + return func(obj client.Object) []reconcile.Request { + if err := c.List(context.Background(), list, &client.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(key, client.ObjectKeyFromObject(obj).String()), + }); err != nil { + return []reconcile.Request{} + } + + requests := make([]reconcile.Request, len(list.List())) + for i, item := range list.List() { + requests[i] = reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: item.GetName(), + Namespace: item.GetNamespace(), + }, + } + } + + return requests + } +} diff --git a/controllers/localization_controller.go b/controllers/localization_controller.go index c18ba329..0d0b1e0f 100644 --- a/controllers/localization_controller.go +++ b/controllers/localization_controller.go @@ -151,7 +151,7 @@ func (r *LocalizationReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -290,7 +290,7 @@ func (r *LocalizationReconciler) reconcile( return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } -// The purpose of the findObjects function is to identify whether a given Kubernetes object +// The purpose of the findRegistrySecrets function is to identify whether a given Kubernetes object // is referenced by a Localization. This is done by checking whether the object is a ComponentVersion // or a Snapshot. If it's a ComponentVersion, we look for all Configurations that reference // it by name. If it's a Snapshot, we first identify its owner and then look for Localization diff --git a/controllers/mutate_condition_status.go b/controllers/mutate_condition_status.go index 108da4f2..63f72274 100644 --- a/controllers/mutate_condition_status.go +++ b/controllers/mutate_condition_status.go @@ -8,9 +8,15 @@ import ( kuberecorder "k8s.io/client-go/tools/record" ) -// MarkNotReady sets the condition and progressive status of an Object to the set reason, msg, and format for the -// progressive status. +// MarkNotReady sets the condition status of an Object to Not Ready. func MarkNotReady(recorder kuberecorder.EventRecorder, obj conditions.Setter, reason, msg string) { conditions.MarkFalse(obj, meta.ReadyCondition, reason, msg) event.New(recorder, obj, eventv1.EventSeverityError, msg, nil) } + +// MarkAsStalled sets the condition status of an Object to Stalled. +func MarkAsStalled(recorder kuberecorder.EventRecorder, obj conditions.Setter, reason, msg string) { + conditions.MarkFalse(obj, meta.ReadyCondition, reason, msg) + conditions.MarkStalled(obj, reason, msg) + event.New(recorder, obj, eventv1.EventSeverityError, msg, nil) +} diff --git a/controllers/register_status_defer.go b/controllers/register_status_defer.go index 714a3703..6899ec2e 100644 --- a/controllers/register_status_defer.go +++ b/controllers/register_status_defer.go @@ -13,8 +13,8 @@ import ( kuberecorder "k8s.io/client-go/tools/record" ) -// UpdateStatus takes an object which can identify itself and updates its status including ObservedGeneration. -func UpdateStatus( +// updateStatus takes an object which can identify itself and updates its status including ObservedGeneration. +func updateStatus( ctx context.Context, patchHelper *patch.SerialPatcher, obj IdentifiableClientObject, diff --git a/controllers/resource_controller.go b/controllers/resource_controller.go index 8fae751e..31757a3e 100644 --- a/controllers/resource_controller.go +++ b/controllers/resource_controller.go @@ -100,7 +100,7 @@ func (r *ResourceReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -170,9 +170,9 @@ func (r *ResourceReconciler) reconcile( octx, err := r.OCMClient.CreateAuthenticatedOCMContext(ctx, &componentVersion) if err != nil { err = fmt.Errorf("failed to create authenticated client: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error()) + MarkAsStalled(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error()) - return ctrl.Result{}, err + return ctrl.Result{}, nil } reader, digest, err := r.OCMClient.GetResource(ctx, octx, &componentVersion, obj.Spec.SourceRef.ResourceRef) diff --git a/controllers/snapshot_controller.go b/controllers/snapshot_controller.go index 527c67ae..a7273d75 100644 --- a/controllers/snapshot_controller.go +++ b/controllers/snapshot_controller.go @@ -93,7 +93,7 @@ func (r *SnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, 0); derr != nil { + if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, 0); derr != nil { err = errors.Join(err, derr) } }() From 1301b28d2c658588c685cad16c7823b23081b0ec Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Wed, 8 Nov 2023 15:36:36 +0100 Subject: [PATCH 7/8] removed the interface as it was not needed --- api/v1alpha1/componentversion_types.go | 11 ------ api/v1alpha1/configuration_types.go | 11 ------ api/v1alpha1/localization_types.go | 11 ------ api/v1alpha1/resource_types.go | 11 ------ controllers/componentversion_controller.go | 29 ++++++++++++++- controllers/configuration_controller.go | 2 +- controllers/find_registry_secrets.go | 42 ---------------------- controllers/localization_controller.go | 2 +- 8 files changed, 30 insertions(+), 89 deletions(-) delete mode 100644 controllers/find_registry_secrets.go diff --git a/api/v1alpha1/componentversion_types.go b/api/v1alpha1/componentversion_types.go index b451c2f1..642104ca 100644 --- a/api/v1alpha1/componentversion_types.go +++ b/api/v1alpha1/componentversion_types.go @@ -11,7 +11,6 @@ import ( "github.com/fluxcd/pkg/apis/meta" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -190,16 +189,6 @@ func (in ComponentVersion) GetRequeueAfter() time.Duration { return in.Spec.Interval.Duration } -func (in *ComponentVersionList) List() []client.Object { - var result []client.Object - for _, o := range in.Items { - o := o - result = append(result, &o) - } - - return result -} - //+kubebuilder:object:root=true //+kubebuilder:resource:shortName=cv //+kubebuilder:subresource:status diff --git a/api/v1alpha1/configuration_types.go b/api/v1alpha1/configuration_types.go index 2afb39ea..9dd80180 100644 --- a/api/v1alpha1/configuration_types.go +++ b/api/v1alpha1/configuration_types.go @@ -8,7 +8,6 @@ import ( "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) //+kubebuilder:object:root=true @@ -77,16 +76,6 @@ func (in *Configuration) GetStatus() *MutationStatus { return &in.Status } -func (in *ConfigurationList) List() []client.Object { - var result []client.Object - for _, o := range in.Items { - o := o - result = append(result, &o) - } - - return result -} - //+kubebuilder:object:root=true // ConfigurationList contains a list of Configuration diff --git a/api/v1alpha1/localization_types.go b/api/v1alpha1/localization_types.go index b2ceca84..2a5a550c 100644 --- a/api/v1alpha1/localization_types.go +++ b/api/v1alpha1/localization_types.go @@ -8,7 +8,6 @@ import ( "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) //+kubebuilder:object:root=true @@ -76,16 +75,6 @@ func (in *Localization) GetStatus() *MutationStatus { return &in.Status } -func (in *LocalizationList) List() []client.Object { - var result []client.Object - for _, o := range in.Items { - o := o - result = append(result, &o) - } - - return result -} - //+kubebuilder:object:root=true // LocalizationList contains a list of Localization diff --git a/api/v1alpha1/resource_types.go b/api/v1alpha1/resource_types.go index 0a8bf861..4152cbd7 100644 --- a/api/v1alpha1/resource_types.go +++ b/api/v1alpha1/resource_types.go @@ -9,7 +9,6 @@ import ( ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) // ResourceSpec defines the desired state of Resource @@ -116,16 +115,6 @@ func (in Resource) GetSnapshotName() string { return in.Status.SnapshotName } -func (in *ResourceList) List() []client.Object { - var result []client.Object - for _, o := range in.Items { - o := o - result = append(result, &o) - } - - return result -} - //+kubebuilder:object:root=true // ResourceList contains a list of Resource diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index a2295f6f..9c8c7bd8 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -18,7 +18,9 @@ import ( 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/fields" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" kuberecorder "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -27,6 +29,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" "github.com/open-component-model/ocm/pkg/contexts/ocm" @@ -83,10 +86,34 @@ func (r *ComponentVersionReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&v1alpha1.ComponentVersion{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). Watches( &source.Kind{Type: &corev1.Secret{}}, - handler.EnqueueRequestsFromMapFunc(findRegistrySecrets(r.Client, sourceKey, &v1alpha1.ComponentVersionList{}))). + handler.EnqueueRequestsFromMapFunc(r.findObjects(sourceKey))). Complete(r) } +// findObjects finds component versions that have a key for the secret that triggered this watch event. +func (r *ComponentVersionReconciler) findObjects(key string) handler.MapFunc { + return func(obj client.Object) []reconcile.Request { + list := &v1alpha1.ComponentVersionList{} + if err := r.List(context.Background(), list, &client.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(key, client.ObjectKeyFromObject(obj).String()), + }); err != nil { + return []reconcile.Request{} + } + + requests := make([]reconcile.Request, len(list.Items)) + for i, item := range list.Items { + requests[i] = reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: item.GetName(), + Namespace: item.GetNamespace(), + }, + } + } + + return requests + } +} + // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, retErr error) { diff --git a/controllers/configuration_controller.go b/controllers/configuration_controller.go index d936a097..1a79f740 100644 --- a/controllers/configuration_controller.go +++ b/controllers/configuration_controller.go @@ -309,7 +309,7 @@ func (r *ConfigurationReconciler) reconcile( return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } -// The purpose of the findRegistrySecrets function is to identify whether a given Kubernetes object +// The purpose of the findObjects function is to identify whether a given Kubernetes object // is referenced by a Configuration. This is done by checking whether the object is a ComponentVersion // or a Snapshot. If it's a ComponentVersion, we look for all Configurations that reference // it by name. If it's a Snapshot, we first identify its owner and then look for Configurations diff --git a/controllers/find_registry_secrets.go b/controllers/find_registry_secrets.go deleted file mode 100644 index 0f1f0316..00000000 --- a/controllers/find_registry_secrets.go +++ /dev/null @@ -1,42 +0,0 @@ -package controllers - -import ( - "context" - - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -type Lister interface { - client.ObjectList - - List() []client.Object -} - -// findRegistrySecrets finds secrets that have a specific key for a given kind. -// Note, this is not working with ObjectList or Unstructured. ObjectList doesn't -// allow listing objects, and unstructured doesn't support fields. -func findRegistrySecrets(c client.Client, key string, list Lister) handler.MapFunc { - return func(obj client.Object) []reconcile.Request { - if err := c.List(context.Background(), list, &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(key, client.ObjectKeyFromObject(obj).String()), - }); err != nil { - return []reconcile.Request{} - } - - requests := make([]reconcile.Request, len(list.List())) - for i, item := range list.List() { - requests[i] = reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: item.GetName(), - Namespace: item.GetNamespace(), - }, - } - } - - return requests - } -} diff --git a/controllers/localization_controller.go b/controllers/localization_controller.go index 0d0b1e0f..7fbc742a 100644 --- a/controllers/localization_controller.go +++ b/controllers/localization_controller.go @@ -290,7 +290,7 @@ func (r *LocalizationReconciler) reconcile( return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } -// The purpose of the findRegistrySecrets function is to identify whether a given Kubernetes object +// The purpose of the findObjects function is to identify whether a given Kubernetes object // is referenced by a Localization. This is done by checking whether the object is a ComponentVersion // or a Snapshot. If it's a ComponentVersion, we look for all Configurations that reference // it by name. If it's a Snapshot, we first identify its owner and then look for Localization From 2893b28742593c6c65b74557ae9e03b6ba7444c3 Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Tue, 14 Nov 2023 12:09:46 +0100 Subject: [PATCH 8/8] extracted status update logic and removed hardcoded statuses --- api/v1alpha1/condition_types.go | 21 +++++++++++ controllers/componentversion_controller.go | 21 +++++------ controllers/configuration_controller.go | 31 ++++++++-------- controllers/localization_controller.go | 35 ++++++++++--------- controllers/resource_controller.go | 21 +++++------ controllers/snapshot_controller.go | 5 +-- .../status}/identifiable_contract.go | 8 ++--- .../status}/mutate_condition_status.go | 2 +- .../status}/register_status_defer.go | 6 ++-- 9 files changed, 88 insertions(+), 62 deletions(-) rename {controllers => pkg/status}/identifiable_contract.go (74%) rename {controllers => pkg/status}/mutate_condition_status.go (97%) rename {controllers => pkg/status}/register_status_defer.go (93%) diff --git a/api/v1alpha1/condition_types.go b/api/v1alpha1/condition_types.go index 1af7bcb2..e9009a6c 100644 --- a/api/v1alpha1/condition_types.go +++ b/api/v1alpha1/condition_types.go @@ -61,4 +61,25 @@ const ( // CreateRepositoryNameReason is used when the generating a new repository name fails. CreateRepositoryNameReason = "CreateRepositoryNameFailed" + + // ConfigRefNotReadyWithErrorReason is used when configuration reference is not ready yet with an error. + ConfigRefNotReadyWithErrorReason = "ConfigRefNotReadyWithError" + + // ConfigRefNotReadyReason is used when configuration ref is not ready yet and there was no error. + ConfigRefNotReadyReason = "ConfigRefNotReady" + + // SourceRefNotReadyWithErrorReason is used when the source ref is not ready and there was an error. + SourceRefNotReadyWithErrorReason = "SourceRefNotReadyWithError" + + // SourceRefNotReadyReason is used when the source ref is not ready and there was no error. + SourceRefNotReadyReason = "SourceRefNotReady" + + // PatchStrategicMergeSourceRefNotReadyWithErrorReason is used when source ref for patch strategic merge is not ready and there was an error. + PatchStrategicMergeSourceRefNotReadyWithErrorReason = "PatchStrategicMergeSourceRefNotReadyWithError" + + // PatchStrategicMergeSourceRefNotReadyReason is used when source ref for patch strategic merge is not ready and there was no error. + PatchStrategicMergeSourceRefNotReadyReason = "PatchStrategicMergeSourceRefNotReady" + + // SnapshotNameEmptyReason is used for a failure to generate a snapshot name. + SnapshotNameEmptyReason = "SnapshotNameEmpty" ) diff --git a/controllers/componentversion_controller.go b/controllers/componentversion_controller.go index 9c8c7bd8..f3d99ed6 100644 --- a/controllers/componentversion_controller.go +++ b/controllers/componentversion_controller.go @@ -15,6 +15,7 @@ import ( "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" + "github.com/open-component-model/ocm-controller/pkg/status" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -139,7 +140,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := status.UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { retErr = errors.Join(retErr, derr) } }() @@ -153,7 +154,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req if err != nil { // we don't fail here, because all manifests might have been applied at once or the secret // for authentication is being reconciled. - MarkAsStalled( + status.MarkAsStalled( r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, @@ -167,7 +168,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req update, version, err := r.checkVersion(ctx, octx, obj) if err != nil { // The component might not be there yet. We don't fail but keep polling instead. - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.CheckVersionFailedReason, @@ -196,7 +197,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req ok, err := r.OCMClient.VerifyComponent(ctx, octx, obj, version) if err != nil { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.VerificationFailedReason, @@ -209,7 +210,7 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req } if !ok { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.VerificationFailedReason, @@ -233,7 +234,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con cv, err := r.OCMClient.GetComponentVersion(ctx, octx, obj, obj.Spec.Component, version) if err != nil { err = fmt.Errorf("failed to get component version: %w", err) - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.ComponentVersionInvalidReason, @@ -249,7 +250,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con cd, err := dv.ConvertFrom(cv.GetDescriptor()) if err != nil { err = fmt.Errorf("failed to convert component descriptor: %w", err) - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.ConvertComponentDescriptorFailedReason, @@ -264,7 +265,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con componentName, err := component.ConstructUniqueName(cd.GetName(), cd.GetVersion(), nil) if err != nil { err = fmt.Errorf("failed to generate name: %w", err) - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, @@ -301,7 +302,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con if err != nil { err = fmt.Errorf("failed to create or update component descriptor: %w", err) - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.CreateOrUpdateComponentDescriptorFailedReason, @@ -325,7 +326,7 @@ func (r *ComponentVersionReconciler) reconcile(ctx context.Context, octx ocm.Con componentDescriptor.References, err = r.parseReferences(ctx, octx, obj, cv.GetDescriptor().References) if err != nil { err = fmt.Errorf("failed to parse references: %w", err) - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, v1alpha1.ParseReferencesFailedReason, diff --git a/controllers/configuration_controller.go b/controllers/configuration_controller.go index 1a79f740..83eacc0b 100644 --- a/controllers/configuration_controller.go +++ b/controllers/configuration_controller.go @@ -15,6 +15,7 @@ import ( "github.com/fluxcd/pkg/runtime/patch" rreconcile "github.com/fluxcd/pkg/runtime/reconcile" sourcev1 "github.com/fluxcd/source-controller/api/v1" + "github.com/open-component-model/ocm-controller/pkg/status" "golang.org/x/exp/slices" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -168,7 +169,7 @@ func (r *ConfigurationReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := status.UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -181,16 +182,16 @@ func (r *ConfigurationReconciler) Reconcile( // check dependencies are ready ready, err := r.checkReadiness(ctx, obj.GetNamespace(), &obj.Spec.SourceRef) if err != nil { - MarkNotReady(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceRefNotReadyWithErrorReason, err.Error()) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "SourceRefNotReady", + v1alpha1.SourceRefNotReadyReason, fmt.Sprintf("source ref not yet ready: %s", obj.Spec.SourceRef.Name), ) @@ -200,20 +201,20 @@ func (r *ConfigurationReconciler) Reconcile( if obj.Spec.ConfigRef != nil { ready, err := r.checkReadiness(ctx, obj.GetNamespace(), obj.Spec.ConfigRef) if err != nil { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "ConfigRefNotReadyWithError", + v1alpha1.ConfigRefNotReadyWithErrorReason, fmt.Sprintf("config ref not yet ready with error: %s: %s", obj.Spec.ConfigRef.Name, err), ) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "ConfigRefNotReady", + v1alpha1.ConfigRefNotReadyReason, fmt.Sprintf("config ref not yet ready: %s", obj.Spec.ConfigRef.Name), ) @@ -224,10 +225,10 @@ func (r *ConfigurationReconciler) Reconcile( if obj.Spec.PatchStrategicMerge != nil { ready, err := r.checkFluxSourceReadiness(ctx, obj.Spec.PatchStrategicMerge.Source.SourceRef) if err != nil { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "PatchStrategicMergeSourceRefNotReadyWithError", + v1alpha1.PatchStrategicMergeSourceRefNotReadyWithErrorReason, fmt.Sprintf("patch strategic merge source ref not yet ready with error: %s: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name, err), ) @@ -235,10 +236,10 @@ func (r *ConfigurationReconciler) Reconcile( } if !ready { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "PatchStrategicMergeSourceRefNotReady", + v1alpha1.PatchStrategicMergeSourceRefNotReadyReason, fmt.Sprintf("patch strategic merge source ref not yet ready: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name), ) @@ -252,7 +253,7 @@ func (r *ConfigurationReconciler) Reconcile( name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { err := fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -288,13 +289,13 @@ func (r *ConfigurationReconciler) reconcile( if errors.Is(err, errTar) { err = fmt.Errorf("source resource is not a tar archive: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) return ctrl.Result{}, err } err = fmt.Errorf("failed to reconcile mutation object: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) return ctrl.Result{}, err } diff --git a/controllers/localization_controller.go b/controllers/localization_controller.go index 7fbc742a..55568ad8 100644 --- a/controllers/localization_controller.go +++ b/controllers/localization_controller.go @@ -16,6 +16,7 @@ import ( rreconcile "github.com/fluxcd/pkg/runtime/reconcile" sourcev1 "github.com/fluxcd/source-controller/api/v1" "github.com/fluxcd/source-controller/api/v1beta2" + "github.com/open-component-model/ocm-controller/pkg/status" "golang.org/x/exp/slices" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -151,7 +152,7 @@ func (r *LocalizationReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := status.UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -164,16 +165,16 @@ func (r *LocalizationReconciler) Reconcile( // check dependencies are ready ready, err := r.checkReadiness(ctx, obj.GetNamespace(), &obj.Spec.SourceRef) if err != nil { - MarkNotReady(r.EventRecorder, obj, "SourceRefNotReadyWithError", err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceRefNotReadyWithErrorReason, err.Error()) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "SourceRefNotReady", + v1alpha1.SourceRefNotReadyReason, fmt.Sprintf("source ref not yet ready: %s", obj.Spec.SourceRef.Name), ) @@ -183,20 +184,20 @@ func (r *LocalizationReconciler) Reconcile( if obj.Spec.ConfigRef != nil { ready, err := r.checkReadiness(ctx, obj.GetNamespace(), obj.Spec.ConfigRef) if err != nil { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "ConfigRefNotReadyWithError", + v1alpha1.ConfigRefNotReadyWithErrorReason, fmt.Sprintf("config ref not yet ready with error: %s: %s", obj.Spec.ConfigRef.Name, err), ) return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } if !ready { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "ConfigRefNotReady", + v1alpha1.ConfigRefNotReadyReason, fmt.Sprintf("config ref not yet ready: %s", obj.Spec.ConfigRef.Name), ) @@ -207,10 +208,10 @@ func (r *LocalizationReconciler) Reconcile( if obj.Spec.PatchStrategicMerge != nil { ready, err := r.checkFluxSourceReadiness(ctx, obj.Spec.PatchStrategicMerge.Source.SourceRef) if err != nil { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "PatchStrategicMergeSourceRefNotReadyWithError", + v1alpha1.PatchStrategicMergeSourceRefNotReadyWithErrorReason, fmt.Sprintf("patch strategic merge source ref not yet ready with error: %s: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name, err), ) @@ -218,10 +219,10 @@ func (r *LocalizationReconciler) Reconcile( } if !ready { - MarkNotReady( + status.MarkNotReady( r.EventRecorder, obj, - "PatchStrategicMergeSourceRefNotReady", + v1alpha1.PatchStrategicMergeSourceRefNotReadyReason, fmt.Sprintf("patch strategic merge source ref not yet ready: %s", obj.Spec.PatchStrategicMerge.Source.SourceRef.Name), ) @@ -235,7 +236,7 @@ func (r *LocalizationReconciler) Reconcile( name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { err = fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -269,13 +270,13 @@ func (r *LocalizationReconciler) reconcile( if errors.Is(err, errTar) { err = fmt.Errorf("source resource is not a tar archive: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.SourceReasonNotATarArchiveReason, err.Error()) return ctrl.Result{}, err } err = fmt.Errorf("failed to reconcile mutation object: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.ReconcileMutationObjectFailedReason, err.Error()) return ctrl.Result{}, err } @@ -431,9 +432,9 @@ func makeRequestsForLocalizations(ll ...v1alpha1.Localization) []reconcile.Reque requests := make([]reconcile.Request, len(refs)) for i, item := range refs { - // if the observedgeneration is -1 + // if the ObservedGeneration is -1 // then the object has not been initialised yet - // so we should not trigger a reconcilation for sourceRef/configRefs + // so we should not trigger a reconciliation for sourceRef/configRefs if item.Status.ObservedGeneration != -1 { requests[i] = reconcile.Request{ NamespacedName: types.NamespacedName{ diff --git a/controllers/resource_controller.go b/controllers/resource_controller.go index 31757a3e..6fda44a4 100644 --- a/controllers/resource_controller.go +++ b/controllers/resource_controller.go @@ -18,6 +18,7 @@ import ( "github.com/open-component-model/ocm-controller/pkg/component" "github.com/open-component-model/ocm-controller/pkg/ocm" "github.com/open-component-model/ocm-controller/pkg/snapshot" + "github.com/open-component-model/ocm-controller/pkg/status" ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -100,7 +101,7 @@ func (r *ResourceReconciler) Reconcile( // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { + if derr := status.UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, obj.GetRequeueAfter()); derr != nil { err = errors.Join(err, derr) } }() @@ -116,7 +117,7 @@ func (r *ResourceReconciler) Reconcile( name, err := snapshot.GenerateSnapshotName(obj.GetName()) if err != nil { err = fmt.Errorf("failed to generate snapshot name for: %s: %s", obj.GetName(), err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.NameGenerationFailedReason, err.Error()) return ctrl.Result{}, err } @@ -154,13 +155,13 @@ func (r *ResourceReconciler) reconcile( } err = fmt.Errorf("failed to get component version: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentVersionNotFoundReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentVersionNotFoundReason, err.Error()) return ctrl.Result{}, err } if !conditions.IsReady(&componentVersion) { - MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentVersionNotReadyReason, "component version not ready yet") + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentVersionNotReadyReason, "component version not ready yet") return ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}, nil } @@ -170,7 +171,7 @@ func (r *ResourceReconciler) reconcile( octx, err := r.OCMClient.CreateAuthenticatedOCMContext(ctx, &componentVersion) if err != nil { err = fmt.Errorf("failed to create authenticated client: %w", err) - MarkAsStalled(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error()) + status.MarkAsStalled(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error()) return ctrl.Result{}, nil } @@ -178,7 +179,7 @@ func (r *ResourceReconciler) reconcile( reader, digest, err := r.OCMClient.GetResource(ctx, octx, &componentVersion, obj.Spec.SourceRef.ResourceRef) if err != nil { err = fmt.Errorf("failed to get resource: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.GetResourceFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.GetResourceFailedReason, err.Error()) return ctrl.Result{}, err } @@ -194,7 +195,7 @@ func (r *ResourceReconciler) reconcile( componentDescriptor, err := component.GetComponentDescriptor(ctx, r.Client, obj.GetReferencePath(), componentVersion.Status.ComponentDescriptor) if err != nil { err = fmt.Errorf("failed to get component descriptor for resource: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.GetComponentDescriptorFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.GetComponentDescriptorFailedReason, err.Error()) return ctrl.Result{}, err } @@ -204,14 +205,14 @@ func (r *ResourceReconciler) reconcile( "couldn't find component descriptor for reference '%s' or any root components", obj.GetReferencePath(), ) - MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentDescriptorNotFoundReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.ComponentDescriptorNotFoundReason, err.Error()) return ctrl.Result{}, err } if obj.GetSnapshotName() == "" { err := errors.New("snapshot name should not be empty") - MarkNotReady(r.EventRecorder, obj, "SnapshotNameEmpty", err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.SnapshotNameEmptyReason, err.Error()) return ctrl.Result{}, err } @@ -250,7 +251,7 @@ func (r *ResourceReconciler) reconcile( }) if err != nil { err = fmt.Errorf("failed to create or update snapshot: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.CreateOrUpdateSnapshotFailedReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.CreateOrUpdateSnapshotFailedReason, err.Error()) return ctrl.Result{}, err } diff --git a/controllers/snapshot_controller.go b/controllers/snapshot_controller.go index a7273d75..774b11c9 100644 --- a/controllers/snapshot_controller.go +++ b/controllers/snapshot_controller.go @@ -16,6 +16,7 @@ import ( rreconcile "github.com/fluxcd/pkg/runtime/reconcile" "github.com/google/go-containerregistry/pkg/v1/remote/transport" "github.com/open-component-model/ocm-controller/pkg/cache" + "github.com/open-component-model/ocm-controller/pkg/status" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" kuberecorder "k8s.io/client-go/tools/record" @@ -93,7 +94,7 @@ func (r *SnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r // Always attempt to patch the object and status after each reconciliation. defer func() { - if derr := updateStatus(ctx, patchHelper, obj, r.EventRecorder, 0); derr != nil { + if derr := status.UpdateStatus(ctx, patchHelper, obj, r.EventRecorder, 0); derr != nil { err = errors.Join(err, derr) } }() @@ -106,7 +107,7 @@ func (r *SnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Request) (r name, err := ocm.ConstructRepositoryName(obj.Spec.Identity) if err != nil { err = fmt.Errorf("failed to construct name: %w", err) - MarkNotReady(r.EventRecorder, obj, v1alpha1.CreateRepositoryNameReason, err.Error()) + status.MarkNotReady(r.EventRecorder, obj, v1alpha1.CreateRepositoryNameReason, err.Error()) return ctrl.Result{}, err } diff --git a/controllers/identifiable_contract.go b/pkg/status/identifiable_contract.go similarity index 74% rename from controllers/identifiable_contract.go rename to pkg/status/identifiable_contract.go index 4ff832ff..a4fed602 100644 --- a/controllers/identifiable_contract.go +++ b/pkg/status/identifiable_contract.go @@ -1,4 +1,4 @@ -package controllers +package status import ( "github.com/fluxcd/pkg/runtime/conditions" @@ -6,15 +6,15 @@ import ( // IdentifiableClientObject defines an object which can create an identity for itself. type IdentifiableClientObject interface { - StatusMutator + Mutator conditions.Setter // GetVID constructs an identifier for an object. GetVID() map[string]string } -// StatusMutator allows mutating specific status fields of an object. -type StatusMutator interface { +// Mutator allows mutating specific status fields of an object. +type Mutator interface { // SetObservedGeneration mutates the observed generation field of an object. SetObservedGeneration(v int64) } diff --git a/controllers/mutate_condition_status.go b/pkg/status/mutate_condition_status.go similarity index 97% rename from controllers/mutate_condition_status.go rename to pkg/status/mutate_condition_status.go index 63f72274..4990691a 100644 --- a/controllers/mutate_condition_status.go +++ b/pkg/status/mutate_condition_status.go @@ -1,4 +1,4 @@ -package controllers +package status import ( eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" diff --git a/controllers/register_status_defer.go b/pkg/status/register_status_defer.go similarity index 93% rename from controllers/register_status_defer.go rename to pkg/status/register_status_defer.go index 6899ec2e..3b48406c 100644 --- a/controllers/register_status_defer.go +++ b/pkg/status/register_status_defer.go @@ -1,4 +1,4 @@ -package controllers +package status import ( "context" @@ -13,8 +13,8 @@ import ( kuberecorder "k8s.io/client-go/tools/record" ) -// updateStatus takes an object which can identify itself and updates its status including ObservedGeneration. -func updateStatus( +// UpdateStatus takes an object which can identify itself and updates its status including ObservedGeneration. +func UpdateStatus( ctx context.Context, patchHelper *patch.SerialPatcher, obj IdentifiableClientObject,