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

feat: use squashfuse ready notifier if available #528

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

ariel-miculas
Copy link
Contributor

When using squasfuse, check whether it supports -o notifypipe. If it does, then use that instead of our manual checking the mountpoint inode, which is less reliable.

Co-Developed-by: Serge Hallyn serge@hallyn.com

See previous PR
Changes compared to the original PR:

  • use a version check for figuring out whether the mount notification mechanism is supported
  • check whether the pipe signals a successful or an unsuccessful squashfuse mount

This is a new PR because I cannot amend Serge's original PR.

What type of PR is this?
feature

Which issue does this PR fix:
none

What does this PR do / Why do we need it:
It uses the newly available squashfuse notification mechanism.

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

Testing done on this change:

Tested with the following program:

package main
import (
    "fmt"
    "os/exec"
    "strings"
    "path/filepath"
    "github.com/Masterminds/semver/v3"
    "syscall"
    "time"
    "os"
    "github.com/pkg/errors"
    )

func sqfuseSupportsMountNotification(sqfuse string) (bool) {
    fmt.Println("hello world")
    cmd := exec.Command("squashfuse")
    out, _ := cmd.CombinedOutput()
    // Don't care about errors
    first_line := strings.Split(string(out[:]), "\n")[0];
    version := strings.Split(first_line, " ")[1];
    v, err := semver.NewVersion(version)
    if err != nil {
        return false
    }
    // squashfuse notify mechanism was merged in 0.5.0
    constraint, err := semver.NewConstraint(">= 0.5.0");
    if err != nil {
        return false
    }
    if constraint.Check(v) {
        fmt.Printf("v: %s is at least than 0.5.0\n", version);
        return true
    }
    return false
}

func mountSqfs(squashFile string, mountpoint string) (*exec.Cmd, error) {
    sqfuse := "squashfuse"
    sqNotify := sqfuseSupportsMountNotification(sqfuse)
    var cmd *exec.Cmd
    var err error

    notifyOpts := ""
    notifyPath := ""
    if sqNotify {
        sockdir, err := os.MkdirTemp("", "sock")
        if err != nil {
            return cmd, err
        }
        notifyPath = filepath.Join(sockdir, "notifypipe")
        if err := syscall.Mkfifo(notifyPath, 0640); err != nil {
            return cmd, err
        }
        notifyOpts = "notify_pipe=" + notifyPath

    }

    optionArgs := "debug"
    if notifyOpts != "" {
        optionArgs += "," + notifyOpts
    }
    cmd = exec.Command(sqfuse, "-f", "-o", optionArgs, squashFile, mountpoint)
    cmd.Stdin = nil
    fmt.Printf("Extracting %s -> %s with %s\n", squashFile, mountpoint, sqfuse)
    err = cmd.Start()
    if err != nil {
        return cmd, err
    }

    // now poll/wait for one of 3 things to happen
    // a. child process exits - if it did, then some error has occurred.
    // b. the directory Entry is different than it was before the call
    //    to sqfuse.  We have to do this because we do not have another
    //    way to know when the mount has been populated.
    //    https://github.com/vasi/squashfuse/issues/49
    // c. a timeout (timeLimit) was hit
    timeLimit := 30 * time.Second
    alarmCh := make(chan struct{})
    go func() {
        cmd.Wait()
        close(alarmCh)
    }()
    if sqNotify {
        notifyCh := make(chan byte)
        fmt.Printf("%s supports notify pipe, watching %q\n", sqfuse, notifyPath)
        go func() {
            f, err := os.Open(notifyPath)
            if err != nil {
                return
            }
            defer f.Close()
            b1 := make([]byte, 1)
            for {
                n1, err := f.Read(b1)
                if err != nil {
                    return
                }
                if err == nil && n1 >= 1 {
                    fmt.Println(string(b1[:]))
                    break
                }
            }
            notifyCh <- b1[0]
        }()
        if err != nil {
            return cmd, errors.Wrapf(err, "Failed reading %q", notifyPath)
        }

        select {
        case <-alarmCh:
            fmt.Printf("Killing process %p\n", cmd.Process)
            cmd.Process.Kill()
            return cmd, errors.Wrapf(err, "Gave up on squashFuse mount of %s with %s after %s", squashFile, sqfuse, timeLimit)
        case ret := <-notifyCh:
            if ret == 's' {
                fmt.Println("success")
                return cmd, nil
            } else {
                fmt.Println("failure")
                return cmd, errors.New("squashfuse returned an error")
            }
        }
    }
    fmt.Printf("%s does not support notify pipe\n", sqfuse)

    return cmd, nil
}

