multiple: review feedback

Always write CPUAccounting, improve usage of fmt and bytes.Buffer, improve docs, fix typos

snap/quota/quota.go

@@ -296,6 +296,8 @@ func (grp *Group) getQuotaAllocations(allQuotas map[string]*groupQuotaAllocation
 			continue
 		}
 
+		// cyclic checks are made by visitTree so we make the assumption here
+		// that no cyclic dependencies exists.
 		subGroupLimits := subGroup.getQuotaAllocations(allQuotas, skipGrp)
 
 		// As we count up the usage of quotas across our sub-groups we must either use the actual

snap/quota/quota_test.go

@@ -781,7 +781,7 @@ func (ts *quotaTestSuite) TestCurrentTaskUsage(c *C) {
 			c.Assert(args, DeepEquals, []string{"is-active", "snap.group.slice"})
 			return []byte("active"), nil
 		case 3:
-			// and the memory count can be non-zero like
+			// and the task count can be non-zero like
 			c.Assert(args, DeepEquals, []string{"show", "--property", "TasksCurrent", "snap.group.slice"})
 			return []byte("TasksCurrent=32"), nil
 
@@ -795,7 +795,7 @@ func (ts *quotaTestSuite) TestCurrentTaskUsage(c *C) {
 			return []byte("TasksCurrent=0"), nil
 
 		default:
-			c.Errorf("too many systemctl calls (%d) (current call is %+v)", systemctlCalls, args)
+			c.Errorf("unexpected number of systemctl calls (%d) (current call is %+v)", systemctlCalls, args)
 			return []byte("broken test"), fmt.Errorf("broken test")
 		}
 	})
