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/mounting squashfs extract #514

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

smoser
Copy link
Contributor

@smoser smoser commented Sep 28, 2023

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@hallyn
Copy link
Contributor

hallyn commented Oct 2, 2023

It seems to me that in the original code, if we do 20 ExtractSingleSquash()s, and are root in a non-init user namespace with full UID range, we will only try kernel mounting once, but with your updated code we will try it every time we call ExtractSinglesquash(). Maybe that doesn't matter, as that surely must be a very rare case.

@smoser
Copy link
Contributor Author

smoser commented Oct 3, 2023

It seems to me that in the original code, if we do 20 ExtractSingleSquash()s, and are root in a non-init user namespace with full UID range, we will only try kernel mounting once, but with your updated code we will try it every time we call ExtractSinglesquash(). Maybe that doesn't matter, as that surely must be a very rare case.

In the original code it basically tries each option in order every time (kernel-mouint, squashfuse, unsquashfs). It never evicts an option from the list due to failure. The new code tries each option in order only once and then subsequently uses only the option that worked.

I tried to explain that inline. I also tried to make it readable, but possibly failed on that. I'm open to suggestions on how to make it better.

I also acknowledge that there is a problem where if the first time ExtractSingleSquash is called it is given a squashfs image that does not work with kernel mounting but did work with squashfuse. it would never try kernel mounting again. i dont' think this is a really big deal. The problem really is just that we don't have a good way to determine why something failed.

@hallyn hallyn force-pushed the fix/mounting-squashfs-extract branch from 0fc8954 to 844c80a Compare October 4, 2023 00:49
@smoser smoser force-pushed the fix/mounting-squashfs-extract branch 3 times, most recently from 521ee24 to 2be7900 Compare October 4, 2023 15:15
@smoser
Copy link
Contributor Author

smoser commented Oct 4, 2023

I'm happy with the state of this PR now, but I kind of expect it to fail c-i.
The reason is that builds will fail when building from a squashfs oci reference and using kernel mounts.

I'm attaching a test script that shows the failure that i'm hitting.
build-from-squash.txt

One thing to note... current stacker main branch does not see this error, although it should. The reason it does not is that the kernel mount gets done once, then ExtratSingleSquash gets called again and the kernel mount fails (because it is already mounted) and a squashfuse mount is put over the kernel mount. the end result is that master uses squashfuse and thus doesn't show the problem.

The failed run output with this branch is below for easy viewing.

execute: ./stacker --debug --work-dir=tgo-data-root build --layer-type=squashfs --layer-type=tar --stacker-file=s2.yaml
stacker version v1.0.0-rc5-18-g1666822
nsexec-ing [-- /home/smoser/src/stacker/stacker --internal-userns --debug --work-dir=tgo-data-root build --layer-type=squashfs --layer-type=tar --stacker-file=s2.yaml]
stacker version v1.0.0-rc5-18-g1666822
initializing stacker recipe: s2.yaml
substituting $STACKER_ROOTFS_DIR to /home/smoser/src/stacker/tgo-data-root/roots
substituting $STACKER_STACKER_DIR to /home/smoser/src/stacker/tgo-data-root/.stacker
substituting $STACKER_OCI_DIR to /home/smoser/src/stacker/tgo-data-root/oci
substituting $STACKER_WORK_DIR to tgo-data-root
stacker build order:
0 build /home/smoser/src/stacker/s2.yaml: requires: []
building: 0 /home/smoser/src/stacker/s2.yaml
substituting $STACKER_ROOTFS_DIR to /home/smoser/src/stacker/tgo-data-root/roots
substituting $STACKER_STACKER_DIR to /home/smoser/src/stacker/tgo-data-root/.stacker
substituting $STACKER_OCI_DIR to /home/smoser/src/stacker/tgo-data-root/oci
substituting $STACKER_WORK_DIR to tgo-data-root
Dependency Order [bare2]
preparing image bare2...
overlay-dirs, possibly modified after import: []
loading oci:/home/smoser/src/stacker/tgo-data-root/oci:bare-squashfs
unpacking to /home/smoser/src/stacker/tgo-data-root/roots/bare2
Selected squashfs extractor kmount
lxc rootfs overlay arg overlayfs:/home/smoser/src/stacker/tgo-data-root/roots/sha256_b8646ebdc2a7b931b675b200d513e9a1c02c249834af67c2ed6f7df1656a95b2/overlay:/home/smoser/src/stacker/tgo-data-root/roots/sha256_12c6518b83d59b9bb87d242955a776940e68fbd6b580c4c3a92ebe074e81135c/overlay:/home/smoser/src/stacker/tgo-data-root/roots/bare2/overlay
stacker version v1.0.0-rc5-18-g1666822
stacker subcommand: [/home/smoser/src/stacker/stacker --oci-dir /home/smoser/src/stacker/tgo-data-root/oci --roots-dir /home/smoser/src/stacker/tgo-data-root/roots --stacker-dir /home/smoser/src/stacker/tgo-data-root/.stacker --storage-type overlay --debug internal-go check-aa-profile lxc-container-default-cgns]
bind mounting /home/smoser/src/stacker/tgo-data-root/.stacker/imports/bare2 into container
+ echo Building bare2
Building bare2
+ mkdir -p /layers
+ date -R
creating layer bare2 (type=tar) by converting layer bare2-squashfs (type=squashfs+verity)
error: operation not supported
get xattr list
github.com/opencontainers/umoci/oci/layer.(*tarGenerator).AddFile
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/oci/layer/tar_generate.go:204
github.com/opencontainers/umoci/oci/layer.GenerateInsertLayer.func1.2
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/oci/layer/generate.go:168
github.com/opencontainers/umoci/pkg/unpriv.walk
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/pkg/unpriv/unpriv.go:537
github.com/opencontainers/umoci/oci/layer.GenerateInsertLayer.func1.Walk.func3
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/pkg/unpriv/unpriv.go:576
github.com/opencontainers/umoci/pkg/unpriv.Wrap
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/pkg/unpriv/unpriv.go:75
github.com/opencontainers/umoci/pkg/unpriv.Walk
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/pkg/unpriv/unpriv.go:571
github.com/opencontainers/umoci/oci/layer.GenerateInsertLayer.func1
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/oci/layer/generate.go:153
runtime.goexit
        /usr/lib/go/src/runtime/asm_amd64.s:1650
