PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy #3701

natasha41575 · 2022-12-22T19:25:13Z

Depends on #3678. Only the last two commits here are new. This should be reviewed after #3678 is merged and this is rebased.

Relevant issue: #3488

This PR:

renames DownstreamPackage to PackageVariant
implements deletion/orphaning (based on spec.deletionPolicy) of package revisions when they are no longer needed
adopting existing target package revisions according to the value of spec.adoptionPolicy
adds unit tests, mostly to cover the downstream package targetting

johnbelamaric · 2022-12-22T21:12:28Z

Awesome, looking forward to this. I am already thinking about new enhancements (injection targeting API, rather than just "same GVK and matching cluster name"). But let's wrap up what we already have underway!

mortent

Just a few comments, but mostly looks good.

Separately from this, we should start looking into how we can improve the porch PackageRevision API so a controller like this doesn't have to list all PackageRevisions.

porch/Makefile

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

mortent · 2023-01-10T19:54:40Z

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

 	// the first package revision number that porch assigns is "v1",
 	// so use v0 as a placeholder for comparison
 	latestVersion := "v0"

 	for _, pr := range prList.Items {
-		// look for the downstream package in the target repo
+		owned := r.hasOurOwnerReference(pv, pr.ObjectMeta.OwnerReferences)


Finding the downstream packagerevisions by using the owner reference will require us to always fetch all packagerevisions in the namespace and then go through them all. And if we decide to allow cross-namespace cloning in porch, this will require us to fetch all packagerevisions in the cluster. The way this is usually done with controllers is to add label to the generated resources and then only list resources that match a label selector. We should consider if we can use this pattern here too, as to avoid having to list them all.

It might be that we currently have to list all packagerevisions anyway, since we don't yet have a good way to look up the upstream packagerevision..

Yeah as we discussed offline, I agree that we should eventually have a way to find the upstream packagerevision without having to list all the package revisions (imo this could be a blocker for cross-namespace cloning). But since we don't have that yet, this controller needs to list all of them.

When this controller no longer needs to list all the packagerevisions, we can update this controller to only list certain packagerevisions. I added a TODO for that.

A caveat here is if the adoptionPolicy field is set to adoptExisting, we will still need to list all the packagerevisions, so we can figure out which ones this controller needs to adopt. A mechanism to filter out packagerevisions by repo and/or package name would be helpful for that.

Sounds good. I think we can consider whether the controller should only adopt if the downstream has the correct label. There might be times when we don't want it to update all downstream packages.

mortent · 2023-01-18T18:25:59Z

porch/controllers/packagevariants/api/v1alpha1/packagevariant_types.go

-type DownstreamPackageStatus struct{}
+// PackageVariantStatus defines the observed state of PackageVariant
+type PackageVariantStatus struct {
+	ValidationErrors []string `json:"validationErrors,omitempty"`


We should consider if the status is better expressed through conditions (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties). But we can look at that in a follow-up.

mortent · 2023-01-18T18:31:04Z

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

 	// the first package revision number that porch assigns is "v1",
 	// so use v0 as a placeholder for comparison
 	latestVersion := "v0"

 	for _, pr := range prList.Items {
-		// look for the downstream package in the target repo
+		owned := r.hasOurOwnerReference(pv, pr.ObjectMeta.OwnerReferences)


Sounds good. I think we can consider whether the controller should only adopt if the downstream has the correct label. There might be times when we don't want it to update all downstream packages.

* porch: don't save empty patches (#3695) * docs: fixes for some minor documentation typos (#3699) * docs: Update the kpt book with more details about namespaces and RBAC for porch (#3692) * Log enabled controllers and warn if no controllers are enabled (#3710) Because the default is to enable no controllers, it is easy to mistakenly start a no-op controller. * Extract out common parse-package logic (#3711) We had this code duplicated in a few places also. * refactor pod warmup to avoid vet warning (#3713) By refactoring the parallel operation into a separate function, it should be easier to read and we avoid a loop-closure go-tcha. * Bump json5 from 2.2.0 to 2.2.3 in /site (#3717) Bumps [json5](https://github.com/json5/json5) from 2.2.0 to 2.2.3. - [Release notes](https://github.com/json5/json5/releases) - [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md) - [Commits](json5/json5@v2.2.0...v2.2.3) --- updated-dependencies: - dependency-name: json5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * tests: add more logging around problematic test timeout (#3718) Trying to figure out why this test keeps timing out. * Refactor controller logic for getting RESTConfig to a remote cluster (#3712) We had two copies, rationalize and take the best of each. Also remove the HACK_ENABLE_LOOPBACK hack now that we can target remote clusters. * add a deletion approval flow with a validation webhook (#3678) * PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy (#3701) * e2e: add delay after registering Repository (#3741) I believe this will help avoid the "failed to list resources" error immediately after registering a repository. * licensescan: fix ignore handling (#3740) The previous logic did not work correctly. * licensescan: Add licenses for more libraries. (#3736) Updating our database with the latest libraries, as needed by some other projects sharing this DB! * Docs: Updating 'Developing in Go' (#3715) * licensescan: Fix missing pipe character in README (#3739) The command is not correct without it. * RemoteRootSyncSet: able to specify a packageRef to a package (#3734) This makes it easy to apply packages we create. * chore: Upgrade cli-utils to v0.34.0 (#3746) Upgrades cli-utils to v0.34.0 which contains an upgrade to Go v1.18 and Kubernetes v1.25 resources. This PR was origininally authored by rquitales #3642 * rollouts: added top level directory * rollouts: scaffolded the project using kubebuilder (#3689) * rollouts: added cluster discovery and selection (#3696) * Rollouts package discovery (#3697) * rollouts: added remoterootsync API (#3698) * rollouts: add package cluster matcher (#3700) * rollouts: add AllAtOnce strategy (#3703) * rollouts: allow packages to be discovered from multiple repositories (#3702) * rollouts: rename packages git source to github (#3708) * rollouts: allow the root directory of a repository to be synced (#3709) * rollouts: add caching for discovered packages (#3706) * rollouts: add rolling update strategy (#3714) * rollouts: added API for ProgressiveRolloutStrategy (#3716) * rollouts: refine package to cluster matcher (#3720) * rollouts: implement progressive strategy (#3719) * rollouts: update progressive strategy to pause after wave (#3721) * rollouts: added skeleton CLI (#3724) * rollouts: added skeleton CLI * added table display * rollouts: add rollout summary status (#3725) * rollouts: tidy up go.mod/sum (#3726) * rollouts: duplicate target fix (#3727) * rollouts: sort cluster status list (#3728) * rollouts: conditionally show wave status (#3729) * rollouts: CLI now supports displaying waves and progress counts (#3730) * rollouts: cli can now advance waves on progressive rollouts (#3731) * rollouts: enable server side throttling for cli (#3732) * rollouts: add container cluster watch (#3738) * rollouts: delete remote root sync when no longer needed (#3742) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Natasha Sarkar <natashasarkar@google.com> Co-authored-by: James Brook <jbrook@google.com> Co-authored-by: Morten Torkildsen <mortent@google.com> Co-authored-by: Justin Santa Barbara <justinsb@google.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: abangser <bangser.a@gmail.com> Co-authored-by: Christopher Fry <christopherfry@google.com>

* porch: don't save empty patches (kptdev#3695) * docs: fixes for some minor documentation typos (kptdev#3699) * docs: Update the kpt book with more details about namespaces and RBAC for porch (kptdev#3692) * Log enabled controllers and warn if no controllers are enabled (kptdev#3710) Because the default is to enable no controllers, it is easy to mistakenly start a no-op controller. * Extract out common parse-package logic (kptdev#3711) We had this code duplicated in a few places also. * refactor pod warmup to avoid vet warning (kptdev#3713) By refactoring the parallel operation into a separate function, it should be easier to read and we avoid a loop-closure go-tcha. * Bump json5 from 2.2.0 to 2.2.3 in /site (kptdev#3717) Bumps [json5](https://github.com/json5/json5) from 2.2.0 to 2.2.3. - [Release notes](https://github.com/json5/json5/releases) - [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md) - [Commits](json5/json5@v2.2.0...v2.2.3) --- updated-dependencies: - dependency-name: json5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * tests: add more logging around problematic test timeout (kptdev#3718) Trying to figure out why this test keeps timing out. * Refactor controller logic for getting RESTConfig to a remote cluster (kptdev#3712) We had two copies, rationalize and take the best of each. Also remove the HACK_ENABLE_LOOPBACK hack now that we can target remote clusters. * add a deletion approval flow with a validation webhook (kptdev#3678) * PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy (kptdev#3701) * e2e: add delay after registering Repository (kptdev#3741) I believe this will help avoid the "failed to list resources" error immediately after registering a repository. * licensescan: fix ignore handling (kptdev#3740) The previous logic did not work correctly. * licensescan: Add licenses for more libraries. (kptdev#3736) Updating our database with the latest libraries, as needed by some other projects sharing this DB! * Docs: Updating 'Developing in Go' (kptdev#3715) * licensescan: Fix missing pipe character in README (kptdev#3739) The command is not correct without it. * RemoteRootSyncSet: able to specify a packageRef to a package (kptdev#3734) This makes it easy to apply packages we create. * chore: Upgrade cli-utils to v0.34.0 (kptdev#3746) Upgrades cli-utils to v0.34.0 which contains an upgrade to Go v1.18 and Kubernetes v1.25 resources. This PR was origininally authored by rquitales kptdev#3642 * rollouts: added top level directory * rollouts: scaffolded the project using kubebuilder (kptdev#3689) * rollouts: added cluster discovery and selection (kptdev#3696) * Rollouts package discovery (kptdev#3697) * rollouts: added remoterootsync API (kptdev#3698) * rollouts: add package cluster matcher (kptdev#3700) * rollouts: add AllAtOnce strategy (kptdev#3703) * rollouts: allow packages to be discovered from multiple repositories (kptdev#3702) * rollouts: rename packages git source to github (kptdev#3708) * rollouts: allow the root directory of a repository to be synced (kptdev#3709) * rollouts: add caching for discovered packages (kptdev#3706) * rollouts: add rolling update strategy (kptdev#3714) * rollouts: added API for ProgressiveRolloutStrategy (kptdev#3716) * rollouts: refine package to cluster matcher (kptdev#3720) * rollouts: implement progressive strategy (kptdev#3719) * rollouts: update progressive strategy to pause after wave (kptdev#3721) * rollouts: added skeleton CLI (kptdev#3724) * rollouts: added skeleton CLI * added table display * rollouts: add rollout summary status (kptdev#3725) * rollouts: tidy up go.mod/sum (kptdev#3726) * rollouts: duplicate target fix (kptdev#3727) * rollouts: sort cluster status list (kptdev#3728) * rollouts: conditionally show wave status (kptdev#3729) * rollouts: CLI now supports displaying waves and progress counts (kptdev#3730) * rollouts: cli can now advance waves on progressive rollouts (kptdev#3731) * rollouts: enable server side throttling for cli (kptdev#3732) * rollouts: add container cluster watch (kptdev#3738) * rollouts: delete remote root sync when no longer needed (kptdev#3742) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Natasha Sarkar <natashasarkar@google.com> Co-authored-by: James Brook <jbrook@google.com> Co-authored-by: Morten Torkildsen <mortent@google.com> Co-authored-by: Justin Santa Barbara <justinsb@google.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: abangser <bangser.a@gmail.com> Co-authored-by: Christopher Fry <christopherfry@google.com>

natasha41575 force-pushed the PackageVariant branch 5 times, most recently from 1aeb6cd to 6d5eb69 Compare December 22, 2022 19:56

natasha41575 force-pushed the PackageVariant branch 8 times, most recently from 879a7a2 to b57e568 Compare December 28, 2022 21:06

natasha41575 changed the title ~~WIP: PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy~~ PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy Dec 28, 2022

natasha41575 requested a review from mortent December 28, 2022 21:07

natasha41575 marked this pull request as ready for review December 28, 2022 21:07

natasha41575 mentioned this pull request Dec 29, 2022

Add PackageVariantSet controller #3707

Merged

natasha41575 force-pushed the PackageVariant branch from b57e568 to 6a35f08 Compare January 10, 2023 18:12

mortent reviewed Jan 10, 2023

View reviewed changes

natasha41575 requested a review from mortent January 13, 2023 23:40

natasha41575 added 2 commits January 17, 2023 12:03

rename DownstreamPackage to PackageVariant

4a7844b

implement pruning, deletionPolicy, adoptionPolicy

4808986

natasha41575 force-pushed the PackageVariant branch from 0f6d256 to 1c5baa1 Compare January 17, 2023 18:05

address code review comments

4386e92

natasha41575 force-pushed the PackageVariant branch from 1c5baa1 to 4386e92 Compare January 17, 2023 20:51

mortent approved these changes Jan 18, 2023

View reviewed changes

natasha41575 merged commit af926a0 into kptdev:main Jan 18, 2023

natasha41575 deleted the PackageVariant branch January 18, 2023 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy #3701

PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy #3701

natasha41575 commented Dec 22, 2022 •

edited

johnbelamaric commented Dec 22, 2022

mortent left a comment

mortent Jan 10, 2023

natasha41575 Jan 13, 2023

mortent Jan 18, 2023

mortent Jan 18, 2023

mortent Jan 18, 2023

PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy #3701

PackageVariant controller: implement pruning, deletionPolicy, and adoptionPolicy #3701

Conversation

natasha41575 commented Dec 22, 2022 • edited

johnbelamaric commented Dec 22, 2022

mortent left a comment

Choose a reason for hiding this comment

mortent Jan 10, 2023

Choose a reason for hiding this comment

natasha41575 Jan 13, 2023

Choose a reason for hiding this comment

mortent Jan 18, 2023

Choose a reason for hiding this comment

mortent Jan 18, 2023

Choose a reason for hiding this comment

mortent Jan 18, 2023

Choose a reason for hiding this comment

natasha41575 commented Dec 22, 2022 •

edited