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

Grab changes form moby/moby/graphdriver for overlay quota #98

Merged
merged 14 commits into from
Sep 26, 2017

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 7, 2017

This led to lots of packages being updated from moby
I tried not to pull in the snap driver stuff.

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

@rhatdan
Copy link
Member Author

rhatdan commented Sep 7, 2017

The goal of this patch set is to satisfy #97

@rhatdan rhatdan changed the title [WIP] We want to grab changes form moby/moby/graphdriver for overlay quota … [WIP] Grab changes form moby/moby/graphdriver for overlay quota Sep 7, 2017
@rhatdan
Copy link
Member Author

rhatdan commented Sep 7, 2017

@nalind @rhvgoyal @mrunalp @runcom PTAL

@runcom
Copy link
Member

runcom commented Sep 7, 2017

looks good (assuming travis will pass) and the integration with CRI-O is smooth :) (and as long as overlay quota works)

t.Fatalf("Error should not be nil because dirs with id 1 should be deleted: %s", p)
}
if _, err := os.Stat(path.Join(tmp, p, "1-removing")); err == nil {
t.Fatalf("Error should not be nil because dirs with id 1-removing should be deleted: %s", p)
Copy link
Member

Choose a reason for hiding this comment

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

I'm back and forth. Should 1-removing be '1-removing'? Just to set it off more quickly?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't support aufs anyways, This code is just copied from Moby

@rhatdan
Copy link
Member Author

rhatdan commented Sep 7, 2017

@runcom a lot of "if's"

@runcom
Copy link
Member

runcom commented Sep 7, 2017

@runcom a lot of "if's"

well, it's still marked WIP :) I'll take a better look once it looks ready for you for sure :)

@@ -5,14 +5,21 @@ package idtools
import (
"os"

"github.com/containers/storage/pkg/system"
"github.com/docker/docker/pkg/system"
Copy link
Member

Choose a reason for hiding this comment

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

Is this change right?

Copy link
Member Author

Choose a reason for hiding this comment

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

NOPE

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 7, 2017

The first patch in the series also removes a couple of packages from the vendor directory, so it should mention that, and might as well remove them from vendor.conf.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 8, 2017

@nalind @rhvgoyal @mrunalp @runcom Tests are passing now, PTAL

@nalind
Copy link
Member

nalind commented Sep 14, 2017

if c := d.ctr.Decrement(mergedDir); c <= 0 {
syscall.Unmount(mergedDir, 0)
if retErr != nil {
if 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.

We're switching from checking err to checking retErr here, so this line and its matching closing brace shouldn't be 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.

Yup I screwed this patch up. Will push the correct change from upstream.

@@ -31,7 +33,7 @@ func init() {
func testInit(dir string, t testing.TB) graphdriver.Driver {
d, err := Init(dir, nil, nil, nil)
if err != nil {
if errors.Cause(err) == graphdriver.ErrNotSupported {
if err == graphdriver.ErrNotSupported {
Copy link
Member

Choose a reason for hiding this comment

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

Should keep unwrapping here, per #73.

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

@@ -567,14 +664,14 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro
// version of aufs.
func useDirperm() bool {
enableDirpermLock.Do(func() {
base, err := ioutil.TempDir("", "storage-aufs-base")
base, err := ioutil.TempDir("", "docker-aufs-base")
Copy link
Member

Choose a reason for hiding this comment

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

Did we lose a rename 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.

Fixed

if err != nil {
logrus.Errorf("error checking dirperm1: %v", err)
return
}
defer os.RemoveAll(base)

union, err := ioutil.TempDir("", "storage-aufs-union")
union, err := ioutil.TempDir("", "docker-aufs-union")
Copy link
Member

Choose a reason for hiding this comment

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

Did we lose a rename 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.

Fixed

// "reg-" stands for "regular file".
// In the future we might use "dev-" for "device file", etc.
// container-maj,min[-inode] stands for:
// - Managed by container storage
// storage-maj,min[-inode] stands for:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: comment doesn't match the string below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes to container

@nalind
Copy link
Member

nalind commented Sep 26, 2017

Okay, I think the current state of things answers all of my comments, so LGTM.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 26, 2017

Ok I will squash the commits and push again.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
We don't support plugin drivers in containers/storage
at this time.  Remove them so we don't have to maintain them.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Need this driver to get overlay quota on xfs support

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

nalind commented Sep 27, 2017

@nalind nalind mentioned this pull request Oct 3, 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.

6 participants