func main() {
    if supported := sqfuseSupportsMountNotification("squashfuse"); supported {
        fmt.Println("squashfuse supports notification!");
    } else {
        fmt.Println("squashfuse doesn't support notification!");
    }

    squashFile := "/home/amiculas/work/cisco/test-puzzlefs/barehost.sqhs"
    mountpoint := "/tmp/sqfs-mount"
    _, err := mountSqfs(squashFile, mountpoint);
    if err != nil {
        fmt.Println("error detected:", err)
        return
    }
}

Output:

$ go run mount.go
hello world
v: 0.5.0 is at least than 0.5.0
squashfuse supports notification!
hello world
v: 0.5.0 is at least than 0.5.0
Extracting /home/amiculas/work/cisco/test-puzzlefs/barehost.sqhs -> /tmp/sqfs-mount with squashfuse
squashfuse supports notify pipe, watching "/tmp/sock2231501866/notifypipe"
s
success
~/work/hello-go/squash-mount

$ go run mount.go
hello world
v: 0.5.0 is at least than 0.5.0
squashfuse supports notification!
hello world
v: 0.5.0 is at least than 0.5.0
Extracting /home/amiculas/work/cisco/test-puzzlefs/barehost.sqhs -> /tmp/sqfs-mount with squashfuse
squashfuse supports notify pipe, watching "/tmp/sock2289448691/notifypipe"
f
failure
error detected: squashfuse returned an error

The first time it succeeds and the second time it fails, since the fuse mountpoint already exists.

With stacker:

amiculas@ubuntu-vm:~/stacker$ cat hello-stacker
hello-stacker:
  from:
    type: docker
    url: docker://zothub.io/tools/busybox:stable
  run: |
    mkdir -p /hello-stacker-app
    echo 'echo "Hello Stacker!"' > /hello-stacker-app/hello.sh
    chmod +x /hello-stacker-app/hello.sh
  entrypoint: /hello-stacker-app/hello.sh

amiculas@ubuntu-vm:~/stacker$ ./stacker build --layer-type=squashfs -f hello-stacker

Old squashfuse (<0.5.0)

amiculas@ubuntu-vm:~/stacker$ which squashfuse
/usr/bin/squashfuse

amiculas@ubuntu-vm:~/stacker$ squashfuse
squashfuse 0.1.103 (c) 2012 Dave Vasilevsky

amiculas@ubuntu-vm:~/stacker$ ./stacker internal-go atomfs mount hello-stacker-squashfs sqfs
/usr/bin/squashfuse does not support notify pipe
/usr/bin/squashfuse does not support notify pipe
error: couldn't do overlay mount to sqfs, opts: index=off,xino=on,userxattr,lowerdir=/home/amiculas/stacker/atomfs-metadata/mounts/8c4a55c9fd4dbc29a4f9d47d663a73753c877ef4419835f90afb497b8e9907c1:/home/amiculas/stacker/atomfs-metadata/mounts/f20ee95456ef2f3c9761a8e10f1279331980bd6d53cac785294be19ff6fccb55: operation not permitted

Note that the overlay fails, but the squashfuse mounts have been created:

