Skip to content

WIP: pkg/archive: move compression to a separate package #49167

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

Closed
wants to merge 4 commits into from

Conversation

thaJeztah
Copy link
Member

This is really (really!) WIP; something I was considering if this would make sense to do; fun fact; when I started doing this, I noticed that containerd's fork of pkg/archive did exactly the same, so doing this would align our implementation more with containerd's approach; https://github.com/containerd/containerd/tree/v2.0.1/pkg/archive/compression

containerd has some minor differences in the Compression type;

In reverse, Moby;

  • Supports additional compression types (e.g. Xz, Bzip2), which are not supported in containers)
  • Has an atomic closed field for readCloserWrapper

    moby/pkg/archive/archive.go

    Lines 221 to 233 in a72026a

    closed atomic.Bool
    }
    func (r *readCloserWrapper) Close() error {
    if !r.closed.CompareAndSwap(false, true) {
    log.G(context.TODO()).Error("subsequent attempt to close readCloserWrapper")
    if log.GetLevel() >= log.DebugLevel {
    log.G(context.TODO()).Errorf("stack trace: %s", string(debug.Stack()))
    }
    return nil
    }
    return r.closer()

I'm not sure if we actually need that one though, as it was added to work around an OTEL issue, and not sure if we actually hit that codepath in this code; 585d74b (#46419)

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@dmcgowan
Copy link
Member

Supports additional compression types (e.g. Xz, Bzip2), which are not supported in containers)

These can still be supported with containerd, they just need to be registered

@thaJeztah
Copy link
Member Author

Ah, right; yes, I was also looking if we should split out parts to make them more modular "register compression X"

Comment on lines 131 to 134
// DecompressStream decompresses the archive and returns a ReaderCloser with the decompressed archive.
//
// Deprecated: use [compression.DecompressStream].
func DecompressStream(archive io.Reader) (io.ReadCloser, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! Right, linter won't be happy yet, because I did not yet update code calling these moved functions;

internal/testutils/archive.go:12:13: SA1019: archive.DecompressStream is deprecated: use [compression.DecompressStream]. (staticcheck)
	rd, err := archive.DecompressStream(compressedTar)
	           ^

type Compression int

const (
Uncompressed Compression = 0 // Uncompressed represents the uncompressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about compression.None instead of compression.Uncompressed?

Suggested change
Uncompressed Compression = 0 // Uncompressed represents the uncompressed.
None Compression = 0 // None represents the uncompressed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, that works for me; nice and short.

TODO: align with containerd pkg/archive/compression, which

- also moved the utilities to Compress/Decompress streams
- changed the Extension to not include ".tar"

Signed-off-by: Sebastiaan van Stijn <[email protected]>
It's only used in tests now, so move it there, and simplify, removing
the atomic bool and logs.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

Hmm.. failure could be related; need to check what changed for that test

=== RUN   TestIsArchivePathTar
    archive_test.go:82: Fail to create an archive file for test : tar: Removing leading `/' from member names
        gzip: /tmp/archive: No such file or directory

@thaJeztah
Copy link
Member Author

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