Have PackageVariant set readiness gate on PackageRevisions #156
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JamesMcDermott The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test presubmit-nephio-go-test |
| @@ -334,6 +350,13 @@ func (r *PackageVariantReconciler) ensurePackageVariant(ctx context.Context, | |||
| if err = r.Client.Create(ctx, newPR); err != nil { | |||
| return nil, err | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Looks good. I'd be interested to see how this works in a full test-infra e2e run.
Also, may be somewhat aligned with what @kispaljr was working on?
|
/test presubmit-nephio-go-test |
5 similar comments
|
/test presubmit-nephio-go-test |
|
/test presubmit-nephio-go-test |
|
/test presubmit-nephio-go-test |
|
/test presubmit-nephio-go-test |
|
/test presubmit-nephio-go-test |
| ConditionTypeReady = "Ready" // whether or not the reconciliation succeeded | ||
| 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 |
There was a problem hiding this comment.
I suggest to rename it to PackagePipelinePassed or similar, since the success of the validation pipeline should be checked, as well, not just the mutation pipeline.
There was a problem hiding this comment.
This of course also applies to all other places where it is referenced as a "mutation pipeline" (e.g. in conditions)
| if err := r.Client.Get(ctx, types.NamespacedName{Name: newPR.GetName(), Namespace: newPR.GetNamespace()}, &refreshedPR); err != nil { | ||
| return nil, err | ||
| } | ||
| newPR.ResourceVersion = refreshedPR.ResourceVersion |
There was a problem hiding this comment.
Why are only ResourceVersion and Tasks are copied and every other potential changes are reverted back?
Why aren't we simply Get() the PR into &newPR, set the condition and update?
There was a problem hiding this comment.
Done - the copy/revert thing with refreshedPR was left over from solving an earlier hurdle and it isn't needed now.
| @@ -347,6 +371,29 @@ func (r *PackageVariantReconciler) ensurePackageVariant(ctx context.Context, | |||
| } | |||
| } | |||
|
|
|||
| // TODO: remove if OK to exclude PackageRevision from cache of r.Client | |||
There was a problem hiding this comment.
remove this comment block if not needed
There was a problem hiding this comment.
In the findAndUpdateExistingRevisions() method, at line 473:
// Save the updated PackageRevisionResources
if err := r.updatePackageResources(ctx, prr, pv); err != nil {
return nil, err
}
The pipeline will also be executed here, so the Condition should be set according to the results.
There was a problem hiding this comment.
in updateDraft() at line 736:
err := r.Client.Update(ctx, draft)
if err != nil {
return nil, err
}
this is tricky, but technically this Update() call will also result in the re-execution of the pipeline, so we should record the results in the Condition. The tricky part is that I don't know how to get the rendering results, and without it, the best we can do is to report the error or success of Update() itself.
There was a problem hiding this comment.
Done both:
- the
findAndUpdateExistingRevisions()one could be a bit better by including the initial set of the Condition directly inprrinstead of doing a separater.Client.Update()- I want to look into automated (and a few more manual) tests first though - the
updateDraft()one took more time to work around to avoid ending up in an endless loop where doing theUpdate()to set the Condition would trigger the pipeline, resulting in a rendering result which we'd need to record in the Condition by doing anUpdate(), triggering the pipeline... you get the idea. :)- thanks to @liamfallon for suggesting we have the server detect and skip the pipeline when an update only sets the Condition/readiness info
- works as desired when creating a PackageVariant - unable to test in this update flow due to immediate Git conflicts as seen in #469
- PackageVariant controller now uses a readiness gate to allow
a PackageRevision to complete its mutation pipeline before it
is allowed to be proposed/approved
- refactored conversion of Kptfiles to YAML since readiness condition
information is stored in the package Kptfile
- unified all cases to the same kyaml/yaml-based method
(KptFile.ToYamlString() and ToYamlString(*fn.KubeObject))
- this ensures consistency in the YAML (indentation, field order etc.)
- and reduces the chances of Git conflicts when setting and updating
readiness conditions
- added more info to error message in case of Git conflict when applying
a patch
nephio-project/nephio#615
- previous solution broke existing functionality to copy comments between Kptfile objects
- resulting in failing unit tests
- fix for that brought back the Git conflicts caused by indentation, field order etc.
- re-refactored conversion of Kptfiles to YAML
- KubeObject-related conversion methods (KubeObject/YAML and YAML/KubeObject)
now live out in the util package as they aren't Kptfile-specific any more
nephio-project/nephio#615
JamesMcDermott
force-pushed
the
pv-pipeline-readiness-gates
branch
from
3cff6bb to
60cb107
Compare
… 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
Implements #615