amiculas@ubuntu-vm:~/stacker$ mount | grep fuse.squashfuse
squashfuse on /home/amiculas/stacker/atomfs-metadata/mounts/8c4a55c9fd4dbc29a4f9d47d663a73753c877ef4419835f90afb497b8e9907c1 type fuse.squashfuse (rw,nosuid,nodev,relatime,user_id=1001,group_id=1001,allow_other)
squashfuse on /home/amiculas/stacker/atomfs-metadata/mounts/f20ee95456ef2f3c9761a8e10f1279331980bd6d53cac785294be19ff6fccb55 type fuse.squashfuse (rw,nosuid,nodev,relatime,user_id=1001,group_id=1001,allow_other)

New squashfuse (>=0.5.0)

amiculas@ubuntu-vm:~/stacker$ which squashfuse
/usr/local/bin/squashfuse

amiculas@ubuntu-vm:~/stacker$ squashfuse
squashfuse 0.5.0 (c) 2012 Dave Vasilevsky

amiculas@ubuntu-vm:~/stacker$ which squashfuse_ll
/usr/local/bin/squashfuse_ll

amiculas@ubuntu-vm:~/stacker$ squashfuse_ll
squashfuse 0.5.0 (c) 2012 Dave Vasilevsky

amiculas@ubuntu-vm:~/stacker$ ./stacker internal-go atomfs mount hello-stacker-squashfs sqfs
/usr/local/bin/squashfuse_ll supports notify pipe, watching "/tmp/sock3579536881/notifypipe"
/usr/local/bin/squashfuse_ll supports notify pipe, watching "/tmp/sock4245449089/notifypipe"
error: couldn't do overlay mount to sqfs, opts: index=off,xino=on,userxattr,lowerdir=/home/amiculas/stacker/atomfs-metadata/mounts/8c4a55c9fd4dbc29a4f9d47d663a73753c877ef4419835f90afb497b8e9907c1:/home/amiculas/stacker/atomfs-metadata/mounts/f20ee95456ef2f3c9761a8e10f1279331980bd6d53cac785294be19ff6fccb55: operation not permitted

Note that the overlay fails, but the squashfuse mounts have been created:

amiculas@ubuntu-vm:~/stacker$ mount | grep fuse.squashfuse
squashfuse_ll on /home/amiculas/stacker/atomfs-metadata/mounts/8c4a55c9fd4dbc29a4f9d47d663a73753c877ef4419835f90afb497b8e9907c1 type fuse.squashfuse_ll (rw,nosuid,nodev,relatime,user_id=1001,group_id=1001,allow_other)
squashfuse_ll on /home/amiculas/stacker/atomfs-metadata/mounts/f20ee95456ef2f3c9761a8e10f1279331980bd6d53cac785294be19ff6fccb55 type fuse.squashfuse_ll (rw,nosuid,nodev,relatime,user_id=1001,group_id=1001,allow_other)

Automation added to e2e:

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:

No

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

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #528 (6857ed5) into main (565b032) will decrease coverage by 0.21%.
Report is 3 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #528      +/-   ##
==========================================
- Coverage   13.34%   13.14%   -0.21%     
==========================================
  Files          40       40              
  Lines        5852     5943      +91     
==========================================
  Hits          781      781              
- Misses       4943     5034      +91     
  Partials      128      128              
Files Coverage Δ
pkg/squashfs/squashfs.go 7.85% <0.00%> (-1.47%) ⬇️

... and 5 files with indirect coverage changes

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

@smoser
Copy link
Contributor

smoser commented Oct 19, 2023

If it does, then use that instead of our manual checking the mountpoint inode, which is less reliable.

Is it actually less reliable? I agree that responding to an event is nicer than polling, but have we actually found polling to be "unreliable"?

Copy link
Contributor

@smoser smoser left a comment

Choose a reason for hiding this comment

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

I think this looks sane.
It'd be nice if we could only check once (sync.Once) for presense of squashfuse executable and also whether or not it has notify pipe support.

I don't think its a blocker (we're currently checking squashfuse every time) but would be nice.

@ariel-miculas
Copy link
Contributor Author

If it does, then use that instead of our manual checking the mountpoint inode, which is less reliable.

