Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Sep 7, 2016

Closes #25779

This PR tries to tackle #25779 by supporting the OCI image layout when saving and loading Docker images.

Right now, this PR contains just the code for docker save --format oci because I found various aspects that still need to be discussed (I left many comments around in the code, please review them).

I'm also opening this in RFC because I didn't want to go that far and finding out that what I've been doing was completely wrong, so I'm here to gather feedback and comments to go forward (with docker load --format oci)

Changes:

  • Implement everything from the linked issue wrt docker save (plus I guessed on something like docker save --format oci name@digest
  • This PR vendors containers/image and the image spec to be able to deal with the OCI image layout and w/o the need to re-implement everything in the Docker code base.
  • added a bunch of integration tests to make sure we're on the same page wrt how this should work (at least with docker save now...)

TODO:

  • discuss around saving image IDs and images by digest (name@digest)
  • discuss API
  • finish docker load --format oci
  • integration tests for docker load and combined docker save --format oci | docker load -
  • documentation

Alright, here's the net result as a simple example of docker save, please do review the integration tests to have a picture of how this looks like in other scenarios:

$ docker save --format oci busybox:latest | tar t
blobs/
blobs/sha256/
blobs/sha256/2a2b6168c040d25148a4972cb20108b737adc51685d4dc68a8e01d55416d27a1
blobs/sha256/2b8fd9751c4c0f5dd266fcae00707e67a2545ef34f9a29354585f93dac906749
blobs/sha256/8ddc19f16526912237dd8af81971d5e4dd0587907234be2b83e249518d5b673f
oci-layout
refs/
refs/latest

$ mkdir busybox-oci
$ docker save --format oci busybox:latest |  tar -C busybox-oci -xf -

$ go get github.com/opencontainers/image-spec/cmd/oci-image-tool
$ oci-image-tool validate --ref latest busybox-oci
reference "latest": OK
busybox-oci: OK

/cc @stevvooe @aaronlehmann @vdemeester @vbatts @rhatdan @cpuguy83 @duglin @icecrime @estesp @tonistiigi

@runcom
Copy link
Member Author

runcom commented Sep 7, 2016

@jhowardmsft do you know why the following test fails with:

11:21:49 ----------------------------------------------------------------------
11:21:49 FAIL: docker_cli_save_load_test.go:516: DockerSuite.TestSaveOCIInternals
11:21:49 
11:21:49 docker_cli_save_load_test.go:528:
11:21:49     c.Assert(err, checker.IsNil, check.Commentf(string(out)))
11:21:49 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc083ab1ce0), Stderr:[]uint8(nil)} ("exit status 128")
11:21:49 ... tar: Cannot connect to C: resolve failed
11:21:49 
11:21:49 
11:22:08 
11:22:08 ----------------------------------------------------------------------

The test fails at:

9     // smoke test of the correctness of the OCI image layout                    
520     tmpDir, err := ioutil.TempDir("", "oci-image-layout")                       
521     c.Assert(err, checker.IsNil)                                                
522                                                                                 
523     tarFile := filepath.Join(tmpDir, "test.tar")                                
524                                                                                 
525     dockerCmd(c, "save", "--format", "oci", "-o", tarFile, "busybox:latest")    
526                                                                                 
527     out, err := exec.Command("tar", "xO", "refs/latest", "-f", tarFile).CombinedOutput()
528     c.Assert(err, checker.IsNil, check.Commentf(string(out))) 

Is there anything I could do to fix this?

@lowenna
Copy link
Member

lowenna commented Sep 7, 2016

Yes, docker save is currently broken on Windows. @jstarks

@runcom
Copy link
Member Author

runcom commented Sep 7, 2016

Yes, docker save is currently broken on Windows. @jstarks

@jhowardmsft it doesn't seem to fail when piping docker save to tar though

@lowenna
Copy link
Member

lowenna commented Sep 7, 2016

@runcom Are you sure it's not just piping whatever comes out of stderr? AFAIK the issue is known, just not fixed, and @jstarks had a fix in the pipeline. You would see something like the following if you attempt to docker save on master on Windows RS1 builds today:

