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

Fix Tar writing to include directory records #323

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

Danielku15
Copy link
Contributor

@Danielku15 Danielku15 commented Feb 6, 2023

This PR adds the following changes which are all related to #304

  1. It changes the Tar writing to use Pax. This seem to be the default in docker save, in .net
  2. It ensures that for each file added to the Tar, the respective directory entries are also created. I first though of merging Layer.FromDirectory and Layer.FromFiles and then traverse the FS tree building the items but I decided against it then. I found some tests aiming to support a list of files from different directories.
  3. Added a missing using to the TarWriter writing the file sent to docker.

On a Windows Setup (Docker Desktop) where Linux Containers and Windows Containers (side-by-side with experimental flag and Hyper-V used) Docker can load Linux containers build with sdk-container-builds.

Building Windows containers unfortunately still fails.

baronfel added a commit to baronfel/sdk-container-demo that referenced this pull request Feb 6, 2023
@baronfel
Copy link
Member

baronfel commented Feb 6, 2023

Closes #159.
Related to #160.

This looks great! Thank you for the incredibly detailed investigation, as well as the fix. I've built and used this PR in this GitHub Actions run that pushed all new layers and this second run that reused layers of my test project. The jobs seem mostly green!

This seems like an unambiguous win to me.

I enabled the test runs and one of the descriptor size tests failed - can you take a look?

@Danielku15
Copy link
Contributor Author

Tests are green with the higher size range on my fork: https://github.com/Danielku15/sdk-container-builds/actions/runs/4105875803/jobs/7083347414

To get more deterministic sizes I could add a hidden test option which disables gzipping of the layer tar, then the size and hashes shouldn't differ anymore between test runs. But we also wouldn't be testing the main production usecase anymore which will use gzip.

I also thought of making a more strict helper for checking the tar contents instead and attributes instead of the general sizes and hashes. But didn't want to overengineer and change too much at this point. Might be annoying to maintain a more strict test if the layer contents regularly change with new features.

@baronfel
Copy link
Member

baronfel commented Feb 6, 2023

@dotnet/sdk-container-builds-maintainers I can't assign the team for review for some reason, but PTAL at your convenience.

Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

Just a minor remark about using local function.

/// <param name="tar">The tar into which to add the directory entries.</param>
/// <param name="directoryEntries">The lookup of all known directory entries. </param>
/// <param name="filePathSegments">The segments of the file within the tar for which to create the folders</param>
private static void EnsureDirectoryEntries(TarWriter tar, ISet<string> directoryEntries, IReadOnlyList<string> filePathSegments)
Copy link
Member

Choose a reason for hiding this comment

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

nit: can be better as local function, then passing directoryEntries is not needed.
With current implementation directoryEntries is being modified, which is quite implicit.

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.

4 participants