cmd/snap/quota: refactor quota CLI as per new design

* client/quota.go: support current-memory key

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* cmd/snap/quota: refactor quota CLI as per new design

* Make quota command only responsible for showing/displaying quota group
  information.
* Introduce new set-quota command which creates or updates quota groups
* Move the display of memory limit under a map section "constraints" which then
  has a memory key to allow future resource types to live under this section.
* Display current memory usage for quota groups under a new section "current",
  which like "constraints" can be expanded for future resource types too.

Also update the spread test and unit tests for the new output formats.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota-groups: check the cgroup procs file in a loop

This too is a race condition, thanks to Maciej for pointing this out.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/snap-quota-groups: compare slice mem usage from both snapd and kernel

This check ensures that what the kernel says the current memory usage is and
what snapd says the current memory usage is don't differ by more than 10%. In
practice they should be exactly equal if the program is doing nothing as the
go-example-webserver should be, but something does happen to change it \
shouldn't change drastically.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/snap-quota: add a snap with a service to one of the groups

This ensures that we can check the `snap quota` and `snap quotas` output for
the "current" section with the memory usage in it.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota: fix trusty check

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota: increase limits so the webserver snap can start

These limits were too low so the service was killed and no memory usage was
reported in the output, but we want to see some usage from the output to check
that it works, so increase the limits so the server is not killed due to OOM.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota: fix group-one line in output

Also fix the defer statement which was missing the snap to unset the config on.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota-groups: refactor memory check for empty quota groups

We only should be checking what snapd says about memory usage for these empty
groups, we don't need to compare with what the kernel says.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/snap-quota-groups: fix typo

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota-groups: treat memory=4096 as memory=0 for new systems

On the following systems:

- Arch Linux
- Fedora 33
- Fedora 34
- Debian sid
- Ubuntu 21.04

The memory usage of an "empty" but active cgroup ends up being 4K for some
reason, so handle this in the expected output.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/snap-quota-groups: silly typos

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota: fix more typos

non capturing groups are not a thing in grep -E because of course they aren't

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota-groups: use python if python3 is not available

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* tests/main/snap-quota: adjust memory limits for test groups to be at least 4K

On some systems, an empty cgroup with just cgroups nested inside it (but not
necessarily any processes) will have 4K memory usage, so on these systems, we
should make sure that the nested cgroups have enough space in them.

In a follow-up, we will adjust the minimum usable memory limit for a given
cgroup to be 4K to prevent this situation from happening in practice.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

client/quota.go

@@ -36,11 +36,12 @@ type postQuotaData struct {
 }
 
 type QuotaGroupResult struct {
-	GroupName string   `json:"group-name"`
-	Parent    string   `json:"parent,omitempty"`
-	Subgroups []string `json:"subgroups,omitempty"`
-	Snaps     []string `json:"snaps,omitempty"`
-	MaxMemory uint64   `json:"max-memory"`
+	GroupName     string   `json:"group-name"`
+	Parent        string   `json:"parent,omitempty"`
+	Subgroups     []string `json:"subgroups,omitempty"`
+	Snaps         []string `json:"snaps,omitempty"`
+	MaxMemory     uint64   `json:"max-memory"`
+	CurrentMemory uint64   `json:"current-memory"`
 }
 
 // EnsureQuota creates a quota group or updates an existing group.

client/quota_test.go

@@ -72,19 +72,20 @@ func (cs *clientSuite) TestGetQuotaGroup(c *check.C) {
 	cs.rsp = `{
 		"type": "sync",
 		"status-code": 200,
-		"result": {"group-name":"foo", "parent":"bar", "subgroups":["foo-subgrp"], "snaps":["snap-a"], "max-memory":999}
+		"result": {"group-name":"foo", "parent":"bar", "subgroups":["foo-subgrp"], "snaps":["snap-a"], "max-memory":999, "current-memory":450}
 	}`
 
 	grp, err := cs.cli.GetQuotaGroup("foo")
 	c.Assert(err, check.IsNil)
 	c.Check(cs.req.Method, check.Equals, "GET")
 	c.Check(cs.req.URL.Path, check.Equals, "/v2/quotas/foo")
 	c.Check(grp, check.DeepEquals, &client.QuotaGroupResult{
-		GroupName: "foo",
-		Parent:    "bar",
-		Subgroups: []string{"foo-subgrp"},
-		MaxMemory: 999,
-		Snaps:     []string{"snap-a"},
+		GroupName:     "foo",
+		Parent:        "bar",
+		Subgroups:     []string{"foo-subgrp"},
+		MaxMemory:     999,
+		CurrentMemory: 450,
+		Snaps:         []string{"snap-a"},
 	})
 }
 

