feature: add diskquota support regular expression #1057

rudyfly · 2018-04-04T09:53:19Z

Ⅰ. Describe what this PR did

Add diskquota support regular expression.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

We can use pouch run --disk-quota="key=value" to set quota for container. This pr can make it support to use regular expression key, such as: --disk-quota=".=1g", it means all the quota of mount points are '1g', the priority of key '.' is the lowest. If it has no 'key', just have 'value', such as: --disk-quota=1g, it means the quota of rootfs is 1g. If the mountpoint is a volume, the quota of volume is base on volume's size, the disk-quota is not take effect on a volume.

Ⅳ. Describe how to verify it

The example is test case: TestRunWithDiskQuotaRegular

create a volume with size limit: 256m

# pouch volume create -n diskquota-volume -o size=256m -o mount=/data/volume
Name:         diskquota-volume
Scope:
Status:       map[mount:/data/volume sifter:Default size:256m]
CreatedAt:    2018-4-8 14:18:35
Driver:       local
Labels:       map[]
Mountpoint:   /data/volume/diskquota-volume

create a container with diskquota

# pouch run --name diskquota-test -ti --disk-quota="1024m" --disk-quota=".*=512m" --disk-quota="/mnt/mount1=768m" -v /data/mount1:/mnt/mount1 -v /data/mount2:/mnt/mount2 -v diskquota-volume:/mnt/diskquota-volume registry.docker-cn.com/library/busybox:latest df
Filesystem           1K-blocks      Used Available Use% Mounted on
overlay                1048576        40   1048536   0% /
tmpfs                    65536         0     65536   0% /dev
shm                      65536         0     65536   0% /dev/shm
tmpfs                    65536         0     65536   0% /run
/dev/sdb2               786432         4    786428   0% /mnt/mount1
/dev/sdb2               524288         4    524284   0% /mnt/mount2
/dev/sdb2               262144         4    262140   0% /mnt/diskquota-volume
tmpfs                    65536         0     65536   0% /proc/kcore
tmpfs                    65536         0     65536   0% /proc/timer_list
tmpfs                    65536         0     65536   0% /proc/sched_debug
tmpfs                  2008224         0   2008224   0% /sys/firmware
tmpfs                  2008224         0   2008224   0% /proc/scsi

Ⅴ. Special notes for reviews

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

codecov-io · 2018-04-04T10:20:01Z

Codecov Report

Merging #1057 into master will increase coverage by 0.07%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1057      +/-   ##
==========================================
+ Coverage   15.62%   15.69%   +0.07%     
==========================================
  Files         172      172              
  Lines        9672     9673       +1     
==========================================
+ Hits         1511     1518       +7     
+ Misses       8052     8045       -7     
- Partials      109      110       +1

Impacted Files	Coverage Δ
daemon/mgr/spec_blkio.go	`0% <0%> (ø)`	⬆️
daemon/mgr/container.go	`4.7% <0%> (-0.29%)`	⬇️
cli/container.go	`0% <0%> (ø)`	⬆️
cli/common_flags.go	`0% <0%> (ø)`	⬆️
cli/cli.go	`0% <0%> (ø)`	⬆️
ctrd/watch.go	`0% <0%> (ø)`	⬆️
client/client.go	`28.57% <0%> (+0.57%)`	⬆️
pkg/kernel/kernel.go	`72.72% <0%> (+22.72%)`	⬆️

allencloud · 2018-04-07T14:13:07Z

daemon/mgr/container.go

@@ -1424,6 +1432,66 @@ func (mgr *ContainerManager) parseBinds(ctx context.Context, meta *ContainerMeta
 	return nil
 }

+func (mgr *ContainerManager) setMountPointDiskQuota(ctx context.Context, c *ContainerMeta) error {
+	type diskQuotaRegExp struct {


I do not think defining a struct in function setMountPointDiskQuota is a good idea.
Maybe we could move this into diskquota package. WDYT? @rudyfly

allencloud · 2018-04-07T14:17:37Z

pkg/quota/grpquota.go

@@ -144,7 +144,7 @@ func (quota *GrpQuota) SetDiskQuota(dir string, size string, quotaID int) error
 		return fmt.Errorf("mountpoint not found: %s", dir)
 	}

-	id, err := quota.SetSubtree(dir, uint32(quotaID))
+	id, err := quota.SetSubtree(dir, quotaID)


Do we need to judge whether err is not nil first?

allencloud · 2018-04-07T14:17:52Z

pkg/quota/prjquota.go

@@ -114,7 +114,7 @@ func (quota *PrjQuota) SetDiskQuota(dir string, size string, quotaID int) error
 		return fmt.Errorf("mountpoint not found: %s", dir)
 	}

