Support package base layer descriptor in package images #2932

hasheddan · 2022-03-01T21:09:37Z

Description of your changes

Updates the package manager (primarily the package revision controller) to honor io.crossplane.xpkg: base layers in Crossplane packages, as described in the xpkg specification. A few notes on the implementation:

The ImageBackend used for the package parser, which IMO was pretty bloated (and still is to some extent, but further improvements require restructuring parser interface in crossplane-runtime), is refactored to only deal with fetching package images and extracting the YAML stream. It does not handle caching.
If one layer descriptor is annotated with io.crossplane.xpkg: base, the ImageBackend will only download it for extracting the YAML stream. If no layer descriptor is annotated, the whole image is downloaded and flattened to extract the YAML stream, but only the stream contents are cached.
The revision reconciler is refactored to handle all caching directly, rather than deferring some of it to the ImageBackend, which created a leaky separation of concern.
In the event that package contents are not in the cache, we read the YAML stream contents while also passing them along to the cache to be written to a gzipped file concurrently.
In the event that package contents are in the cache, we stream them through the gzip reader to the parser.

Note: potential future optimizations can be made around the use of afero.TarFs, which reads file contents into a map. The simplest improvement would be to just return the tar.Reader in annotated packages since we require that the package.yaml is the only file in that layer. I am running some benchmarks to illustrate the change in performance before implementing.

Fixes #2888

Note: as detailed in the issue, additional machinery will be added to crank / crossplane-runtime to support writing annotated package layer descriptors.

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested with both existing packages (registry.upbound.io/upbound/platform-ref-azure:v0.1.0) and one with annotated layer descriptors (hasheddan/cool-layers:v0.0.3). Cache used a local directory mount:

🤖 (crossplane) k get pkgrev
NAME                                                                              HEALTHY   REVISION   IMAGE                                                   STATE    DEP-FOUND   DEP-INSTALLED   AGE
configurationrevision.pkg.crossplane.io/upbound-platform-ref-azure-2941a425862c   True      1          registry.upbound.io/upbound/platform-ref-azure:v0.1.0   Active   2           2               18s

NAME                                                                        HEALTHY   REVISION   IMAGE                                                   STATE    DEP-FOUND   DEP-INSTALLED   AGE
providerrevision.pkg.crossplane.io/crossplane-provider-azure-4ce0c0d45ac9   True      1          registry.upbound.io/crossplane/provider-azure:v0.18.1   Active                               14s
providerrevision.pkg.crossplane.io/crossplane-provider-helm-295bcd0e6dc3    True      1          registry.upbound.io/crossplane/provider-helm:v0.9.0     Active                               8s
providerrevision.pkg.crossplane.io/hasheddan-cool-layers-0a2ec46c4ef1       True      1          hasheddan/cool-layers:v0.0.3                            Active                               3m14s

🤖 (crossplane) ls -la .work/inttest-package-cache/ --block-size=K
total 420K
drwxrwxrwx 2 dan   dan     4K Mar  1 14:37 .
drwxrwxr-x 6 dan   dan     4K Mar  1 14:31 ..
-rw-r--r-- 1 65532 65532  33K Mar  1 14:37 crossplane-provider-azure-4ce0c0d45ac9.gz
-rw-r--r-- 1 65532 65532  13K Mar  1 14:37 crossplane-provider-helm-295bcd0e6dc3.gz
-rw-r--r-- 1 65532 65532 347K Mar  1 14:35 hasheddan-cool-layers-0a2ec46c4ef1.gz
-rw-r--r-- 1 65532 65532   9K Mar  1 14:37 upbound-platform-ref-azure-2941a425862c.gz

Also verified that unzipped contents of items in the package cache matched expected YAML stream.

Refactors the image cache to accept and return an io.ReadCloser, which it expects to deliver a valid package YAML stream. The stream is stored as a gzipped file. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the package cache tests to reflect the use of gzipped YAML streams rather than full OCI images. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the mock package cache to implement the required Has method. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the path name builder to allow passing file extension. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the crank build command to explicitly pass the .xpkg extension to the path builder. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Refactors the image parser backend to not depend on cache and instead only deal with pulling and extracting YAML streams from images. The cache will now be managed separately from the image backend, and the image backend will only be called in the event we don't have the data in the cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates image backend tests to remove caching functionality and exercise the layer annotation checks. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the revision reconciler to directly manage the cache instead of deferring the reponsibility to the image backend. The uncompressed file contents are read by the package parser and forwarded to the cache concurrently in the event that the contents are not already cached. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates revision reconciler tests with direct caching such that image backend is no longer responsible for writing new packages to the cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the crossplane start entrypoint to use PackageCache instead of ImageCache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

gzipFileReader allows returns an io.ReadCloser while still ensuring that the underlying file can be read, while the TeeReadCloser wraps TeeReader such that the Writer is closed when Close() is called. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

internal/controller/pkg/revision/imageback.go

