Implement configuration injection in PackageVariant #3939
Conversation
johnbelamaric
changed the title
johnbelamaric
force-pushed
the
pv-config-injection
branch
from
a0dca8d
to
49e7f81
Compare
Note that ensurePackageVariant needed some fixes. When I introduced applyMutations, I didn't properly update it to account for subsequent reconciliations; if an error happened in the first reconciation, it was never retried and rectified in later reconciliations. So, this change also includes that fix. Still more tests to do.
porch/controllers/packagevariants/pkg/controllers/packagevariant/injection.go
Show resolved
Hide resolved
porch/controllers/packagevariants/pkg/controllers/packagevariant/injection.go
Outdated
Show resolved
Hide resolved
porch/controllers/packagevariants/pkg/controllers/packagevariant/injection.go
Show resolved
Hide resolved
porch/controllers/packagevariants/pkg/controllers/packagevariant/fake_client.go
Show resolved
Hide resolved
| if err := r.Client.Update(ctx, downstream); err != nil { | ||
| klog.Errorf("error updating package revision lifecycle: %v", err) | ||
| } | ||
| if downstream.Spec.Lifecycle == porchapi.PackageRevisionLifecycleDeletionProposed { |
There was a problem hiding this comment.
Was there a reason to move this and the next few lines out of the isUpToDate condition? If the package revision with lifecycle DeletionProposed is not up to date and needs updating, then we are going to make and update a new draft, so I don't think it's needed to update the lifecycle of the old existing package revision?
There was a problem hiding this comment.
Good question, I meant to ask you to give the changes in this method particular attention.
I was working with the understanding that LifecycleIsPublished would return false for those with deletion proposed, but now that you ask it, maybe that's not true? I was concerned that we would end up cloning a deletion proposed package, and that the published version would then remain in deletion proposed even though it's not supposed to.
My thinking was that we needed to clear this state since it's no longer proposed for deletion, regardless of what else needs to be done to it, and that it was safe to do that even if it's not up to date. That did require saving it before the clone.
Probably we should have some sort of e2e test or something to see if this works the way we think? Do we have an easy way to do this sort of complex test?
There was a problem hiding this comment.
Yeah, LifecycleIsPublished returns true for Published or DeletionProposed. Those two lifecycles are treated pretty much the same everywhere, with the only exception being the package revision deletion webhook that disallows Published packages to be deleted. Draft and Proposed package revisions don't need to be proposed for deletion - they can just be deleted at any time.
I was concerned that we would end up cloning a deletion proposed package, and that the published version would then remain in deletion proposed even though it's not supposed to.
The previous logic did copy a deletion proposed package, but the copied version will start out in draft state and will move through the normal package revision lifecycles of Draft -> Proposed -> Published -> DeletionProposed, requiring a client to move it to the next lifecycle at each step, so the copied version shouldn't get to the deletionProposed stage unless someone explicitly changed it to that.
My thinking was that we needed to clear this state since it's no longer proposed for deletion, regardless of what else needs to be done to it, and that it was safe to do that even if it's not up to date. That did require saving it before the clone.
Got it. So we are thinking of the proposedDeletion package revision as the target, and therefore we don't want it to be deleted anymore. Copying it later is just part of updating it. I agree that it is safe and I think your reasoning is correct, so your changes make sense.
Probably we should have some sort of e2e test or something to see if this works the way we think? Do we have an easy way to do this sort of complex test?
Unfortunately we don't have e2e tests set up for these controllers. We should try to set something up at some point rather than relying on developers to do a bunch of manual testing, but we don't have a concrete plan for that. We do have complex tests for the experimental rollouts controllers, so we might be able to draw some inspiration from there.
There was a problem hiding this comment.
Thanks Natasha, that explanation helps a lot.
* Add injection API * Add validation for config injectors * Add updated CRD * Partial implementation of config injection * Test and fix setInjectionPointConditionsAndGates * Fix PVS test due to changed hash * Initial complete injection implementation Note that ensurePackageVariant needed some fixes. When I introduced applyMutations, I didn't properly update it to account for subsequent reconciliations; if an error happened in the first reconciation, it was never retried and rectified in later reconciliations. So, this change also includes that fix. Still more tests to do. * Additional tests, cleanup, bug fixes * Address review comments * Add TODO about watches for injected objects, fix nit
Implement Configuration Injection as described in https://github.com/GoogleContainerTools/kpt/blob/main/docs/design-docs/08-package-variant.md
Fixes nephio-project/nephio#43