Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Package resource: defining types and interface methods #3469

Merged
merged 8 commits into from
Sep 1, 2022

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented Aug 17, 2022

The complete implementation of a Package resource was getting quite large so I decided to split it up into multiple PRs. This is the first one. Issue link: #3213

This PR defines the types and some methods needed to support implementation of the 'Package' resource. It leaves some TODOs that I will tackle in a followup PR - specifically, I will address all the TODOs for the git implementation of a Package.

This PR results in the following output:

$ kubectl api-resources | grep porch
repositories                                   config.porch.kpt.dev/v1alpha1          true         Repository
functions                                      porch.kpt.dev/v1alpha1                 true         Function
packagerevisionresources                       porch.kpt.dev/v1alpha1                 true         PackageRevisionResources
packagerevisions                               porch.kpt.dev/v1alpha1                 true         PackageRevision
packages                                       porch.kpt.dev/v1alpha1                 true         Package
$ kubectl apply -f package.yaml 
Error from server (InternalError): error when creating "package.yaml": Internal error occurred: CreatePackage not yet supported for git repos
$ kubectl get packages -ndefault
Error from server: ListPackages not yet supported for git repos

@natasha41575 natasha41575 changed the title Package resource Define types and methods needed for Package resource Aug 17, 2022
@natasha41575 natasha41575 changed the title Define types and methods needed for Package resource Add Package resource: defining types and interface methods Aug 17, 2022
@natasha41575
Copy link
Contributor Author

cc @mortent @justinsb

@natasha41575
Copy link
Contributor Author

@justinsb @mortent CI is finally passing and this is ready for review, PTAL whenever you can

Copy link
Contributor

@mortent mortent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR seems to mostly contain two things:

  • Renaming of existing code where we have referred to packagerevisions as packages.
  • The scaffolding necessary to handle the package resource.

The first set of changes seems pretty straightforward. For the second, I'm wondering if we can reduce the amount of duplicate code or is that easier to handle separately?

