create DownstreamPackage controller #3684

natasha41575 · 2022-12-02T23:12:57Z

What this PR does:

adds a DownstreamPackage controller as discussed in Porch: BaseRevision controller aka Fan Out controller - but more #3488
implements creation and updating of deployment packages

What this PR does not do (will be coming in a followup PR):

Implement pruning of deployment packages that are no longer needed
Implement the proposed AdoptionPolicy field, which will allow the user to choose whether the controller adopts existing package revisions or ignore them

natasha41575 · 2022-12-02T23:13:30Z

cc @johnbelamaric

porch/controllers/downstreampackages/api/v1alpha1/downstreampackage_types.go

mortent · 2022-12-03T17:07:57Z

porch/controllers/downstreampackages/api/v1alpha1/downstreampackage_types.go

+
+type Upstream struct {
+	Repo     string `json:"repo,omitempty"`
+	Package  string `json:"package,omitempty"`


It seems like this essentially identifies a single PackageRevision resource. Should we use the name of the resource instead of Package and Revision?

Given that the resource name of package revisions is auto-generated, that would make this resource very hard to create and read for users, and would require the creation of the package revision prior to the creation of this resource. Generally that's probably going to be the case, but frankly the package revision name is more like a machine generated pointer than something useful. Of course, if this resource is being generated by a controller, this is not really an issue. Usually that is probably the case.

The expected general use of this would be to update the Revision number and trigger upgrades (or downgrades). So keeping these separate makes that easier. This does raise the issue of what happens if someone points this resource at a completely different upstream package, with respect to updates.

I see @johnbelamaric's point that if someone is trying to manage a DownstreamPackage resource manually, looking up the package revision resource name for each update can be tedious and perhaps error prone, since it is an auto-generated hash. But the DownstreamPackage is generally going to be created by the UpstreamPolicy controller when it is ready, so this will be less of an issue.

Having the repo/package/revision separate means that the DownstreamPackage controller has put a little more effort into looking up the upstream package revision. If it instead takes the name of the packagerevision resource, the UpstreamPolicy controller will have to do the lookup and pass that to the DownstreamPackage. Either way, we will need to have that lookup somewhere; I think the question is just which controller should we put it in. Personally I don't think that it matters too much, but since the UpstreamPolicy controller is already going to be somewhat complex, maybe it makes sense to delegate the lookup to the DownstreamPackage, and keep the repo/package/revision separate here. That said, I don't mind changing it if you feel differently.

This does raise the issue of what happens if someone points this resource at a completely different upstream package, with respect to updates.

IIRC porch doesn't let you update a package to a completely different package. We started discussing what that means here. I think that in order to completely change the upstream package of the DownstreamPackage, you'd also have to change (or delete) the downstream package target, essentially resulting in a brand new deployment package.

Yeah, I'm ok with using Package and Revision. We should see if we can use field selectors to be able to do the filtering server-side instead of client-side.

I don't actually remember if kpt errors out if the new upstream is a completely different package. We should probably verify the behavior here. Regardless we need to be prepared for situations where the automatic update fails, since there might be conflicts that can't be automatically resolved.

...rollers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go

mortent · 2022-12-03T17:36:26Z

...rollers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go

+func (r *DownstreamPackageReconciler) isUpToDate(dp *api.DownstreamPackage, downstream *porchapi.PackageRevision) bool {
+	upstreamLock := downstream.Status.UpstreamLock
+	lastIndex := strings.LastIndex(upstreamLock.Git.Ref, "/")
+	if strings.HasPrefix(upstreamLock.Git.Ref, "drafts") {


Can't we just check the lifecycle field on the PackageRevision for this?

We have the target upstream package revision (the one we want to update to), and the current downstream package revision. But we don't have the upstream package that the current downstream package is currently based on - which is what we need in order to determine if downstream package revision is up to date.

All we have is the reference to that current upstream package revision in the downstream package's upstreamLock.Git.Ref - so no, we don't have a PackageRevision.Lifecycle field to refer to here.

Fetching that current upstream package revision involves tedious loops and string parsing of upstreamLock.Git.Ref (and somewhere in that logic would still include checking for the drafts prefix so we can parse the ref properly), so IMO just checking for the drafts prefix is the best thing to do here.

So we could use the lifecycle field for checking if the PackageRevision is a draft, but agree we would still need to parse the upstreamLock to determine the current upstream packagerevision it is based on. So I think doing it this way is fine.
Long term we need to figure out a better way to expose information about the upstream of a package in PackageRevision API. If we want this controller to also support OCI, this would get pretty complicated.

...rollers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go

mortent

Just two small nits, but overall looks good.

We also need to find a pattern for how we want to test the controllers, but that is an issue with all of them at the moment.

mortent · 2022-12-07T15:13:51Z

porch/controllers/downstreampackages/api/v1alpha1/downstreampackage_types.go

+	AdoptionPolicyAdoptNone     AdoptionPolicy = "adoptNone"
+
+	DeletionPolicyDelete DeletionPolicy = "delete"
+	DeletionPolicyDisown DeletionPolicy = "orphan"


Nit: We should also update the variable name to use the term orphan.

mortent · 2022-12-07T15:18:00Z

porch/controllers/downstreampackages/api/v1alpha1/downstreampackage_types.go

+
+type Upstream struct {
+	Repo     string `json:"repo,omitempty"`
+	Package  string `json:"package,omitempty"`


Yeah, I'm ok with using Package and Revision. We should see if we can use field selectors to be able to do the filtering server-side instead of client-side.

I don't actually remember if kpt errors out if the new upstream is a completely different package. We should probably verify the behavior here. Regardless we need to be prepared for situations where the automatic update fails, since there might be conflicts that can't be automatically resolved.

mortent · 2022-12-07T15:21:53Z

porch/controllers/downstreampackages/config/rbac/rolebinding.yaml

+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: porch-system:porch-controllers-downstreampackage


The convention I've been trying to follow with the other controllers is that the suffix here should match the name of the directory for the controller. So we should make this porch-controllers-downstreampackages.

mortent · 2022-12-07T15:40:58Z

...rollers/downstreampackages/pkg/controllers/downstreampackage/downstreampackage_controller.go

+func (r *DownstreamPackageReconciler) isUpToDate(dp *api.DownstreamPackage, downstream *porchapi.PackageRevision) bool {
+	upstreamLock := downstream.Status.UpstreamLock
+	lastIndex := strings.LastIndex(upstreamLock.Git.Ref, "/")
+	if strings.HasPrefix(upstreamLock.Git.Ref, "drafts") {


So we could use the lifecycle field for checking if the PackageRevision is a draft, but agree we would still need to parse the upstreamLock to determine the current upstream packagerevision it is based on. So I think doing it this way is fine.
Long term we need to figure out a better way to expose information about the upstream of a package in PackageRevision API. If we want this controller to also support OCI, this would get pretty complicated.

johnbelamaric · 2022-12-07T16:45:34Z

Regardless we need to be prepared for situations where the automatic update fails, since there might be conflicts that can't be automatically resolved

Yeah, I think we'll need to raise an error in the package revision status.

natasha41575 requested review from mortent and justinsb December 2, 2022 23:12

natasha41575 force-pushed the downstreamPackageController branch from 1771f69 to 7cd6edd Compare December 2, 2022 23:15

mortent reviewed Dec 3, 2022

View reviewed changes

mortent approved these changes Dec 7, 2022

View reviewed changes

natasha41575 added 3 commits December 7, 2022 16:44

create DownstreamPackage controller

7a36aba

address code review comments

d770faf

address review comments round 2

ec996c8

natasha41575 force-pushed the downstreamPackageController branch from 4bc787a to ec996c8 Compare December 7, 2022 22:45

natasha41575 merged commit 3657aec into kptdev:main Dec 8, 2022

natasha41575 deleted the downstreamPackageController branch December 8, 2022 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create DownstreamPackage controller #3684

create DownstreamPackage controller #3684

natasha41575 commented Dec 2, 2022

natasha41575 commented Dec 2, 2022

mortent Dec 3, 2022

johnbelamaric Dec 3, 2022

natasha41575 Dec 5, 2022 •

edited

Loading

mortent Dec 7, 2022

mortent Dec 3, 2022

natasha41575 Dec 5, 2022 •

edited

Loading

mortent Dec 7, 2022

mortent left a comment

mortent Dec 7, 2022

mortent Dec 7, 2022

mortent Dec 7, 2022

mortent Dec 7, 2022

johnbelamaric commented Dec 7, 2022

create DownstreamPackage controller #3684

create DownstreamPackage controller #3684

Conversation

natasha41575 commented Dec 2, 2022

natasha41575 commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbelamaric commented Dec 7, 2022

natasha41575 Dec 5, 2022 •

edited

Loading

natasha41575 Dec 5, 2022 •

edited

Loading