unpriv.walk
github.com/opencontainers/umoci/oci/layer.GenerateInsertLayer.func1.Walk.func3
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/pkg/unpriv/unpriv.go:581
github.com/opencontainers/umoci/pkg/unpriv.Wrap
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/pkg/unpriv/unpriv.go:75
github.com/opencontainers/umoci/pkg/unpriv.Walk
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/pkg/unpriv/unpriv.go:571
github.com/opencontainers/umoci/oci/layer.GenerateInsertLayer.func1
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/oci/layer/generate.go:153
runtime.goexit
        /usr/lib/go/src/runtime/asm_amd64.s:1650
generate insert layer
github.com/opencontainers/umoci/oci/layer.GenerateInsertLayer.func1.1
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/oci/layer/generate.go:140
github.com/opencontainers/umoci/oci/layer.GenerateInsertLayer.func1
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/oci/layer/generate.go:153
runtime.goexit
        /usr/lib/go/src/runtime/asm_amd64.s:1650
copy to temporary blob
github.com/opencontainers/umoci/oci/cas/dir.(*dirEngine).PutBlob
        /stacker-tree/.build/gopath/pkg/mod/github.com/opencontainers/[email protected]/oci/cas/dir/dir.go:173
stackerbuild.io/stacker/pkg/overlay.ociPutBlob
        /stacker-tree/pkg/overlay/pack.go:277
stackerbuild.io/stacker/pkg/overlay.ConvertAndOutput
        /stacker-tree/pkg/overlay/pack.go:127
stackerbuild.io/stacker/pkg/overlay.(*overlay).initializeBasesInOutput
        /stacker-tree/pkg/overlay/pack.go:212
stackerbuild.io/stacker/pkg/overlay.(*overlay).Repack
        /stacker-tree/pkg/overlay/pack.go:242
stackerbuild.io/stacker/pkg/stacker.(*Builder).build
        /stacker-tree/pkg/stacker/build.go:512
stackerbuild.io/stacker/pkg/stacker.(*Builder).BuildMultiple
        /stacker-tree/pkg/stacker/build.go:610
main.doBuild
        /stacker-tree/cmd/stacker/build.go:118
github.com/urfave/cli/v2.(*Command).Run
        /stacker-tree/.build/gopath/pkg/mod/github.com/urfave/cli/[email protected]/command.go:273
github.com/urfave/cli/v2.(*Command).Run
        /stacker-tree/.build/gopath/pkg/mod/github.com/urfave/cli/[email protected]/command.go:266
