Skip to content

Implement windowsDiff.Compare to allow outputting OCI images #4399

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

Merged
jterry75 merged 4 commits into from
Apr 12, 2021
Merged

Implement windowsDiff.Compare to allow outputting OCI images #4399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b399e2e
Don't lose Compare failure if aborting diff upload fails
TBBle Jul 17, 2020
149fa36
Don't tease the logger with a %-less format string
TBBle Jul 17, 2020
a64a768
Replace inline applyWindowsLayer using hcsshim
TBBle Jan 16, 2021
c750498
Implement windowsDiff.Compare via hcsshim/pkg/ociwclayer
TBBle Jul 17, 2020
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
25 changes: 23 additions & 2 deletions archive/tar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,34 @@ func Diff(ctx context.Context, a, b string) io.ReadCloser {
}

// WriteDiff writes a tar stream of the computed difference between the
// provided directories.
// provided paths.
//
// Produces a tar using OCI style file markers for deletions. Deleted
// files will be prepended with the prefix ".wh.". This style is
// based off AUFS whiteouts.
// See https://github.com/opencontainers/image-spec/blob/master/layer.md
func WriteDiff(ctx context.Context, w io.Writer, a, b string) error {
func WriteDiff(ctx context.Context, w io.Writer, a, b string, opts ...WriteDiffOpt) error {
var options WriteDiffOptions
for _, opt := range opts {
if err := opt(&options); err != nil {
return errors.Wrap(err, "failed to apply option")
}
}
if options.writeDiffFunc == nil {
options.writeDiffFunc = writeDiffNaive
}

return options.writeDiffFunc(ctx, w, a, b, options)
}

// writeDiffNaive writes a tar stream of the computed difference between the
// provided directories on disk.
//
// Produces a tar using OCI style file markers for deletions. Deleted
// files will be prepended with the prefix ".wh.". This style is
// based off AUFS whiteouts.
// See https://github.com/opencontainers/image-spec/blob/master/layer.md
func writeDiffNaive(ctx context.Context, w io.Writer, a, b string, _ WriteDiffOptions) error {
cw := newChangeWriter(w, b)
err := fs.Changes(ctx, a, b, cw.HandleChange)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions archive/tar_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestOverlayApplyNoParents(t *testing.T) {
}
fstest.FSSuite(t, overlayDiffApplier{
tmp: base,
diff: func(ctx context.Context, w io.Writer, a, b string) error {
diff: func(ctx context.Context, w io.Writer, a, b string, _ ...WriteDiffOpt) error {
cw := newChangeWriter(w, b)
cw.addedDirs = nil
err := fs.Changes(ctx, a, b, cw.HandleChange)
Expand All @@ -85,7 +85,7 @@ func TestOverlayApplyNoParents(t *testing.T) {

type overlayDiffApplier struct {
tmp string
diff func(context.Context, io.Writer, string, string) error
diff func(context.Context, io.Writer, string, string, ...WriteDiffOpt) error
t *testing.T
}

Expand Down
10 changes: 10 additions & 0 deletions archive/tar_opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,13 @@ func WithParents(p []string) ApplyOpt {
return nil
}
}

// WriteDiffOptions provides additional options for a WriteDiff operation
type WriteDiffOptions struct {
ParentLayers []string // Windows needs the full list of parent layers
Comment thread
jterry75 marked this conversation as resolved.
Outdated

writeDiffFunc func(context.Context, io.Writer, string, string, WriteDiffOptions) error
}

// WriteDiffOpt allows setting mutable archive write properties on creation
type WriteDiffOpt func(options *WriteDiffOptions) error
43 changes: 43 additions & 0 deletions archive/tar_opts_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@

package archive

import (
"context"
"io"

"github.com/Microsoft/hcsshim/pkg/ociwclayer"
)

// applyWindowsLayer applies a tar stream of an OCI style diff tar of a Windows layer
// See https://github.com/opencontainers/image-spec/blob/master/layer.md#applying-changesets
func applyWindowsLayer(ctx context.Context, root string, r io.Reader, options ApplyOptions) (size int64, err error) {
return ociwclayer.ImportLayerFromTar(ctx, r, root, options.Parents)
}

// AsWindowsContainerLayer indicates that the tar stream to apply is that of
// a Windows Container Layer. The caller must be holding SeBackupPrivilege and
// SeRestorePrivilege.
Expand All @@ -27,3 +40,33 @@ func AsWindowsContainerLayer() ApplyOpt {
return nil
}
}

// writeDiffWindowsLayers writes a tar stream of the computed difference between the
// provided Windows layers
//
// Produces a tar using OCI style file markers for deletions. Deleted
// files will be prepended with the prefix ".wh.". This style is
// based off AUFS whiteouts.
// See https://github.com/opencontainers/image-spec/blob/master/layer.md
func writeDiffWindowsLayers(ctx context.Context, w io.Writer, _, layer string, options WriteDiffOptions) error {
return ociwclayer.ExportLayerToTar(ctx, w, layer, options.ParentLayers)
}

// AsWindowsContainerLayerPair indicates that the paths to diff are a pair of
// Windows Container Layers. The caller must be holding SeBackupPrivilege.
func AsWindowsContainerLayerPair() WriteDiffOpt {
Copy link
Copy Markdown
Contributor

@jterry75 jterry75 Jan 11, 2021

Choose a reason for hiding this comment

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

I actually dont understand this one? What does this mean to you :)

Copy link
Copy Markdown
Contributor Author

@TBBle TBBle Jan 12, 2021

Choose a reason for hiding this comment

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

It's like AsWindowsContainerLayer, except instead of applying a layer, it's diffing a pair of layers. This whole redirection via applyFunc/writeDiffFunc is a bit weird, which was my thinking in #4399 (comment). However, I didn't want to refactor the Apply and Compare paths as part of the same PR that implemented the Compare path to match the Apply path.

Copy link
Copy Markdown

@katiewasnothere katiewasnothere Feb 10, 2021

Choose a reason for hiding this comment

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

The place that we actually use this config option, we've already verified the two paths are layers. IMO it makes more sense to make this generic, such as WithDiffFunc(df diffFunc) or something like that. What do you think?

Copy link
Copy Markdown
Contributor Author

@TBBle TBBle Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm. I could refactor AsWindowsContainerLayer and AsWindowsContainerLayerPair to be WithApplyFunc and WithDiffFunc, and then applyWindowsLayer and writeDiffWindowsLayer would move into diff/windows/windows.go, passed in by (s windowsDiff) Apply and (s windowsDiff) Compare respectively.

However, that means that anyone else trying to use github.com/containerd/containerd/archive.Apply or github.com/containerd/containerd/archive.WriteDiff for WCOW would need to provide their own diff funcs to pass in, unless we make applyWindowsLayer and writeDiffWindowsLayer public, which I'm not super-fond of.

Copy link
Copy Markdown
Contributor Author

@TBBle TBBle Mar 2, 2021

Choose a reason for hiding this comment

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

Maybe as a clarifying question, how "public" is github.com/containerd/containerd/archive support for WCOW layers supposed to be, as an API?

Apart from github.com/containerd/containerd/diff/windows/, nothing in this tree seems to be prepared for WCOW usage of archive.Apply or archive.Diff, so it might make sense to replace archive.Apply(..., AsWindowsContainerLayer()) and archive.Diff(..., AsWindowsContainerLayerPair()) with functions directly in github.com/containerd/containerd/diff/windows/, and leave github.com/containerd/containerd/archive to only support the walking-diff use-case, perhaps with a slight special-case to recognise WCOW layer archives and pretend nothing outside Files/ exists, i.e. replacing the strings.Replace(hdr.Name, "Files", "", 1) in /install.go with one performed during the walk, to allow some rational behaviour for non-Windows platforms manipulating WCOW layer archives.

Copy link
Copy Markdown
Contributor Author

@TBBle TBBle Mar 2, 2021

Choose a reason for hiding this comment

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

Overall, I'd prefer not to mix such refactorings into this simply because the current approach implements Compare to match Apply, and any further changes should be done to both at once, and as an API change that affects an existing public API, be clearly distinguished as-such.

return func(options *WriteDiffOptions) error {
options.writeDiffFunc = writeDiffWindowsLayers
return nil
}
}

// WithParentLayers provides the Windows Container Layers that are the parents
// of the target (right-hand, "upper") layer, if any. The source (left-hand, "lower")
// layer passed to WriteDiff should be "" in this case.
func WithParentLayers(p []string) WriteDiffOpt {
return func(options *WriteDiffOptions) error {
options.ParentLayers = p
return nil
}
}
137 changes: 0 additions & 137 deletions archive/tar_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,33 +20,14 @@ package archive

import (
"archive/tar"
"bufio"
"context"
"fmt"
"io"
"os"
"path"
"path/filepath"
"strings"

"github.com/Microsoft/go-winio"
"github.com/Microsoft/go-winio/backuptar"
"github.com/Microsoft/hcsshim"
"github.com/containerd/containerd/sys"
"github.com/pkg/errors"
)

var (
// mutatedFiles is a list of files that are mutated by the import process
// and must be backed up and restored.
mutatedFiles = map[string]string{
"UtilityVM/Files/EFI/Microsoft/Boot/BCD": "bcd.bak",
"UtilityVM/Files/EFI/Microsoft/Boot/BCD.LOG": "bcd.log.bak",
"UtilityVM/Files/EFI/Microsoft/Boot/BCD.LOG1": "bcd.log1.bak",
"UtilityVM/Files/EFI/Microsoft/Boot/BCD.LOG2": "bcd.log2.bak",
}
)

// tarName returns platform-specific filepath
// to canonical posix-style path for tar archival. p is relative
// path.
Expand Down Expand Up @@ -141,121 +122,3 @@ func copyDirInfo(fi os.FileInfo, path string) error {
func copyUpXAttrs(dst, src string) error {
return nil
}

// applyWindowsLayer applies a tar stream of an OCI style diff tar of a Windows
// layer using the hcsshim layer writer and backup streams.
// See https://github.com/opencontainers/image-spec/blob/master/layer.md#applying-changesets
func applyWindowsLayer(ctx context.Context, root string, r io.Reader, options ApplyOptions) (size int64, err error) {
home, id := filepath.Split(root)
info := hcsshim.DriverInfo{
HomeDir: home,
}

w, err := hcsshim.NewLayerWriter(info, id, options.Parents)
if err != nil {
return 0, err
}
defer func() {
if err2 := w.Close(); err2 != nil {
// This error should not be discarded as a failure here
// could result in an invalid layer on disk
if err == nil {
err = err2
}
}
}()

tr := tar.NewReader(r)
buf := bufio.NewWriter(nil)
hdr, nextErr := tr.Next()
// Iterate through the files in the archive.
for {
select {
case <-ctx.Done():
return 0, ctx.Err()
default:
}

if nextErr == io.EOF {
// end of tar archive
break
}
if nextErr != nil {
return 0, nextErr
}

// Note: path is used instead of filepath to prevent OS specific handling
// of the tar path
base := path.Base(hdr.Name)
if strings.HasPrefix(base, whiteoutPrefix) {
dir := path.Dir(hdr.Name)
originalBase := base[len(whiteoutPrefix):]
originalPath := path.Join(dir, originalBase)
if err := w.Remove(filepath.FromSlash(originalPath)); err != nil {
return 0, err
}
hdr, nextErr = tr.Next()
} else if hdr.Typeflag == tar.TypeLink {
err := w.AddLink(filepath.FromSlash(hdr.Name), filepath.FromSlash(hdr.Linkname))
if err != nil {
return 0, err
}
hdr, nextErr = tr.Next()
} else {
name, fileSize, fileInfo, err := backuptar.FileInfoFromHeader(hdr)
if err != nil {
return 0, err
}
if err := w.Add(filepath.FromSlash(name), fileInfo); err != nil {
return 0, err
}
size += fileSize
hdr, nextErr = tarToBackupStreamWithMutatedFiles(buf, w, tr, hdr, root)
}
}

return
}

// tarToBackupStreamWithMutatedFiles reads data from a tar stream and
// writes it to a backup stream, and also saves any files that will be mutated
// by the import layer process to a backup location.
func tarToBackupStreamWithMutatedFiles(buf *bufio.Writer, w io.Writer, t *tar.Reader, hdr *tar.Header, root string) (nextHdr *tar.Header, err error) {
var (
bcdBackup *os.File
bcdBackupWriter *winio.BackupFileWriter
)
if backupPath, ok := mutatedFiles[hdr.Name]; ok {
bcdBackup, err = os.Create(filepath.Join(root, backupPath))
if err != nil {
return nil, err
}
defer func() {
cerr := bcdBackup.Close()
if err == nil {
err = cerr
}
}()

bcdBackupWriter = winio.NewBackupFileWriter(bcdBackup, false)
defer func() {
cerr := bcdBackupWriter.Close()
if err == nil {
err = cerr
}
}()

buf.Reset(io.MultiWriter(w, bcdBackupWriter))
} else {
buf.Reset(w)
}

defer func() {
ferr := buf.Flush()
if err == nil {
err = ferr
}
}()

return backuptar.WriteBackupStreamFromTarFile(buf, t, hdr)
}
4 changes: 2 additions & 2 deletions diff/walking/differ.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ func (s *walkingDiff) Compare(ctx context.Context, lower, upper []mount.Mount, o
if err != nil {
cw.Close()
if newReference {
if err := s.store.Abort(ctx, config.Reference); err != nil {
log.G(ctx).WithField("ref", config.Reference).Warnf("failed to delete diff upload")
if abortErr := s.store.Abort(ctx, config.Reference); abortErr != nil {
log.G(ctx).WithError(abortErr).WithField("ref", config.Reference).Warnf("failed to delete diff upload")
}
}
}
Expand Down
Loading