PS E:\Docker\ci\W2W\runCI> docker save busybox -o bb.out
Error response from daemon: file integrity checksum failed for "UtilityVM/Files/EFI/Microsoft/Boot/BCD"

@runcom
Copy link
Member Author

runcom commented Sep 7, 2016

@jhowardmsft I was referring to the test I've added here for instance https://github.com/docker/docker/pull/26369/files#diff-00a3ffcc2e471d5f664f602aac761d7bR441. I cannot see how this test passes if the output of the command pipeline is just an error since it's doing some check on the output itself.

And all the other tests that do docker save busybox | tar t are clearly working on win2lin so I'm not sure what's going on.

@lowenna
Copy link
Member

lowenna commented Sep 7, 2016

Oh sorry, you're referring to Windows to Linux, not Windows to Windows. In which case, I don't know.

@runcom
Copy link
Member Author

runcom commented Sep 7, 2016

Oh sorry, you're referring to Windows to Linux, not Windows to Windows. In which case, I don't know.

@jhowardmsft yeah, it may be something with tar xO dirinsietar/file -f file.tar - is that supported on windows?

@lowenna
Copy link
Member

lowenna commented Sep 7, 2016

This is what I see logged onto one of the Windows boxes used in Win2Lin CI.

C:\temp>tar --help
Usage: tar [OPTION...] [FILE]...
GNU 'tar' saves many files together into a single tape or disk archive, and can
restore individual files from the archive.