github.com/urfave/cli/v2.(*App).RunContext
        /stacker-tree/.build/gopath/pkg/mod/github.com/urfave/cli/[email protected]/app.go:332
github.com/urfave/cli/v2.(*App).Run
        /stacker-tree/.build/gopath/pkg/mod/github.com/urfave/cli/[email protected]/app.go:309
main.main
        /stacker-tree/cmd/stacker/main.go:328
runtime.main
        /usr/lib/go/src/runtime/proc.go:267
runtime.goexit
        /usr/lib/go/src/runtime/asm_amd64.s:1650
error: exit status 1
stackerbuild.io/stacker/pkg/container.MaybeRunInNamespace
        /stacker-tree/pkg/container/userns.go:102
main.main.func3
        /stacker-tree/cmd/stacker/main.go:323
github.com/urfave/cli/v2.(*Command).Run
        /stacker-tree/.build/gopath/pkg/mod/github.com/urfave/cli/[email protected]/command.go:213
github.com/urfave/cli/v2.(*App).RunContext
        /stacker-tree/.build/gopath/pkg/mod/github.com/urfave/cli/[email protected]/app.go:332
github.com/urfave/cli/v2.(*App).Run
        /stacker-tree/.build/gopath/pkg/mod/github.com/urfave/cli/[email protected]/app.go:309
main.main
        /stacker-tree/cmd/stacker/main.go:328
runtime.main
        /usr/lib/go/src/runtime/proc.go:267
runtime.goexit
        /usr/lib/go/src/runtime/asm_amd64.s:1650
failed build s2.yaml

@hallyn
Copy link
Contributor

hallyn commented Oct 4, 2023

error: operation not supported
get xattr list

this looks like another place where caller needs to accept that squashsfs/overlay can return EOPNOTSUPP for some files while other files support it

@smoser smoser force-pushed the fix/mounting-squashfs-extract branch from 2be7900 to 39984ea Compare October 4, 2023 18:54
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #514 (fd41a4b) into main (4689ad5) will decrease coverage by 0.43%.
Report is 2 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
- Coverage   13.77%   13.34%   -0.43%     
==========================================
  Files          40       40              
  Lines        5671     5852     +181     
==========================================
  Hits          781      781              
- Misses       4762     4943     +181     
  Partials      128      128              
