amadan

carlos / platform Public

instancectl: skip a cold start's set-property when the slice already has the tier

startGen's drop-in check is now one show of the unit and its host slice.
When the slice already carries every value setTier would give it (it
keeps them from the last generation: set-property --runtime lasts until a
reboot), the set-property is skipped: one systemctl call fewer a wake. Any
value off, a slice missing from the answer, or a CPU quota format this
does not recognise sets the tier as before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude d418a5a48ecb2367b08fbbfcf12b6467ef397392 parent 9c268ff
2 files changed, +102 −21
  • internal/instancectl/gen.go +45 −18
  • internal/instancectl/server_gen_test.go +57 −3
diff --git a/internal/instancectl/gen.go b/internal/instancectl/gen.go
index 54b308362..154da8771 100644
--- a/internal/instancectl/gen.go
+++ b/internal/instancectl/gen.go
@@ -113,24 +113,51 @@ func (s *Server) slicePopulated(host string) bool {
return !strings.Contains(string(b), "populated 0")
}
-// genDropInApplied reports whether systemd's view of unit is the drop-in
-// just written (GenDropIn). The unit's name fixes the host and generation,
-// and so every line of the drop-in but the tier's: the slice says systemd
-// read a drop-in at all, and the memory lines say it was this tier's.
-// NeedDaemonReload=no says no file changed since systemd loaded the unit,
-// though only by mtime, which is why the tier is checked as well (Astra
-// review: a reused, still-loaded generation and a clock that went back).
-// Any failure to answer is a no.
-func (s *Server) genDropInApplied(ctx context.Context, host, unit string, tier cgroup.Limit) (slice string, ok bool) {
- out, err := s.Systemctl(ctx, "show", "-p", "Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax", unit)
+// genView is startGen's one look at systemd, a single show of the unit and
+// its host slice. applied reports whether systemd's view of unit is the
+// drop-in just written (GenDropIn). The unit's name fixes the host and
+// generation, and so every line of the drop-in but the tier's: the slice
+// says systemd read a drop-in at all, and the memory lines say it was this
+// tier's. NeedDaemonReload=no says no file changed since systemd loaded
+// the unit, though only by mtime, which is why the tier is checked as well
+// (Astra review: a reused, still-loaded generation and a clock that went
+// back). tierSet reports whether the slice already carries every value
+// setTier would give it, so a cold start can skip that call (wake latency,
+// pass 2c). Any failure to answer is a no to both.
+func (s *Server) genView(ctx context.Context, host, unit string, tier cgroup.Limit) (slice string, applied, tierSet bool) {
+ out, err := s.Systemctl(ctx, "show", "-p", "Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax,CPUWeight,CPUQuotaPerSecUSec,IOWeight", unit, SliceName(host))
if err != nil {
- return "", false
+ return "", false, false
}
- m := parseShow(out)
+ blocks := strings.SplitN(strings.TrimLeft(out, "\n"), "\n\n", 2)
+ m := parseShow(blocks[0])
max := strconv.FormatInt(tier.MemoryMax, 10)
- return m["Slice"], m["Slice"] == SliceName(host) && m["NeedDaemonReload"] == "no" &&
- m["MemoryMax"] == max && m["MemorySwapMax"] == max &&
- m["MemoryHigh"] == strconv.FormatInt(cgroup.HighOf(tier.MemoryMax), 10)
+ high := strconv.FormatInt(cgroup.HighOf(tier.MemoryMax), 10)
+ applied = m["Slice"] == SliceName(host) && m["NeedDaemonReload"] == "no" &&
+ m["MemoryMax"] == max && m["MemorySwapMax"] == max && m["MemoryHigh"] == high
+ if len(blocks) == 2 {
+ sl := parseShow(blocks[1])
+ sh := cgroup.SharesOf(tier)
+ tierSet = sl["MemoryMax"] == max && sl["MemorySwapMax"] == max && sl["MemoryHigh"] == high &&
+ sl["CPUWeight"] == strconv.Itoa(sh.CPUWeight) && sl["IOWeight"] == strconv.Itoa(sh.IOWeight) &&
+ sl["CPUQuotaPerSecUSec"] == cpuQuotaShown(sh.CPUQuotaPercent)
+ }
+ return m["Slice"], applied, tierSet
+}
+
+// cpuQuotaShown is CPUQuota=<pct>% as systemctl show prints its
+// CPUQuotaPerSecUSec (systemd 257): 500ms, 1s, 1.500000s. A format this
+// gets wrong only costs the set-property call it would have saved.
+func cpuQuotaShown(pct int) string {
+ usec := int64(pct) * 10000
+ switch {
+ case usec%1_000_000 == 0:
+ return strconv.FormatInt(usec/1_000_000, 10) + "s"
+ case usec < 1_000_000:
+ return strconv.FormatInt(usec/1000, 10) + "ms"
+ default:
+ return strconv.FormatFloat(float64(usec)/1e6, 'f', 6, 64) + "s"
+ }
}
// startGen is /start with a generation. The host slice is the parent (spec
@@ -180,20 +207,20 @@ func (s *Server) startGen(w http.ResponseWriter, r *http.Request, req request) {
// straight away, so the next reference reads the drop-in fresh: the
// common case needs no reload, which on a small box was a third of the
// whole wake. The check is the same either way (genDropInApplied).
- slice, applied := s.genDropInApplied(ctx, req.host, unit, req.tier)
+ slice, applied, tierSet := s.genView(ctx, req.host, unit, req.tier)
if !applied {
reloadStart := time.Now()
if out, err := s.Systemctl(ctx, "daemon-reload"); err != nil {
fail(http.StatusBadGateway, "daemon-reload: "+strings.TrimSpace(out+" "+err.Error()))
return
}
- if slice, applied = s.genDropInApplied(ctx, req.host, unit, req.tier); !applied {
+ if slice, applied, tierSet = s.genView(ctx, req.host, unit, req.tier); !applied {
fail(http.StatusBadGateway, "not starting "+unit+": its drop-in did not apply (slice "+strconv.Quote(slice)+")")
return
}
reload = time.Since(reloadStart).Round(time.Millisecond).String()
}
- if !s.slicePopulated(req.host) {
+ if !s.slicePopulated(req.host) && !tierSet {
if err := s.setTier(ctx, req.host, req.tier); err != nil {
fail(http.StatusBadGateway, "set-property: "+err.Error())
return
diff --git a/internal/instancectl/server_gen_test.go b/internal/instancectl/server_gen_test.go
index 6f2f61937..577b5baf1 100644
--- a/internal/instancectl/server_gen_test.go
+++ b/internal/instancectl/server_gen_test.go
@@ -19,6 +19,14 @@ const genHost = "bill-2.example.com"
var genUnit = GenUnitName(genHost, 7)
+// genShow is startGen's one look at systemd: the unit's drop-in lines and
+// its host slice's tier, in one call.
+var genShow = "show -p Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax,CPUWeight,CPUQuotaPerSecUSec,IOWeight " + genUnit + " " + SliceName(genHost)
+
+// genSliceAtTier is that call's second block for a slice already at the
+// default tier.
+const genSliceAtTier = "\nSlice=carlos.slice\nNeedDaemonReload=no\nMemoryMax=536870912\nMemoryHigh=483183819\nMemorySwapMax=536870912\nCPUWeight=100\nCPUQuotaPerSecUSec=1s\nIOWeight=100\n"
+
// genTierShow is systemd's answer for the default tier's memory lines.
const genTierShow = "MemoryMax=536870912\nMemoryHigh=483183819\nMemorySwapMax=536870912\n"
@@ -74,7 +82,7 @@ func TestGenStartWritesTheDropInChecksItThenStartsWithoutAReload(t *testing.T) {
t.Fatalf("exe app link = %q, %v", target, err)
}
want := strings.Join([]string{
- "show -p Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax " + genUnit,
+ genShow,
`set-property --runtime carlos-instances-bill\x2d2.example.com.slice MemoryMax=536870912 MemoryHigh=483183819 MemorySwapMax=536870912 CPUWeight=100 CPUQuota=100% IOWeight=100`,
"start " + genUnit,
"show -p InvocationID --value " + genUnit,
@@ -112,9 +120,9 @@ func TestGenStartReloadsWhenSystemdHoldsAStaleCopy(t *testing.T) {
t.Fatal(err)
}
want := strings.Join([]string{
- "show -p Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax " + genUnit,
+ genShow,
"daemon-reload",
- "show -p Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax " + genUnit,
+ genShow,
}, "\n")
if got := ctl.joined(); !strings.HasPrefix(got, want+"\n") || !strings.Contains(got, "\nstart "+genUnit) {
t.Errorf("calls =\n%s\nwant them to begin\n%s\nand then start", got, want)
@@ -777,3 +785,49 @@ func TestAbandonedGenStartResetsFailedOnlyAfterAProvenKill(t *testing.T) {
t.Errorf("reset-failed after a failed kill:\n%s", ctl.joined())
}
}
+
+// A cold start whose slice already carries the tier (it kept it from the
+// last generation: set-property --runtime lasts until a reboot) skips the
+// set-property: one systemctl call fewer on a wake. The same show read it.
+func TestGenStartSkipsTheTierWhenTheSliceAlreadyHasIt(t *testing.T) {
+ ctl := genCtl()
+ ctl.answers["show -p Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax"] = "Slice=" + SliceName(genHost) + "\nNeedDaemonReload=no\n" + genTierShow + genSliceAtTier
+ _, sock, _ := genServer(t, ctl)
+ if _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax); err != nil {
+ t.Fatal(err)
+ }
+ if got := ctl.joined(); strings.Contains(got, "set-property") {
+ t.Errorf("the slice already had the tier but was set again:\n%s", got)
+ }
+}
+
+// Any one value off, or the slice missing from the answer, and the tier is
+// set as before.
+func TestGenStartSetsTheTierWhenTheSliceDiffers(t *testing.T) {
+ for name, block := range map[string]string{
+ "memory": strings.Replace(genSliceAtTier, "MemoryMax=536870912", "MemoryMax=1073741824", 1),
+ "cpu quota": strings.Replace(genSliceAtTier, "CPUQuotaPerSecUSec=1s", "CPUQuotaPerSecUSec=2s", 1),
+ "io weight": strings.Replace(genSliceAtTier, "IOWeight=100", "IOWeight=[not set]", 1),
+ "no slice block": "",
+ } {
+ t.Run(name, func(t *testing.T) {
+ ctl := genCtl()
+ ctl.answers["show -p Slice,NeedDaemonReload,MemoryMax,MemoryHigh,MemorySwapMax"] = "Slice=" + SliceName(genHost) + "\nNeedDaemonReload=no\n" + genTierShow + block
+ _, sock, _ := genServer(t, ctl)
+ if _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax); err != nil {
+ t.Fatal(err)
+ }
+ if got := ctl.joined(); !strings.Contains(got, "set-property --runtime "+SliceName(genHost)) {
+ t.Errorf("the slice differs (%s) but the tier was not set:\n%s", name, got)
+ }
+ })
+ }
+}
+
+func TestCPUQuotaShowsAsSystemdShowsIt(t *testing.T) {
+ for pct, want := range map[int]string{50: "500ms", 100: "1s", 150: "1.500000s", 200: "2s", 250: "2.500000s"} {
+ if got := cpuQuotaShown(pct); got != want {
+ t.Errorf("cpuQuotaShown(%d) = %q, want %q (systemd 257)", pct, got, want)
+ }
+ }
+}