diff --git a/CHANGELOG.md b/CHANGELOG.md index e24780b..4763f63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,17 @@ useful than saying where the real one is. ## [Unreleased] +### Fixed + +- **`wsl.auto-memory-reclaim` now takes effect** ([#550](https://github.com/wslkit/skrog/issues/550)). + `skrog wsl-config apply` wrote `autoMemoryReclaim` under `[wsl2]`, but WSL reads it only + as `experimental.autoMemoryReclaim`: it ignored the line and printed `Unknown key + 'wsl2.autoMemoryReclaim'` when the VM started, so the setting never worked. It is now + written under `[experimental]`, and a line an earlier release left under `[wsl2]` is + moved by the next `apply` — the diff shows the move before anything is written. `skrog + wsl-config` and `skrog top` no longer report the ignored line as in effect. + `wsl-config --json` pending changes gain `section`, and `moved_from` for a move. + ### Added - **An engine per project** ([#539](https://github.com/wslkit/skrog/issues/539)). diff --git a/cmd/skrog/jsonshapes.go b/cmd/skrog/jsonshapes.go index c6227b8..98376d4 100644 --- a/cmd/skrog/jsonshapes.go +++ b/cmd/skrog/jsonshapes.go @@ -342,6 +342,10 @@ type wslConfigChangeJSON struct { Old string `json:"old,omitempty"` New string `json:"new"` Added bool `json:"added"` + // Section is where the key is written: wsl2, or experimental (#550). + Section string `json:"section"` + // MovedFrom names the section a dead copy is removed from; absent otherwise. + MovedFrom string `json:"moved_from,omitempty"` } // statsJSON is the `stats` object `skrog status --stats --json` adds (#179). diff --git a/cmd/skrog/jsonshapes_test.go b/cmd/skrog/jsonshapes_test.go index b2b9fc9..3a1bdbb 100644 --- a/cmd/skrog/jsonshapes_test.go +++ b/cmd/skrog/jsonshapes_test.go @@ -269,7 +269,7 @@ func TestWSLConfigShape(t *testing.T) { t.Error("pending should be omitted when there is nothing to do") } c := roundTrip(t, wslConfigChangeJSON{Key: "memory", New: "4GB", Added: true}) - requireKeys(t, c, "key", "new", "added") + requireKeys(t, c, "key", "new", "added", "section") if _, ok := c["old"]; ok { t.Error("old should be omitted when the key is being added") } diff --git a/cmd/skrog/top.go b/cmd/skrog/top.go index ae13000..2ed6555 100644 --- a/cmd/skrog/top.go +++ b/cmd/skrog/top.go @@ -309,8 +309,9 @@ func renderTop(w io.Writer, s vmtop.Snapshot, reclaim string) { } if advised { if reclaim == "" { - fmt.Fprintln(w, "~/.wslconfig does not set autoMemoryReclaim; `skrog config set wsl.auto-memory-reclaim gradual`\n"+ - "asks WSL to hand idle memory back. See docs/vm-sizing.md.") + fmt.Fprintln(w, "~/.wslconfig does not set [experimental] autoMemoryReclaim; `skrog config set\n"+ + "wsl.auto-memory-reclaim gradual` then `skrog wsl-config apply` asks WSL to hand idle\n"+ + "memory back. See docs/vm-sizing.md.") } else { fmt.Fprintf(w, "~/.wslconfig sets autoMemoryReclaim=%s. See docs/vm-sizing.md.\n", reclaim) } diff --git a/cmd/skrog/wslconfigcmd.go b/cmd/skrog/wslconfigcmd.go index 85cb74f..7ec6046 100644 --- a/cmd/skrog/wslconfigcmd.go +++ b/cmd/skrog/wslconfigcmd.go @@ -105,6 +105,7 @@ func runWSLConfigShow(args []string) int { for _, c := range plan { out.Pending = append(out.Pending, wslConfigChangeJSON{ Key: c.Key, Old: c.Old, New: c.New, Added: c.Added, + Section: wslconfig.SectionOf(c.Key), MovedFrom: c.StaleSection, }) } return emitJSON(out) @@ -198,6 +199,7 @@ func runWSLConfigApply(args []string) int { for _, c := range plan { out.Pending = append(out.Pending, wslConfigChangeJSON{ Key: c.Key, Old: c.Old, New: c.New, Added: c.Added, + Section: wslconfig.SectionOf(c.Key), MovedFrom: c.StaleSection, }) } return emitJSON(out) diff --git a/docs/cli-json.md b/docs/cli-json.md index 08bb0ef..5c13f57 100644 --- a/docs/cli-json.md +++ b/docs/cli-json.md @@ -600,11 +600,17 @@ The WSL2 VM''s sizing, from the global `~/.wslconfig` (#148): "exists": true, "effective": { "memory": "4GB", "processors": "2", "autoMemoryReclaim": "gradual" }, "desired": { "memory": "4GB", "processors": "2", "autoMemoryReclaim": "gradual" }, - "pending": [ { "key": "memory", "old": "8GB", "new": "4GB", "added": false } ], + "pending": [ { "key": "memory", "old": "8GB", "new": "4GB", "added": false, "section": "wsl2" } ], "applied": false } ``` +- `effective` reads each key from the section WSL reads it from: + `autoMemoryReclaim` from `[experimental]`, the rest from `[wsl2]` (#550). +- Each `pending` change names its `section`. `moved_from` is present when a + copy WSL ignores is being removed from another section — an + `autoMemoryReclaim` an earlier release wrote under `[wsl2]`. + - `effective` is what the file says now; `desired` is what Skrog's own settings ask for; `pending` is the difference — so a converge script can tell "already right" from "would change something" without parsing prose. diff --git a/docs/memory.md b/docs/memory.md index 275707a..e2679dc 100644 --- a/docs/memory.md +++ b/docs/memory.md @@ -171,8 +171,9 @@ held on the host: ``` Windows holds 288.3 MiB more for the VM than the VM is using: memory freed inside the VM that has not been handed back yet. -~/.wslconfig does not set autoMemoryReclaim; `skrog config set wsl.auto-memory-reclaim gradual` -asks WSL to hand idle memory back. See docs/vm-sizing.md. +~/.wslconfig does not set [experimental] autoMemoryReclaim; `skrog config set +wsl.auto-memory-reclaim gradual` then `skrog wsl-config apply` asks WSL to hand idle +memory back. See docs/vm-sizing.md. ``` The two figures are different measures (Windows' private working set, the @@ -190,8 +191,9 @@ holds — `top` says so: ``` 3.0 GiB of the VM's memory is page cache: file data Linux will drop when it needs the room, but which Windows counts as Vmmem until it is reclaimed. -~/.wslconfig does not set autoMemoryReclaim; `skrog config set wsl.auto-memory-reclaim gradual` -asks WSL to hand idle memory back. See docs/vm-sizing.md. +~/.wslconfig does not set [experimental] autoMemoryReclaim; `skrog config set +wsl.auto-memory-reclaim gradual` then `skrog wsl-config apply` asks WSL to hand idle +memory back. See docs/vm-sizing.md. ``` That cache is not a leak and nothing needs killing: Linux gives it up the diff --git a/docs/vm-sizing.md b/docs/vm-sizing.md index 8dd4fec..eb786a4 100644 --- a/docs/vm-sizing.md +++ b/docs/vm-sizing.md @@ -21,9 +21,21 @@ skrog wsl-config apply # shows the diff, asks, then writes | `wsl.memory` | `memory` | cap on the VM's RAM (`4GB`, `512MB`) | | `wsl.processors` | `processors` | how many CPUs the VM sees | | `wsl.swap` | `swap` | swap file size; `0` disables it | -| `wsl.auto-memory-reclaim` | `autoMemoryReclaim` | `gradual`, `dropcache` or `disabled` — hands idle memory back to Windows | +| `wsl.auto-memory-reclaim` | `autoMemoryReclaim`, under **`[experimental]`** | `gradual`, `dropcache` or `disabled` — hands idle memory back to Windows | | `wsl.virtiofs` | `virtiofs` | `true` mounts Windows drives over virtiofs instead of 9p — see below | +Every key goes under `[wsl2]` except `autoMemoryReclaim`, which WSL reads only +as `experimental.autoMemoryReclaim`. Under `[wsl2]` it is ignored, and WSL +prints `Unknown key 'wsl2.autoMemoryReclaim'` the next time the VM starts — +which is where Skrog wrote it before +[#550](https://github.com/wslkit/skrog/issues/550). If an earlier release left +it there, `skrog wsl-config` shows the move and `apply` makes it: + +``` + - autoMemoryReclaim=gradual (under [wsl2], where WSL ignores it) + + autoMemoryReclaim=gradual (under [experimental]) +``` + Values are validated when you set them, in the way WSL reads them, so a typo fails at `skrog config set` rather than silently sizing the VM as something else. `memory=4` in `.wslconfig` means *four bytes*; `skrog config set @@ -108,7 +120,7 @@ C:\Users\me\.wslconfig is shared by every WSL2 distro on this machine. Skrog would change: + memory=4GB + processors=2 - + autoMemoryReclaim=gradual + + autoMemoryReclaim=gradual (under [experimental]) apply these changes? [y/N] ``` diff --git a/internal/wslconfig/wslconfig.go b/internal/wslconfig/wslconfig.go index b2468c9..dbed0e9 100644 --- a/internal/wslconfig/wslconfig.go +++ b/internal/wslconfig/wslconfig.go @@ -29,9 +29,35 @@ import ( "strings" ) -// Section is the only section this package writes into. +// Section is where a managed key lives unless SectionOf says otherwise. const Section = "wsl2" +// SectionExperimental holds the keys WSL has not promoted to [wsl2]. +const SectionExperimental = "experimental" + +// SectionOf is the section WSL reads a managed key from. +// +// autoMemoryReclaim is `experimental.autoMemoryReclaim` in WSL's own config +// table, with no `wsl2.` alias (networkingMode has both). Written under [wsl2] +// it is an "Unknown key" WSL ignores, which is what every Skrog release before +// #550 did. +func SectionOf(key string) string { + if strings.EqualFold(key, KeyAutoMemoryReclaim) { + return SectionExperimental + } + return Section +} + +// staleSection is where an earlier Skrog wrote a key WSL does not read there, +// or "" when the key was always written to the right place. Plan moves such a +// line rather than leaving a dead one behind. +func staleSection(key string) string { + if strings.EqualFold(key, KeyAutoMemoryReclaim) { + return Section + } + return "" +} + // Keys are the settings Skrog manages, in the order a diff shows them. const ( KeyMemory = "memory" @@ -99,7 +125,9 @@ func Load(path string) (*File, error) { // Existed reports whether the file was there before. func (f *File) Existed() bool { return f.existed } -// Get returns a key's current value in [wsl2] and whether it was set. +// Get returns a key's current value in the section WSL reads it from, and +// whether it was set there. A copy in any other section is not reported: WSL +// ignores it, so it is not the effective value. func (f *File) Get(key string) (string, bool) { i := f.find(key) if i < 0 { @@ -121,14 +149,19 @@ func (f *File) All() map[string]string { return out } -// find locates a key inside the [wsl2] section, or -1. Comparison is -// case-insensitive on both the section and the key, because WSL is. +// find locates a key inside the section WSL reads it from, or -1. func (f *File) find(key string) int { + return f.findIn(SectionOf(key), key) +} + +// findIn locates a key inside the named section, or -1. Comparison is +// case-insensitive on both the section and the key, because WSL is. +func (f *File) findIn(section, key string) int { inSection := false for i, line := range f.lines { t := strings.TrimSpace(line) if strings.HasPrefix(t, "[") && strings.HasSuffix(t, "]") { - inSection = strings.EqualFold(strings.Trim(t, "[]"), Section) + inSection = strings.EqualFold(strings.Trim(t, "[]"), section) continue } if !inSection || t == "" || strings.HasPrefix(t, "#") || strings.HasPrefix(t, ";") { @@ -157,14 +190,35 @@ type Change struct { New string // Added is true when the key is not in the file yet. Added bool + // StaleSection names the section a dead copy of the key is removed from + // (an earlier Skrog wrote it where WSL does not read it); empty otherwise. + StaleSection string + // Stale is the value on that dead line. + Stale string } // String renders a change as a diff line, which is what consent is given to. +// A key outside [wsl2] names its section, and a moved line says why, because +// "- x / + x" with the same value would otherwise read as a no-op. func (c Change) String() string { + where := "" + if s := SectionOf(c.Key); s != Section { + where = " (under [" + s + "])" + } + var b strings.Builder + if c.StaleSection != "" { + fmt.Fprintf(&b, "- %s=%s (under [%s], where WSL ignores it)\n", c.Key, c.Stale, c.StaleSection) + } if c.Added { - return fmt.Sprintf("+ %s=%s", c.Key, c.New) + fmt.Fprintf(&b, "+ %s=%s%s", c.Key, c.New, where) + return b.String() } - return fmt.Sprintf("- %s=%s\n+ %s=%s", c.Key, c.Old, c.Key, c.New) + if strings.EqualFold(c.Old, c.New) { + // Only the dead copy goes; the live line is already right. + return strings.TrimSuffix(b.String(), "\n") + } + fmt.Fprintf(&b, "- %s=%s%s\n+ %s=%s%s", c.Key, c.Old, where, c.Key, c.New, where) + return b.String() } // Plan returns the changes needed to make the managed keys in the file match @@ -178,12 +232,17 @@ func (f *File) Plan(desired map[string]string) []Change { continue // unset in Skrog's config: not ours to touch } cur, present := f.Get(k) - switch { - case !present: - out = append(out, Change{Key: k, New: want, Added: true}) - case !strings.EqualFold(cur, want): - out = append(out, Change{Key: k, Old: cur, New: want}) + c := Change{Key: k, Old: cur, New: want, Added: !present} + if s := staleSection(k); s != "" { + if i := f.findIn(s, k); i >= 0 { + c.StaleSection = s + _, c.Stale = splitKV(f.lines[i]) + } + } + if present && strings.EqualFold(cur, want) && c.StaleSection == "" { + continue } + out = append(out, c) } return out } @@ -196,12 +255,17 @@ func (f *File) Apply(changes []Change) error { return nil } for _, c := range changes { + if c.StaleSection != "" { + if i := f.findIn(c.StaleSection, c.Key); i >= 0 { + f.lines = append(f.lines[:i], f.lines[i+1:]...) + } + } if i := f.find(c.Key); i >= 0 { // Keep any inline comment: someone wrote it for a reason. f.lines[i] = c.Key + "=" + c.New + inlineComment(f.lines[i]) continue } - f.insertIntoSection(c.Key + "=" + c.New) + f.insertIntoSection(SectionOf(c.Key), c.Key+"="+c.New) } return f.write() } @@ -214,10 +278,10 @@ func inlineComment(line string) string { return "" } -// insertIntoSection appends a line at the end of [wsl2], creating the section -// when the file has none. Appending rather than sorting keeps the user's own -// ordering intact. -func (f *File) insertIntoSection(line string) { +// insertIntoSection appends a line at the end of the named section, creating +// the section when the file has none. Appending rather than sorting keeps the +// user's own ordering intact. +func (f *File) insertIntoSection(section, line string) { start := -1 end := len(f.lines) for i, l := range f.lines { @@ -225,7 +289,7 @@ func (f *File) insertIntoSection(line string) { if !strings.HasPrefix(t, "[") || !strings.HasSuffix(t, "]") { continue } - if strings.EqualFold(strings.Trim(t, "[]"), Section) { + if strings.EqualFold(strings.Trim(t, "[]"), section) { start = i end = len(f.lines) continue @@ -236,11 +300,11 @@ func (f *File) insertIntoSection(line string) { } } if start < 0 { - // No [wsl2] at all: add one, separated from whatever precedes it. + // No such section at all: add one, separated from whatever precedes it. if len(f.lines) > 0 && strings.TrimSpace(f.lines[len(f.lines)-1]) != "" { f.lines = append(f.lines, "") } - f.lines = append(f.lines, "["+Section+"]", line) + f.lines = append(f.lines, "["+section+"]", line) return } // Insert after the section's last non-blank line, so a trailing blank line diff --git a/internal/wslconfig/wslconfig_test.go b/internal/wslconfig/wslconfig_test.go index fc75433..250328b 100644 --- a/internal/wslconfig/wslconfig_test.go +++ b/internal/wslconfig/wslconfig_test.go @@ -339,7 +339,124 @@ func TestVirtiofsKeySpelling(t *testing.T) { if wslconfig.KeyVirtiofs != "virtiofs" { t.Errorf("wslconfig.KeyVirtiofs = %q; WSL reads wsl2.virtiofs", wslconfig.KeyVirtiofs) } - if wslconfig.Section != "wsl2" { - t.Errorf("wslconfig.Section = %q; virtiofs lives under [wsl2]", wslconfig.Section) + if s := wslconfig.SectionOf(wslconfig.KeyVirtiofs); s != "wsl2" { + t.Errorf("SectionOf(virtiofs) = %q; virtiofs lives under [wsl2]", s) + } +} + +// keySection returns the section a key's live line sits in, "" when absent, +// and how many uncommented lines set it anywhere. Reads the file as WSL does: +// the section is whatever [header] last preceded the line. +func keySection(t *testing.T, path, key string) (string, int) { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + section, found, n := "", "", 0 + for _, line := range strings.Split(strings.ReplaceAll(string(b), "\r\n", "\n"), "\n") { + l := strings.TrimSpace(line) + if strings.HasPrefix(l, "[") && strings.HasSuffix(l, "]") { + section = strings.ToLower(strings.Trim(l, "[]")) + continue + } + if k, _, ok := strings.Cut(l, "="); ok && strings.EqualFold(strings.TrimSpace(k), key) { + found = section + n++ + } + } + return found, n +} + +// #550: WSL reads autoMemoryReclaim only as experimental.autoMemoryReclaim. +// Under [wsl2] it prints "Unknown key 'wsl2.autoMemoryReclaim'" and ignores +// it -- reproduced on WSL 3.0.3 -- which is where every release before the fix +// wrote it. +func TestAutoMemoryReclaimIsWrittenUnderExperimental(t *testing.T) { + p := write(t, "[wsl2]\nmemory=8GB\n") + f := load(t, p) + if err := f.Apply(f.Plan(map[string]string{"memory": "4GB", "autoMemoryReclaim": "gradual"})); err != nil { + t.Fatalf("Apply: %v", err) + } + if s, n := keySection(t, p, "autoMemoryReclaim"); s != "experimental" || n != 1 { + t.Errorf("autoMemoryReclaim is under [%s] (%d lines); WSL only reads it under [experimental]", s, n) + } + if s, _ := keySection(t, p, "memory"); s != "wsl2" { + t.Errorf("memory moved to [%s]; it belongs under [wsl2]", s) + } +} + +func TestAutoMemoryReclaimUnderWsl2IsNotEffective(t *testing.T) { + // `wsl-config show` and doctor report Get/All as what WSL applies. A line + // WSL ignores must not be reported as in effect. + f := load(t, write(t, "[wsl2]\nautoMemoryReclaim=gradual\n")) + if v, ok := f.Get("autoMemoryReclaim"); ok { + t.Errorf("Get(autoMemoryReclaim) = %q from [wsl2]; WSL ignores it there", v) + } + if _, ok := f.All()["autoMemoryReclaim"]; ok { + t.Error("All() reports autoMemoryReclaim from [wsl2] as effective") + } +} + +func TestStaleWsl2AutoMemoryReclaimIsMoved(t *testing.T) { + // The file an earlier Skrog left behind: same value, wrong section. The + // plan must not be empty (that would leave the setting dead forever), and + // the diff must say why a line with an unchanged value moves. + p := write(t, "[wsl2]\nmemory=4GB\nautoMemoryReclaim=gradual\n") + desired := map[string]string{"memory": "4GB", "autoMemoryReclaim": "gradual"} + f := load(t, p) + plan := f.Plan(desired) + if len(plan) != 1 || plan[0].Key != "autoMemoryReclaim" || plan[0].StaleSection != "wsl2" { + t.Fatalf("plan = %+v, want one move of autoMemoryReclaim out of [wsl2]", plan) + } + if s := plan[0].String(); !strings.Contains(s, "where WSL ignores it") || !strings.Contains(s, "[experimental]") { + t.Errorf("diff does not explain the move:\n%s", s) + } + if err := f.Apply(plan); err != nil { + t.Fatalf("Apply: %v", err) + } + if s, n := keySection(t, p, "autoMemoryReclaim"); s != "experimental" || n != 1 { + t.Errorf("after apply autoMemoryReclaim is under [%s] (%d lines), want one line under [experimental]", s, n) + } + if again := load(t, p).Plan(desired); len(again) != 0 { + t.Errorf("second plan = %v, want empty", again) + } +} + +func TestStaleCopyRemovedWhenTheLiveOneIsAlreadyRight(t *testing.T) { + p := write(t, "[wsl2]\nautoMemoryReclaim=dropcache\n\n[experimental]\nautoMemoryReclaim=gradual\n") + f := load(t, p) + plan := f.Plan(map[string]string{"autoMemoryReclaim": "gradual"}) + if len(plan) != 1 { + t.Fatalf("plan = %+v, want only the removal of the [wsl2] copy", plan) + } + if s := plan[0].String(); strings.Contains(s, "+ ") { + t.Errorf("diff adds a line that is already there:\n%s", s) + } + if err := f.Apply(plan); err != nil { + t.Fatalf("Apply: %v", err) + } + if s, n := keySection(t, p, "autoMemoryReclaim"); s != "experimental" || n != 1 { + t.Errorf("autoMemoryReclaim under [%s], %d lines; want exactly one, under [experimental]", s, n) + } +} + +func TestStaleCopyLeftAloneWhenSkrogHasNoOpinion(t *testing.T) { + // Unset in Skrog's config means not ours, even for a line we can tell is + // dead: someone may have written it themselves. + f := load(t, write(t, "[wsl2]\nautoMemoryReclaim=gradual\n")) + if plan := f.Plan(map[string]string{"autoMemoryReclaim": ""}); len(plan) != 0 { + t.Errorf("plan = %v, want empty", plan) + } +} + +func TestSectionOf(t *testing.T) { + for key, want := range map[string]string{ + "memory": "wsl2", "processors": "wsl2", "swap": "wsl2", "virtiofs": "wsl2", + "autoMemoryReclaim": "experimental", + } { + if got := wslconfig.SectionOf(key); got != want { + t.Errorf("SectionOf(%q) = %q, want %q", key, got, want) + } } }