-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Remove btrfs quota groups after containers destroyed #29427
Remove btrfs quota groups after containers destroyed #29427
Conversation
421140b
to
57c74de
Compare
Sorry for late reply. LGTM. |
57c74de
to
44dee9c
Compare
Previously the PR didn't take into consideration that, if Now the PR has been updated to add the check for |
Also wondering the same here as on #29835 (comment) |
I had a quick look at the code, it looks like @thaJeztah may be correct. Would wait on @cpuguy83 for confirmation |
if I'm correct, we may need some mechanism to detect if a quota is set (perhaps querying btrfs), otherwise it's possible we don't know when to cleanup / manage quotas |
I think you are right. On daemon restore it could really be a problem. |
@cpuguy83 @tonistiigi @yongtang so, what we're going to do with this and #29385 |
44dee9c
to
ba10de8
Compare
Here is the complete steps to verify that the issue is fixed (Including --live-restore case). Would be good to setup integration test for btrfs in Jenkins Note: this PR has been updated to incorporate #30497. If this one is merged then #30497 could be closed pre-fix behavior with qgroup leftover:
PR with qgroup cleaned (non --live-restore case):
PR with qgroup cleaned (--live-restore case):
|
daemon/graphdriver/btrfs/btrfs.go
Outdated
@@ -320,7 +337,7 @@ func (d *Driver) subvolEnableQuota() error { | |||
func (d *Driver) subvolDisableQuota() error { | |||
if !d.quotaEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be turned into a isQuotaEnabled()
routine to avoid this code to be duplicated?
Also, quotaEnabled
is just a boolean, but don't we need a lock around it?
ping @cpuguy83 @tonistiigi |
d7f3c09
to
4d312d6
Compare
Thanks @mlaventure for the review. The PR has been updated with |
daemon/graphdriver/btrfs/btrfs.go
Outdated
} | ||
|
||
func (d *Driver) subvolEnableQuota() error { | ||
d.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the lock in isQuotaEnabled
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mlaventure.
Inside the subvolEnableQuota
the d.quotaEnabled
will be changed again to true
at the end of subvolEnableQuota
(located in Line 343). For that I placed the Lock()
outside of isQuotaEnabled()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my bad. Thanks for clarifying.
ping @cpuguy83 @tonistiigi this is waiting for reviews 👼 |
ping @cpuguy83 @tonistiigi PTAL |
c4604b4
to
e09393e
Compare
This is not re-applying qutoas on daemon restart.
|
It looks like we need to rescan for quota in |
Or I guess write out a file to trigger an auto-enable of the quota on restart. |
ping @yongtang 👼 |
ping @yongtang do you still have time to work on this? |
@thaJeztah Sorry for the late response. Let me spend the next several days to get this one fixed. |
8106d15
to
9792f99
Compare
@cpuguy83 The PR has been updated. Now the quota is saved in |
Alternatively, you can just wait for this patchset to land: http://www.spinics.net/lists/linux-btrfs/msg65928.html |
@sargun nice! Unfortunately we don't control the host, so cannot guarantee that patch is present on each distro / version, so not sure we can rely on that |
daemon/graphdriver/btrfs/btrfs.go
Outdated
if quota, err := ioutil.ReadFile(d.quotasDirID(id)); err == nil { | ||
if size, err := strconv.ParseUint(string(quota), 10, 64); err == nil && size >= d.options.minSpace { | ||
d.subvolEnableQuota() | ||
subvolLimitQgroup(dir, size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
daemon/graphdriver/btrfs/btrfs.go
Outdated
@@ -557,6 +635,13 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { | |||
return "", fmt.Errorf("%s: not a directory", dir) | |||
} | |||
|
|||
if quota, err := ioutil.ReadFile(d.quotasDirID(id)); err == nil { | |||
if size, err := strconv.ParseUint(string(quota), 10, 64); err == nil && size >= d.options.minSpace { | |||
d.subvolEnableQuota() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
daemon/graphdriver/btrfs/btrfs.go
Outdated
@@ -533,7 +606,12 @@ func (d *Driver) Remove(id string) error { | |||
if _, err := os.Stat(dir); err != nil { | |||
return err | |||
} | |||
if err := subvolDelete(d.subvolumesDir(), id); err != nil { | |||
os.Remove(d.quotasDirID(id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cpuguy83. Done.
daemon/graphdriver/btrfs/btrfs.go
Outdated
if err := idtools.MkdirAllAs(quotas, 0700, rootUID, rootGID); err != nil { | ||
return err | ||
} | ||
if err := ioutil.WriteFile(path.Join(quotas, id), []byte(fmt.Sprint(driver.options.size)), 0666); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
644?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cpuguy83. The PR has been updated.
This fix tries to address the issue raised in 29325 where btrfs quota groups are not clean up even after containers have been destroyed. The reason for the issue is that btrfs quota groups have to be explicitly destroyed. This fix fixes this issue. This fix is tested manually in Ubuntu 16.04, with steps specified in 29325. This fix fixes 29325. Signed-off-by: Yong Tang <[email protected]>
This commit is an extension of fix for 29325 based on the review comment. In this commit, the quota size for btrfs is kept in `/var/lib/docker/btrfs/quotas` so that a daemon restart keeps quota. Signed-off-by: Yong Tang <[email protected]>
9792f99
to
16328cc
Compare
Thanks @cpuguy83 for the review. The PR has been updated. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
All tests passed now, merging... |
- What I did
This fix tries to address the issue raised in #29325 where btrfs quota groups are not clean up even after containers have been destroyed.
- How I did it
The reason for the issue is that btrfs quota groups have to be explicitly destroyed. This fix fixes this issue.
- How to verify it
This fix is tested manually in Ubuntu 16.04:
pre-fix behavior with qgroup leftover:
PR with qgroup cleaned (non --live-restore case):
PR with qgroup cleaned (--live-restore case):
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
This fix fixes #29325.
Signed-off-by: Yong Tang [email protected]