-	id, err := quota.SetSubtree(dir, uint32(quotaID))
+	id, err := quota.SetSubtree(dir, quotaID)


Do we need to judge whether err is not nil first?

allencloud · 2018-04-07T14:19:06Z

pkg/quota/set_diskquota.go

@@ -37,13 +38,15 @@ func processSetQuotaReexec() {
 		}
 	}()

-	if len(os.Args) != 3 {
+	if len(os.Args) != 4 {
 		err = fmt.Errorf("invalid arguments: %v, it should be: %s: <path> <size>", os.Args, os.Args[0])


If the next statement is return, I am wondering what is the usage of err = fmt.Errorf("invalid arguments: %v, it should be: %s: <path> <size>", os.Args, os.Args[0]).

oh，I forgot to modify this error.

allencloud · 2018-04-08T02:59:40Z

Actually I could not quite follow you via the title.
Could you help to provide more information about the context?

rudyfly · 2018-04-11T08:35:15Z

PTAL @allencloud @yyb196

allencloud · 2018-04-12T13:54:21Z

pkg/quota/set_diskquota.go

-	if len(os.Args) != 3 {
-		err = fmt.Errorf("invalid arguments: %v, it should be: %s: <path> <size>", os.Args, os.Args[0])
+	if len(os.Args) != 4 {
+		err = fmt.Errorf("invalid arguments: %v, it should be: %s: <path> <size> <quota id>", os.Args, os.Args[0])


This err assignment is useless. I think we need to add a log to record this.

logrus.Errorf("invalid arguments: %v, it should be: %s: <path> <size> <quota id>", os.Args, os.Args[0])

It will return the error to runc and then print it. All the hooks do it like this.

allencloud · 2018-04-12T13:54:53Z

pkg/quota/set_diskquota.go

 		if err != nil {
 			logrus.Errorf("failed to set subtree: %v", err)
 			return
 		}

-		err = SetDiskQuota(dir, size, int(qid))
+		err = SetDiskQuota(dir, size, qid)
 		if err != nil {
 			logrus.Errorf("failed to set disk quota: %v", err)


Do we need to return at the err point?

Thank you for your reminder.

allencloud · 2018-04-12T13:56:33Z

test/cli_network_test.go

@@ -326,7 +326,7 @@ func (suite *PouchNetworkSuite) TestNetworkPortMapping(c *check.C) {
 		"-p", "9999:80",
 		image).Assert(c, icmd.Success)

-	time.Sleep(1 * time.Second)
+	time.Sleep(10 * time.Second)


Why to change this?

The network test is always fail, so I increase the delay time.

allencloud · 2018-04-16T02:11:00Z

daemon/mgr/container.go

+	)
+
+	if c.Config.QuotaID != "" {
+		id, err := strconv.Atoi(c.Config.QuotaID)


In the future, we will move this kind of validation out of package mgr. And it should be in the scope of bridge layer.

allencloud · 2018-04-16T02:17:05Z

apis/swagger.yml

@@ -1784,6 +1784,9 @@ definitions:
        type: "object"
        additionalProperties:
          type: "string"
+      QuotaID:
+        type: "string"
+        description: "set disk quota by specified quota id"


Please add more description that how many kinds of quota is there and what do they mean?

allencloud · 2018-04-16T02:20:39Z

pkg/quota/quota.go

@@ -120,3 +120,22 @@ func SetFileAttrNoOutput(dir string, id uint32) {
 func GetNextQuatoID() (uint32, error) {
 	return Gquota.GetNextQuatoID()
 }
+
+//GetDefaultQuota returns the default quota.
+func GetDefaultQuota(quotas map[string]string) string {


Could you change this into GetDefaultQuotaSize ?

allencloud · 2018-04-16T02:22:31Z

daemon/mgr/container.go

+	quotas := c.Config.DiskQuota
+	defaultQuota := quota.GetDefaultQuota(quotas)
+	if setQuotaID && defaultQuota == "" {
+		return fmt.Errorf("set quota id but have no set quota size")


I am afraid we need to emphasize the default, since default will take / and *. into consideration.

set quota id but have no set default quota size?

set quota id but have no set default quota size?

It will be failed when create container.

allencloud · 2018-04-16T02:38:12Z

daemon/mgr/container.go

+		for _, re := range res {
+			findStr := re.Pattern.FindString(mp.Destination)
+			if findStr == mp.Destination {
+				quotas[mp.Destination] = re.Size


If quotas is nil, maybe here we will run into a panic.

allencloud · 2018-04-16T02:41:17Z

pkg/quota/set_diskquota.go

 		return
 	}

 	basefs := os.Args[1]
 	size := os.Args[2]
+	id, _ := strconv.Atoi(os.Args[3])


I am wondering if we should not ignore the error. Maybe we can just record it.

Add diskquota support regular expression. We can use pouch run --disk-quota="key=value" to set quota for container. This pr can make it support to use regular expression key, such as: --disk-quota=".=1g", it means all the quota of mount points are'1g', the priority of key'.'is the lowest. If it has no'key', just have'value', such as: --disk-quota=1g, it means the quota of rootfs is 1g. If the mountpoint is a volume, the quota of volume is base on volume's size, the disk-quota is not take effect on a volume. Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

Specified quota id for a container, if id < 0, it means pouchd alloc a unique quota id. If specified quota id, it will ignore the quota's regular expression, but the volume will keep its size as before. Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

allencloud · 2018-04-16T12:37:04Z

daemon/mgr/container.go

+
+		// if QuotaID is < 0, it means pouchd alloc a unique quota id.
+		if id < 0 {
+			qid, err = quota.GetNextQuatoID()


s/GetNextQuatoID/GetNextQuotaID

allencloud · 2018-04-16T12:37:30Z

pkg/quota/quota.go

@@ -120,3 +120,22 @@ func SetFileAttrNoOutput(dir string, id uint32) {
 func GetNextQuatoID() (uint32, error) {


s/GetNextQuatoID/GetNextQuotaID

allencloud · 2018-04-16T13:01:43Z

LGTM

pouchrobot added kind/feature size/XL labels Apr 4, 2018

pouchrobot added the conflict/needs-rebase label Apr 7, 2018

allencloud reviewed Apr 7, 2018

View reviewed changes

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Apr 7, 2018

rudyfly force-pushed the diskquota branch from e3f86dc to 5430363 Compare April 8, 2018 03:52

pouchrobot removed the conflict/needs-rebase label Apr 8, 2018

rudyfly force-pushed the diskquota branch 2 times, most recently from 85b3773 to 2d0c056 Compare April 8, 2018 07:13

allencloud reviewed Apr 12, 2018

View reviewed changes

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Apr 13, 2018

rudyfly force-pushed the diskquota branch from 2d0c056 to f3d3d24 Compare April 13, 2018 08:02

pouchrobot removed the conflict/needs-rebase label Apr 13, 2018

rudyfly force-pushed the diskquota branch from 49966d5 to e09c6db Compare April 16, 2018 01:51

allencloud reviewed Apr 16, 2018

View reviewed changes

rudyfly added 2 commits April 16, 2018 15:05

rudyfly force-pushed the diskquota branch from e09c6db to f1ca332 Compare April 16, 2018 07:29

allencloud reviewed Apr 16, 2018

View reviewed changes

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

allencloud merged commit 95302f6 into AliyunContainerService:master Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add diskquota support regular expression #1057

feature: add diskquota support regular expression #1057

rudyfly commented Apr 4, 2018 •

edited

Loading

codecov-io commented Apr 4, 2018 •

edited

Loading

allencloud Apr 7, 2018

allencloud Apr 7, 2018

allencloud Apr 7, 2018

allencloud Apr 7, 2018

rudyfly Apr 8, 2018

allencloud commented Apr 8, 2018

rudyfly commented Apr 11, 2018

allencloud Apr 12, 2018

rudyfly Apr 13, 2018

allencloud Apr 12, 2018

rudyfly Apr 13, 2018

allencloud Apr 12, 2018

rudyfly Apr 13, 2018

allencloud Apr 16, 2018

allencloud Apr 16, 2018

allencloud Apr 16, 2018

allencloud Apr 16, 2018 •

edited

Loading

rudyfly Apr 16, 2018 •

edited

Loading

allencloud Apr 16, 2018

allencloud Apr 16, 2018

allencloud Apr 16, 2018 •

edited

Loading

allencloud Apr 16, 2018

allencloud commented Apr 16, 2018

		@@ -120,3 +120,22 @@ func SetFileAttrNoOutput(dir string, id uint32) {
		func GetNextQuatoID() (uint32, error) {

feature: add diskquota support regular expression #1057

feature: add diskquota support regular expression #1057

Conversation

rudyfly commented Apr 4, 2018 • edited 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

codecov-io commented Apr 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Apr 8, 2018

rudyfly commented Apr 11, 2018

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

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

Choose a reason for hiding this comment

allencloud Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

rudyfly Apr 16, 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 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Apr 16, 2018

rudyfly commented Apr 4, 2018 •

edited

Loading

codecov-io commented Apr 4, 2018 •

edited

Loading

allencloud Apr 16, 2018 •

edited

Loading

rudyfly Apr 16, 2018 •

edited

Loading

allencloud Apr 16, 2018 •

edited

Loading