cmd/snap/cmd_quota.go

@@ -22,6 +22,7 @@ package main
 import (
 	"fmt"
 	"sort"
+	"strings"
 
 	"github.com/jessevdk/go-flags"
 
@@ -30,13 +31,11 @@ import (
 	"github.com/snapcore/snapd/strutil"
 )
 
-var shortQuotaHelp = i18n.G("Create, update or show quota group for a set of snaps")
+var shortQuotaHelp = i18n.G("Show quota group for a set of snaps")
 var longQuotaHelp = i18n.G(`
-The quota command creates, updates or shows quota groups for a a set of snaps.
-
-A quota group sets resource limits (currently maximum memory only) on the set of
-snaps that belong to it. Snaps can be at most in one quota group. Quota groups
-can be nested.
+The quota command shows information about a quota group, including the set of 
+snaps and sub-groups that are in a group, as well as the resource constraints 
+and current usage of the resource constraints.
 `)
 
 var shortQuotasHelp = i18n.G("Show quota groups")
@@ -45,24 +44,47 @@ The quotas command shows all quota groups.
 `)
 
 var shortRemoveQuotaHelp = i18n.G("Remove quota group")
-var longRemoveQuotaHelp = i18n.G(`The remove-quota command removes the given quota group.`)
+var longRemoveQuotaHelp = i18n.G(`
+The remove-quota command removes the given quota group. 
 
-type cmdQuota struct {
-	clientMixin
-	// MaxMemory and MemoryMax are mutually exclusive and provided for
-	// convienience, they mean the same.
-	MaxMemory  string `long:"max-memory" optional:"true"`
-	MemoryMax  string `long:"memory-max" optional:"true"`
-	Parent     string `long:"parent" optional:"true"`
-	Positional struct {
-		GroupName string              `positional-arg-name:"<group-name>" required:"true"`
-		Snaps     []installedSnapName `positional-arg-name:"<snap>" optional:"true"`
-	} `positional-args:"yes"`
-}
+Currently, only quota groups with no sub-groups can be removed. In order to 
+remove a quota group with sub-groups, the sub-groups must first be removed until
+there are no sub-groups for the group, then the group itself can be removed.
+`)
+
+var shortSetQuotaHelp = i18n.G(`Create or update a quota group.`)
+var longSetQuotaHelp = i18n.G(`
+The set-quota command updates or creates a quota group with the specified set of
+snaps.
+
+A quota group sets resource limits (currently maximum memory only) on the set of
+snaps that belong to it. Snaps can be at most in one quota group. Quota groups
+can be nested.
+
+All snaps provided are appended to the group; to remove a snap from a
+quota group the entire group must be removed with remove-quota and recreated 
+without the quota group. To remove a sub-group from the quota group, the 
+sub-group must be removed directly with the remove-quota command.
+
+The memory limit for a quota group can be increased, but cannot be decreased. To
+decrease the memory limit for a quota group, the entire group must be removed
+with the remove-quota command and recreated with the lower limit. Increasing the
+memory limit for a quota group does not restart any services associated with 
+snaps in the quota group.
+
+Adding new snaps to a quota group will result in all non-disabled services in 
+that snap being restarted.
+
+One cannot modify the parent of an existing sub-quota group, nor can an existing
+sub-quota group be moved from one parent to another.
+`)
 
 func init() {
-	cmd := addCommand("quota", shortQuotaHelp, longQuotaHelp, func() flags.Commander { return &cmdQuota{} }, nil, nil)
-	// XXX: unhide
+	// TODO: unhide the commands when non-experimental
+	cmd := addCommand("set-quota", shortSetQuotaHelp, longSetQuotaHelp, func() flags.Commander { return &cmdSetQuota{} }, nil, nil)
+	cmd.hidden = true
+
+	cmd = addCommand("quota", shortQuotaHelp, longQuotaHelp, func() flags.Commander { return &cmdQuota{} }, nil, nil)
 	cmd.hidden = true
 
 	cmd = addCommand("quotas", shortQuotasHelp, longQuotasHelp, func() flags.Commander { return &cmdQuotas{} }, nil, nil)
@@ -72,57 +94,127 @@ func init() {
 	cmd.hidden = true
 }
 
-func (x *cmdQuota) Execute(args []string) (err error) {
+type cmdSetQuota struct {
+	clientMixin
+
+	MemoryMax  string `long:"memory" optional:"true"`
+	Parent     string `long:"parent" optional:"true"`
+	Positional struct {
+		GroupName string              `positional-arg-name:"<group-name>" required:"true"`
+		Snaps     []installedSnapName `positional-arg-name:"<snap>" optional:"true"`
+	} `positional-args:"yes"`
+}
+
+func (x *cmdSetQuota) Execute(args []string) (err error) {
 	var maxMemory string
 	switch {
-	case x.MaxMemory != "" && x.MemoryMax != "":
-		return fmt.Errorf("cannot use --max-memory and --memory-max together")
-	case x.MaxMemory != "":
-		maxMemory = x.MaxMemory
 	case x.MemoryMax != "":
 		maxMemory = x.MemoryMax
 	}
 
-	if maxMemory == "" && x.Parent == "" && len(x.Positional.Snaps) == 0 {
-		return x.showQuotaGroupInfo(x.Positional.GroupName)
-	}
+	names := installedSnapNames(x.Positional.Snaps)
 
-	// TODO: update without max-memory (i.e. append snaps operation).
-	// Also how do we remove snaps from a group? Should the default be append?
-	// Do we want a "reset" operation to start from scratch?
-	if maxMemory == "" {
-		return fmt.Errorf("missing required --max-memory argument")
+	// figure out if the group exists or not to make error messages more useful
+	groupExists := false
+	if _, err = x.client.GetQuotaGroup(x.Positional.GroupName); err == nil {
+		groupExists = true
 	}
 
-	mem, err := strutil.ParseByteSize(maxMemory)
-	if err != nil {
-		return err
+	switch {
+	case maxMemory == "" && x.Parent == "" && len(x.Positional.Snaps) == 0:
+		// no snaps were specified, no memory limit was specified, and no parent
+		// was specified, so just the group name was provided - this is not
+		// supported since there is nothing to change/create
+
+		if groupExists {
+			return fmt.Errorf("no options set to change quota group")
+		}
+		return fmt.Errorf("cannot create quota group without memory limit")
+
+	case maxMemory == "" && x.Parent != "" && len(x.Positional.Snaps) == 0:
+		// this is either trying to create a new group with a parent and forgot
+		// to specify the memory limit for the new group, or the user is trying
+		// to re-parent a group, i.e. move it from the current parent to a
+		// different one, which is currently unsupported
+
+		if groupExists {
+			// TODO: or this could be setting the parent to the existing parent,
+			// which is effectively no change or update but maybe we allow since
+			// it's a noop?
+			return fmt.Errorf("cannot move a quota group to a new parent")
+		}
+		return fmt.Errorf("cannot create quota group without memory limit")
+
+	case maxMemory != "":
+		// we have a memory limit to set for this group, so specify that along
+		// with whatever snaps may have been provided and whatever parent may
+		// have been specified
+
+		mem, err := strutil.ParseByteSize(maxMemory)
+		if err != nil {
+			return err
+		}
+
+		// note that the group could currently exist with a parent, and we could
+		// be specifying x.Parent as "" here - in the future that may mean to
+		// orphan a sub-group to no longer have a parent, but currently it just
+		// means leave the group with whatever parent it has, or if it doesn't
+		// currently exist, create the group without a parent group
+		return x.client.EnsureQuota(x.Positional.GroupName, x.Parent, names, uint64(mem))
+
+	case len(x.Positional.Snaps) != 0:
+		// there are snaps specified for this group but no memory limit, so the
+		// group must already exist and we must be adding the specified snaps to
+		// the group
+
+		// TODO: this case may someday also imply overwriting the current set of
+		// snaps with whatever was specified with some option, but we don't
+		// currently support that, so currently all snaps specified here are
+		// just added to the group
+
+		return x.client.EnsureQuota(x.Positional.GroupName, x.Parent, names, 0)
+
+	default:
+		// should be logically impossible to reach here
+		panic("impossible set of options")
 	}
+}
 
-	names := installedSnapNames(x.Positional.Snaps)
-	return x.client.EnsureQuota(x.Positional.GroupName, x.Parent, names, uint64(mem))
+type cmdQuota struct {
+	clientMixin
+
+	Positional struct {
+		GroupName string `positional-arg-name:"<group-name>" required:"true"`
+	} `positional-args:"yes"`
 }
 
-func (x *cmdQuota) showQuotaGroupInfo(groupName string) error {
-	w := Stdout
+func (x *cmdQuota) Execute(args []string) (err error) {
+	if len(args) != 0 {
+		return fmt.Errorf("too many arguments provided")
+	}
+
 	group, err := x.client.GetQuotaGroup(x.Positional.GroupName)
 	if err != nil {
 		return err
 	}
 
-	// TODO: show current quota usage
+	w := tabWriter()
+	defer w.Flush()
 
-	fmt.Fprintf(w, "name: %s\n", group.GroupName)
+	fmt.Fprintf(w, "name:\t%s\n", group.GroupName)
 	if group.Parent != "" {
-		fmt.Fprintf(w, "parent: %s\n", group.Parent)
+		fmt.Fprintf(w, "parent:\t%s\n", group.Parent)
 	}
+	fmt.Fprintf(w, "constraints:\n")
+	fmt.Fprintf(w, "  memory:\t%s\n", strings.TrimSpace(fmtSize(int64(group.MaxMemory))))
+	fmt.Fprintf(w, "current:\n")
+	fmt.Fprintf(w, "  memory:\t%s\n", strings.TrimSpace(fmtSize(int64(group.CurrentMemory))))
 	if len(group.Subgroups) > 0 {
 		fmt.Fprint(w, "subgroups:\n")
 		for _, name := range group.Subgroups {
 			fmt.Fprintf(w, "  - %s\n", name)
 		}
 	}
-	fmt.Fprintf(w, "max-memory: %s\n", fmtSize(int64(group.MaxMemory)))
 	if len(group.Snaps) > 0 {
 		fmt.Fprint(w, "snaps:\n")
 		for _, snapName := range group.Snaps {
@@ -159,9 +251,19 @@ func (x *cmdQuotas) Execute(args []string) (err error) {
 	}
 
 	w := tabWriter()
-	fmt.Fprintf(w, "Quota\tParent\tMax-Memory\n")
+	fmt.Fprintf(w, "Quota\tParent\tConstraints\tCurrent\n")
 	err = processQuotaGroupsTree(res, func(q *client.QuotaGroupResult) {
-		fmt.Fprintf(w, "%s\t%s\t%s\n", q.GroupName, q.Parent, fmtSize(int64(q.MaxMemory)))
+		constraintMem := ""
+		if q.MaxMemory != 0 {
+			constraintMem = "memory=" + strings.TrimSpace(fmtSize(int64(q.MaxMemory)))
+		}
+
+		currentMem := ""
+		if q.CurrentMemory != 0 {
+			currentMem = "memory=" + strings.TrimSpace(fmtSize(int64(q.CurrentMemory)))
+		}
+
+		fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", q.GroupName, q.Parent, constraintMem, currentMem)
 	})
 	if err != nil {
 		return err