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

CLI flag for docker create(run) to change block device size. #19367

Merged
merged 1 commit into from Mar 29, 2016

Conversation

Projects
None yet
@shishir-a412ed
Contributor

shishir-a412ed commented Jan 15, 2016

Signed-off-by: Shishir Mahajan shishir.mahajan@redhat.com

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Jan 15, 2016

Contributor

Now that daemon option (--storage-opt dm.basesize) has been introduced upstream, I would like to rekindle the discussion on introducing CLI flag (e.g. --storage-opt size:30G) for docker create/run which would allow a user to set containers rootfs size at creation time (provided it is greater than the base device size)

There were two major problems with the existing implementation (daemon option):

  1. This daemon option would allow the user to expand the basedevice size, however that does not necessarily mean that all new containers would be of this new size. I ll give an example. Lets assume my current docker has 2 images (fedora, ubuntu) and 2 containers (fedora_container, ubuntu_container) with the default basedevice size=10G.
    Now I restart the daemon and expanded the base device size to 30G (docker daemon --storage-opt dm.basesize=30G).

If I create a new container on any of the existing images (fedora, ubuntu) the rootfs size of those containers would still be 10G as they are snapshots of the existing image block device and not the basedevice which we just expanded. I feel this would be a little confusing to the user as he might be expecting the new size to show up. To make it work, I would need to remove my existing images (possibly containers associated with it) and repull them in order for my new containers rootfs to be of new base device size (dm.basesize=30G)

  1. With this approach the heaviest application (container) would dictate the size for the rest of the containers. e.g. If I want to have 100 containers on my infrastructure and one of them is a data intensive application requiring 100G of space. I would have to set the base device size (dm.basesize=100G) to 100G. Even though there are other 99 containers which only needed 200 MB of space.

This solution gives more flexibility and solves these problems.

Shishir

Contributor

shishir-a412ed commented Jan 15, 2016

Now that daemon option (--storage-opt dm.basesize) has been introduced upstream, I would like to rekindle the discussion on introducing CLI flag (e.g. --storage-opt size:30G) for docker create/run which would allow a user to set containers rootfs size at creation time (provided it is greater than the base device size)

There were two major problems with the existing implementation (daemon option):

  1. This daemon option would allow the user to expand the basedevice size, however that does not necessarily mean that all new containers would be of this new size. I ll give an example. Lets assume my current docker has 2 images (fedora, ubuntu) and 2 containers (fedora_container, ubuntu_container) with the default basedevice size=10G.
    Now I restart the daemon and expanded the base device size to 30G (docker daemon --storage-opt dm.basesize=30G).

If I create a new container on any of the existing images (fedora, ubuntu) the rootfs size of those containers would still be 10G as they are snapshots of the existing image block device and not the basedevice which we just expanded. I feel this would be a little confusing to the user as he might be expecting the new size to show up. To make it work, I would need to remove my existing images (possibly containers associated with it) and repull them in order for my new containers rootfs to be of new base device size (dm.basesize=30G)

  1. With this approach the heaviest application (container) would dictate the size for the rest of the containers. e.g. If I want to have 100 containers on my infrastructure and one of them is a data intensive application requiring 100G of space. I would have to set the base device size (dm.basesize=100G) to 100G. Even though there are other 99 containers which only needed 200 MB of space.

This solution gives more flexibility and solves these problems.

Shishir

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Jan 25, 2016

Contributor

ping @calavera @rhvgoyal @tiborvass

Can we atleast start the design discussions ?

Shishir

Contributor

shishir-a412ed commented Jan 25, 2016

ping @calavera @rhvgoyal @tiborvass

Can we atleast start the design discussions ?

Shishir

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Feb 2, 2016

Contributor

@calavera @rhvgoyal @tiborvass
Any updates on this one ?

Shishir

Contributor

shishir-a412ed commented Feb 2, 2016

@calavera @rhvgoyal @tiborvass
Any updates on this one ?

Shishir

@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan Feb 4, 2016

Contributor

Please fix test failures.

Contributor

anusha-ragunathan commented Feb 4, 2016

Please fix test failures.

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Feb 4, 2016

Contributor

@anusha-ragunathan Fixed. Please check now.
Don't worry about the vendor failure. That change would eventually go to engine-api and will be vendored into docker. Lets atleast start the design discussions.