@@ -68,6 +72,20 @@ func (c *cachedPackageRevision) GetPackageRevision() *v1alpha1.PackageRevision {

var _ repository.PackageRevision = &cachedPackageRevision{}

type cachedPackage struct {
repository.Package
latestPackageRevision string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we make sure that this is up-to-date? Does performing approval/deletion directly on a packagerevision cause this to be updated?

Copy link
Contributor Author

@natasha41575 natasha41575 Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current PR, deletion flushes the cache. I didn't consider approval, but I will update the PR so that approval flushes the latestPackageRevision part of the cache. I'll take a closer look at what packagerevisions does. Thanks for the catch!

Copy link
Contributor Author

@natasha41575 natasha41575 Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like with packagerevisions, the latest package revisions gets updated in the cached package revisions every time a draft "closes" (which includes approval). I added a TODO this PR (in *cachedRepository.update) to also update the cached packages' latest revisions in the same place, which I can implement in the next PR.

if c.latestPackageRevision != "" {
return c.latestPackageRevision
}
return c.Package.GetLatestRevision()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an expensive operation? If not, do we need to cache it separately?

Copy link
Contributor Author

@natasha41575 natasha41575 Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be a nontrivial operation, though I haven't fully thought out the implementation yet (it is currently just an unimplemented interface). I think the implementation will be something like getting all the package revisions related to this package (possibly through git, or possibly from the already cached revisions), and then calculating the latest revision for each package with semver comparisons.

It's not necessary to cache if we are okay with iterating over the package revisions each time, but that seems like a lot of duplicate work to me.

FWIW, cachedpackagerevisions does cache an isLatestRevision bool, so that may be a precedent to follow.

porch/pkg/engine/engine.go Show resolved Hide resolved
@@ -246,6 +272,8 @@ func (r *gitRepository) CreatePackageRevision(ctx context.Context, obj *v1alpha1

draft := createDraftName(obj.Spec.PackageName, obj.Spec.Revision)

// TODO: This should also create a new 'Package' resource if one does not already exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a little tricky. We do end up with two separate paths for creating a package, and they are not equivalent since it is not possible to create a package based on a clone through the Package resource.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this. If we better define how PackageRevisions evolve (ref #3467) so that v2 always is an edit/copy of v1, then a package will always have a single starting point that must be the same across all revisions. If so, we could potentially define the clone/init information in the package resource.

porch/api/go.mod Outdated Show resolved Hide resolved
@@ -138,6 +156,37 @@ func (r *cachedRepository) getPackages(ctx context.Context, filter repository.Li
return toPackageRevisionSlice(packages, filter), nil
}

func (r *cachedRepository) getPackages(ctx context.Context, filter repository.ListPackageFilter, forceRefresh bool) ([]repository.Package, error) {
r.mutex.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this similar to the code used for packagerevisions? The locking scheme here looks somewhat complicated, so I'm wondering if we should keep it simple until we know whether this is a critical path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, a lot of the code for packages is almost identical to packagerevisions. I think your suggestion below to use generics may help simplify this, so we don't duplicate the complicated locking scheme.

Copy link
Contributor Author

@natasha41575 natasha41575 Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this section of the code, PTAL and let me know what you think. I think thecachedPackages is as critical to lock as cachedPackageRevisions (they're serving the same purpose), so I do think the lock/unlock mechanism is important to keep here. I just refactored it slightly and would like your feedback.

porch/pkg/registry/porch/packagecommon.go Show resolved Hide resolved
porch/pkg/registry/porch/package.go Show resolved Hide resolved
@natasha41575
Copy link
Contributor Author

For the second, I'm wondering if we can reduce the amount of duplicate code or is that easier to handle separately?

Is there a way we can split some of this out into a common library? We have upgraded to go 1.18, so we can use generics.

That's a good idea, I'll spend some time taking a look at generics to see if we can reuse the common boilerplate.

@natasha41575 natasha41575 force-pushed the PackageResource branch 2 times, most recently from 5443ae2 to 2a604b2 Compare August 24, 2022 22:39
latestPackageRevision string
}

func (c *cachedPackage) GetLatestRevision() string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took the definitions here and in cache/packagerevision.go out of cache/repository.go for organization

// TODO: Currently we support repositories with homogenous content (only packages xor functions). Model this more optimally?
cachedFunctions []repository.Function
// Error encountered on repository refresh by the refresh goroutine.
// This is returned back by the cache to the background goroutine when it calls periodicall to resync repositories.
refreshError error
refreshRevisionsError error
refreshPkgsError error
Copy link
Contributor Author

@natasha41575 natasha41575 Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I separated this into two because I felt that an error with getting packages didn't need to block getting package revisions (since refreshError is returned whenever there is a value stored in it).

Combining it into one bool leads to weird error messages. For example:

$ kubectl get packagerevisions -ndefault
Error from server: ListPackages not yet supported for git repos

$ kpt alpha rpkg get
Error from server: ListPackages not yet supported for git repos (this is the refreshError stored from the previous failure)

So I figured it would be better to separate so instead we get

$ kubectl get packagerevisions -ndefault
Error from server: ListPackages not yet supported for git repos

$ kpt alpha rpkg get
.... (normal output)

@natasha41575 natasha41575 force-pushed the PackageResource branch 2 times, most recently from b519f88 to 59ab260 Compare August 24, 2022 23:19
@natasha41575
Copy link
Contributor Author

@mortent I updated this PR to deduplicate some of the code (and address a few of your other comments). Still looking into the CI failure, but other than that this is ready for another review.

CreatePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, obj *api.PackageRevision) (repository.PackageRevision, error)
UpdatePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, oldPackage repository.PackageRevision, old, new *api.PackageRevision) (repository.PackageRevision, error)

Create(ctx context.Context, repositoryObj *configapi.Repository, obj api.PackageObject) (runtime.Object, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit uncertain about this. I think it is clearer if we have separate functions on the interface for Package and PackageRevision. We can do things on the implementation to avoid duplication, but it seems like the interface becomes hard to understand in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, got pulled into something else this morning.

I'll change the interface back so that we have separate functions here.

Copy link
Contributor Author

@natasha41575 natasha41575 Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time today on this, I think this comment (which I do strongly agree with) and trying to avoid awkward switch statements with a lot of type casting (your other comment) led me back to something a little bit closer to https://github.com/GoogleContainerTools/kpt/pull/3469/files/07f6cac489b04f32bea97f70e19fd5ef94246215

I've refactored the code in these files to avoid duplication where I can, by moving the common things out to packageCommon. I experimented with generics but didn't see a clear way for them to help here. Happy to revisit that there was something there that I didn't see.

I think me calling it "boilerplate" earlier was not the most accurate. It is similar, but not identical, to the definition of packagerevisions and so I think keeping it in separate files makes sense, with the common functionality moved to packageCommon. There are also other parts of the code (for example, the ListPackageFilter and ListPackageRevisionFilter) where again, they look very similar, but IMO make sense to separate because they are handling the different types. If there are other areas of code that could be refactored out, please feel free to give suggestions.

@@ -246,6 +272,8 @@ func (r *gitRepository) CreatePackageRevision(ctx context.Context, obj *v1alpha1

draft := createDraftName(obj.Spec.PackageName, obj.Spec.Revision)

// TODO: This should also create a new 'Package' resource if one does not already exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this. If we better define how PackageRevisions evolve (ref #3467) so that v2 always is an edit/copy of v1, then a package will always have a single starting point that must be the same across all revisions. If so, we could potentially define the clone/init information in the package resource.

porch/pkg/repository/repository.go Outdated Show resolved Hide resolved
porch/pkg/git/testing_repo.go Outdated Show resolved Hide resolved
porch/api/porch/v1alpha1/register.go Outdated Show resolved Hide resolved
porch/pkg/repository/repository.go Outdated Show resolved Hide resolved
@natasha41575 natasha41575 force-pushed the PackageResource branch 2 times, most recently from 443039a to 3f5b0c9 Compare August 27, 2022 00:22
@natasha41575 natasha41575 merged commit 6c78c2e into kptdev:main Sep 1, 2022
@natasha41575 natasha41575 deleted the PackageResource branch September 1, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants