Add overlay2.size daemon storage-opt#32977
Conversation
|
@GordonTheTurtle @amir73il any comments? |
There was a problem hiding this comment.
please wrap err with a human-friendly text
There was a problem hiding this comment.
Do you suggest to have something like graphdriver.ErrPrjQuotaNotSupported ? or graphdriver.FeatureNotSupported ?
Please suggest a name otherwise. Thanks :-)
There was a problem hiding this comment.
Looked at some of the other drivers and made the change. Thanks
There was a problem hiding this comment.
One problem with the way this code is structured is that it doesn't allow the user to override the default size with 0 (i.e. no quota for container).
Another problem is that it is just unneeded code duplication.
How about setting
driver.options.quota = d.options.quota
just before d.parseStorageOpt
This will also be generic enough to apply future defaults if Quota struct ever
gets values like soft limit, inode limit etc...
There was a problem hiding this comment.
Thanks for the suggestion. Made the change.
There was a problem hiding this comment.
Because the storage option name in create/start command is the same for all graphdrivers that support it (i.e. "size=") I think the option name for dockerd storage option should be equally generic (i.e. "default_size="). This will also make documentation more generic. BTW, how about the documentation?
There was a problem hiding this comment.
Added the documentation portion.
Made the change to the option being called as overlay2.size instead of overlay2.default_size
There was a problem hiding this comment.
So you removed the "default_" part and not the "overlay2." part. Oh well, I guess there are arguments to justify both alternatives. Either are fine by me.
30754f3 to
2c63e6a
Compare
2c63e6a to
2188281
Compare
|
Does this apply to only containers or to image as well? |
There was a problem hiding this comment.
I think we should if we are on xfs and we can enable quota. Otherwise we should fail during Init() time itself if user wants to use a default size but we can't enforce that default size.
There was a problem hiding this comment.
IOW, in Init() this check should be there and driver initialization should fail if quota can't be supported and user asked for it.
There was a problem hiding this comment.
In sync with the other comments have made the change.
There was a problem hiding this comment.
@amir73il Q: so the overlay2.size storage opt sets the default for RW layer only or includes the base image(Rootfs)?
Pls clarify.
|
The quota is only applied to container root dir, which is where the overlay upper dir is, |
There was a problem hiding this comment.
Oh, I see, here is the check and error out if quota is not supported. But this will run only if we are running on to of xfs.
How about moving this check out of backingFs == "xfs" if block.
Also error message could probably be more verbose. Say.
If we are not running on xfs, that could be first check and error message could be.
Storage option overlay2.size not supported. Underlying filesystem needs to be xfs
And if we are running on xfs but not supporting quota, we could say.
Storage option overlay2.size not supported. Filesystem is does not support project quota
There was a problem hiding this comment.
That was a race in review :) I did not notice you posted this. Agreed.
Typo "Filesystem is does not ..."
There was a problem hiding this comment.
Made the change as suggested. The messages are little different but mean the same.
There was a problem hiding this comment.
As @rhvgoyal noted correctly, this test should move outside the backingFs == "xfs" block
(i.e. if !projectQuotaSupported) and error message should be made more clear to the user, something like the error in Create():
"overlay2.size option is supported only for overlay over xfs with 'pquota' mount option"
|
@amir73il We seem to enforce quota restrictions in Create(). And IIUC, that's used both by images as well as container root. So IIUC, this quota will be applied to both images as well as container (after this patch). Previously, it will not apply to images because option was per container and there was no way to pass this option for images. Am I missing something? |
|
I think applying these limits for images is a good idea though. People have raised concerns about pulling images with sparse files in it. And being able to put a limit on the max size of each layer will help prevent somebody trying to explode 1TB sparse file on disk. |
|
I see. confession: I don't know docker that well... I guess applying limits to images make sense. So an image will contain single or multi lower layers, which will all limited cumulatively by the project quota limit? |
|
Right. -init is another intermediate layer where docker might want to scribble something and then use it as lower layer for actual container rootfs. So quota will be applied to that too. Though contents written there by docker are very small, so applying quota to -init layer is not a problem. I think being able to apply it to container rootfs and to image layer are going to be very useful. |
|
That's my understanding that every image layer will be limited to project quota specified by overlay2.size. But I also have not spent too much time in looking at overlay2 code. So @imkin might have more definitive answer. |
2188281 to
e5ec2be
Compare
|
/cc @dmcgowan |
|
@rhvgoyal I do see that all the layers comply with the "size" parameter. This, however, is same as what the existing size storage option for the docker run command. So I believe if anything else needs to comply with this "size" param then it should be a holistic issue of the overlayfs2's docker driver here and not in the scope of this PR. |
|
The powerpc test failure is a flake. #33041 |
|
@amir73il Made the suggested changes. Please bless :-) |
|
@imkin, I am just trying to understand the behavior better. Previously though it was in create path, there was no way to pass in this during create for images so it did not impact us. But with your changes, this being a daemon flag, it will apply to both image layer create and container rootfs. And it will impact image pull. Given your changes impact that behavior I want to understand the impact on images. So this will pull size restriction on image layer as well, right? Only thing seems to be that size limit will |
amir73il
left a comment
amir73il
left a comment
There was a problem hiding this comment.
code looks fine to me. still need to understand the implication of image pull. and there is a typo in commit message (daemon daemon)
|
@rhvgoyal just now did a docker pull of a image larger than what the overlay2.size parameter allowed. bash-4.3# docker pull openjdk So @rhvgoyal @amir73il it docker pull also honors the size parameter. The pull is successful when I increase the value of the "size" param. |
There was a problem hiding this comment.
So if we don't want Create to be called with quota; perhaps we should add a non-exported create() that's both called by Create() and CreateReadWrite();
func (d *Driver) Create(id, parent string, opts *graphdriver.CreateOpts) (retErr error) {
if opts != nil && len(opts.StorageOpt) != 0 && !projectQuotaSupported {
return fmt.Errorf("--storage-opt is supported only for overlay over xfs with 'pquota' mount option")
}
return d.create(id, parent string, opts)
}The check in Create() could even be;
if opts != nil && len(opts.StorageOpt) != 0 {
// error...
}Given that Create() then won't support any options
There was a problem hiding this comment.
Looks like we now always pass opts, even if no option is set on dockerd or is passed with docker run; should the code here check if either opts.StorageOpt is set or d.options.quota.Size is set, and otherwise just keep opts as nil?
There was a problem hiding this comment.
When we reach CreateReadWrite we have lost the information whether opts.StorageOpt is defined or not. We only have d.options.quota.Size. Unintialized "d.options.quota.Size" is 0. I am not sure if it is semantically right to assume d.options.quota.Size with "0" as "Not-Initialized". The current "size" option for docker run allows you to define this as "0".
|
@thaJeztah I could not comment on your second question since it may not be a "git review comment" so paraphrasing you here. The check of "projectquotasupport" is not a concern of "Create()" but of CreateReadWrite() since only quota should only be set in the ReadWrite layer. Hence the check is moved to CreateReadWrite. |
The point is that By splitting the Exported |
|
thaJeztah : Q: How is the default for container storage size set for other drivers like dm , btrfs and zfs? Is this just a one-off for Overlay2 driver only?? |
|
@thaJeztah quota support does not make sense for read layers because they are anyways "readonly". Calling |
|
@imkin @rhvgoyal Earlier goyal did a test that shows that the size parameter also affects the "base image". On the client side, i thought that it just impacted the CoW RW layers. "@rhvgoyal just now did a docker pull of a image larger than what the overlay2.size parameter allowed. bash-4.3# docker pull openjdk So @rhvgoyal @amir73il it docker pull also honors the size parameter. The pull is successful when I increase the value of the "size" param." |
This commit adds the overlay2.size option to the daemon daemon storage opts. The user can override this option by the "docker run --storage-opt" options. Signed-off-by: Dhawal Yogesh Bhanushali <[email protected]>
9a48175 to
a63d5bc
Compare
|
@thaJeztah made the changes as per the discussion. Can you check if it satisfies your comments? |
|
@thaJeztah Wish we had taken the feedback in #32977 (comment) before merging this. The parameter |
|
@stevvooe This is not a new option, only able to specified at the daemon level in addition to per-container. But sorry, your suggestion definitely got lost in the sea of comments. Review->RequestChanges may help here in the future. |
|
|
|
@stevvooe I did respond to you but did not hear back from you. I am not sure why "quota" should be mentioned. Thats an implementation detail right? |
"Project Quota" is an implementation detail, yes, but the semantic meaning of The conversation is happening in #33903. |
This adds bash completion for - docker#215 - moby/moby#32977 Signed-off-by: Harald Albers <[email protected]>
This adds bash completion for - docker/cli#215 - moby/moby#32977 Signed-off-by: Harald Albers <[email protected]> Upstream-commit: a4b1769bb600edf9c7542247fccc3e7dae074da0 Component: cli
This adds bash completion for - docker#215 - moby/moby#32977 Signed-off-by: Harald Albers <[email protected]>
12 participants
This commit adds the overlay2.size option to the daemon daemon storage opts.
The user can override this option by the "docker run --storage-opts" options.
Signed-off-by: Dhawal Yogesh Bhanushali [email protected]
closes #30491
- What I did
Added the daemon storage opt overlay2.size option
docker daemon --storage-opt overlay2.size=5G