Shishir

Contributor

shishir-a412ed commented Feb 4, 2016

@anusha-ragunathan Fixed. Please check now.
Don't worry about the vendor failure. That change would eventually go to engine-api and will be vendored into docker. Lets atleast start the design discussions.

Shishir

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Feb 11, 2016

Contributor

@tiborvass @calavera @anusha-ragunathan Any updates on this ?
27 days and running :) No response.

Contributor

shishir-a412ed commented Feb 11, 2016

@tiborvass @calavera @anusha-ragunathan Any updates on this ?
27 days and running :) No response.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 11, 2016

Contributor

@shishir-a412ed Not sure if you noticed, but we just did a release, and are working on a patch release.
Hopefully can discuss soon.

Contributor

cpuguy83 commented Feb 11, 2016

@shishir-a412ed Not sure if you noticed, but we just did a release, and are working on a patch release.
Hopefully can discuss soon.

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Feb 11, 2016

Contributor

@cpuguy83 Thanks.

Contributor

shishir-a412ed commented Feb 11, 2016

@cpuguy83 Thanks.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 18, 2016

Member

OK, discussed this in the maintainers meeting, moving forward to code review

Member

thaJeztah commented Feb 18, 2016

OK, discussed this in the maintainers meeting, moving forward to code review

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Feb 25, 2016

Collaborator

@shishir-a412ed please update vendoring

Collaborator

tiborvass commented Feb 25, 2016

@shishir-a412ed please update vendoring

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@darstahl

This comment has been minimized.

Show comment
Hide comment
@darstahl

darstahl Mar 1, 2016

Contributor

I'm looking to add something similar for Windows, which would involve a lot of duplicate work to what is done here. What is the current status?

Contributor

darstahl commented Mar 1, 2016

I'm looking to add something similar for Windows, which would involve a lot of duplicate work to what is done here. What is the current status?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 2, 2016

Member

#20863 contains a vendor update for engine-api, so you can rebase after that is merged

Member

thaJeztah commented Mar 2, 2016

#20863 contains a vendor update for engine-api, so you can rebase after that is merged

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Mar 2, 2016

Contributor

@thaJeztah Thanks !

Contributor

shishir-a412ed commented Mar 2, 2016

@thaJeztah Thanks !

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Mar 4, 2016

Member

@shishir-a412ed unit tests failing:

18:25:19 # github.com/docker/docker/daemon/graphdriver/aufs
18:25:19 daemon/graphdriver/aufs/aufs_test.go:753: not enough arguments in call to d.Create
18:25:19 daemon/graphdriver/aufs/aufs_test.go:757: not enough arguments in call to d.Create
18:25:19 daemon/graphdriver/aufs/aufs_test.go:773: not enough arguments in call to d.Create
Member

runcom commented Mar 4, 2016

@shishir-a412ed unit tests failing:

18:25:19 # github.com/docker/docker/daemon/graphdriver/aufs
18:25:19 daemon/graphdriver/aufs/aufs_test.go:753: not enough arguments in call to d.Create
18:25:19 daemon/graphdriver/aufs/aufs_test.go:757: not enough arguments in call to d.Create
18:25:19 daemon/graphdriver/aufs/aufs_test.go:773: not enough arguments in call to d.Create
@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

@runcom Fixed.

Contributor

shishir-a412ed commented Mar 4, 2016