JoinedReadCloser joins an io.Reader and an io.Closer. It is typically used in the case where a ReadCloser is wrapped by another Reader, but the caller needs to be able close the underlying ReadCloser when finished. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the image backend for the package parser to only read the package file contents from an image or image layer, rather than reading the entire filesystem contents into memory just to be able to extract the package file contents. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan

@muvaf @negz I updated to eliminate the tarfs usage as mentioned previously and when using a non-annotated fat package (using hasheddan/provider-aws-fat:v0.0.2 for testing), the reduction in allocations is significant as we are no longer reading the whole filesystem into memory just to get the package.yaml.

Note: these benchmarks are run only on ImageBackend.Init, not on the subsequent parsing, which is still quite inefficient due to how we are consuming the upstream k8s tooling.

With tarfs:

🤖 (revision) go test -bench=. -benchmem -memprofile memprofiletarfs.out -benchtime=20s
goos: linux
goarch: amd64
pkg: github.com/crossplane/crossplane/internal/controller/pkg/revision
cpu: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
BenchmarkInit-12    	      15	1536714280 ns/op	291899693 B/op	  237475 allocs/op
PASS
ok  	github.com/crossplane/crossplane/internal/controller/pkg/revision	55.692s

Without:

🤖 (revision) go test -bench=. -benchmem -memprofile memprofile.out -benchtime=20s
goos: linux
goarch: amd64
pkg: github.com/crossplane/crossplane/internal/controller/pkg/revision
cpu: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
BenchmarkInit-12    	      69	 336935786 ns/op	 4640937 B/op	    5012 allocs/op
PASS
ok  	github.com/crossplane/crossplane/internal/controller/pkg/revision	29.939s

hasheddan · 2022-03-03T17:14:03Z

A comparison of some of the cached content sizes as well:

🤖 (crossplane) ls .work/inttest-package-cache/ -la --block-size=K
total 2668K
drwxrwxrwx 2 dan   dan      4K Mar  3 12:09 .
drwxrwxr-x 6 dan   dan      4K Mar  1 14:31 ..
-rw-r--r-- 1 65532 65532  364K Mar  3 12:09 crossplane-provider-aws-f78664a342f1.gz
-rw-r--r-- 1 65532 65532   33K Mar  1 14:37 crossplane-provider-azure-4ce0c0d45ac9.gz
-rw-r--r-- 1 65532 65532   13K Mar  1 14:37 crossplane-provider-helm-295bcd0e6dc3.gz
-rw-r--r-- 1 65532 65532  299K Mar  3 12:07 hasheddan-provider-aws-fat-6ae095a00184.gz

Note: hasheddan/provider-aws-fat is using a slightly older version of provider-aws, hence the size difference from crossplane/provider-aws.

hasheddan/provider-aws-fat show the most obvious improvement here as the compressed image size is 25MB, but we are caching 299K.

negz

A few thoughts, but nothing blocking. I like the refactoring to pull the cache out of the image backend. Thanks @hasheddan!

negz · 2022-03-05T02:24:47Z

internal/controller/pkg/revision/imageback.go

