Merge pull request #10488 from anonymouse64/feature/quota-groups-the-final-countdown-4

daemon/api_quotas.go: handle conflicts, returning conflict response Use a more proper error kind for quota conflicts, supporting the error type *servicestate.QuotaChangeConflictError inside errToResponse and indicating what sort of change the conflict is being used for. Also update the unit tests for the daemon error response to check for this.

7 files changed, 225 insertions, 13 deletions

diff --git a/client/errors.go b/client/errors.go
index f26a468aab..87769074e9 100644
--- a/client/errors.go
+++ b/client/errors.go
@@ -104,6 +104,13 @@ const (
	// `snap-name`, `change-kind` of the ongoing change.
	ErrorKindSnapChangeConflict ErrorKind = "snap-change-conflict"
 
+	// ErrorKindQuotaChangeConflict: the requested operation would
+	// conflict with a currently ongoing change affecting the quota
+	// group. This is a temporary error. The error `value` is an
+	// object with optional fields `quota-name`, `change-kind` of the
+	// ongoing change.
+	ErrorKindQuotaChangeConflict ErrorKind = "quota-change-conflict"
+
	// ErrorKindNotSnap: the given snap or directory does not
	// look like a snap.
	ErrorKindNotSnap ErrorKind = "snap-not-a-snap"
diff --git a/client/quota.go b/client/quota.go
index 3e13061cfa..dfdcb12817 100644
--- a/client/quota.go
+++ b/client/quota.go
@@ -67,8 +67,7 @@ func (client *Client) EnsureQuota(groupName string, parent string, snaps []strin
	chgID, err := client.doAsync("POST", "/v2/quotas", nil, nil, &body)
 
	if err != nil {
-	fmt := "cannot create or update quota group: %w"
-	return "", xerrors.Errorf(fmt, err)
+	return "", err
	}
	return chgID, nil
 }
diff --git a/client/quota_test.go b/client/quota_test.go
index 17818dc00b..54fbae755b 100644
--- a/client/quota_test.go
+++ b/client/quota_test.go
@@ -64,7 +64,7 @@ func (cs *clientSuite) TestEnsureQuotaGroupError(c *check.C) {
	cs.status = 500
	cs.rsp = `{"type": "error"}`
	_, err := cs.cli.EnsureQuota("foo", "bar", []string{"snap-a"}, 1)
-	c.Check(err, check.ErrorMatches, `cannot create or update quota group: server error: "Internal Server Error"`)
+	c.Check(err, check.ErrorMatches, `server error: "Internal Server Error"`)
 }
 
 func (cs *clientSuite) TestGetQuotaGroupInvalidName(c *check.C) {
diff --git a/daemon/api_quotas.go b/daemon/api_quotas.go
index e55b6bfb38..84037d6c61 100644
--- a/daemon/api_quotas.go
+++ b/daemon/api_quotas.go
@@ -176,8 +176,7 @@ func postQuotaGroup(c *Command, r *http.Request, _ *auth.UserState) Response {
	// then we need to create the quota
	ts, err = servicestateCreateQuota(st, data.GroupName, data.Parent, data.Snaps, quantity.Size(data.MaxMemory))
	if err != nil {
-	// XXX: dedicated error type?
-	return BadRequest(err.Error())
+	return errToResponse(err, nil, BadRequest, "cannot create quota group: %v")
	}
	chgSummary = "Create quota group"
	} else if err == nil {
@@ -188,7 +187,7 @@ func postQuotaGroup(c *Command, r *http.Request, _ *auth.UserState) Response {
	}
	ts, err = servicestateUpdateQuota(st, data.GroupName, updateOpts)
	if err != nil {
-	return BadRequest(err.Error())
+	return errToResponse(err, nil, BadRequest, "cannot update quota group: %v")
	}
	chgSummary = "Update quota group"
	}
@@ -197,7 +196,7 @@ func postQuotaGroup(c *Command, r *http.Request, _ *auth.UserState) Response {
	var err error
	ts, err = servicestateRemoveQuota(st, data.GroupName)
	if err != nil {
-	return BadRequest(err.Error())
+	return errToResponse(err, nil, BadRequest, "cannot remove quota group: %v")
	}
	chgSummary = "Remove quota group"
	default:
diff --git a/daemon/api_quotas_test.go b/daemon/api_quotas_test.go
index 0f699667ce..b24bc92804 100644
--- a/daemon/api_quotas_test.go
+++ b/daemon/api_quotas_test.go
@@ -34,6 +34,7 @@ import (
	"github.com/snapcore/snapd/overlord/configstate/config"
	"github.com/snapcore/snapd/overlord/servicestate"
	"github.com/snapcore/snapd/overlord/servicestate/servicestatetest"
+	"github.com/snapcore/snapd/overlord/snapstate"
	"github.com/snapcore/snapd/overlord/state"
	"github.com/snapcore/snapd/snap/quota"
 )
@@ -124,7 +125,7 @@ func (s *apiQuotaSuite) TestPostEnsureQuotaUnhappy(c *check.C) {
	c.Assert(err, check.IsNil)
	rspe := s.errorReq(c, req, nil)
	c.Check(rspe.Status, check.Equals, 400)
-	c.Check(rspe.Message, check.Matches, `boom`)
+	c.Check(rspe.Message, check.Matches, `cannot create quota group: boom`)
	c.Assert(s.ensureSoonCalled, check.Equals, 0)
 }
 
@@ -158,6 +159,64 @@ func (s *apiQuotaSuite) TestPostEnsureQuotaCreateHappy(c *check.C) {
	c.Assert(s.ensureSoonCalled, check.Equals, 1)
 }
 
+func (s *apiQuotaSuite) TestPostEnsureQuotaCreateQuotaConflicts(c *check.C) {
+	var createCalled int
+	r := daemon.MockServicestateCreateQuota(func(st *state.State, name string, parentName string, snaps []string, memoryLimit quantity.Size) (*state.TaskSet, error) {
+	c.Check(name, check.Equals, "booze")
+	c.Check(parentName, check.Equals, "foo")
+	c.Check(snaps, check.DeepEquals, []string{"some-snap"})
+	c.Check(memoryLimit, check.DeepEquals, quantity.Size(1000))
+
+	createCalled++
+	switch createCalled {
+	case 1:
+	// return a quota conflict as if we were trying to create this quota in
+	// another task
+	return nil, &servicestate.QuotaChangeConflictError{Quota: "booze", ChangeKind: "quota-control"}
+	case 2:
+	// return a snap conflict as if we were trying to disable the
+	// some-snap in the quota group to be created
+	return nil, &snapstate.ChangeConflictError{Snap: "some-snap", ChangeKind: "disable"}
+	default:
+	c.Errorf("test broken")
+	return nil, fmt.Errorf("test broken")
+	}
+	})
+	defer r()
+
+	data, err := json.Marshal(daemon.PostQuotaGroupData{
+	Action: "ensure",
+	GroupName: "booze",
+	Parent: "foo",
+	Snaps: []string{"some-snap"},
+	MaxMemory: 1000,
+	})
+	c.Assert(err, check.IsNil)
+
+	req, err := http.NewRequest("POST", "/v2/quotas", bytes.NewBuffer(data))
+	c.Assert(err, check.IsNil)
+	rspe := s.errorReq(c, req, nil)
+	c.Assert(rspe.Status, check.Equals, 409)
+	c.Check(rspe.Message, check.Equals, `quota group "booze" has "quota-control" change in progress`)
+	c.Check(rspe.Value, check.DeepEquals, map[string]interface{}{
+	"change-kind": "quota-control",
+	"quota-name": "booze",
+	})
+
+	req, err = http.NewRequest("POST", "/v2/quotas", bytes.NewBuffer(data))
+	c.Assert(err, check.IsNil)
+
+	rspe = s.errorReq(c, req, nil)
+	c.Assert(rspe.Status, check.Equals, 409)
+	c.Check(rspe.Message, check.Equals, `snap "some-snap" has "disable" change in progress`)
+	c.Check(rspe.Value, check.DeepEquals, map[string]interface{}{
+	"change-kind": "disable",
+	"snap-name": "some-snap",
+	})
+
+	c.Assert(createCalled, check.Equals, 2)
+}
+
 func (s *apiQuotaSuite) TestPostEnsureQuotaUpdateHappy(c *check.C) {
	st := s.d.Overlord().State()
	st.Lock()
@@ -200,6 +259,75 @@ func (s *apiQuotaSuite) TestPostEnsureQuotaUpdateHappy(c *check.C) {
	c.Assert(s.ensureSoonCalled, check.Equals, 1)
 }
 
+func (s *apiQuotaSuite) TestPostEnsureQuotaUpdateConflicts(c *check.C) {
+	st := s.d.Overlord().State()
+	st.Lock()
+	err := servicestatetest.MockQuotaInState(st, "ginger-ale", "", nil, 5000)
+	st.Unlock()
+	c.Assert(err, check.IsNil)
+
+	r := daemon.MockServicestateCreateQuota(func(st *state.State, name string, parentName string, snaps []string, memoryLimit quantity.Size) (*state.TaskSet, error) {
+	c.Errorf("should not have called create quota")
+	return nil, fmt.Errorf("broken test")
+	})
+	defer r()
+
+	updateCalled := 0
+	r = daemon.MockServicestateUpdateQuota(func(st *state.State, name string, opts servicestate.QuotaGroupUpdate) (*state.TaskSet, error) {
+	updateCalled++
+	c.Assert(name, check.Equals, "ginger-ale")
+	c.Assert(opts, check.DeepEquals, servicestate.QuotaGroupUpdate{
+	AddSnaps: []string{"some-snap"},
+	NewMemoryLimit: 9000,
+	})
+	switch updateCalled {
+	case 1:
+	// return a quota conflict as if we were trying to update this quota
+	// in another task
+	return nil, &servicestate.QuotaChangeConflictError{Quota: "ginger-ale", ChangeKind: "quota-control"}
+	case 2:
+	// return a snap conflict as if we were trying to disable the
+	// some-snap in the quota group to be added to the group
+	return nil, &snapstate.ChangeConflictError{Snap: "some-snap", ChangeKind: "disable"}
+	default:
+	c.Errorf("test broken")
+	return nil, fmt.Errorf("test broken")
+	}
+	})
+	defer r()
+
+	data, err := json.Marshal(daemon.PostQuotaGroupData{
+	Action: "ensure",
+	GroupName: "ginger-ale",
+	Snaps: []string{"some-snap"},
+	MaxMemory: 9000,
+	})
+	c.Assert(err, check.IsNil)
+
+	req, err := http.NewRequest("POST", "/v2/quotas", bytes.NewBuffer(data))
+	c.Assert(err, check.IsNil)
+	rspe := s.errorReq(c, req, nil)
+	c.Assert(rspe.Status, check.Equals, 409)
+	c.Check(rspe.Message, check.Equals, `quota group "ginger-ale" has "quota-control" change in progress`)
+	c.Check(rspe.Value, check.DeepEquals, map[string]interface{}{
+	"change-kind": "quota-control",
+	"quota-name": "ginger-ale",
+	})
+
+	req, err = http.NewRequest("POST", "/v2/quotas", bytes.NewBuffer(data))
+	c.Assert(err, check.IsNil)
+
+	rspe = s.errorReq(c, req, nil)
+	c.Assert(rspe.Status, check.Equals, 409)
+	c.Check(rspe.Message, check.Equals, `snap "some-snap" has "disable" change in progress`)
+	c.Check(rspe.Value, check.DeepEquals, map[string]interface{}{
+	"change-kind": "disable",
+	"snap-name": "some-snap",
+	})
+
+	c.Assert(updateCalled, check.Equals, 2)
+}
+
 func (s *apiQuotaSuite) TestPostRemoveQuotaHappy(c *check.C) {
	var removeCalled int
	r := daemon.MockServicestateRemoveQuota(func(st *state.State, name string) (*state.TaskSet, error) {
@@ -227,6 +355,63 @@ func (s *apiQuotaSuite) TestPostRemoveQuotaHappy(c *check.C) {
	c.Assert(s.ensureSoonCalled, check.Equals, 1)
 }
 
+func (s *apiQuotaSuite) TestPostRemoveQuotaConflict(c *check.C) {
+	st := s.d.Overlord().State()
+	st.Lock()
+	err := servicestatetest.MockQuotaInState(st, "ginger-ale", "", []string{"some-snap"}, 5000)
+	st.Unlock()
+	c.Assert(err, check.IsNil)
+
+	var removeCalled int
+	r := daemon.MockServicestateRemoveQuota(func(st *state.State, name string) (*state.TaskSet, error) {
+	removeCalled++
+	c.Check(name, check.Equals, "booze")
+	switch removeCalled {
+	case 1:
+	// return a quota conflict as if we were trying to update this quota
+	// in another task
+	return nil, &servicestate.QuotaChangeConflictError{Quota: "booze", ChangeKind: "quota-control"}
+	case 2:
+	// return a snap conflict as if we were trying to disable the
+	// some-snap in the quota group to be added to the group
+	return nil, &snapstate.ChangeConflictError{Snap: "some-snap", ChangeKind: "disable"}
+	default:
+	c.Errorf("test broken")
+	return nil, fmt.Errorf("test broken")
+	}
+	})
+	defer r()
+
+	data, err := json.Marshal(daemon.PostQuotaGroupData{
+	Action: "remove",
+	GroupName: "booze",
+	})
+	c.Assert(err, check.IsNil)
+
+	req, err := http.NewRequest("POST", "/v2/quotas", bytes.NewBuffer(data))
+	c.Assert(err, check.IsNil)
+	rspe := s.errorReq(c, req, nil)
+	c.Assert(rspe.Status, check.Equals, 409)
+	c.Check(rspe.Message, check.Equals, `quota group "booze" has "quota-control" change in progress`)
+	c.Check(rspe.Value, check.DeepEquals, map[string]interface{}{
+	"change-kind": "quota-control",
+	"quota-name": "booze",
+	})
+
+	req, err = http.NewRequest("POST", "/v2/quotas", bytes.NewBuffer(data))
+	c.Assert(err, check.IsNil)
+
+	rspe = s.errorReq(c, req, nil)
+	c.Assert(rspe.Status, check.Equals, 409)
+	c.Check(rspe.Message, check.Equals, `snap "some-snap" has "disable" change in progress`)
+	c.Check(rspe.Value, check.DeepEquals, map[string]interface{}{
+	"change-kind": "disable",
+	"snap-name": "some-snap",
+	})
+
+	c.Assert(removeCalled, check.Equals, 2)
+}
+
 func (s *apiQuotaSuite) TestPostRemoveQuotaUnhappy(c *check.C) {
	r := daemon.MockServicestateRemoveQuota(func(st *state.State, name string) (*state.TaskSet, error) {
	c.Check(name, check.Equals, "booze")
@@ -244,7 +429,7 @@ func (s *apiQuotaSuite) TestPostRemoveQuotaUnhappy(c *check.C) {
	c.Assert(err, check.IsNil)
	rspe := s.errorReq(c, req, nil)
	c.Check(rspe.Status, check.Equals, 400)
-	c.Check(rspe.Message, check.Matches, `boom`)
+	c.Check(rspe.Message, check.Matches, `cannot remove quota group: boom`)
	c.Check(s.ensureSoonCalled, check.Equals, 0)
 }
 
diff --git a/daemon/errors.go b/daemon/errors.go
index 2a0ecf2713..3a5c52c8bb 100644
--- a/daemon/errors.go
+++ b/daemon/errors.go
@@ -26,6 +26,7 @@ import (
 
	"github.com/snapcore/snapd/arch"
	"github.com/snapcore/snapd/client"
+	"github.com/snapcore/snapd/overlord/servicestate"
	"github.com/snapcore/snapd/overlord/snapstate"
	"github.com/snapcore/snapd/snap"
	"github.com/snapcore/snapd/store"
@@ -187,8 +188,8 @@ func SnapRevisionNotAvailable(snapName string, rnaErr *store.RevisionNotAvailabl
	}
 }
 
-// SnapChangeConflict is an error responder used when an operation is
-// conflicts with another change.
+// SnapChangeConflict is an error responder used when an operation would
+// conflict with another ongoing change.
 func SnapChangeConflict(cce *snapstate.ChangeConflictError) *apiError {
	value := map[string]interface{}{}
	if cce.Snap != "" {
@@ -206,6 +207,25 @@ func SnapChangeConflict(cce *snapstate.ChangeConflictError) *apiError {
	}
 }
 
+// QuotaChangeConflict is an error responder used when an operation would
+// conflict with another ongoing change.
+func QuotaChangeConflict(qce *servicestate.QuotaChangeConflictError) *apiError {
+	value := map[string]interface{}{}
+	if qce.Quota != "" {
+	value["quota-name"] = qce.Quota
+	}
+	if qce.ChangeKind != "" {
+	value["change-kind"] = qce.ChangeKind
+	}
+
+	return &apiError{
+	Status: 409,
+	Message: qce.Error(),
+	Kind: client.ErrorKindQuotaChangeConflict,
+	Value: value,
+	}
+}
+
 // InsufficientSpace is an error responder used when an operation cannot
 // be performed due to low disk space.
 func InsufficientSpace(dserr *snapstate.InsufficientSpaceError) *apiError {
@@ -296,6 +316,8 @@ func errToResponse(err error, snaps []string, fallback errorResponder, format st
	snapName = err.Snap
	case *snapstate.ChangeConflictError:
	return SnapChangeConflict(err)
+	case *servicestate.QuotaChangeConflictError:
+	return QuotaChangeConflict(err)
	case *snapstate.SnapNeedsDevModeError:
	kind = client.ErrorKindSnapNeedsDevMode
	snapName = err.Snap
diff --git a/tests/main/snap-quota/task.yaml b/tests/main/snap-quota/task.yaml
index 792c251e3d..f3d567f379 100644
--- a/tests/main/snap-quota/task.yaml
+++ b/tests/main/snap-quota/task.yaml
@@ -14,7 +14,7 @@ execute: |
 # old systemd versions, we need at least 230 to avoid buggy slice usage
 # reporting
 
- snap set-quota foobar --memory=1MB 2>&1 | MATCH "systemd version too old: snap quotas requires systemd 230 and newer \(currently have 2[0-2][0-9]\)"
+ snap set-quota foobar --memory=1MB 2>&1 | tr '\n' ' ' | tr -s ' ' | MATCH "systemd version too old: snap quotas requires systemd 230 and newer \(currently have 2[0-2][0-9]\)"
 exit 0
 fi
 
@@ -31,7 +31,7 @@ execute: |
 snap set-quota group-sub-sub-one --parent=group-sub-one --memory=5KB
 
 echo "Trying to add snap to more than one group fails"
- snap set-quota group-bad --memory=1MB hello-world 2>&1 | MATCH 'error: cannot create or update quota group: cannot add snap "hello-world" to group "group-bad": snap already in quota group "group-one"'
+ snap set-quota group-bad --memory=1MB hello-world 2>&1 | tr '\n' ' ' | tr -s ' ' | MATCH 'error: cannot create quota group: cannot add snap "hello-world" to group "group-bad": snap already in quota group "group-one"'
 
 echo "Adding a snap to group-one"
 snap set-quota group-one go-example-webserver