Examples:
  tar -cf archive.tar foo bar  # Create archive.tar from files foo and bar.
  tar -tvf archive.tar         # List all files in archive.tar verbosely.
  tar -xf archive.tar          # Extract all files from archive.tar.

 Main operation mode:

  -A, --catenate, --concatenate   append tar files to an archive
  -c, --create               create a new archive
  -d, --diff, --compare      find differences between archive and file system
      --delete               delete from the archive (not on mag tapes!)
  -r, --append               append files to the end of an archive
  -t, --list                 list the contents of an archive
      --test-label           test the archive volume label and exit
  -u, --update               only append files newer than copy in archive
  -x, --extract, --get       extract files from an archive

 Operation modifiers:

      --check-device         check device numbers when creating incremental
                             archives (default)
  -g, --listed-incremental=FILE   handle new GNU-format incremental backup
  -G, --incremental          handle old GNU-format incremental backup
      --ignore-failed-read   do not exit with nonzero on unreadable files
      --level=NUMBER         dump level for created listed-incremental archive
  -n, --seek                 archive is seekable
      --no-check-device      do not check device numbers when creating
                             incremental archives
      --no-seek              archive is not seekable
      --occurrence[=NUMBER]  process only the NUMBERth occurrence of each file
                             in the archive; this option is valid only in
                             conjunction with one of the subcommands --delete,
                             --diff, --extract or --list and when a list of
                             files is given either on the command line or via
                             the -T option; NUMBER defaults to 1
      --sparse-version=MAJOR[.MINOR]
                             set version of the sparse format to use (implies
                             --sparse)
  -S, --sparse               handle sparse files efficiently

 Overwrite control:

  -k, --keep-old-files       don't replace existing files when extracting,
                             treat them as errors
      --keep-directory-symlink   preserve existing symlinks to directories when
                             extracting
      --keep-newer-files     don't replace existing files that are newer than
                             their archive copies
      --no-overwrite-dir     preserve metadata of existing directories
      --one-top-level[=DIR]  create a subdirectory to avoid having loose files
                             extracted
      --overwrite            overwrite existing files when extracting
      --overwrite-dir        overwrite metadata of existing directories when
                             extracting (default)
      --recursive-unlink     empty hierarchies prior to extracting directory
      --remove-files         remove files after adding them to the archive
      --skip-old-files       don't replace existing files when extracting,
                             silently skip over them
  -U, --unlink-first         remove each file prior to extracting over it
  -W, --verify               attempt to verify the archive after writing it

 Select output stream:

      --ignore-command-error ignore exit codes of children
      --no-ignore-command-error   treat non-zero exit codes of children as
                             error
  -O, --to-stdout            extract files to standard output
      --to-command=COMMAND   pipe extracted files to another program

 Handling of file attributes:

      --atime-preserve[=METHOD]   preserve access times on dumped files, either
                             by restoring the times after reading
                             (METHOD='replace'; default) or by not setting the
                             times in the first place (METHOD='system')
      --delay-directory-restore   delay setting modification times and
                             permissions of extracted directories until the end
                             of extraction
      --group=NAME           force NAME as group for added files
      --mode=CHANGES         force (symbolic) mode CHANGES for added files
      --mtime=DATE-OR-FILE   set mtime for added files from DATE-OR-FILE
  -m, --touch                don't extract file modified time
      --no-delay-directory-restore
                             cancel the effect of --delay-directory-restore
                             option
      --no-same-owner        extract files as yourself (default for ordinary
                             users)
      --no-same-permissions  apply the user's umask when extracting permissions
                             from the archive (default for ordinary users)
      --numeric-owner        always use numbers for user/group names
      --owner=NAME           force NAME as owner for added files
  -p, --preserve-permissions, --same-permissions
                             extract information about file permissions
                             (default for superuser)
      --preserve             same as both -p and -s
      --same-owner           try extracting files with the same ownership as
                             exists in the archive (default for superuser)
  -s, --preserve-order, --same-order
                             member arguments are listed in the same order as
                             the files in the archive
      --sort=ORDER           directory sorting order: none (default), name or
                             inode

 Handling of extended file attributes:

      --acls                 Enable the POSIX ACLs support
      --no-acls              Disable the POSIX ACLs support
      --no-selinux           Disable the SELinux context support
      --no-xattrs            Disable extended attributes support
      --selinux              Enable the SELinux context support
      --xattrs               Enable extended attributes support
      --xattrs-exclude=MASK  specify the exclude pattern for xattr keys
      --xattrs-include=MASK  specify the include pattern for xattr keys

 Device selection and switching:

  -f, --file=ARCHIVE         use archive file or device ARCHIVE
      --force-local          archive file is local even if it has a colon
  -F, --info-script=NAME, --new-volume-script=NAME
                             run script at end of each tape (implies -M)
  -L, --tape-length=NUMBER   change tape after writing NUMBER x 1024 bytes
  -M, --multi-volume         create/list/extract multi-volume archive
      --rmt-command=COMMAND  use given rmt COMMAND instead of rmt
      --rsh-command=COMMAND  use remote COMMAND instead of rsh
      --volno-file=FILE      use/update the volume number in FILE

 Device blocking:

  -b, --blocking-factor=BLOCKS   BLOCKS x 512 bytes per record
  -B, --read-full-records    reblock as we read (for 4.2BSD pipes)
  -i, --ignore-zeros         ignore zeroed blocks in archive (means EOF)
      --record-size=NUMBER   NUMBER of bytes per record, multiple of 512

 Archive format selection:

  -H, --format=FORMAT        create archive of the given format

 FORMAT is one of the following:

    gnu                      GNU tar 1.13.x format
    oldgnu                   GNU format as per tar <= 1.12
    pax                      POSIX 1003.1-2001 (pax) format
    posix                    same as pax
    ustar                    POSIX 1003.1-1988 (ustar) format
    v7                       old V7 tar format

      --old-archive, --portability
                             same as --format=v7
      --pax-option=keyword[[:]=value][,keyword[[:]=value]]...
                             control pax keywords
      --posix                same as --format=posix
  -V, --label=TEXT           create archive with volume name TEXT; at
                             list/extract time, use TEXT as a globbing pattern
                             for volume name

 Compression options:

  -a, --auto-compress        use archive suffix to determine the compression
                             program
  -I, --use-compress-program=PROG
                             filter through PROG (must accept -d)
  -j, --bzip2                filter the archive through bzip2
  -J, --xz                   filter the archive through xz
      --lzip                 filter the archive through lzip
      --lzma                 filter the archive through lzma
      --lzop                 filter the archive through xz
      --no-auto-compress     do not use archive suffix to determine the
                             compression program
  -z, --gzip, --gunzip, --ungzip   filter the archive through gzip
  -Z, --compress, --uncompress   filter the archive through compress

 Local file selection:

      --add-file=FILE        add given FILE to the archive (useful if its name
                             starts with a dash)
      --backup[=CONTROL]     backup before removal, choose version CONTROL
  -C, --directory=DIR        change to directory DIR
      --exclude=PATTERN      exclude files, given as a PATTERN
      --exclude-backups      exclude backup and lock files
      --exclude-caches       exclude contents of directories containing
                             CACHEDIR.TAG, except for the tag file itself
      --exclude-caches-all   exclude directories containing CACHEDIR.TAG
      --exclude-caches-under exclude everything under directories containing
                             CACHEDIR.TAG
      --exclude-ignore=FILE  read exclude patterns for each directory from
                             FILE, if it exists
      --exclude-ignore-recursive=FILE
                             read exclude patterns for each directory and its
                             subdirectories from FILE, if it exists
      --exclude-tag=FILE     exclude contents of directories containing FILE,
                             except for FILE itself
      --exclude-tag-all=FILE exclude directories containing FILE
      --exclude-tag-under=FILE   exclude everything under directories
                             containing FILE
      --exclude-vcs          exclude version control system directories
      --exclude-vcs-ignores  read exclude patterns from the VCS ignore files
  -h, --dereference          follow symlinks; archive and dump the files they
                             point to
      --hard-dereference     follow hard links; archive and dump the files they
                             refer to
  -K, --starting-file=MEMBER-NAME
                             begin at member MEMBER-NAME when reading the
                             archive
      --newer-mtime=DATE     compare date and time when data changed only
      --no-null              disable the effect of the previous --null option
      --no-recursion         avoid descending automatically in directories
      --no-unquote           do not unquote input file or member names
      --null                 -T reads null-terminated names, disable -C
  -N, --newer=DATE-OR-FILE, --after-date=DATE-OR-FILE
                             only store files newer than DATE-OR-FILE
      --one-file-system      stay in local file system when creating archive
  -P, --absolute-names       don't strip leading '/'s from file names
      --recursion            recurse into directories (default)
      --suffix=STRING        backup before removal, override usual suffix ('~'
                             unless overridden by environment variable
                             SIMPLE_BACKUP_SUFFIX)
  -T, --files-from=FILE      get names to extract or create from FILE
      --unquote              unquote input file or member names (default)
  -X, --exclude-from=FILE    exclude patterns listed in FILE

 File name transformations:

      --strip-components=NUMBER   strip NUMBER leading components from file
                             names on extraction
      --transform=EXPRESSION, --xform=EXPRESSION
                             use sed replace EXPRESSION to transform file
                             names

 File name matching options (affect both exclude and include patterns):

      --anchored             patterns match file name start
      --ignore-case          ignore case
      --no-anchored          patterns match after any '/' (default for
                             exclusion)
      --no-ignore-case       case sensitive matching (default)
      --no-wildcards         verbatim string matching
      --no-wildcards-match-slash   wildcards do not match '/'
      --wildcards            use wildcards (default for exclusion)
      --wildcards-match-slash   wildcards match '/' (default for exclusion)

 Informative output:

      --checkpoint[=NUMBER]  display progress messages every NUMBERth record
                             (default 10)
      --checkpoint-action=ACTION   execute ACTION on each checkpoint
      --full-time            print file time to its full resolution
      --index-file=FILE      send verbose output to FILE
  -l, --check-links          print a message if not all links are dumped
      --no-quote-chars=STRING   disable quoting for characters from STRING
      --quote-chars=STRING   additionally quote characters from STRING
      --quoting-style=STYLE  set name quoting style; see below for valid STYLE
                             values
  -R, --block-number         show block number within archive with each message

      --show-defaults        show tar defaults
      --show-omitted-dirs    when listing or extracting, list each directory
                             that does not match search criteria
      --show-snapshot-field-ranges
                             show valid ranges for snapshot-file fields
      --show-transformed-names, --show-stored-names
                             show file or archive names after transformation
      --totals[=SIGNAL]      print total bytes after processing the archive;
                             with an argument - print total bytes when this
                             SIGNAL is delivered; Allowed signals are: SIGHUP,
                             SIGQUIT, SIGINT, SIGUSR1 and SIGUSR2; the names
                             without SIG prefix are also accepted
      --utc                  print file modification times in UTC
  -v, --verbose              verbosely list files processed
      --warning=KEYWORD      warning control
  -w, --interactive, --confirmation
                             ask for confirmation for every action

 Compatibility options:

  -o                         when creating, same as --old-archive; when
                             extracting, same as --no-same-owner

 Other options:

  -?, --help                 give this help list
      --restrict             disable use of some potentially harmful options
      --usage                give a short usage message
      --version              print program version

