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

Update packages to match latest code in moby/pkg #100

Merged
merged 1 commit into from
Sep 12, 2017

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 8, 2017

Had to vendor in a new version of golang.org/x/net to build
Also had to make some changes to drivers to handle
archive.Reader -> io.Reader
archive.Archive -> io.ReadCloser

Also update .gitingore to ignore emacs files, containers-storage.*
and generated man pages.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@rhatdan
Copy link
Member Author

rhatdan commented Sep 8, 2017

@nalind PTAL
Just updateding pkg directory, needed for updates to overlay driver to support quota size.

@rhatdan rhatdan force-pushed the master branch 4 times, most recently from 140cc44 to 48832b6 Compare September 8, 2017 15:05
@rhatdan
Copy link
Member Author

rhatdan commented Sep 8, 2017

@nalind Now passes tests. PTAL

@@ -2,7 +2,6 @@ language: go
go:
- tip
- 1.8
- 1.7
Copy link
Member

Choose a reason for hiding this comment

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

Is dropping 1.7 necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No but why are we still doing it while everyone else has dropped it. runc/moby/cri-o ...
1.8 is supported on RHEL.

@@ -181,7 +181,7 @@ func changes(layers []string, rw string, dc deleteChange, sc skipChange) ([]Chan
// If /foo/bar/file.txt is modified, then /foo/bar must be part of the changed files.
// This block is here to ensure the change is recorded even if the
// modify time, mode and size of the parent directory in the rw and ro layers are all equal.
// Check https://github.com/docker/docker/pull/13590 for details.
// Check https://github.com/containers/storage/pull/13590 for details.
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to leave this one alone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

options.InUserNS = true
}

if tmpDir, err = ioutil.TempDir("/", "temp-docker-extract"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This reverts part of #17.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -98,7 +108,7 @@ func applyLayerHandler(dest string, layer archive.Reader, options *archive.TarOp
return 0, fmt.Errorf("ApplyLayer json encode: %v", err)
}

cmd := reexec.Command("storage-applyLayer", dest)
cmd := reexec.Command("docker-applyLayer", dest)
Copy link
Member

Choose a reason for hiding this comment

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

This has to agree with pkg/chrootarchive/archive_unix.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

#include <libdevmapper.h>
#include <linux/fs.h> // FIXME: present only for BLKGETSIZE64, maybe we can remove it?

// FIXME: Can't we find a way to do the logging in pure Go?
extern void StorageDevmapperLogCallback(int level, char *file, int line, int dm_errno_or_class, char *str);
extern void DevmapperLogCallback(int level, char *file, int line, int dm_errno_or_class, char *str);
Copy link
Member

Choose a reason for hiding this comment

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

This reverts #14.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -15,8 +15,7 @@ import (
var (
// ErrNotFound plugin not found
ErrNotFound = errors.New("plugin not found")
socketsPath = "/run/containers/storage/plugins"
specsPaths = []string{"/etc/containers/storage/plugins", "/usr/lib/containers/storage/plugins"}
socketsPath = "/run/docker/plugins"
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to make this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

no

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed


package plugins

var specsPaths = []string{"/etc/docker/plugins", "/usr/lib/docker/plugins"}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

"path/filepath"
)

var specsPaths = []string{filepath.Join(os.Getenv("programdata"), "docker", "plugins")}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@nalind
Copy link
Member

nalind commented Sep 8, 2017

It looks like some of the things we've changed in those files need to be reintegrated.

@rhatdan rhatdan force-pushed the master branch 3 times, most recently from 99570a1 to 02a6e7f Compare September 8, 2017 21:08
@rhatdan
Copy link
Member Author

rhatdan commented Sep 8, 2017

@nalind I think I made all of the sed 's/docker-/storage-/g' changes back in.

if err == nil {
t.Fatalf("UntarPath should throw an error if the destination if a file")
}
}

// Do the same test as above but with the destination folder already exists
// and the destination file is a directory
// It's working, see https://github.com/docker/docker/issues/10040
// It's working, see https://github.com/containers/storage/issues/10040
Copy link
Member

Choose a reason for hiding this comment

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

This one should be changed back.

@@ -191,14 +180,14 @@ func TestChangesWithChanges(t *testing.T) {
checkChanges(expectedChanges, changes, t)
}

// See https://github.com/docker/docker/pull/13590
// See https://github.com/containers/storage/pull/13590
Copy link
Member

Choose a reason for hiding this comment

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

This one should be changed back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

// in libdm (even debug ones because there's no way of setting the verbosity
// level for an external logging callback).
//export DevmapperLogCallback
func DevmapperLogCallback(level C.int, file *C.char, line, dmErrnoOrClass C.int, message *C.char) {
Copy link
Member

Choose a reason for hiding this comment

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

We need to leave this rename in place, to avoid symbol multiply-defined errors when this version of the library is pulled in by a newer containers/image into dockerd, per #14.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok changed back

Copy link
Member

Choose a reason for hiding this comment

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

No, it needs to be left renamed to StorageDevmapperLogCallback, as it was before this PR.

}

path := filepath.Join(tmpdir, "echo.spec")
addr := "unix://var/lib/docker/plugins/echo.sock"
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking: might want to move this. The spec file that points to the listening socket is still written in the location where the discovery logic looks, so this test passes, but it's a little confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to containers/storage

@nalind
Copy link
Member

nalind commented Sep 11, 2017

Mostly minor things around renaming, otherwise LGTM.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 12, 2017

Will merge once tests pass.

Had to vendor in a new version of golang.org/x/net to build
Also had to make some changes to drivers to handle
archive.Reader -> io.Reader
archive.Archive -> io.ReadCloser

Also update .gitingore to ignore emacs files, containers-storage.*
and generated man pages.

Also no longer test travis against golang 1.7, cri-o, moby have also
done this.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member Author

rhatdan commented Sep 12, 2017

Tests pass, so merging.

@rhatdan rhatdan merged commit e3e3387 into containers:master Sep 12, 2017
@nalind
Copy link
Member

nalind commented Sep 14, 2017

@nalind
Copy link
Member

nalind commented Sep 27, 2017

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.

2 participants