GNU bug report logs - #62153
[PATCH 0/2] Add Docker layered image for pack and system

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: Oleg Pykhalov <go.wigust <at> gmail.com>
Cc: Greg Hogan <code <at> greghogan.com>, 62153 <at> debbugs.gnu.org
Subject: [bug#62153] [PATCH v4 1/2] guix: docker: Build layered image.
Date: Fri, 22 Dec 2023 23:10:20 +0100

Oleg Pykhalov <go.wigust <at> gmail.com> skribis:

> * doc/guix.texi (Invoking guix pack): Document docker-layered format.
> (image Reference): Same.
> (image-type Reference): Document docker-layered-image-type.
> * gnu/image.scm
> (validate-image-format)[docker-layered]: New image format.
> * gnu/system/image.scm
> (docker-layered-image, docker-layered-image-type): New variables.
> (system-docker-image)[layered-image?]: New argument.
> (system-docker-layered-image): New procedure.
> (image->root-file-system)[docker-layered]: New image format.
> * gnu/tests/docker.scm (%test-docker-layered-system): New test.
> * guix/docker.scm (%docker-image-max-layers): New variable.
> (build-docker-image)[stream-layered-image, root-system]: New arguments.
> * guix/scripts/pack.scm (stream-layered-image.py): New variable.
> (docker-image)[layered-image?]: New argument.
> (docker-layered-image): New procedure.
> (%formats)[docker-layered]: New format.
> (show-formats): Document this.
> * guix/scripts/system.scm
> (system-derivation-for-action)[docker-layered-image]: New action.
> (show-help): Document this.
> (actions)[docker-layered-image]: New action.
> (process-action): Add this.
> * tests/pack.scm: Add "docker-layered-image + localstatedir" test.

[...]

> +  #:use-module (guix diagnostics)
> +  #:use-module (guix i18n)

(guix docker) shouldn’t need these.

> +  #:use-module (ice-9 popen)
> +  #:use-module (ice-9 rdelim)
> +  #:use-module (ice-9 receive)

For consistency, I’d recommend (srfi srfi-71) instead of (ice-9
receive).

> +(define %docker-image-max-layers
> +  100)

I’d add a comment on the second line, like “;; Maximum number of layers
allowed in a Docker image according to …”.

> +(define (paths-split-sort paths)
> +  "Split list of PATHS at %DOCKER-IMAGE-MAX-LAYERS and sort by disk usage."

Nitpick: maybe (define (size-sorted-store-items items) …)?

> +  (let* ((paths-length (length paths))
> +         (port (apply open-pipe* OPEN_READ
> +                      (append '("du" "--summarize") paths)))
> +         (output (read-string port)))
> +    (close-port port)

How about:

  (map (lambda (item)
         (cons item (file-size item)))
       items)

?

See (guix build store-copy) for the definition of ‘file-size’.

That way we avoid the dependency on Coreutils and code that “parses” the
output of ‘du’.

> +  (define layers-hashes

A short comment explaining what the inputs and outputs of this procedure
are would be great.

> +    (match-lambda
> +      (((head ...) (tail ...) id)
> +       (create-empty-tar "image.tar")
> +       (let* ((head-layers
> +               (map
> +                (lambda (file)
> +                  (invoke "tar" "cf" "layer.tar" file)
> +                  (let* ((file-hash (layer-diff-id "layer.tar"))
> +                         (file-name (string-append file-hash "/layer.tar")))
> +                    (mkdir file-hash)
> +                    (rename-file "layer.tar" file-name)
> +                    (invoke "tar" "-rf" "image.tar" file-name)
> +                    (delete-file file-name)
> +                    file-hash))
> +                head))
> +              (tail-layer
> +               (begin
> +                 (create-empty-tar "layer.tar")
> +                 (for-each (lambda (file)
> +                             (invoke "tar" "-rf" "layer.tar" file))
> +                           tail)
> +                 (let* ((file-hash (layer-diff-id "layer.tar"))
> +                        (file-name (string-append file-hash "/layer.tar")))
> +                   (mkdir file-hash)
> +                   (rename-file "layer.tar" file-name)
> +                   (invoke "tar" "-rf" "image.tar" file-name)
> +                   (delete-file file-name)
> +                   file-hash)))
> +              (customization-layer
> +               (let* ((file-id (string-append id "/layer.tar"))
> +                      (file-hash (layer-diff-id file-id))
> +                      (file-name (string-append file-hash "/layer.tar")))
> +                 (mkdir file-hash)
> +                 (rename-file file-id file-name)
> +                 (invoke "tar" "-rf" "image.tar" file-name)
> +                 file-hash))
> +              (all-layers
> +               (append head-layers (list tail-layer customization-layer))))

Maybe this can be factorized a bit with:

  (define (seal-layer)
    ;; Add 'layer.tar' to 'image.tar' under the right name.  Return its hash.
    (let* ((file-hash (layer-diff-id "layer.tar"))
           (file-name (string-append file-hash "/layer.tar")))
      (mkdir file-hash)
      (rename-file "layer.tar" file-name)
      (invoke "tar" "-rf" "image.tar" file-name)
      (delete-file file-name)
      file-hash)))

?

Apart from this stylistic issues, it looks great to me.

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.