Mandatory or optional arguments to long options are also mandatory or optional
for any corresponding short options.

The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX.
The version control may be set with --backup or VERSION_CONTROL, values are:

  none, off       never make backups
  t, numbered     make numbered backups
  nil, existing   numbered if numbered backups exist, simple otherwise
  never, simple   always make simple backups

Valid arguments for the --quoting-style option are:

  literal
  shell
  shell-always
  c
  c-maybe
  escape
  locale
  clocale

*This* tar defaults to:
--format=gnu -f- -b20 --quoting-style=escape --rmt-command=/usr/lib/tar/rmt.exe
--rsh-command=/usr/bin/rsh

Report bugs to <[email protected]>.

C:\temp>

From that, it looks like -x -O and -f are valid options.

@runcom
Copy link
Member Author

runcom commented Sep 7, 2016

@jhowardmsft the error in #26369 (comment) shows something about not being able to resolve C: - no clue here

@lowenna
Copy link
Member

lowenna commented Sep 7, 2016

Could that be that it's a posix utility and expecting /c/ rather than Windows-style drive letters?

@runcom
Copy link
Member Author

runcom commented Sep 7, 2016

It could be, though, I don't have any way to test it since I don't have windows vm around. I guess I'll re-push and see what will happen. (or any way you can test this out quickly? :))

