Nephio is a Kubernetes-based automation platform for deploying and managing highly distributed, interconnected workloads such as 5G Network Functions, and the underlying infrastructure on which those workloads depend.
Original issue URL: https://github.com/kptdev/kpt/issues/3630
Original issue user: https://github.com/mortent
Original issue created at: 2022-10-19T17:29:53Z
Original issue last updated at: 2023-10-19T15:48:21Z
Original issue body: ## Background
Porch is primarily built as an aggregated APIServer with the PackageRevision and PackageRevisionResources resources, as well as their subresources, implemented that way rather than as CRDs. Up until recently the only datastore used for these resources were git/oci, although with https://github.com/GoogleContainerTools/kpt/pull/3579 we also store data pertaining to each PackageRevision in CRs as well.
It has become clear that in order to make the resources provided by the aggregated APIServer behave like regular resources, it is not possible to only rely on storing data in git commits. That we recently started using CRs in addition to git is a consequence of this (We also discussed alternatives here), but having to storage backends for a resource leads to other issues. It is also non-trivial to implement all aspects of the Kubernetes API through an aggregated APIServer when using a different storage backend that etcd. Examples are proper resourceVersions, generations, and correct semantics for watches.
As a result, we have discussed migrating the PackageRevision type to a CRD.
Details
Implementing the PackageRevision type as a CRD has many benefits, as CRDs already provide all the operations that are expected from KRM resources. But it also comes with limitations as compared to an aggregated apiserver:
CRDs does not support other subresources than “status” and “scale’.
Validation is more limited, although some of this can be handled with webhooks.
All “logic” must be handled asynchronously by controllers.
As a result, switching to using a CRD involves addressing at least the following items (more will probably come up):
Migrate the repository polling logic into a controller that will create PackageRevisions through the regular API rather than doing it internally.
Figure out how we can handle both deletion of PackageRevision (The PackageRevision resource is delete from the Kubernetes API and the underlying data in git/oci is removed) and unregistration of a PackageRevision (The PackageRevision resource is deleted from the Kubernetes API, but the data remains in git). The latter is typically needed when a repository is removed from Porch, which also requires that the PackageRevisions in the Repository are also removed.
Figure out how to handle what we currently implement as subresources, primarily the “approve” subresource.
All “logic” related to changes in state for PackageRevisions must happen in controllers, and therefore will happen asynchronously. We need to identify what logic we actually do have related to PackageRevisions that will not be automatically happened by the APIServer with CRDs. As an example, all syncing to git will need to happen asynchronously.
The PackageRevisionResources type will almost certainly still need to be supported through the aggregated APIServer. Storing package content in CRDs does not seem like a feasible solution. But we need to determine how the PackageRevisionResources type will align with a PackageRevision type that is a CRD. Currently, we consider them as two separate views of the same underlying resource. We have discussed an alternative where we only allow push and pull of resources through the PackageRevisionResource, but it needs more thought. If we could have a PackageRevision CRD with subresources backed with an aggregated APIServer, it seems like we would like to have something like packagerevisions/push and packagerevisions/pull, but combining CRDs and aggregated APIServer in this way is not possible.
How do we get there?
We essentially have two ways to approach this.
Full rewrite now
This is taking the “quick(?) and painful” approach and do the full rewrite in one go. This seems very risky as this is a major change to Porch with several challenges, and we will almost certainly discover additional challenges on the way.
Gradual rewrite
So instead of trying to do all these changes in one massive effort, we can do this gradually.
What makes this approach compelling, is that we can make all the necessary changes without actually migrating to using a CRD. We can migrate away from using subresources, move the polling of repositories, and migrate logic to controllers using the current type from the Aggregated APIServer, but it allows us to combine this effort with adding new features (that hopefully doesn’t increase our dependency on the aggregated APIServer). This also allows to reconsider this change if discover that it is misguided.
The path forward is essentially to start addressing the issues listed above and start documenting additional issues we come across.
Original issue comments:
Comment user: https://github.com/natasha41575
Comment created at: 2022-10-19T19:59:33Z
Comment last updated at: 2022-10-19T20:18:14Z
Comment body: I agree that switching to a CRD makes the most sense in the long run. A couple short comments:
Gradual rewrite sounds much more compelling to me than the full rewrite. Seems like a much more feasible and less risky approach.
For deletion vs unregister, there is a similar-ish problem for the UpstreamPolicy/BaseRevisions controller. For that, we discussed having a deletionPolicy field in the spec, that determines whether to completely delete or just "disown". Perhaps we could consider something similar here?
Comment user: https://github.com/mortent
Comment created at: 2023-01-26T04:19:47Z
Comment last updated at: 2023-01-26T04:19:47Z
Comment body: We should try to split this up into several issues. The list in this issue is a start, but I suspect there will be more.
Comment user: https://github.com/johnbelamaric
Comment created at: 2023-10-19T15:46:39Z
Comment last updated at: 2023-10-19T15:48:21Z
Comment body: So, what would a "v1alpha2" PackageRevision resource look like if it is completely CRD-based? I think we're going to need a thorough design on this. I see it as an opportunity to revisit the whole Package/PR/PRR model. A few thoughts, these are just questions that come up immediately for me:
As @mortent said, PR would no longer be a "view" of the underlying resource. I think instead it would be a sort of "index" into the underlying repository, and a control structure for coordinating use of that index.
Since it no longer directly represents the underlying resource, we should now be able to create a PackageRevision with any name, that simply references a specific repository, directory, branch, tag, or commit. What should be immutable? Today, a specific PR refers to a specific tag or branch, and package name == directory at the root of the Repository resource, but the underlying thing they point to changes as the lifecycle changes. So, we can't really make those immutable.
Our opinionated versioning scheme has led to some awkwardness at times in Nephio.
If we are creating the PRs directly, we can manually create them, or they can be auto-discovered. Our current repo polling in porch-server would move to a "package revision discovery" controller which creates and owns PRs. But there is no reason why we can't run Porch without that controller, and just manually create resources pointing to specific tags or whatever.
PR names can become more sensible, perhaps something like <repo>-<package>-<something>, but I am not sure.
I am not sure how lifecycle would work. Like I said above, today when we change lifecycle from "Draft" to "Proposed", we move things to a different branch. When we move from "Proposed" to "Published" we move to the main branch and a tag. I am not sure we want to keep doing it that way. It leads to some weirdness, because the version on the "main" branch has some special significance, and I am not sure what the meaning of that is in the model. For example, if you "rollback" from v8 to v7, then "main" has v7 but v8 is still the latest. And how do you not have two v7s (the original commit plus the new main, because maybe 50 other things happened in the repo between original v7 tag and the new main version of v7 - is that now v9? How do we do that today?)
It might be useful to step back and look at what we have learned and see what our ideal model would look like, then figure out how to get there from here.
Original issue URL: https://github.com/kptdev/kpt/issues/3630 Original issue user: https://github.com/mortent Original issue created at: 2022-10-19T17:29:53Z Original issue last updated at: 2023-10-19T15:48:21Z Original issue body: ## Background Porch is primarily built as an aggregated APIServer with the PackageRevision and PackageRevisionResources resources, as well as their subresources, implemented that way rather than as CRDs. Up until recently the only datastore used for these resources were git/oci, although with https://github.com/GoogleContainerTools/kpt/pull/3579 we also store data pertaining to each PackageRevision in CRs as well.
It has become clear that in order to make the resources provided by the aggregated APIServer behave like regular resources, it is not possible to only rely on storing data in git commits. That we recently started using CRs in addition to git is a consequence of this (We also discussed alternatives here), but having to storage backends for a resource leads to other issues. It is also non-trivial to implement all aspects of the Kubernetes API through an aggregated APIServer when using a different storage backend that etcd. Examples are proper resourceVersions, generations, and correct semantics for watches.
As a result, we have discussed migrating the PackageRevision type to a CRD.
Details
Implementing the PackageRevision type as a CRD has many benefits, as CRDs already provide all the operations that are expected from KRM resources. But it also comes with limitations as compared to an aggregated apiserver:
As a result, switching to using a CRD involves addressing at least the following items (more will probably come up):
The PackageRevisionResources type will almost certainly still need to be supported through the aggregated APIServer. Storing package content in CRDs does not seem like a feasible solution. But we need to determine how the PackageRevisionResources type will align with a PackageRevision type that is a CRD. Currently, we consider them as two separate views of the same underlying resource. We have discussed an alternative where we only allow push and pull of resources through the PackageRevisionResource, but it needs more thought. If we could have a PackageRevision CRD with subresources backed with an aggregated APIServer, it seems like we would like to have something like
packagerevisions/pushandpackagerevisions/pull, but combining CRDs and aggregated APIServer in this way is not possible.How do we get there?
We essentially have two ways to approach this.
Full rewrite now
This is taking the “quick(?) and painful” approach and do the full rewrite in one go. This seems very risky as this is a major change to Porch with several challenges, and we will almost certainly discover additional challenges on the way.
Gradual rewrite
So instead of trying to do all these changes in one massive effort, we can do this gradually.
What makes this approach compelling, is that we can make all the necessary changes without actually migrating to using a CRD. We can migrate away from using subresources, move the polling of repositories, and migrate logic to controllers using the current type from the Aggregated APIServer, but it allows us to combine this effort with adding new features (that hopefully doesn’t increase our dependency on the aggregated APIServer). This also allows to reconsider this change if discover that it is misguided.
The path forward is essentially to start addressing the issues listed above and start documenting additional issues we come across.
Original issue comments: Comment user: https://github.com/natasha41575 Comment created at: 2022-10-19T19:59:33Z Comment last updated at: 2022-10-19T20:18:14Z Comment body: I agree that switching to a CRD makes the most sense in the long run. A couple short comments:
deletionPolicyfield in the spec, that determines whether to completely delete or just "disown". Perhaps we could consider something similar here?Comment user: https://github.com/mortent Comment created at: 2023-01-26T04:19:47Z Comment last updated at: 2023-01-26T04:19:47Z Comment body: We should try to split this up into several issues. The list in this issue is a start, but I suspect there will be more.
Comment user: https://github.com/johnbelamaric Comment created at: 2023-10-19T15:46:39Z Comment last updated at: 2023-10-19T15:48:21Z Comment body: So, what would a "v1alpha2" PackageRevision resource look like if it is completely CRD-based? I think we're going to need a thorough design on this. I see it as an opportunity to revisit the whole Package/PR/PRR model. A few thoughts, these are just questions that come up immediately for me:
<repo>-<package>-<something>, but I am not sure.