From bd30aac72c6d39021e059b48748d4b7ef479fef2 Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Fri, 20 Dec 2024 11:37:33 +0000 Subject: [PATCH] Issue #615 - skip pipeline on PackageRevision update if only updating pipeline-passed condition - review comments to increase applicability of pipeline-passed readiness condition - to enable this: if an incoming PackageRevision update changes only that readiness condition, detect that, skip the pipeline/rendering/etc., and only update the Kptfile with the readiness condition update - other tidy-up/wording comments https://github.com/nephio-project/nephio/issues/615 --- .../packagevariant_controller.go | 56 +++++++++------- pkg/registry/porch/packagecommon.go | 26 ++++---- pkg/task/generictaskhandler.go | 65 +++++++++++++++++-- pkg/util/util.go | 4 +- 4 files changed, 110 insertions(+), 41 deletions(-) diff --git a/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go b/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go index cbb39454..6286d076 100644 --- a/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go +++ b/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go @@ -57,9 +57,9 @@ type PackageVariantReconciler struct { const ( workspaceNamePrefix = "packagevariant-" - ConditionTypeStalled = "Stalled" // whether or not the packagevariant object is making progress or not - ConditionTypeReady = "Ready" // whether or not the reconciliation succeeded - ConditionTypePipelinePassed = "MutationPipelinePassed" // whether or not the mutation pipeline has completed successufully + ConditionTypeStalled = "Stalled" // whether or not the packagevariant object is making progress + ConditionTypeReady = "Ready" // whether or not the reconciliation succeeded + ConditionTypePipelinePassed = "PackagePipelinePassed" // whether or not the package's pipeline has completed successfully ) var ( @@ -67,13 +67,13 @@ var ( Type: ConditionTypePipelinePassed, Status: porchapi.ConditionFalse, Reason: "WaitingOnPipeline", - Message: "waiting for mutation pipeline to pass", + Message: "waiting for package pipeline to pass", } conditionPipelinePassed = porchapi.Condition{ Type: ConditionTypePipelinePassed, Status: porchapi.ConditionTrue, Reason: "PipelinePassed", - Message: "mutation pipeline completed successfully", + Message: "package pipeline completed successfully", } ) @@ -371,23 +371,9 @@ func (r *PackageVariantReconciler) ensurePackageVariant(ctx context.Context, } } - // TODO: remove if OK to exclude PackageRevision from cache of r.Client - // - // var tmpClient client.Client - // if tmpClient, err = client.New(config.GetConfigOrDie(), client.Options{ - // Cache: &client.CacheOptions{ - // DisableFor: []client.Object{ - // &porchapi.PackageRevisionResources{}}, - // }, - // }); err != nil { - // return nil, err - // } - var refreshedPR porchapi.PackageRevision - if err := r.Client.Get(ctx, types.NamespacedName{Name: newPR.GetName(), Namespace: newPR.GetNamespace()}, &refreshedPR); err != nil { + if err := r.Client.Get(ctx, types.NamespacedName{Name: newPR.GetName(), Namespace: newPR.GetNamespace()}, newPR); err != nil { return nil, err } - newPR.ResourceVersion = refreshedPR.ResourceVersion - newPR.Spec.Tasks = refreshedPR.Spec.Tasks setPrStatusCondition(newPR, conditionPipelinePassed) if err := r.Client.Update(ctx, newPR); err != nil { @@ -416,10 +402,18 @@ func (r *PackageVariantReconciler) findAndUpdateExistingRevisions(ctx context.Co downstream.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished // We update this now, because later we may use a Porch call to clone or update // and we want to make sure the server is in sync with us + + setPrReadinessGate(downstream, ConditionTypePipelinePassed) + setPrStatusCondition(downstream, conditionPipelineNotPassed) if err := r.Client.Update(ctx, downstream); err != nil { klog.Errorf("error updating package revision lifecycle: %v", err) return nil, err } + + setPrStatusCondition(downstream, conditionPipelinePassed) + if err := r.Client.Update(ctx, downstream); err != nil { + return nil, err + } } // see if the package needs updating due to an upstream change @@ -470,9 +464,20 @@ func (r *PackageVariantReconciler) findAndUpdateExistingRevisions(ctx context.Co } // Save the updated PackageRevisionResources + + setPrReadinessGate(downstream, ConditionTypePipelinePassed) + setPrStatusCondition(downstream, conditionPipelineNotPassed) + if err := r.Client.Update(ctx, downstream); err != nil { + return nil, err + } if err := r.updatePackageResources(ctx, prr, pv); err != nil { return nil, err } + + setPrStatusCondition(downstream, conditionPipelinePassed) + if err := r.Client.Update(ctx, downstream); err != nil { + return nil, err + } } } return downstreams, nil @@ -741,8 +746,15 @@ func (r *PackageVariantReconciler) updateDraft(ctx context.Context, updateTask.Update.Upstream.UpstreamRef.Name = newUpstreamPR.Name draft.Spec.Tasks = append(tasks, updateTask) - err := r.Client.Update(ctx, draft) - if err != nil { + setPrReadinessGate(draft, ConditionTypePipelinePassed) + setPrStatusCondition(draft, conditionPipelineNotPassed) + + if err := r.Client.Update(ctx, draft); err != nil { + return nil, err + } + + setPrStatusCondition(draft, conditionPipelinePassed) + if err := r.Client.Update(ctx, draft); err != nil { return nil, err } return draft, nil diff --git a/pkg/registry/porch/packagecommon.go b/pkg/registry/porch/packagecommon.go index 78400c2d..dda487e7 100644 --- a/pkg/registry/porch/packagecommon.go +++ b/pkg/registry/porch/packagecommon.go @@ -309,19 +309,7 @@ func (r *packageCommon) updatePackageRevision(ctx context.Context, name string, parentPackage = p } - if !isCreate { - rev, err := r.cad.UpdatePackageRevision(ctx, "", &repositoryObj, oldRepoPkgRev, oldApiPkgRev.(*api.PackageRevision), newApiPkgRev, parentPackage) - if err != nil { - return nil, false, apierrors.NewInternalError(err) - } - - updated, err := rev.GetPackageRevision(ctx) - if err != nil { - return nil, false, apierrors.NewInternalError(err) - } - - return updated, false, nil - } else { + if isCreate { rev, err := r.cad.CreatePackageRevision(ctx, &repositoryObj, newApiPkgRev, parentPackage) if err != nil { klog.Infof("error creating package: %v", err) @@ -334,6 +322,18 @@ func (r *packageCommon) updatePackageRevision(ctx context.Context, name string, return createdApiPkgRev, true, nil } + + rev, err := r.cad.UpdatePackageRevision(ctx, "", &repositoryObj, oldRepoPkgRev, oldApiPkgRev.(*api.PackageRevision), newApiPkgRev, parentPackage) + if err != nil { + return nil, false, apierrors.NewInternalError(err) + } + + updated, err := rev.GetPackageRevision(ctx) + if err != nil { + return nil, false, apierrors.NewInternalError(err) + } + + return updated, false, nil } // Common implementation of Package update logic. diff --git a/pkg/task/generictaskhandler.go b/pkg/task/generictaskhandler.go index 3454882b..db3b189b 100644 --- a/pkg/task/generictaskhandler.go +++ b/pkg/task/generictaskhandler.go @@ -16,7 +16,10 @@ package task import ( "context" + "encoding/json" "fmt" + "reflect" + "slices" api "github.com/nephio-project/porch/api/porch/v1alpha1" configapi "github.com/nephio-project/porch/api/porchconfig/v1alpha1" @@ -33,7 +36,26 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) -var _ TaskHandler = &genericTaskHandler{} +var ( + _ TaskHandler = &genericTaskHandler{} + + conditionPipelineNotPassed = api.Condition{ + Type: ConditionTypePipelinePassed, + Status: api.ConditionFalse, + Reason: "WaitingOnPipeline", + Message: "waiting for package pipeline to pass", + } + conditionPipelinePassed = api.Condition{ + Type: ConditionTypePipelinePassed, + Status: api.ConditionTrue, + Reason: "PipelinePassed", + Message: "package pipeline completed successfully", + } +) + +const ( + ConditionTypePipelinePassed = "PackagePipelinePassed" // whether or not the package's pipeline has completed successfully +) type genericTaskHandler struct { runnerOptionsResolver func(namespace string) fnruntime.RunnerOptions @@ -153,21 +175,30 @@ func (th *genericTaskHandler) DoPRMutations(ctx context.Context, namespace strin // If any of the fields in the API that are projections from the Kptfile // must be updated in the Kptfile as well. - kfPatchTask, created, err := createKptfilePatchTask(ctx, repoPR, newObj) + kfPatchTask, kfPatchCreated, err := createKptfilePatchTask(ctx, repoPR, newObj) if err != nil { return err } - if created { - kfPatchMutation, err := buildPatchMutation(ctx, kfPatchTask) + var kfPatchMutation mutation + if kfPatchCreated { + kfPatchMutation, err = buildPatchMutation(ctx, kfPatchTask) if err != nil { return err } + mutations = append(mutations, kfPatchMutation) } // Re-render if we are making changes. mutations = th.conditionalAddRender(newObj, mutations) + // if all this update does is set the PackagePipelinePassed + // readiness condition, we don't need to run the full mutation + // pipeline - just update the Kptfile and leave it at that + if updateOnlySetsPipelineCondition(oldObj, newObj) { + mutations = []mutation{kfPatchMutation} + } + // TODO: Handle the case if alongside lifecycle change, tasks are changed too. // Update package contents only if the package is in draft state if oldObj.Spec.Lifecycle == api.PackageRevisionLifecycleDraft { @@ -431,6 +462,32 @@ func isRenderMutation(m mutation) bool { return isRender } +func updateOnlySetsPipelineCondition(oldObj *api.PackageRevision, newObj *api.PackageRevision) bool { + setsCondition := func() bool { + oldObjHasCondNotPassed := slices.Contains(oldObj.Status.Conditions, conditionPipelineNotPassed) + oldObjHasCondPassed := slices.Contains(oldObj.Status.Conditions, conditionPipelinePassed) + newObjHasCondNotPassed := slices.Contains(newObj.Status.Conditions, conditionPipelineNotPassed) + newObjHasCondPassed := slices.Contains(newObj.Status.Conditions, conditionPipelinePassed) + return (!oldObjHasCondNotPassed && newObjHasCondNotPassed) || + (!oldObjHasCondPassed && newObjHasCondPassed) || + (oldObjHasCondNotPassed && newObjHasCondPassed) || + (oldObjHasCondPassed && newObjHasCondNotPassed) + }() + + noOtherChanges := func() bool { + copyOld := oldObj.DeepCopy() + copyOld.Spec.ReadinessGates = newObj.Spec.ReadinessGates + copyOld.Status.Conditions = newObj.Status.Conditions + + oldJson, _ := json.Marshal(copyOld) + newJson, _ := json.Marshal(newObj) + equalExceptReadinessInfo := reflect.DeepEqual(oldJson, newJson) + return equalExceptReadinessInfo + }() + + return setsCondition && noOtherChanges +} + // applyResourceMutations mutates the resources and returns the most recent renderResult. func applyResourceMutations(ctx context.Context, draft repository.PackageRevisionDraft, baseResources repository.PackageResources, mutations []mutation) (applied repository.PackageResources, renderStatus *api.RenderStatus, err error) { var lastApplied mutation diff --git a/pkg/util/util.go b/pkg/util/util.go index 6c40309b..06876ce0 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -125,6 +125,6 @@ func YamlToKubeObject(yaml string) (kubeObject *fn.KubeObject, err error) { // Writes a KubeObject out to string-form YAML. // Wrapped in this function to unify serialisation into a single approach // everywhere in Porch. -func KubeObjectToYaml(kptfileKubeObject *fn.KubeObject) string { - return kptfileKubeObject.String() +func KubeObjectToYaml(kubeObject *fn.KubeObject) string { + return kubeObject.String() }