compression: support zstd with skippable frame by dkkb · Pull Request #42862 · moby/moby

dkkb · 2021-09-17T12:25:11Z

Signed-off-by: Da McGrady dabkb@aol.com

- What I did

As a matter of fact, there are two frame formats defined by Zstandard: Zstandard frames and Skippable frames.
So we should probably support zstd algorithms with skippable frames.

See https://tools.ietf.org/id/draft-kucherawy-dispatch-zstd-00.html#rfc.section.2 for more details.
The structure of a single Zstandard frame is as follows, the magic number of Zstandard frame is 0xFD2FB528.

  +--------------------+------------+
  |    Magic_Number    | 4 bytes    |
  +--------------------+------------+
  |    Frame_Header    | 2-14 bytes |
  +--------------------+------------+
  |     Data_Block     | n bytes    |
  +--------------------+------------+
  | [More Data Blocks] |            |
  +--------------------+------------+
  | [Content Checksum] | 0-4 bytes  |
  +--------------------+------------+

Skippable frames allow the insertion of user-defined data into a flow of concatenated frames. Its design is pretty straightforward, with the sole objective to allow the decoder to quickly skip over user-defined data and continue decoding.

  +--------------+------------+-----------+
  | Magic_Number | Frame_Size | User_Data |
  +--------------+------------+-----------+
  |    4 bytes   |   4 bytes  |  n bytes  |
  +--------------+------------+-----------+

Magic_Number: 0x184D2A5?, which means any value from 0x184D2A50 to 0x184D2A5F.
Frame_Size: This is the size n of the following UserData, 4 bytes, little-endian format, unsigned 32-bits.

- How I did it
In order to accommodate more complicated detectors for Zstd, I changed the DetectCompression function.

- How to verify it

To verify this PR, I created a new test named TestDetectCompression.

- Description for the changelog

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

pkg/archive/archive.go

thaJeztah · 2021-09-17T12:59:23Z

Left a quick thought above.

Also, would it be possible to amend the commit message to use your (quite informative) PR description in the commit message itself?

dkkb · 2021-09-17T13:12:45Z

Left a quick thought above.

Also, would it be possible to amend the commit message to use your (quite informative) PR description in the commit message itself?

Sure.

pkg/archive/archive.go

pkg/archive/archive_test.go

thaJeztah

LGTM, thanks!

thaJeztah · 2021-09-23T09:44:19Z

@tonistiigi PTAL (perhaps we need a test image for this as well)

thaJeztah · 2021-10-14T19:09:50Z

@dkkb - looking at this with @tonistiigi and he pointed out that DecompressStream takes 10 bytes for the detection, which may not be enough for the detection needed here;

moby/pkg/archive/archive.go

Lines 178 to 182 in 6014c1e

    
           // DecompressStream decompresses the archive and returns a ReaderCloser with the decompressed archive. 
        
           func DecompressStream(archive io.Reader) (io.ReadCloser, error) { 
        
           	p := pools.BufioReader32KPool 
        
           	buf := p.Get(archive) 
        
           	bs, err := buf.Peek(10)

So, the test in the PR passes, but detection may not work as expected when used by DecompressStream

dkkb · 2021-10-15T09:12:23Z

@dkkb - looking at this with @tonistiigi and he pointed out that DecompressStream takes 10 bytes for the detection, which may not be enough for the detection needed here;

moby/pkg/archive/archive.go

Lines 178 to 182 in 6014c1e

// DecompressStream decompresses the archive and returns a ReaderCloser with the decompressed archive.

func DecompressStream(archive io.Reader) (io.ReadCloser, error) {

p := pools.BufioReader32KPool

buf := p.Get(archive)

bs, err := buf.Peek(10)

So, the test in the PR passes, but detection may not work as expected when used by DecompressStream

To avoid misidentifying Zstd, I use an ordered list to detect the compression algorithm for compatibility and performance so that we can quickly identify the skippable frame and remove the unnecessary code.
Do you think it is a feasible plan?

thaJeztah · 2021-10-15T09:16:52Z

To avoid misidentifying Zstd, I use an ordered list to detect the compression algorithm for compatibility and performance so that we can quickly identify the skippable frame and remove the unnecessary code. Do you think it is a feasible plan?

@tonistiigi could you have a look?

As a matter of fact, there are two frame formats defined by Zstandard: Zstandard frames and Skippable frames. So we should probably support zstd algorithms with skippable frames. See https://tools.ietf.org/id/draft-kucherawy-dispatch-zstd-00.html#rfc.section.2 for more details. Signed-off-by: Da McGrady <dabkb@aol.com>

tonistiigi · 2021-10-15T18:11:52Z

@klauspost @giuseppe sgty?

If accepted we need the same change in containerd & buildkit as all of them only check single magic atm.

klauspost

LGTM

thaJeztah

LGTM

thaJeztah · 2021-10-21T18:29:19Z

Thank you!

tonistiigi · 2021-10-28T22:11:10Z

@dkkb Could you make the same update in containerd and buildkit repos?

dkkb · 2021-10-29T02:02:47Z

@dkkb Could you make the same update in containerd and buildkit repos?

Sure.

thaJeztah added the status/2-code-review label Sep 17, 2021

thaJeztah reviewed Sep 17, 2021

View reviewed changes

pkg/archive/archive.go Outdated Show resolved Hide resolved

dkkb force-pushed the feature/zstd_with_skippable_frame branch from ee5e675 to d6ba284 Compare September 17, 2021 13:11

thaJeztah reviewed Sep 23, 2021

View reviewed changes

dkkb force-pushed the feature/zstd_with_skippable_frame branch 2 times, most recently from d8f3cde to e155411 Compare September 23, 2021 09:20

thaJeztah approved these changes Sep 23, 2021

View reviewed changes

thaJeztah added this to the 21.xx milestone Sep 23, 2021

thaJeztah added impact/changelog area/distribution Image Distribution labels Sep 23, 2021

dkkb force-pushed the feature/zstd_with_skippable_frame branch from e155411 to eb0eb4e Compare October 15, 2021 09:02

dkkb force-pushed the feature/zstd_with_skippable_frame branch from eb0eb4e to 23abee4 Compare October 15, 2021 09:24

klauspost approved these changes Oct 18, 2021

View reviewed changes

thaJeztah approved these changes Oct 21, 2021

View reviewed changes

thaJeztah merged commit 872c64c into moby:master Oct 21, 2021

tianon mentioned this pull request Oct 28, 2021

Add support for zstd compressed layers #40820

Closed

dkkb mentioned this pull request Oct 29, 2021

compression: support zstd with skippable frame containerd/containerd#6177

Merged

Conversation

dkkb commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Sep 17, 2021

Uh oh!

dkkb commented Sep 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 23, 2021

Uh oh!

thaJeztah commented Oct 14, 2021

Uh oh!

dkkb commented Oct 15, 2021

Uh oh!

thaJeztah commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Oct 15, 2021

Uh oh!

klauspost left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 21, 2021

Uh oh!

tonistiigi commented Oct 28, 2021

Uh oh!

dkkb commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkkb commented Sep 17, 2021 •

edited

Loading

thaJeztah commented Oct 15, 2021 •

edited

Loading