@runcom Fixed.

Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go
// Grow the container rootfs.
if devinfo.Size > 0 {
info, err := devices.lookupDevice(hash)
if info == nil {

This comment has been minimized.

@runcom

runcom Mar 4, 2016

Member

can you check err here

@runcom

runcom Mar 4, 2016

Member

can you check err here

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go
var size int64
var err error
for _, option := range storageOpt {
val := strings.SplitN(option, ":", 2)

This comment has been minimized.

@runcom

runcom Mar 4, 2016

Member

can you split w/o the 2? and check len(val) != 2 and error out here? I think you could get something like val[0] = "size" and val[1] = "1G:test" and fails below (didn't check :))

@runcom

runcom Mar 4, 2016

Member

can you split w/o the 2? and check len(val) != 2 and error out here? I think you could get something like val[0] = "size" and val[1] = "1G:test" and fails below (didn't check :))

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Even in that case (size:1G:test), a split would produce val[0]=size and val[1]=1G:test.
This would eventually fail at units.RAMInBytes(val[1]) and produce the right results (error).

I think this is more human readable and easier to understand. Unless there is a bug in the current code, then I am happy to take a look and fix it. Otherwise I ll let other maintainers chime in.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Even in that case (size:1G:test), a split would produce val[0]=size and val[1]=1G:test.
This would eventually fail at units.RAMInBytes(val[1]) and produce the right results (error).

I think this is more human readable and easier to understand. Unless there is a bug in the current code, then I am happy to take a look and fix it. Otherwise I ll let other maintainers chime in.

This comment has been minimized.

@runcom

runcom Mar 8, 2016

Member

fine

@runcom

runcom Mar 8, 2016

Member

fine

Show outdated Hide outdated integration-cli/docker_cli_create_test.go
imageTest := "emptyfs"
name := inspectField(c, imageTest, "GraphDriver.Name")
if name != "devicemapper" {

This comment has been minimized.

@runcom

runcom Mar 4, 2016

Member

you can use the Devicemapper test requirement

@runcom

runcom Mar 4, 2016

Member

you can use the Devicemapper test requirement

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

Show outdated Hide outdated integration-cli/docker_cli_create_test.go
c.Skip("requires devicemapper graphdriver")
}
out, _ := dockerCmd(c, "create", "-P", "--storage-opt", "size:120G", "busybox", "echo")

This comment has been minimized.

@runcom

runcom Mar 4, 2016

Member

you don't need -P I suppose

@runcom

runcom Mar 4, 2016

Member

you don't need -P I suppose

This comment has been minimized.

@runcom

runcom Mar 4, 2016

Member

you can also remove echo at the end

@runcom

runcom Mar 4, 2016

Member

you can also remove echo at the end

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

Show outdated Hide outdated integration-cli/docker_cli_create_test.go
imageTest := "emptyfs"
name := inspectField(c, imageTest, "GraphDriver.Name")
if name != "devicemapper" {

This comment has been minimized.

@runcom

runcom Mar 4, 2016

Member

as above

@runcom

runcom Mar 4, 2016

Member

as above

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

Show outdated Hide outdated integration-cli/docker_cli_create_test.go
}
// Ensure this fails
out, exit, _ := dockerCmdWithError("create", "-P", "--storage-opt", "size:80G", "busybox", "echo")

This comment has been minimized.

@runcom

runcom Mar 4, 2016

Member

drop exit, and use err and out as:

c.Assert(err, check.NotNil, check.Commentf(out))
c.Assert(out, checker.Contains, "your check string...")

also drop -P and echo

@runcom

runcom Mar 4, 2016

Member

drop exit, and use err and out as:

c.Assert(err, check.NotNil, check.Commentf(out))
c.Assert(out, checker.Contains, "your check string...")

also drop -P and echo

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 4, 2016

Contributor

Fixed.

Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go
if err != nil {
return err
}
if info == nil {

This comment has been minimized.

@runcom

runcom Mar 6, 2016

Member

no need to check for info == nil since it's already done in lookupDevice and at this point info is != nil

@runcom

runcom Mar 6, 2016

Member

no need to check for info == nil since it's already done in lookupDevice and at this point info is != nil

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Fixed.

Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go
}
devinfo.Size = uint64(size)
default:
return fmt.Errorf("Unknown option %s\n", key)

This comment has been minimized.

@runcom

runcom Mar 6, 2016

Member

no need for \n

@runcom

runcom Mar 6, 2016

Member

no need for \n

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Fixed.

Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go
// Read size to change the block device size per container.
var size int64
var err error

This comment has been minimized.

@runcom

runcom Mar 8, 2016

Member

last thing, can you remove those from here and just use := below?

@runcom

runcom Mar 8, 2016

Member

last thing, can you remove those from here and just use := below?

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 8, 2016

Contributor

Fixed.

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Mar 8, 2016

Member

one small nit, LGTM otherwise

ping @calavera @tiborvass

Member

runcom commented Mar 8, 2016

one small nit, LGTM otherwise

ping @calavera @tiborvass

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 9, 2016

Contributor

LGTM

Contributor

calavera commented Mar 9, 2016

LGTM

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 9, 2016

Contributor

moved to docs review.

Contributor

calavera commented Mar 9, 2016

moved to docs review.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 17, 2016

Contributor

LGTM

Contributor

calavera commented Mar 17, 2016

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 18, 2016

Member

looks like this needs rebase @shishir-a412ed 😢

Member

thaJeztah commented Mar 18, 2016

looks like this needs rebase @shishir-a412ed 😢

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Mar 20, 2016

Contributor

@thaJeztah I have rebased the branch. These errors doesn't seem to be related to this PR. WDYT ?

Contributor

shishir-a412ed commented Mar 20, 2016

@thaJeztah I have rebased the branch. These errors doesn't seem to be related to this PR. WDYT ?

Show outdated Hide outdated docs/reference/api/docker_remote_api_v1.23.md
@@ -325,6 +325,7 @@ Create a container
"Ulimits": [{}],
"LogConfig": { "Type": "json-file", "Config": {} },
"SecurityOpt": [""],
"StorageOpt": [""],

This comment has been minimized.

@thaJeztah

thaJeztah Mar 21, 2016

Member

should this be {} ?

@thaJeztah

thaJeztah Mar 21, 2016

Member

should this be {} ?

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

Show outdated Hide outdated docs/reference/api/docker_remote_api_v1.23.md
@@ -444,6 +445,8 @@ Json Parameters:
`Ulimits: { "Name": "nofile", "Soft": 1024, "Hard": 2048 }`
- **SecurityOpt**: A list of string values to customize labels for MLS
systems, such as SELinux.
- **StorageOpt**: Storage driver options per container. Options can be passed in the form
`["size":"120G"]`

This comment has been minimized.

@thaJeztah

thaJeztah Mar 21, 2016

Member

Think this should be {"size":"120G"} ?

@thaJeztah

thaJeztah Mar 21, 2016

Member

Think this should be {"size":"120G"} ?

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

Show outdated Hide outdated man/docker-create.1.md
@@ -325,6 +326,11 @@ unit, `b` is used. Set LIMIT to `-1` to enable unlimited swap.
"seccomp:unconfined" : Turn off seccomp confinement for the container
"seccomp:profile.json : White listed syscalls seccomp Json file to be used as a seccomp filter
**--storage-opt**=[]
Storage driver options per container
$ docker create -it --storage-opt size=120G fedora /bin/bash

This comment has been minimized.

@thaJeztah

thaJeztah Mar 21, 2016

Member

nit: can you add a blank line before and after the example?

@thaJeztah

thaJeztah Mar 21, 2016

Member

nit: can you add a blank line before and after the example?

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

Show outdated Hide outdated man/docker-run.1.md
@@ -476,6 +477,11 @@ its root filesystem mounted as read only prohibiting any writes.
"apparmor=unconfined" : Turn off apparmor confinement for the container
"apparmor=your-profile" : Set the apparmor confinement profile for the container
**--storage-opt**=[]
Storage driver options per container
$ docker run -it --storage-opt size=120G fedora /bin/bash

This comment has been minimized.

@thaJeztah

thaJeztah Mar 21, 2016

Member

same here

@thaJeztah

thaJeztah Mar 21, 2016

Member

same here

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 22, 2016

Contributor

Fixed.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 21, 2016

Member

ping @runcom @vdemeester can you have another look at the code changes (and docs)?

Member

thaJeztah commented Mar 21, 2016

ping @runcom @vdemeester can you have another look at the code changes (and docs)?

func parseStorageOpts(storageOpts []string) (map[string]string, error) {
m := make(map[string]string)
for _, option := range storageOpts {
if strings.Contains(option, "=") {

This comment has been minimized.

@runcom

runcom Mar 21, 2016

Member

@thaJeztah just because I want to be sure, opts from now on should only contains equal sign (for new opts) otherwise for the old opts we accept both equal and column separator right?

@runcom

runcom Mar 21, 2016

Member

@thaJeztah just because I want to be sure, opts from now on should only contains equal sign (for new opts) otherwise for the old opts we accept both equal and column separator right?

This comment has been minimized.

@thaJeztah

thaJeztah Mar 21, 2016

Member

@runcom correct. having both : and = was something we overlooked in the past, so we only fall back for "old" / "existing" options, but new options should only accept =

@thaJeztah

thaJeztah Mar 21, 2016

Member

@runcom correct. having both : and = was something we overlooked in the past, so we only fall back for "old" / "existing" options, but new options should only accept =

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Mar 21, 2016

Member

just a question, code LGTM otherwise

Member

runcom commented Mar 21, 2016

just a question, code LGTM otherwise

Show outdated Hide outdated docs/reference/api/docker_remote_api_v1.23.md
@@ -325,6 +325,7 @@ Create a container
"Ulimits": [{}],
"LogConfig": { "Type": "json-file", "Config": {} },
"SecurityOpt": [""],
"StorageOpt": [{}],

This comment has been minimized.

@thaJeztah

thaJeztah Mar 23, 2016

Member

The [ and ] shouldn't be there, as it isn't an array, just a map?

@thaJeztah

thaJeztah Mar 23, 2016

Member

The [ and ] shouldn't be there, as it isn't an array, just a map?

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Mar 23, 2016

Contributor

Fixed.

@shishir-a412ed

shishir-a412ed Mar 23, 2016

Contributor

Fixed.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 23, 2016

Member

thanks! docs LGTM

Member

thaJeztah commented Mar 23, 2016

thanks! docs LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 23, 2016

Member

oh boy, looks like it didn't make the cut-off for the 1.23 API / 1.11, so unfortunately the API docs changes need to be moved to API 1.24 now 😢

Member

thaJeztah commented Mar 23, 2016

oh boy, looks like it didn't make the cut-off for the 1.23 API / 1.11, so unfortunately the API docs changes need to be moved to API 1.24 now 😢

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Mar 23, 2016

Contributor

@thaJeztah
I don't see any docker_remote_api_v1.24.md file in docs/reference/api.
Is there any other PR (waiting to be merged) that would include this file, so that I can update it ?

Contributor

shishir-a412ed commented Mar 23, 2016

@thaJeztah
I don't see any docker_remote_api_v1.24.md file in docs/reference/api.
Is there any other PR (waiting to be merged) that would include this file, so that I can update it ?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 23, 2016

Member

@shishir-a412ed it should be there (see 928ea1e)

Member

thaJeztah commented Mar 23, 2016

@shishir-a412ed it should be there (see 928ea1e)

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Mar 23, 2016

Contributor

@thaJeztah Please check now.

Contributor

shishir-a412ed commented Mar 23, 2016

@thaJeztah Please check now.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 23, 2016

Member

Thanks @shishir-a412ed, LGTM

Member

thaJeztah commented Mar 23, 2016

Thanks @shishir-a412ed, LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 23, 2016

Member

janky hitting #21408 (not related)

Member

thaJeztah commented Mar 23, 2016

janky hitting #21408 (not related)

CLI flag for docker create(run) to change block device size.
Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Mar 28, 2016

Contributor

@thaJeztah any updates on this one ?

Contributor

shishir-a412ed commented Mar 28, 2016

@thaJeztah any updates on this one ?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 29, 2016

Member

oh! I think it needs one more LGTM for the documentation

ping @vdemeester @MHBauer ptal

Member

thaJeztah commented Mar 29, 2016

oh! I think it needs one more LGTM for the documentation

ping @vdemeester @MHBauer ptal

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 29, 2016

Member

LGTM 🐮

Member

vdemeester commented Mar 29, 2016

LGTM 🐮

@vdemeester vdemeester merged commit e6aa40a into moby:master Mar 29, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 17066 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 3907 has succeeded
Details
janky Jenkins build Docker-PRs 25889 has succeeded
Details
userns Jenkins build Docker-PRs-userns 8113 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 24229 has succeeded
Details
windowsTP4 Jenkins build Docker-PRs-WoW-TP4 3796 has succeeded
Details
@@ -60,6 +60,27 @@ func (s *DockerSuite) TestCreateArgs(c *check.C) {
}
// Make sure we can grow the container's rootfs at creation time.
func (s *DockerSuite) TestCreateGrowRootfs(c *check.C) {
testRequires(c, Devicemapper)

This comment has been minimized.

@mYmNeo

mYmNeo Mar 29, 2016

Contributor

Is there something missing? I can not find any code about Devicemapper testRequirement

@mYmNeo

mYmNeo Mar 29, 2016

Contributor

Is there something missing? I can not find any code about Devicemapper testRequirement

@thaJeztah thaJeztah added this to the 1.12.0 milestone Mar 29, 2016

shishir-a412ed added a commit to shishir-a412ed/docker that referenced this pull request May 5, 2016

BACKPORT: PR #19367: CLI flag for docker create(run) to change block …
…device size

Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
$ docker create -it --storage-opt size=120G fedora /bin/bash
This (size) will allow to set the container rootfs size to 120G at creation time.

This comment has been minimized.

@tiborvass

tiborvass Jun 17, 2016

Collaborator

@thaJeztah @SvenDowideit it should be noted that htis is devicemapper specific. The flag is not, but the value is. Followup pr someone?

@tiborvass

tiborvass Jun 17, 2016

Collaborator

@thaJeztah @SvenDowideit it should be noted that htis is devicemapper specific. The flag is not, but the value is. Followup pr someone?

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Jun 17, 2016

Contributor

@tiborvass what are you looking for ?

@shishir-a412ed

shishir-a412ed Jun 17, 2016

Contributor

@tiborvass what are you looking for ?

This comment has been minimized.

@tiborvass

tiborvass Jun 17, 2016

Collaborator

do all storage drivers have a size option ?

@tiborvass

tiborvass Jun 17, 2016

Collaborator

do all storage drivers have a size option ?

This comment has been minimized.

@shishir-a412ed

shishir-a412ed Jun 17, 2016

Contributor

@tiborvass I think this would be easier to discuss on irc. We can discuss this on Monday.

@shishir-a412ed

shishir-a412ed Jun 17, 2016

Contributor

@tiborvass I think this would be easier to discuss on irc. We can discuss this on Monday.

This comment has been minimized.

@darstahl

darstahl Jun 20, 2016

Contributor

Note that this is no longer devicemapper specific. Windows will have support when #23391 is merged. ZFS also supports this since #21946. There might be more that I missed as well.

@darstahl

darstahl Jun 20, 2016

Contributor

Note that this is no longer devicemapper specific. Windows will have support when #23391 is merged. ZFS also supports this since #21946. There might be more that I missed as well.

This comment has been minimized.

@DrewRay

This comment has been minimized.

Show comment
Hide comment
@DrewRay

DrewRay Jul 22, 2016

What about on docker build ? can we somehow get it to create the new images with a larger base size?

DrewRay commented Jul 22, 2016

What about on docker build ? can we somehow get it to create the new images with a larger base size?

@shishir-a412ed

This comment has been minimized.

Show comment
Hide comment
@shishir-a412ed

shishir-a412ed Jul 23, 2016

Contributor

@DrewRay Please see discussions going on at #22701

With this patch, I can create/run a container with a size bigger than base device size. However there were certain limitations.

  1. Even if I start a container with bigger size, I cannot commit it. This affects docker commit.
  2. I cannot build an image bigger than base device size. This affects docker build.
  3. I cannot pull an image bigger than base device size. This affects docker pull

Upvote on the Issue #22701, If you feel this is a useful feature.
I have patches for all 3 of them. I can open a PR if there is enough interest in these features.

Shishir

Contributor

shishir-a412ed commented Jul 23, 2016

@DrewRay Please see discussions going on at #22701

With this patch, I can create/run a container with a size bigger than base device size. However there were certain limitations.

  1. Even if I start a container with bigger size, I cannot commit it. This affects docker commit.
  2. I cannot build an image bigger than base device size. This affects docker build.
  3. I cannot pull an image bigger than base device size. This affects docker pull

Upvote on the Issue #22701, If you feel this is a useful feature.
I have patches for all 3 of them. I can open a PR if there is enough interest in these features.

Shishir

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

CLI flag for docker create(run) to change block device size
CloudRAN need this.
cherry-pick from moby#19367

Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
Signed-off-by: Lei Jitang <leijitang@huawei.com>

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Merge branch 'fs_quota' into 'IT-1.11.2'
CLI flag for docker create(run) to change block device size

CloudRAN need this.
cherry-pick from moby#19367

Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
Signed-off-by: Lei Jitang <leijitang@huawei.com>


See merge request docker/docker!245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment