feature: add local disk quota #725

rudyfly · 2018-02-11T03:24:00Z

Ⅰ. Describe what this PR did

Add local disk quota. In alikernel, with dirquota feature to limit
directory block size.

Ⅱ. Does this pull request fix one issue?

fixes #101

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

In alikernel, you can use it like this:
set volume size is 10G

#pouch volume create -d local -n pouchvolume -o size=10g
CreatedAt:
Driver:       local
Mountpoint:
Name:         pouchvolume
Scope:

run a container, use df to see the volume size.

# pouch run -ti -v pouchvolume:/mnt --name test registry.hub.docker.com/library/busybox:latest df -h
Filesystem                Size      Used Available Use% Mounted on
overlay                  29.4G     19.6G      8.3G  70% /
tmpfs                    64.0M         0     64.0M   0% /dev
shm                      64.0M         0     64.0M   0% /dev/shm
tmpfs                    64.0M         0     64.0M   0% /run
/dev/sda3                10.0G      4.0K     10.0G   0% /mnt

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com

allencloud · 2018-02-11T03:55:59Z

volume/modules/local/quota.go

+	return st.Dev, nil
+}
+
+func toByteSize(s string) uint64 {


Please add a unit test for this function? @rudyfly 😄

And this is similar to the function ToBytes in vendor package code.cloudfouncry.org/bytefmt/bytes.go.
We have a plan to remove this vendored package, could you encapsulate them into a package in pkg.
See #356

allencloud · 2018-02-11T03:56:39Z

volume/modules/local/quota.go

+
+// BaseQuota defines the quota operation interface.
+type BaseQuota interface {
+	QuotaDriverStart(dir string) (string, error)


From my own point of view, StartQuotaDriver is better than QuotaDriverStart. WDYT?

codecov-io · 2018-02-11T08:50:57Z

Codecov Report

Merging #725 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #725      +/-   ##
==========================================
- Coverage   13.58%   13.57%   -0.01%     
==========================================
  Files          97       97              
  Lines        5927     5928       +1     
==========================================
  Hits          805      805              
- Misses       5068     5069       +1     
  Partials       54       54

Impacted Files	Coverage Δ
daemon/mgr/container.go	`3.18% <ø> (ø)`	⬆️
daemon/mgr/volume.go	`0% <ø> (ø)`	⬆️
cli/cli.go	`0% <0%> (ø)`	⬆️
pkg/exec/command.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c18ff53...d964f2e. Read the comment docs.

allencloud · 2018-02-14T01:58:04Z

volume/modules/local/grpquota.go

+			continue
+		}
+		parts := strings.Split(line, " ")
+		if len(parts) == 0 {


I think

if len(parts) == 0 { continue }

is redundant.

yeah, it should be < 2

allencloud · 2018-02-14T02:06:31Z

volume/modules/local/quota.go

+
+var (
+	lock        sync.Mutex
+	quotaLastID uint32


Could you please add more description or comments on these global variables in pkg local?
I think this is very useful for others to understand your thoughts. Actually currently I can read your code with logic, but have no idea about your thoughts. 😢

allencloud · 2018-02-14T02:11:50Z

volume/modules/local/grpquota.go

+	return quota.setUserQuota(id, limit, mountPoint)
+}
+
+// CheckMountpoint is used to check mount point.


Could you add one example valid line in /proc/mounts in comment please?
like:

// CheckMountpoint is used to check mount point which is in file /proc/mounts. // one valid line must have the following format: // cgroup /sys/fs/cgroup/hugetlb cgroup rw,nosuid,nodev,noexec,relatime,hugetlb 0 0

You can also tell others more about the exact meanings of each column. 😄

allencloud · 2018-02-14T02:16:50Z

That is a great work for you to complete this. @rudyfly
Actually I am new for project quota, so I am still learning from that. I just left some tiny comments that may need your help.
And I wish to finish my learning and code review in the late today. Thanks a lot. 😄 🍷

allencloud · 2018-02-15T01:57:36Z

volume/modules/local/grpquota.go

+
+	// on
+	exit, stdout, stderr, err := exec.Run(0, "quotaon", "-pg", mountPoint)
+	if err != nil && exit != 1 {


Should this be if err != nil || exit != 0?
Since if the exit code is non-zero, it means running into an error.

exit = 1 is also valid.

Oh, that is some kind of strange. Could you add some illustration here to tell code readers about that?

allencloud · 2018-02-15T03:15:32Z

This is a great work. While I have one thing may need discussion.

Is there any possibility that we make volume quota drive as plugins.

Currently we have grpquota as the default quota driver, and to be honest, I am afraid we have not taken full advantage of the interface BaseQuota we defined. In the code, I found we use a global instance of gquota and function in pkg local func StartQuotaDriver(dir string) to transfer the request. We did not use gquota.StartQuotaDriver(dir) directly. And this way may weaken the usage of interface we defined and enhance the degree of code customization.

I am wondering if we could still

keep the interface BaseQuota
enhance the functionality of it to be capable of multiple specific quota driver plugins
make users have ability to input the quota driver options

Maybe we can define a global variable var QuotaDrivers map[string]BaseQuota, and in every interface BaseQuota implementation(such as grpquota), we add an init function init() to register grpquota instance into QuotaDrivers. Then we can use another function like

func GetQuotaDriver(dr string) (BaseQuota, error){
    if dr == "" {
        return grpquotaDriver,nil
    }
    driver, exist := QuotaDrivers(dr)
    if !exist{
        return fmt.Errorf("....")
    }
    return driver, nil
}

As a result, we could make users input their wanted quota drivers. And others could also add another quota driver via implementing the interface BaseQuota.

allencloud · 2018-02-22T04:55:28Z

apis/server/volume_bridge.go

+		CreatedAt:  volume.CreationTimestamp.Format("2006-1-2 15:04:05"),
+		Status:     make(map[string]interface{}),
+	}
+	for k, v := range volume.Options() {


I think we can move the following codes above just before respVolume's initialization. We can use

var status map[string]string for k, v := range volume.Options() { if k != "" && v != "" { status[k] = v } }

Then we can use this status in the respVolume initialization.

Although two ways can achieve the same effect, maybe we can make code more clear with two parts: data preparation and respVolume initialization. And this makes no separated assignments.

allencloud · 2018-02-22T04:55:59Z

apis/server/volume_bridge.go

-		},
+		Status:     make(map[string]interface{}),
+	}
+	for k, v := range volume.Options() {


The same as the previous comment.

Add local disk quota. In alikernel, with dirquota feature to limit directory block size. Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

rudyfly · 2018-02-23T02:21:03Z

@allencloud PTAL 😄

allencloud · 2018-02-23T05:32:24Z

LGTM, thanks again for the work. How about the test cases and disk quota document? @rudyfly

allencloud · 2018-02-23T05:34:36Z

volume/modules/local/prjquota.go

+		if devID == devID2 {
+			mountPoint = parts[1]
+			fsType = parts[2]
+			for _, opt := range strings.Split(parts[3], ",") {


Could we just use

if strings.Contains(parts[3], "prjquota") { hasQuota = true }

?

allencloud · 2018-02-23T05:40:06Z

volume/modules/local/grpquota.go

+		if devID == devID2 {
+			mountPoint = parts[1]
+			fsType = parts[2]
+			for _, opt := range strings.Split(parts[3], ",") {


Could we just use

if strings.Contains(parts[3], "grpquota") { hasQuota = true }

?

pouchrobot added kind/feature size/XL labels Feb 11, 2018

rudyfly requested a review from allencloud February 11, 2018 03:24

allencloud reviewed Feb 11, 2018

View reviewed changes

rudyfly force-pushed the volume branch 2 times, most recently from 36a4fcf to 2390e5e Compare February 11, 2018 08:31

rudyfly force-pushed the volume branch from 2390e5e to f8bfccd Compare February 12, 2018 03:56

pouchrobot added size/XXL and removed size/XL labels Feb 12, 2018

rudyfly force-pushed the volume branch 2 times, most recently from e0f667f to 5945c19 Compare February 12, 2018 06:08

allencloud reviewed Feb 14, 2018

View reviewed changes

allencloud reviewed Feb 15, 2018

View reviewed changes

rudyfly force-pushed the volume branch from 5945c19 to 23fe3a7 Compare February 18, 2018 07:59

allencloud reviewed Feb 22, 2018

View reviewed changes

rudyfly force-pushed the volume branch 4 times, most recently from 056c8b9 to 36f4f47 Compare February 23, 2018 01:46

feature: add local disk quota

d964f2e

Add local disk quota. In alikernel, with dirquota feature to limit directory block size. Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

rudyfly force-pushed the volume branch from 36f4f47 to d964f2e Compare February 23, 2018 01:56

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Feb 23, 2018

allencloud reviewed Feb 23, 2018

View reviewed changes

allencloud merged commit b5a250c into AliyunContainerService:master Feb 23, 2018

pouchrobot mentioned this pull request Feb 23, 2018

Weekly Report of Pouch from 2018-02-16 to 2018-02-23 #745

Closed

allencloud mentioned this pull request Mar 1, 2018

[feature request] implement disk quota in pouch #101

Closed

allencloud mentioned this pull request Mar 26, 2018

docs: add diskquota for pouch #972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add local disk quota #725

feature: add local disk quota #725

rudyfly commented Feb 11, 2018 •

edited by allencloud

Loading

allencloud Feb 11, 2018

allencloud Feb 11, 2018

allencloud Feb 11, 2018

rudyfly Feb 12, 2018

codecov-io commented Feb 11, 2018 •

edited

Loading

allencloud Feb 14, 2018

rudyfly Feb 18, 2018

allencloud Feb 14, 2018

allencloud Feb 14, 2018 •

edited

Loading

rudyfly Feb 22, 2018

allencloud commented Feb 14, 2018 •

edited

Loading

allencloud Feb 15, 2018

rudyfly Feb 22, 2018

allencloud Feb 22, 2018

allencloud commented Feb 15, 2018

allencloud Feb 22, 2018 •

edited

Loading

rudyfly Feb 22, 2018

allencloud Feb 22, 2018

rudyfly commented Feb 23, 2018

allencloud commented Feb 23, 2018

allencloud Feb 23, 2018 •

edited

Loading

allencloud Feb 23, 2018

feature: add local disk quota #725

feature: add local disk quota #725

Conversation

rudyfly commented Feb 11, 2018 • edited by allencloud Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 11, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Feb 15, 2018

allencloud Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudyfly commented Feb 23, 2018

allencloud commented Feb 23, 2018

allencloud Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudyfly commented Feb 11, 2018 •

edited by allencloud

Loading

codecov-io commented Feb 11, 2018 •

edited

Loading

allencloud Feb 14, 2018 •

edited

Loading

allencloud commented Feb 14, 2018 •

edited

Loading

allencloud Feb 22, 2018 •

edited

Loading

allencloud Feb 23, 2018 •

edited

Loading