o/servicestate/quota_control.go: enforce minimum of 4K for quota groups

daemon/api_quotas_test.go

@@ -62,11 +62,11 @@ func (s *apiQuotaSuite) SetUpTest(c *check.C) {
 }
 
 func mockQuotas(st *state.State, c *check.C) {
-	err := servicestate.CreateQuota(st, "foo", "", nil, 9000)
+	err := servicestate.CreateQuota(st, "foo", "", nil, 11000)
 	c.Assert(err, check.IsNil)
-	err = servicestate.CreateQuota(st, "bar", "foo", nil, 1000)
+	err = servicestate.CreateQuota(st, "bar", "foo", nil, 6000)
 	c.Assert(err, check.IsNil)
-	err = servicestate.CreateQuota(st, "baz", "foo", nil, 2000)
+	err = servicestate.CreateQuota(st, "baz", "foo", nil, 5000)
 	c.Assert(err, check.IsNil)
 }
 
@@ -147,7 +147,7 @@ func (s *apiQuotaSuite) TestPostEnsureQuotaCreateHappy(c *check.C) {
 func (s *apiQuotaSuite) TestPostEnsureQuotaUpdateHappy(c *check.C) {
 	st := s.d.Overlord().State()
 	st.Lock()
-	err := servicestate.CreateQuota(st, "ginger-ale", "", nil, 1000)
+	err := servicestate.CreateQuota(st, "ginger-ale", "", nil, 5000)
 	st.Unlock()
 	c.Assert(err, check.IsNil)
 
@@ -286,19 +286,19 @@ func (s *apiQuotaSuite) TestListQuotas(c *check.C) {
 		{
 			GroupName:     "bar",
 			Parent:        "foo",
-			MaxMemory:     1000,
+			MaxMemory:     6000,
 			CurrentMemory: 500,
 		},
 		{
 			GroupName:     "baz",
 			Parent:        "foo",
-			MaxMemory:     2000,
+			MaxMemory:     5000,
 			CurrentMemory: 1000,
 		},
 		{
 			GroupName:     "foo",
 			Subgroups:     []string{"bar", "baz"},
-			MaxMemory:     9000,
+			MaxMemory:     11000,
 			CurrentMemory: 5000,
 		},
 	})
@@ -330,7 +330,7 @@ func (s *apiQuotaSuite) TestGetQuota(c *check.C) {
 	c.Check(res, check.DeepEquals, client.QuotaGroupResult{
 		GroupName:     "bar",
 		Parent:        "foo",
-		MaxMemory:     1000,
+		MaxMemory:     6000,
 		CurrentMemory: 500,
 	})
 }

overlord/servicestate/quota_handlers.go

@@ -126,6 +126,14 @@ func quotaCreate(st *state.State, t *state.Task, action QuotaControlAction, allG
 		return fmt.Errorf("internal error, MemoryLimit option is mandatory for create action")
 	}
 
+	// make sure the memory limit is at least 4K, that is the minimum size
+	// to allow nesting, otherwise groups with less than 4K will trigger the
+	// oom killer to be invoked when a new group is added as a sub-group to the
+	// larger group.
+	if action.MemoryLimit <= 4*quantity.SizeKiB {
+		return fmt.Errorf("memory limit for group %q is too small, 4KB is minimum size", action.QuotaName)
+	}
+
 	// make sure the specified snaps exist and aren't currently in another group
 	if err := validateSnapForAddingToGroup(st, action.AddSnaps, action.QuotaName, allGrps); err != nil {
 		return err

overlord/servicestate/quota_handlers_test.go

@@ -273,29 +273,36 @@ func (s *quotaHandlersSuite) TestQuotaCreate(c *C) {
 	snapstate.Set(s.state, "test-snap", s.testSnapState)
 	snaptest.MockSnapCurrent(c, testYaml, s.testSnapSideInfo)
 
-	// now we can create the quota group
-	err = servicestate.QuotaCreate(st, nil, qc, allGrps(c, st), nil, nil)
-	c.Assert(err, IsNil)
-
-	// creating the same group again will fail
-	err = servicestate.QuotaCreate(st, nil, qc, allGrps(c, st), nil, nil)
-	c.Assert(err, ErrorMatches, `group "foo" already exists`)
-
-	// we can't add the same snap to a different group
 	qc2 := servicestate.QuotaControlAction{
 		Action:      "create",
-		QuotaName:   "foo2",
-		MemoryLimit: quantity.SizeGiB,
+		QuotaName:   "foo",
+		MemoryLimit: 4 * quantity.SizeKiB,
 		AddSnaps:    []string{"test-snap"},
 	}
 
+	// trying to create a quota with too low of a memory limit fails
 	err = servicestate.QuotaCreate(st, nil, qc2, allGrps(c, st), nil, nil)
-	c.Assert(err, ErrorMatches, `cannot add snap "test-snap" to group "foo2": snap already in quota group "foo"`)
+	c.Assert(err, ErrorMatches, `memory limit for group "foo" is too small, 4KB is minimum size`)
+
+	// but with an adequately sized memory limit, and a snap that exists, we can
+	// create it
+	qc3 := servicestate.QuotaControlAction{
+		Action:      "create",
+		QuotaName:   "foo",
+		MemoryLimit: 4*quantity.SizeKiB + 1,
+		AddSnaps:    []string{"test-snap"},
+	}
+	err = servicestate.QuotaCreate(st, nil, qc3, allGrps(c, st), nil, nil)
+	c.Assert(err, IsNil)
+
+	// creating the same group again will fail
+	err = servicestate.CreateQuota(s.state, "foo", "", []string{"test-snap"}, 4*quantity.SizeKiB+1)
+	c.Assert(err, ErrorMatches, `group "foo" already exists`)
 
 	// check that the quota groups were created in the state
 	checkQuotaState(c, st, map[string]quotaGroupState{
 		"foo": {
-			MemoryLimit: quantity.SizeGiB,
+			MemoryLimit: 4*quantity.SizeKiB + 1,
 			Snaps:       []string{"test-snap"},
 		},
 	})

tests/main/snap-quota-groups/task.yaml

@@ -106,7 +106,7 @@ execute: |
   systemctl show --property=ActiveState "$sliceName" | MATCH "ActiveState=inactive"
 
   echo "Creating a quota with a very small memory limit results in the service being unable to start"
-  snap set-quota too-small --memory=1KB go-example-webserver
+  snap set-quota too-small --memory=4097B go-example-webserver
   # clear "oom-killer" message from dmesg or prepare-restore.sh will fail here.
   tests.cleanup defer dmesg -c