@@ -804,20 +804,23 @@ func (ts *quotaTestSuite) TestCurrentTaskUsage(c *C) {
 	grp1, err := quota.NewGroup("group", quota.NewResourcesBuilder().WithThreadLimit(32).Build())
 	c.Assert(err, IsNil)
 
-	// group initially is inactive, so it has no current memory usage
+	// group initially is inactive, so it has no current task usage
 	currentTasks, err := grp1.CurrentTaskUsage()
 	c.Check(err, IsNil)
 	c.Check(currentTasks, Equals, 0)
+	c.Check(systemctlCalls, Equals, 1)
 
 	// now with the slice mocked as active it has real usage
 	currentTasks, err = grp1.CurrentTaskUsage()
 	c.Check(err, IsNil)
 	c.Check(currentTasks, Equals, 32)
+	c.Check(systemctlCalls, Equals, 3)
 
 	// but it can also have 0 usage
 	currentTasks, err = grp1.CurrentTaskUsage()
 	c.Check(err, IsNil)
 	c.Check(currentTasks, Equals, 0)
+	c.Check(systemctlCalls, Equals, 5)
 }
 
 func (ts *quotaTestSuite) TestGetGroupQuotaAllocations(c *C) {

strutil/strutil.go

@@ -39,7 +39,7 @@ func SizeToStr(size int64) string {
 	panic("SizeToStr got a size bigger than math.MaxInt64")
 }
 
-// IntsToCommaSeparated converts an int array to a comma-separated string
+// IntsToCommaSeparated converts an int array to a comma-separated string without whitespace
 func IntsToCommaSeparated(vals []int) string {
 	b := &strings.Builder{}
 	last := len(vals) - 1

wrappers/services.go

@@ -93,70 +93,55 @@ func min(a, b int) int {
 }
 
 func formatCpuGroupSlice(grp *quota.Group) string {
-	if grp.CPULimit == nil {
-		return ""
-	}
-
-	// calculateSystemdCPULimit calculates the number of cpus and the allowed percentage
-	// to the systemd specific format. The CPUQuota setting is only available since systemd 213
-	calculateSystemdCPULimit := func() string {
-		if grp.CPULimit.Count == 0 && grp.CPULimit.Percentage == 0 {
-			return ""
-		}
-
-		cpuQuotaSnap := max(grp.CPULimit.Count, 1) * grp.CPULimit.Percentage
-		cpuQuotaMax := runtime.NumCPU() * 100
-		return fmt.Sprintf("CPUQuota=%d%%\n", min(cpuQuotaSnap, cpuQuotaMax))
-	}
-
-	// getAllowedCpusValue converts allowed CPUs array to a comma-delimited
-	// string that systemd expects it to be in. If the array is empty then
-	// an empty string is returned
-	getAllowedCpusValue := func() string {
-		if len(grp.CPULimit.AllowedCPUs) == 0 {
-			return ""
-		}
-
-		allowedCpusValue := strutil.IntsToCommaSeparated(grp.CPULimit.AllowedCPUs)
-		return fmt.Sprintf("AllowedCPUs=%s\n", allowedCpusValue)
-	}
-
 	header := `# Always enable cpu accounting, so the following cpu quota options have an effect
 CPUAccounting=true
 `
-	return fmt.Sprint(header, calculateSystemdCPULimit(), getAllowedCpusValue(), "\n")
+	buf := bytes.NewBufferString(header)
+
+	if grp.CPULimit != nil && grp.CPULimit.Percentage != 0 {
+		// convert the number of cores and the allowed percentage
+		// to the systemd specific format.
+		cpuQuotaSnap := max(grp.CPULimit.Count, 1) * grp.CPULimit.Percentage
+		cpuQuotaMax := runtime.NumCPU() * 100
+
+		// The CPUQuota setting is only available since systemd 213
+		fmt.Fprintf(buf, "CPUQuota=%d%%\n", min(cpuQuotaSnap, cpuQuotaMax))
+	}
+
+	if grp.CPULimit != nil && len(grp.CPULimit.AllowedCPUs) != 0 {
+		allowedCpusValue := strutil.IntsToCommaSeparated(grp.CPULimit.AllowedCPUs)
+		fmt.Fprintf(buf, "AllowedCPUs=%s\n", allowedCpusValue)
+	}
+
+	buf.WriteString("\n")
+	return buf.String()
 }
 
 func formatMemoryGroupSlice(grp *quota.Group) string {
-	buf := bytes.Buffer{}
 	header := `# Always enable memory accounting otherwise the MemoryMax setting does nothing.
 MemoryAccounting=true
 `
-	fmt.Fprint(&buf, header)
-
+	buf := bytes.NewBufferString(header)
 	if grp.MemoryLimit != 0 {
 		valuesTemplate := `MemoryMax=%[1]d
 # for compatibility with older versions of systemd
 MemoryLimit=%[1]d
 
 `
-		memoryValues := fmt.Sprintf(valuesTemplate, grp.MemoryLimit)
-		fmt.Fprint(&buf, memoryValues)
+		fmt.Fprintf(buf, valuesTemplate, grp.MemoryLimit)
 	}
 	return buf.String()
 }
 
 func formatTaskGroupSlice(grp *quota.Group) string {
-	buf := bytes.Buffer{}
 	header := `# Always enable task accounting in order to be able to count the processes/
 # threads, etc for a slice
 TasksAccounting=true
 `
-	fmt.Fprint(&buf, header)
+	buf := bytes.NewBufferString(header)
 
 	if grp.TaskLimit != 0 {
-		taskValue := fmt.Sprintf("TasksMax=%d\n", grp.TaskLimit)
-		fmt.Fprint(&buf, taskValue)
+		fmt.Fprintf(buf, "TasksMax=%d\n", grp.TaskLimit)
 	}
 	return buf.String()
 }

wrappers/services_test.go

@@ -388,6 +388,9 @@ Before=slices.target
 X-Snappy=yes
 
 [Slice]
+# Always enable cpu accounting, so the following cpu quota options have an effect
+CPUAccounting=true
+
 # Always enable memory accounting otherwise the MemoryMax setting does nothing.
 MemoryAccounting=true
 MemoryMax=%[2]s
@@ -486,6 +489,9 @@ Before=slices.target
 X-Snappy=yes
 
 [Slice]
+# Always enable cpu accounting, so the following cpu quota options have an effect
+CPUAccounting=true
+
 # Always enable memory accounting otherwise the MemoryMax setting does nothing.
 MemoryAccounting=true
 MemoryMax=%[2]s
@@ -582,6 +588,9 @@ Before=slices.target
 X-Snappy=yes
 
 [Slice]
+# Always enable cpu accounting, so the following cpu quota options have an effect
+CPUAccounting=true
+
 # Always enable memory accounting otherwise the MemoryMax setting does nothing.
 MemoryAccounting=true
 MemoryMax=1024