@lowenna
Copy link
Member

lowenna commented Sep 7, 2016

I don't unfortunately have an easy way to test Windows to Linux outside of CI. It would take some setting up to get me (back) to being able to do that. Note you should be able to create a free tiny Windows VM instance on Azure. Not sure how long that lasts for, or what other terms there are though. eg https://azure.microsoft.com/en-us/free/. Alternately, you could download a trial of Server 2016 TP5 and install that in a VM locally. eg https://www.microsoft.com/en-us/evalcenter/evaluate-windows-server-essentials-technical-preview

Copy link
Contributor

Choose a reason for hiding this comment

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

These are called "MediaTypes".

Copy link
Member Author

Choose a reason for hiding this comment

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

This is vendored code right? I have a fix for those consts though, to be imported from docker/distribution

Copy link
Contributor

Choose a reason for hiding this comment

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

@runcom I understand, but this is confusing. You may want to fix these upstream. They should all be prefixed with MediaTypeDocker. Both OCI and docker specs use the term "Media Type", over mime type.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevvooe I'm making a patch but I'm not following why this is important in this PR review since those aren't even used :/

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevvooe opened containers/image#78 though...

Copy link
Contributor

Choose a reason for hiding this comment

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

@runcom Just commenting on what I saw, I'll provide a more thorough review in the coming days.

@runcom runcom mentioned this pull request Sep 8, 2016
@runcom runcom force-pushed the oci-save-load branch 3 times, most recently from 5e4738b to 86fd0f2 Compare September 8, 2016 11:04
@runcom
Copy link
Member Author

