Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Jun 18, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
54fcd36
o/servicestate/quota_control.go: enforce minimum of 4K for quota groups
anonymouse64 Jun 3, 2021
3d891e0
Merge branch 'master' into feature/quota-groups-remastered-deluxe-dia…
anonymouse64 Jun 8, 2021
7c66d18
tests/main/snap-quota-groups: adjust memory limit to be minimum allowed
anonymouse64 Jun 8, 2021
d28295b
tests/main/snap-quota-groups: adjust memory limit to be slightly larger
anonymouse64 Jun 9, 2021
894388b
Merge branch 'master' into feature/quota-groups-remastered-deluxe-dia…
anonymouse64 Jun 14, 2021
e03fc28
Merge branch 'master' into feature/quota-groups-remastered-deluxe-dia…
anonymouse64 Jun 14, 2021
bef38f5
Merge branch 'master' into feature/quota-groups-remastered-deluxe-dia…
anonymouse64 Jun 15, 2021
3b1bd0a
o/servicestate/quota_handlers.go: adjust error message for clarity
anonymouse64 Jun 15, 2021
ca648fe
Merge remote-tracking branch 'upstream/master' into feature/quota-gro…
mvo5 Jun 17, 2021
6731f9e
Merge branch 'master' into feature/quota-groups-remastered-deluxe-dia…
anonymouse64 Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions daemon/api_quotas_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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,
},
})
Expand Down Expand Up @@ -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,
})
}
Expand Down
8 changes: 8 additions & 0 deletions overlord/servicestate/quota_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,14 @@ func CreateQuota(st *state.State, name string, parentName string, snaps []string
return err
}

// 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 memoryLimit <= 4*quantity.SizeKiB {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I guess a quota the size of a single page is not very useful either 🤣 but alas, if one wants to use and learn the hard way I don't see a reason to prevent that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume with this 4k you can add multiple sub-groups as well? It's not 4k for each sub-group? If so, then 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah well actually funny thing you should ask ... no it's 4K per group it seems but we can't know ahead of time how many sub-groups may go into a group. We could however add accounting to ensure that if a child group is being added that the parent has at least 4096 leftover

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now the thing we will do is to just make the minimum 4K and if folks disagree or have issues we can increase it or make other adjustments later.

return fmt.Errorf("memory limit for group %q is too small, 4KB is minimum size", name)
}

// make sure that the parent group exists if we are creating a sub-group
var grp *quota.Group
updatedGrps := []*quota.Group{}
Expand Down
13 changes: 9 additions & 4 deletions overlord/servicestate/quota_control_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,22 +298,27 @@ func (s *quotaControlSuite) TestCreateQuota(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.CreateQuota(s.state, "foo", "", []string{"test-snap"}, quantity.SizeGiB)
// trying to create a quota with too low of a memory limit fails
err = servicestate.CreateQuota(s.state, "foo", "", []string{"test-snap"}, 4*quantity.SizeKiB)
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
err = servicestate.CreateQuota(s.state, "foo", "", []string{"test-snap"}, 4*quantity.SizeKiB+1)
c.Assert(err, IsNil)

// we can't add the same snap to a different group though
err = servicestate.CreateQuota(s.state, "foo2", "", []string{"test-snap"}, quantity.SizeGiB)
c.Assert(err, ErrorMatches, `cannot add snap "test-snap" to group "foo2": snap already in quota group "foo"`)

// creating the same group again will fail
err = servicestate.CreateQuota(s.state, "foo", "", []string{"test-snap"}, quantity.SizeGiB)
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"},
},
})
Expand Down