Is it actually less reliable? I agree that responding to an event is nicer than polling, but have we actually found polling to be "unreliable"?

These were Serge's words, @hallyn do you have any comments?

@ariel-miculas
Copy link
Contributor Author

I think this looks sane. It'd be nice if we could only check once (sync.Once) for presense of squashfuse executable and also whether or not it has notify pipe support.

I don't think its a blocker (we're currently checking squashfuse every time) but would be nice.

Well, findSquashfusePath doesn't execute the squashfuse binary, but I guess it could return a tuple of (Path, Version) instead of only the Path.

@smoser
Copy link
Contributor

smoser commented Oct 19, 2023

Well, findSquashfusePath doesn't execute the squashfuse binary, but I guess it could return a tuple of (Path, Version) instead of only the Path.

yeah, i'd be fine with that. and it can just store the result in a global

type squashfuse struct {
   Path string
   Version version
   notfiy bool
}

oir something like that.

@ariel-miculas
Copy link
Contributor Author

Well, findSquashfusePath doesn't execute the squashfuse binary, but I guess it could return a tuple of (Path, Version) instead of only the Path.

yeah, i'd be fine with that. and it can just store the result in a global

type squashfuse struct {
   Path string
   Version version
   notfiy bool
}

oir something like that.

And initialize this struct only once? Then, for subsequent calls to the findSquashfusePath function, just return the global struct?

@hallyn
Copy link
Contributor

hallyn commented Oct 20, 2023

If it does, then use that instead of our manual checking the mountpoint inode, which is less reliable.

Is it actually less reliable? I agree that responding to an event is nicer than polling, but have we actually found polling to be "unreliable"?

Yes. Because it checks inode version of the root dentry, I have seen cases where, when already inside an overlayfs, the mount hung because the dentry info never appeared to change.

At least, I think that's what I saw :) Writing a reliable reproducer of that would be kind of nifty.

@hallyn
Copy link
Contributor

hallyn commented Oct 20, 2023

Well, findSquashfusePath doesn't execute the squashfuse binary, but I guess it could return a tuple of (Path, Version) instead of only the Path.

yeah, i'd be fine with that. and it can just store the result in a global

type squashfuse struct {
   Path string
   Version version
   notfiy bool
}

oir something like that.

And initialize this struct only once? Then, for subsequent calls to the findSquashfusePath function, just return the global struct?

Right, if you initialize it in a sync.Once, it'll automatically just happen once.

@hallyn
Copy link
Contributor

hallyn commented Oct 22, 2023

@smoser @ariel-miculas it appears that every overlay mount's root dentry shows a different 'device' in stat(), and that's enough for os.SameFile() to do the right thing.

So the only advantage here is not waiting (not waiting too long, and not risking timeing out too soon). I still think that's worthwhile. But there may not be a correctness issue as I thought there was.

@ariel-miculas
Copy link
Contributor Author

@hallyn what does overlay have to do with squashfuse?

@hallyn
Copy link
Contributor

hallyn commented Oct 25, 2023

@hallyn what does overlay have to do with squashfuse?

Right you are, I was confusing myself.

But just mounting two squashfuse's and stat()ing the root dentry still gives me different device ids.

pkg/squashfs/squashfs.go Outdated Show resolved Hide resolved
pkg/squashfs/squashfs.go Outdated Show resolved Hide resolved
When using squasfuse, check whether it supports -o notifypipe. If it
does, use it instead our manual checking of the mountpoint inode. The
notification mechanism is a better alternative to the existing polling
approach. If it's not available, then use the old mechanism.

This feature is supported starting from squasfuse version 0.5.0, see [1]
for details.

[1] vasi/squashfuse#49

Co-Developed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Ariel Miculas <amiculas@cisco.com>
@ariel-miculas
Copy link
Contributor Author

@smoser, does the PR look good to you now?

@rchincha rchincha merged commit 67d1ffb into project-stacker:main Nov 3, 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.

4 participants