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

nephio-project/nephio#615

controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go

@@ -57,23 +57,23 @@ 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 (
 	conditionPipelineNotPassed = porchapi.Condition{
 		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

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.

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

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()
 }