+		// verify that we aren't just using the first layer annotated as xpkg
+		// base.
+		if foundAnnotated {
+			return nil, errors.New(errMultipleAnnotatedLayers)


Would it make sense to fall back to using the artifacts from the 'flattened' image in this case? I'm thinking no; it would perhaps be more compatible, but also less predictable and in violation of the spec.

It would be a violation of the spec so I would concur that we should not fall back if multiple annotated layers 👍🏻

negz · 2022-03-05T02:30:48Z

internal/xpkg/cache.go

-// thread-safe manner.
-type ImageCache struct {
+// PackageCache stores and retrieves package content in a filesystem-backed
+// cache in a thread-safe manner.


I imagine changing from a cache of packages to a cache of YAML streams has some implications:

We're slightly more committed to "the content of a Crossplane package" as far as the things the package manager needs to operate on a package being a single YAML stream. Probably not a problem?

Folks updating to this implementation will presumably have their package caches invalidated. Is It worth adding logic to cleanup the old image based cache? IMO probably not.

I don't think moving to caching just the YAML stream necessarily prohibits us from extending the package format in the future -- all packages would just need to be pulled again, which shouldn't be a huge issue.

I thought about this, but given that there is not a possibility for conflict with content previously written by the cache, I agree that the added complexity of trying to clean up is not worth it. One area I do want to note in the release notes is that if you had manually populated the cache, you'll need to repopulate it again with a different process. I don't think this is a common use case today though.

negz · 2022-03-05T02:34:26Z

internal/xpkg/cache.go

-type ImageCache struct {
+// PackageCache stores and retrieves package content in a filesystem-backed
+// cache in a thread-safe manner.
+type PackageCache struct {


Naming nit: Aren't all hypothetical instances of Cache (the interface) "a package cache"? What do you think about:

type Cache interface -> type PackageCache interface

type PackageCache struct -> type FilesystemPackageCache struct (or similar)

Great call out -- I think I'll opt for PackageCache and FsPackageCache 👍🏻

negz · 2022-03-05T02:37:17Z

internal/xpkg/cache.go

+	Get(id string) (io.ReadCloser, error)
+	Store(id string, content io.ReadCloser) error


Should we clarify what format content io.ReadCloser is expected to contain - i.e. a gzipped xpkg compliant YAML stream? In particular the fact that it's gzipped wasn't obvious to me until I read the implementation.

The content here is actually not gzipped. Content comes into the cache uncompressed, Store wraps the file being written to in a gzip writer, and Get wraps the file being read in a gzip reader.

negz · 2022-03-05T02:44:04Z

internal/xpkg/reader.go

+}
+
+// GzipFileReader constructs a new gzipFileReader from the passed file.
+func GzipFileReader(f afero.File) (io.ReadCloser, error) {


Does this need to take afero.File as an argument? At a glance it seems like an io.ReadCloser would work.

Nope -- good catch! This can just be a GzipReadCloser 👍🏻

negz · 2022-03-05T02:48:54Z

internal/controller/pkg/revision/reconciler.go

+	}
+
+	var rc io.ReadCloser
+	var err error


Could this err be more tightly scoped? I think it's only used in the if r.cache.Has block, right?

Absolutely, I think this is a relic of a previous iteration, thanks for pointing out!

Changes gzipFileReader to gzipReadCloser and accepts a ReadCloser rather than an afero File. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Uses more specific PackageCache for Cache and FsPackageCache implementation for filesystem backed package cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan added 11 commits March 1, 2022 14:53

Refactor image cache into package content cache

5d1b9fc

Refactors the image cache to accept and return an io.ReadCloser, which it expects to deliver a valid package YAML stream. The stream is stored as a gzipped file. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Update package cache tests to use gzipped files

e5f681d

Updates the package cache tests to reflect the use of gzipped YAML streams rather than full OCI images. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Update mock package cache with Has functionality

89ca24d

Updates the mock package cache to implement the required Has method. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Allos passing file extension to path name builder

aaf08d8

Updates the path name builder to allow passing file extension. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Pass xpkg extension in crank build

8766411

Updates the crank build command to explicitly pass the .xpkg extension to the path builder. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Update image backend tests to remove caching functionality

566ba44

Updates image backend tests to remove caching functionality and exercise the layer annotation checks. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Update revision reconciler tests with direct caching

e01e0ea

Updates revision reconciler tests with direct caching such that image backend is no longer responsible for writing new packages to the cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Use PackageCache instead of ImageCache in crossplane entrypoint

9676870

Updates the crossplane start entrypoint to use PackageCache instead of ImageCache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan requested review from negz and muvaf March 1, 2022 21:09

hasheddan commented Mar 2, 2022

View reviewed changes

internal/controller/pkg/revision/imageback.go Outdated Show resolved Hide resolved

hasheddan added 2 commits March 3, 2022 11:30

hasheddan commented Mar 3, 2022

View reviewed changes

negz approved these changes Mar 5, 2022

View reviewed changes

hasheddan added 2 commits March 6, 2022 12:55

Generalize gzipFileReader as gzipReadCloser

ffcc76c

Changes gzipFileReader to gzipReadCloser and accepts a ReadCloser rather than an afero File. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Change Cache to PackageCache and PackageCache to FsPackageCache

49d656c

Uses more specific PackageCache for Cache and FsPackageCache implementation for filesystem backed package cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan force-pushed the annotated-descriptors branch from 3cd87f7 to 49d656c Compare March 6, 2022 18:08

hasheddan merged commit fff1001 into crossplane:master Mar 7, 2022

This was referenced Mar 7, 2022

feat(ack): bump ack & aws-go-sdk-v1 to 1.42.0 crossplane-contrib/provider-aws#1122

Merged

feat(s3): bucketKeyEnabled implemented crossplane-contrib/provider-aws#1158

Merged

e2e test failed to get pre-cached package with pull policy Never #2944

Closed

hasheddan mentioned this pull request May 5, 2022

Add up xpkg build --controller functionality upbound/up#176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support package base layer descriptor in package images #2932

Support package base layer descriptor in package images #2932

hasheddan commented Mar 1, 2022

hasheddan left a comment •

edited

hasheddan commented Mar 3, 2022

negz left a comment

negz Mar 5, 2022

hasheddan Mar 5, 2022

negz Mar 5, 2022

hasheddan Mar 6, 2022

negz Mar 5, 2022

hasheddan Mar 6, 2022

negz Mar 5, 2022

hasheddan Mar 6, 2022

negz Mar 5, 2022

hasheddan Mar 6, 2022

negz Mar 5, 2022

hasheddan Mar 6, 2022

		Get(id string) (io.ReadCloser, error)
		Store(id string, content io.ReadCloser) error

Support package base layer descriptor in package images #2932

Support package base layer descriptor in package images #2932

Conversation

hasheddan commented Mar 1, 2022

Description of your changes

How has this code been tested

hasheddan left a comment • edited

Choose a reason for hiding this comment

hasheddan commented Mar 3, 2022

negz 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan left a comment •

edited