refact: refact disk quota api

for pouch cli command, support two api: ```bash --disk-quota []string --quota-id string ``` --disk-quota can be set: 1. --disk-quota=60G 2. --disk-quota=/abc=60G 3. --disk-quota=/&/abc=60G 4. --disk-quota=.*=60G --quota-id can be set: 1. --quota-id=0, or haven't set quota id(--quota-id="") 2. --quota-id=-1 3. --quota-id=16777216, or more than 16777216 detail rules you can see disk quota documents. validate disk quota config when create container, add test case for disk quota. Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
AliyunContainerService · Feb 28, 2019 · a373ba9 · a373ba9
1 parent 1972b01
commit a373ba9
Show file tree

Hide file tree

Showing 11 changed files with 347 additions and 172 deletions.
diff --git a/apis/opts/diskquota.go b/apis/opts/diskquota.go
@@ -17,7 +17,7 @@ func ParseDiskQuota(quotas []string) (map[string]string, error) {
 		parts := strings.Split(quota, "=")
 		switch len(parts) {
 		case 1:
-			quotaMaps["/"] = parts[0]
+			quotaMaps[".*"] = parts[0]
 		case 2:
 			quotaMaps[parts[0]] = parts[1]
 		default:
@@ -33,3 +33,32 @@ func ValidateDiskQuota(quotaMaps map[string]string) error {
 	// TODO
 	return nil
 }
+
+// ParseQuotaID parses quota id configurations of container.
+func ParseQuotaID(id string, quotas []string) (string, error) {
+	switch len(quotas) {
+	case 0:
+		if isSetQuotaID(id) {
+			return "", fmt.Errorf("invalid to set quota id(%s) without disk-quota", id)
+		}
+	case 1:
+		if isSetQuotaID(id) {
+			return id, nil
+		}
+
+		parts := strings.Split(quotas[0], "=")
+		if len(parts) == 1 {
+			return "-1", nil
+		}
+	default:
+		if isSetQuotaID(id) {
+			return "", fmt.Errorf("invalid to set quota id(%s) for multi disk-quota", id)
+		}
+	}
+
+	return id, nil
+}
+
+func isSetQuotaID(id string) bool {
+	return id != "" && id != "0"
+}
diff --git a/apis/opts/diskquota_test.go b/apis/opts/diskquota_test.go
@@ -1,6 +1,7 @@
 package opts
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 )
@@ -18,7 +19,7 @@ func TestParseDiskQuota(t *testing.T) {
 		// TODO: Add test cases.
 		{name: "test1", args: args{diskquota: []string{""}}, want: nil, wantErr: true},
 		{name: "test2", args: args{diskquota: []string{"foo=foo=foo"}}, want: nil, wantErr: true},
-		{name: "test3", args: args{diskquota: []string{"foo"}}, want: map[string]string{"/": "foo"}, wantErr: false},
+		{name: "test3", args: args{diskquota: []string{"foo"}}, want: map[string]string{".*": "foo"}, wantErr: false},
 		{name: "test4", args: args{diskquota: []string{"foo=foo"}}, want: map[string]string{"foo": "foo"}, wantErr: false},
 		{name: "test5", args: args{diskquota: []string{"foo=foo", "bar=bar"}}, want: map[string]string{"foo": "foo", "bar": "bar"}, wantErr: false},
 	}
@@ -35,3 +36,39 @@ func TestParseDiskQuota(t *testing.T) {
 		})
 	}
 }
+
+func TestParseQuotaID(t *testing.T) {
+	tests := []struct {
+		id        string
+		quota     []string
+		expectID  string
+		expectErr error
+	}{
+		{id: "", quota: []string{}, expectID: "", expectErr: nil},
+		{id: "", quota: []string{"20G"}, expectID: "-1", expectErr: nil},
+		{id: "", quota: []string{"20G", "/abc=10G"}, expectID: "", expectErr: nil},
+		{id: "0", quota: []string{}, expectID: "0", expectErr: nil},
+		{id: "0", quota: []string{"20G"}, expectID: "-1", expectErr: nil},
+		{id: "0", quota: []string{"20G", "/abc=10G"}, expectID: "0", expectErr: nil},
+		{id: "-1", quota: []string{}, expectID: "", expectErr: fmt.Errorf("invalid to set quota id(-1) without disk-quota")},
+		{id: "-1", quota: []string{"20G"}, expectID: "-1", expectErr: nil},
+		{id: "-1", quota: []string{"20G", "/abc=10G"}, expectID: "", expectErr: fmt.Errorf("invalid to set quota id(-1) for multi disk-quota")},
+		{id: "1", quota: []string{}, expectID: "", expectErr: fmt.Errorf("invalid to set quota id(1) without disk-quota")},
+		{id: "1", quota: []string{"20G"}, expectID: "1", expectErr: nil},
+		{id: "1", quota: []string{"20G", "/abc=10G"}, expectID: "", expectErr: fmt.Errorf("invalid to set quota id(1) for multi disk-quota")},
+	}
+	for _, tt := range tests {
+		got, err := ParseQuotaID(tt.id, tt.quota)
+		if (err != nil && tt.expectErr == nil) ||
+			(err == nil && tt.expectErr != nil) {
+			t.Fatalf("ParseQuotaID(%v %v) error = %v, wantErr %v", tt.id, tt.quota, err, tt.expectErr)
+		}
+		if err != nil && tt.expectErr != nil && err.Error() != tt.expectErr.Error() {
+			t.Fatalf("ParseQuotaID(%v %v) error = %v, wantErr %v", tt.id, tt.quota, err, tt.expectErr)
+		}
+		if got != tt.expectID {
+			t.Fatalf("ParseQuotaID(%v %v) = %v, want %v", tt.id, tt.quota, got, tt.expectID)
+		}
+
+	}
+}
diff --git a/cli/container.go b/cli/container.go
@@ -139,6 +139,11 @@ func (c *container) config() (*types.ContainerCreateConfig, error) {
 		return nil, err
 	}
 