runcom commented Sep 8, 2016

@jhowardmsft I figured - on windows the tar command needs --force-local (it's not required on Linux since linux paths do not contain colons) because on windows paths have colons (es: C:\) and tar try to resolve C as a remote host and so it fails with that weird tar: Cannot connect to C: resolve failed. Hope it's fixed now and linux, as said, it's unaffected by this change (notice win2lin doesn't fail with pipes but only with local files since I'm saving the tar ball on disk under temp).

However, it's ugly to mix both path separators when extracting so I'm using pipelines as the other tests are doing right (and work on both platform, expecting unix paths).

@runcom runcom force-pushed the oci-save-load branch 3 times, most recently from 9410755 to fa6d45b Compare September 8, 2016 15:35
@runcom
Copy link
Member Author

runcom commented Sep 8, 2016

@stevvooe took care of your comment about Media Type(s)

Copy link
Contributor

Choose a reason for hiding this comment

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

Curiosity: are they really adding this in go1.8?

Copy link
Member

Choose a reason for hiding this comment

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

@runcom
Copy link
Member Author

runcom commented Feb 7, 2017

Thanks @stevvooe

@mlaventure
Copy link
Contributor

@stevvooe when is rc5 expected to be tagged so this can be moved along?

@vbatts
Copy link
Contributor

vbatts commented Feb 10, 2017

@mlaventure I'm going to put up a vote for it today. So, in a week or less.

@stevvooe
Copy link
Contributor

@vbatts RC5 won't have the name change, so we'll have to wait for RC6. Let's get that name change merged.

@runcom
Copy link
Member Author

runcom commented Feb 23, 2017

Rebasing this one and adapting code for image-spec RC5

@runcom runcom changed the title Support OCI image layout in docker save/load [WIP] Support OCI image layout in docker save/load Feb 23, 2017
@runcom runcom force-pushed the oci-save-load branch 2 times, most recently from 9aea97b to 5735ad4 Compare February 23, 2017 13:16
@runcom
Copy link
Member Author

runcom commented Feb 23, 2017

Alright, rebased and it's now working again with RC5 - no issues spotted so far for image-spec, I'll work to add testing (and fix them) later today or tomorrow.

@vbatts @stevvooe

@vbatts
Copy link
Contributor

vbatts commented Feb 23, 2017

thanks @runcom !

Signed-off-by: Antonio Murdaca <[email protected]>
x
Signed-off-by: Antonio Murdaca <[email protected]>
@thaJeztah
Copy link
Member

I'm lost on this one; what's the status? I see an LGTM from @stevvooe, code reviews after. Should we close this if this needs more discussion?

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 6, 2017

I believe we are waiting on image spec changes.

@thaJeztah
Copy link
Member

OK, @runcom let me close this PR for now, while we wait for the image specs; you should be able to reopen when we're ready to move foreward again ❤️ 👍

@thaJeztah thaJeztah closed this Apr 6, 2017
@vbatts
Copy link
Contributor

vbatts commented Apr 7, 2017

Last status we that @runcom had rebased on the latest image-spec RC. I don't know of any blockers for the review of this PR.

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 7, 2017

so we'll have to wait for RC6

Just going by @stevvooe's direction here.
Not sure what the "name change" is, though, or how it affects this.

@thaJeztah
Copy link
Member

@vbatts happy to reopen when possible, I can check up with @stevvooe when SF wakes up. Merely closing some PR's temporarily if they are currently blocked, because we had a lot of open ones, and people kept revisiting, only to discover it was not (yet) actionable

@stevvooe
Copy link
Contributor

@thaJeztah Let's re-open this (not sure why I can't).

@thaJeztah
Copy link
Member

Hm can't reopen either, possibly due to the repo move (seen it on other PR's)

@runcom can you open a new PR from this branch?

@runcom
Copy link
Member Author

runcom commented May 22, 2017

Yup, I'll open another PR, rebase and update to image-spec RC6 when it goes out

@stevvooe
Copy link
Contributor

@runcom Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.