Files Coverage Δ
pkg/overlay/overlay.go 0.00% <0.00%> (ø)
pkg/types/layer_type.go 0.00% <0.00%> (ø)
pkg/overlay/pack.go 0.00% <0.00%> (ø)
pkg/squashfs/squashfs.go 9.32% <0.00%> (-6.01%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hallyn
Copy link
Contributor

hallyn commented Oct 4, 2023

error: operation not supported
get xattr list

this looks like another place where caller needs to accept that squashsfs/overlay can return EOPNOTSUPP for some files while other files support it

It is fixed on my system by building with opencontainers/umoci#506

@hallyn hallyn self-requested a review October 5, 2023 04:19
@smoser
Copy link
Contributor Author

smoser commented Oct 5, 2023

Some status on where we are right now on this.
a. it passed c-i in the last run (except for the code coverage)
b. the fact that we are now using kernel squashfs mounts means my provided test case above fails because squashfs does not implement xattrs on a dir. this will require us to pull in serge's linked fix (opencontainers/umoci#506).
c. there is still one issue that needs fixing; in the event that we built a layer its roots//overlay dir might have been populated by generateLayer. In taht case we are currently mounting the squashfs derived image over the top of that overlay dir and we do not have to do that. the general idea in stacker overlay storage is "if 'overlay' exists, then it is populated". that is somewhat sane in that it makes use of the "mkdir" semaphore.
d. i dont' love the scattered locking and i'm not sure what to do about it. i'm considering changing how that is done. i really would like for "extraction" of a layers to be able to be done in parallel.

@hallyn
Copy link
Contributor

hallyn commented Oct 5, 2023

We could keep inside the stacker binary an empty squashfs; every time stacker starts, we try to mount it from and to a tmpdir, and use that to inform whether to use kernel mounting...

It may seem wasteful, but apart from not having to do the Once() for the first kernel mount of an image, it also prevents us from misinterpreting an attempted mount of a bad squashfs blob as insufficient privilege.

@smoser
Copy link
Contributor Author

smoser commented Oct 5, 2023

We could keep inside the stacker binary an empty squashfs; every time stacker starts, we try to mount it from and to a tmpdir, and use that to inform whether to use kernel mounting...

It may seem wasteful, but apart from not having to do the Once() for the first kernel mount of an image, it also prevents us from misinterpreting an attempted mount of a bad squashfs blob as insufficient privilege.

I kind of like it.

$ echo "hi mom" > file.txt
$ mksquashfs file.txt out.squashfs
$ ls -l out.squashfs
-rw-r--r-- 1 smoser smoser 4096 Oct  5 16:09 out.squashfs
$ mkdir mnt
$ mount -t squashfs -o loop,ro out.squashfs mnt
$ cat mnt/file.txt 
hi mom

4096 bytes isn't too wasteful.

@smoser smoser force-pushed the fix/mounting-squashfs-extract branch from 39984ea to b8fba13 Compare October 10, 2023 18:24
unpackOne required the caller to provide it with a boolean 'isSquashfs'
which then made each caller have to consider the layer type.

Update unpackOne to take a Descriptor and let it do the
determination of how to unpack the layer in a single place.

The result of calling unpackLayer is either error, or the contents
available at the provided path.  The caller does not have to check
if the content is already present.

Also here, drop the 'storage' parameter to ExtractSingleSquash
that had become unused.

Signed-off-by: Scott Moser <[email protected]>
Overall, there are 3 "good things" done by this change:
1. Fix bug in the current code which tries mounting with each option
   every time.  The problem with doing that is really that the kernel
   mount option didn't work very well.  It would fail with "already
   mounted", and then squashfuse would end up getting used to mount over
   the top.

2. Fix race conditions in the current code.

   overlay.Unpack starts a thread pool and tries to unpack all layers
   at once.  That is fine if there are no duplicate layers.  But
      if there are duplicate layers used by a stacker.yaml file, then
   there were races on extraction.  The end result really was that things
   would get mounted more than once.

   Example stacker that shows this:

    l1:
      from:
        type: docker
        url: docker://busybox:latest
      run: |
        echo build layer 1

    l2:
      from:
        type: docker
        url: docker://busybox:latest
      run: |
        echo build layer 1

  There, the busybox layer would get extracted multiple times.

  The code here has a single lock on ExtractSingleSquash, it would
  be better to have lock being taken per extractDir.

3. Allow the user to control the list of extractors.

   If they knew that they could not use kernel mounts (or could, but
   didn't want to) or wanted to use unsquashfs they can now do that.

   STACKER_SQUASHFS_EXTRACT_POLICY=kmount stacker build ..

   or

   STACKER_SQUASHFS_EXTRACT_POLICY="squashfuse kmount" stacker build ...

   This adds a SquashExtractor interface, with 3 implementers
   (KernelExtractor, SquashFuseExtractor, UnsquashfsExtractor).

   A ExtractPolicy is basically a list of Extractors to try.
   The first time ExtractPolicy is used it will try each of the Extractors
   in order.  It then stores the result in .Extractor and uses that
   subsequently.

Signed-off-by: Scott Moser <[email protected]>
This seeks to improve some of the existing debug messages and add some
additional debug in the overlay storage.

Signed-off-by: Scott Moser <[email protected]>
@smoser smoser force-pushed the fix/mounting-squashfs-extract branch from b8fba13 to fd41a4b Compare October 10, 2023 18:29
@smoser smoser marked this pull request as ready for review October 10, 2023 19:37
@smoser smoser requested a review from rchincha as a code owner October 10, 2023 19:37
@smoser
Copy link
Contributor Author

smoser commented Oct 10, 2023

@hallyn since you reviewed/approved I did a few things, which can be seen here.

  1. added some more debug (the top commit here)
  2. changed the Mount interface:
- Mount(source, dest string) (bool, error)
+ Mount(source, dest string) error

the 'bool' was intended to be "is mounted", but in all cases if err was != nil then mounted was true.

  1. avoid calling unpackOne for duplicate digests within an image (an image could have multiple layers with the same digest. if it did, calling unpackOne on that would just likely expose races as it is right now).

  2. avoid a mount or extract in unpackOne if the target exists and is populated; also add a comment here.

  3. opened > Include a squashfs image in stacker binary for validation of mount #523 to keep track of the squashfs image idea above.

@hallyn hallyn merged commit 565b032 into project-stacker:main Oct 12, 2023
7 of 9 checks passed
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.

2 participants