+	quotaID, err := opts.ParseQuotaID(c.quotaID, c.diskQuota)
+	if err != nil {
+		return nil, err
+	}
+
 	if err := opts.ValidateDiskQuota(diskQuota); err != nil {
 		return nil, err
 	}
@@ -196,7 +201,7 @@ func (c *container) config() (*types.ContainerCreateConfig, error) {
 			InitScript:          c.initScript,
 			ExposedPorts:        ports,
 			DiskQuota:           diskQuota,
-			QuotaID:             c.quotaID,
+			QuotaID:             quotaID,
 			SpecAnnotation:      specAnnotation,
 			NetPriority:         c.netPriority,
 			SpecificID:          c.specificID,

diff --git a/daemon/mgr/container.go b/daemon/mgr/container.go
@@ -9,7 +9,6 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"time"
 
@@ -29,7 +28,6 @@ import (
 	mountutils "github.com/alibaba/pouch/pkg/mount"
 	"github.com/alibaba/pouch/pkg/streams"
 	"github.com/alibaba/pouch/pkg/utils"
-	"github.com/alibaba/pouch/storage/quota"
 	volumetypes "github.com/alibaba/pouch/storage/volume/types"
 
 	"github.com/containerd/cgroups"
@@ -358,6 +356,11 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty
 		return nil, errors.Wrapf(errtypes.ErrInvalidParam, "NetworkingConfig cannot be empty")
 	}
 
+	// validate disk quota
+	if err := mgr.validateDiskQuota(config); err != nil {
+		return nil, errors.Wrapf(err, "invalid disk quota config")
+	}
+
 	id, err := mgr.generateContainerID(config.SpecificID)
 	if err != nil {
 		return nil, err
@@ -1272,75 +1275,35 @@ func (mgr *ContainerManager) Remove(ctx context.Context, name string, options *t
 	return nil
 }
 
-func (mgr *ContainerManager) updateContainerDiskQuota(ctx context.Context, c *Container, diskQuota map[string]string) error {
+func (mgr *ContainerManager) updateContainerDiskQuota(ctx context.Context, c *Container, diskQuota map[string]string) (err error) {
 	if diskQuota == nil {
 		return nil
 	}
 
+	// backup diskquota
+	origDiskQuota := c.Config.DiskQuota
+	defer func() {
+		if err != nil {
+			c.Lock()
+			c.Config.DiskQuota = origDiskQuota
+			c.Unlock()
+		}
+	}()
+
 	c.Lock()
+	if c.Config.DiskQuota == nil {
+		c.Config.DiskQuota = make(map[string]string)
+	}
 	for dir, quota := range diskQuota {
 		c.Config.DiskQuota[dir] = quota
 	}
 	c.Unlock()
 
 	// set mount point disk quota
-	if err := mgr.setMountPointDiskQuota(ctx, c); err != nil {
+	if err = mgr.setDiskQuota(ctx, c, false); err != nil {
 		return errors.Wrapf(err, "failed to set mount point disk quota")
 	}
 
-	c.Lock()
-	var qid uint32
-	if c.Config.QuotaID != "" {
-		id, err := strconv.Atoi(c.Config.QuotaID)
-		if err != nil {
-			return errors.Wrapf(err, "failed to convert QuotaID %s", c.Config.QuotaID)
-		}
-
-		qid = uint32(id)
-		if id < 0 {
-			// QuotaID is < 0, it means pouchd alloc a unique quota id.
-			qid, err = quota.GetNextQuotaID()
-			if err != nil {
-				return errors.Wrap(err, "failed to get next quota id")
-			}
-
-			// update QuotaID
-			c.Config.QuotaID = strconv.Itoa(int(qid))
-		}
-	}
-	c.Unlock()
-
-	// get rootfs quota
-	defaultQuota := quota.GetDefaultQuota(c.Config.DiskQuota)
-	if qid > 0 && defaultQuota == "" {
-		return fmt.Errorf("set quota id but have no set default quota size")
-	}
-	// update container rootfs disk quota
-	// TODO: add lock for container?
-	rootfs := ""
-	if c.IsRunningOrPaused() && c.Snapshotter != nil {
-		basefs, ok := c.Snapshotter.Data["MergedDir"]
-		if !ok || basefs == "" {
-			return fmt.Errorf("Container is running, but MergedDir is missing")
-		}
-		rootfs = basefs
-	} else {
-		if err := mgr.Mount(ctx, c); err != nil {
-			return errors.Wrapf(err, "failed to mount rootfs: (%s)", c.MountFS)
-		}
-		rootfs = c.MountFS
-
-		defer func() {
-			if err := mgr.Unmount(ctx, c); err != nil {
-				logrus.Errorf("failed to umount rootfs: (%s), err: (%v)", c.MountFS, err)
-			}
-		}()
-	}
-	_, err := quota.SetRootfsDiskQuota(rootfs, defaultQuota, qid)
-	if err != nil {
-		return errors.Wrapf(err, "failed to set container rootfs diskquota")
-	}
-
 	return nil
 }