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.

Sign up for GitHub

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

Quadlet: add -list-images #23065

Closed
wants to merge 1 commit into from
Closed

Quadlet: add -list-images #23065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions cmd/quadlet/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ import (
// for more details.

var (
verboseFlag bool // True if -v passed
noKmsgFlag bool
isUserFlag bool // True if run as quadlet-user-generator executable
dryRunFlag bool // True if -dryrun is used
versionFlag bool // True if -version is used
verboseFlag bool // True if -v passed
noKmsgFlag bool
isUserFlag bool // True if run as quadlet-user-generator executable
dryRunFlag bool // True if -dryrun is used
versionFlag bool // True if -version is used
listImagesFlag string // Set if -list-images is used
)

const (
Expand Down Expand Up @@ -549,7 +550,7 @@ func process() error {
prevError = err
}

if !dryRunFlag && flag.NArg() < 1 {
if !dryRunFlag && flag.NArg() < 1 && len(listImagesFlag) == 0 {
reportError(errors.New("missing output directory argument"))
return prevError
}
Expand Down Expand Up @@ -586,6 +587,31 @@ func process() error {
}
}

if len(listImagesFlag) > 0 {
fd, err := os.OpenFile(listImagesFlag, os.O_WRONLY, 0644)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but best practice to wrap in a bufio.BufWriter right?

if err != nil {
return err
}
for _, unit := range units {
if !strings.HasSuffix(unit.Filename, ".image") {
continue
}
imageName, err := quadlet.ListImage(unit)
if err != nil {
reportError(err)
}
if !isUnambiguousName(imageName) {
Logf("Warning: %s specifies the image \"%s\" which not a fully qualified image name and is hence being ignored.", unit.Filename, imageName)
continue
}
fmt.Fprintf(fd, "%s\n", imageName)
}
if err := fd.Close(); err != nil {
reportError(err)
}
return prevError
}

if !dryRunFlag {
err := os.MkdirAll(outputPath, os.ModePerm)
if err != nil {
Expand Down Expand Up @@ -681,4 +707,5 @@ func init() {
flag.BoolVar(&isUserFlag, "user", false, "Run as systemd user")
flag.BoolVar(&dryRunFlag, "dryrun", false, "Run in dryrun mode printing debug information")
flag.BoolVar(&versionFlag, "version", false, "Print version information and exit")
flag.StringVar(&listImagesFlag, "list-images", "", "Write a list of all images being references in .image Quadlets to specified file")
Copy link
Contributor

@cgwalters cgwalters Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Given that we're likely to want to add something like a new flag to opt-in to pinning like
Bootc=bound or Preload=true or Pin=true (however we call it)

I think we'll need to either:

  • Add some sort of --matches=Pin=true flag to this list operation
  • Just return the entire .image file?

I think the second part is important: Consider the case where I want to use a custom pull secret just for some images. In that general case today, we need the AuthFile flag, etc.

This type of thing implies actually that what we will need as a next step is a way to pass the output of this tool (probably a full .image file) into something like podman pull --from-systemd-image or something`?

Copy link
Member

Choose a reason for hiding this comment

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

Podman should not parse quadlet files. That is breaking the well build separation we have. And quite frankly it cannot work as systemd untis may contain systemd specifiers that must be expanded first and this is not logic we should try to mirror because we will fail to match it 1 to 1.
And even if you try to parse that on bootc you will have the same issue, the AuthFile=%h/somefile you have to know what %h is. And then you also have to deal with env var expansions and so on.

As such I fail to see how we can generate the command exactly the way systemd would execute it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifiers are a complex topic indeed. I think %h (homedir) specifically is out of scope for bootc's use case; we should not be interacting with user containers or home directories.

Would people use e.g. %a (architecture) or %l (hostname) or other things like that in these .image files? Maybe...but, honestly I'd also be OK if we just errored out if you used those things for Pin=true .image files today. I think we'd just need to handle "detect unquoted % in input and error out".

Copy link
Contributor

Choose a reason for hiding this comment

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

So to combine these then, what we could output instead is say JSON, with a list of podman pull ... commands that we would run, and again error out if we find specifiers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although it's a bit messy because if we also want to do pinning via podman create --rm <image>...wait, is there no variant of create that is explicitly ephemeral i.e. its state is tracked in /run? I guess that's not fatal, but it'd be quite nice to have as it means the core logic would just be "on boot, re-pin all images we need to pin" instead of that plus an extra step of "also GC any pin containers that aren't needed anymore".

Copy link
Member Author

@vrothberg vrothberg Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I want to extract some requirements from the conversation:

  1. We want to pre-pull images after an bootc upgrade
  2. Not all .image files may need to be pre-pulled, so we need some kind of opt-in mechanism
  3. The .image files may use custom pull secrets, so Quadlet should ideally have generated units for the file with a fully expanded podman pull
  4. Respecting the separation of concerns, Quadlet should not leave its purpose of a systemd-generator

Copy link
Contributor

Choose a reason for hiding this comment

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

We want to pre-pull images after an bootc upgrade

It's subtler than that I think. We want to "logically bind", and that means that bootc must not queue a new bootloader entry unless the logically bound images have been pulled and are known to be present.

A whole big point of bootc (and ostree before it, and image based systems in general) is having "transactional updates" where the system is known to either be in state A or B.

If we did the naive thing of:

  • bootc upgrade (queuing a new bootloader entry)
  • pull images

Then some tool doing a reboot in between these two phases would create a state like "B, but without app images" which is not desired.

This subtlety is why this is hard to implement without changes to both bootc and podman today.

}
15 changes: 15 additions & 0 deletions pkg/systemd/quadlet/quadlet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,6 +1223,21 @@ func ConvertKube(kube *parser.UnitFile, names map[string]string, isUser bool) (*
return service, nil
}

func ListImage(image *parser.UnitFile) (string, error) {
// Even if we're only interested in the Image= of the unit, it should
// still be valid.
if err := checkForUnknownKeys(image, ImageGroup, supportedImageKeys); err != nil {
return "", err
}

imageName, ok := image.Lookup(ImageGroup, KeyImage)
if !ok || len(imageName) == 0 {
return "", fmt.Errorf("no Image key specified")
}

return imageName, nil
}

func ConvertImage(image *parser.UnitFile) (*parser.UnitFile, string, error) {
service := image.Dup()
service.Filename = replaceExtension(image.Filename, ".service", "", "-image")
Expand Down
37 changes: 37 additions & 0 deletions test/system/252-quadlet.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,43 @@ EOF
run_podman network rm podman-default-kube-network
}

@test "quadlet - list images" {
local quadlet_tmpdir=$PODMAN_TMPDIR/quadlets
mkdir -p $quadlet_tmpdir

local image_name_1=quay.io/quadlet/image:1
local image_name_2=quay.io/quadlet/image:2
local image_name_3=quay.io/quadlet/image:3

cat > $PODMAN_TMPDIR/1.image <<EOF
[Image]
Image=$image_name_1
EOF
cat > $quadlet_tmpdir/2.image <<EOF
[Image]
Image=$image_name_2
EOF
cat > $quadlet_tmpdir/3.image <<EOF
[Image]
Image=$image_name_3
EOF
cat > $quadlet_tmpdir/1.container <<EOF
[Image]
Image=quay.io/do/not/parse:me
EOF

local image_file=$quadlet_tmpdir/image_file
touch $image_file

QUADLET_UNIT_DIRS=$PODMAN_TMPDIR run \
timeout --foreground -v --kill=10 $PODMAN_TIMEOUT \
$QUADLET -list-images=$image_file

assert "$(< $image_file)" = "$image_name_1
$image_name_2
$image_name_3" "-list-images=FILE lists all images referenced in .image Quadlets"
}

@test "quadlet - image files" {
local quadlet_tmpdir=$PODMAN_TMPDIR/quadlets

Expand Down