From b0166c4aad530be051239940e5eac7ae7bb4b280 Mon Sep 17 00:00:00 2001 From: acmore Date: Sun, 5 Jul 2026 19:32:26 +0800 Subject: [PATCH 1/4] feat(sync): multiple disjoint sync mappings, each with its own direction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lift the single-mapping limit for the syncthing engine: spec.sync.paths now accepts multiple mappings as long as their roots are pairwise disjoint on both sides (equal or nested roots are rejected — two syncthing folders cannot share a root since .stignore lives in the directory, and nested roots would sync a subtree through two folders). Each mapping becomes its own syncthing folder. The first entry is the primary mapping: it keeps the legacy "okdev-" folder ID so existing sessions upgrade in place, it is the folder shared to mesh receivers, and it is the only remote cleared by up --reset-workspace / sync reset-remote (with a printed note when extra mappings exist). Additional mappings get stable IDs derived from the pair (reordering does not reset their folders) and stay local<->hub only. Mappings removed from the config have their folders pruned from both syncthing instances on the next start (managed-ID shapes only, other sessions' and user folders untouched). Mode semantics follow: by default every folder syncs in its mapping's direction; an explicit `sync --mode` forces all folders; `okdev up` passes only two-phase-or-empty, and the two-phase bootstrap applies per folder to bi mappings (directional folders configure their final types directly). The sync config hash covers all pairs and directions while hashing single-mapping bi sessions identically to the legacy shape, so upgrades do not restart existing sessions. Ready card, status --details configured paths, and dry-run output render per-path direction arrows. Kind e2e covers: overlap rejection via okdev validate, adding a second disjoint mapping to a live session (code up + results down), both folders syncing in their own directions, and per-path arrows in status. Co-Authored-By: Claude Fable 5 --- docs/command-reference.md | 3 +- docs/config-manifest.md | 17 +- internal/cli/mesh.go | 5 +- internal/cli/status_details.go | 2 +- internal/cli/sync.go | 42 ++-- internal/cli/sync_test.go | 41 ++-- internal/cli/syncthing.go | 249 ++++++++++++++++----- internal/cli/syncthing_multifolder_test.go | 119 ++++++++++ internal/cli/syncthing_test.go | 6 + internal/cli/up.go | 88 +++++--- internal/cli/up_ui_test.go | 45 ++-- internal/config/config.go | 43 +++- internal/config/config_test.go | 44 +++- scripts/e2e_kind_smoke.sh | 87 +++++++ skills/okdev/references/multipod.md | 4 +- 15 files changed, 648 insertions(+), 147 deletions(-) create mode 100644 internal/cli/syncthing_multifolder_test.go diff --git a/docs/command-reference.md b/docs/command-reference.md index c1170f46..8307b019 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -304,7 +304,8 @@ agents can react without launching a diagnostic chain on every blip: ### `okdev sync [--mode up|down|bi] [--foreground] [--reset] [--dry-run]` - Advanced command. Starts detached background sync by default; use `--foreground` for sync debugging, or explicit one-way sync (`up`/`down`). -- `--mode` defaults to the configured path's `direction` (`spec.sync.paths[].direction`, falling back to `bi`); an explicit flag overrides it for that invocation. See the config manifest for choosing a persistent direction — `down` makes the pod the authority so local writes can never clobber pod-generated results. +- By default each configured mapping syncs in its own `direction` (`spec.sync.paths[].direction`, falling back to `bi`); an explicit `--mode` forces that mode onto every mapping for the invocation. See the config manifest for choosing persistent directions — `down` makes the pod the authority so local writes can never clobber pod-generated results. +- With multiple mappings, each becomes its own syncthing folder; the first (primary) mapping is the one shared to mesh receivers, and `sync reset-remote` clears only the primary remote. - For default `--mode bi`, no-op when background sync is already active for the session. - `--background`: explicitly request detached background mode. - `--reset`: check local-to-hub sync and mesh receiver health, then reset only what is broken. Skips the local sync teardown when the primary sync is already healthy. For sessions with mesh receivers, probes each receiver and re-runs mesh setup only when broken or disconnected receivers are found. diff --git a/docs/config-manifest.md b/docs/config-manifest.md index 7f79eff1..c77c6547 100644 --- a/docs/config-manifest.md +++ b/docs/config-manifest.md @@ -233,7 +233,7 @@ spec: | Field | Type | Default | Description | |-------|------|---------|-------------| | `engine` | `string` | — | Sync engine (currently only `syncthing`) | -| `paths` | `[]entry` | — | Sync mappings (max 1 entry). Each entry is either the compact string `local:remote`, or a mapping `{local, remote, direction}` with an optional per-path `direction` | +| `paths` | `[]entry` | — | Sync mappings. Each entry is either the compact string `local:remote`, or a mapping `{local, remote, direction}` with an optional per-path `direction`. Multiple mappings are allowed as long as their roots are disjoint on both sides (equal or nested roots are rejected) | | `paths[].direction` | `string` | `bi` | Sync direction for this mapping's folder: `bi` (bidirectional), `up` (local is the authority: local sendonly, pod receiveonly), `down` (pod is the authority: pod sendonly, local receiveonly) | | `syncthing.version` | `string` | `v1.29.7` | Local Syncthing binary version | | `syncthing.autoInstall` | `bool` | `true` | Auto-install local Syncthing | @@ -243,7 +243,20 @@ spec: | `syncthing.compression` | `bool` | `false` | Use Syncthing `always` compression for peer connections instead of the default `metadata` mode | | `syncthing.versioningDays` | `int` | `30` | Keep files overwritten or deleted by sync in `.stversions` for this many days (staggered versioning, both sides); `0` disables | -**Validation:** `engine` must be `syncthing`; each `paths[]` entry must have non-empty local and remote; `direction` must be `bi`, `up`, or `down`; `versioningDays` must be `>= 0`. +**Validation:** `engine` must be `syncthing`; each `paths[]` entry must have non-empty local and remote; `direction` must be `bi`, `up`, or `down`; mapping roots must be pairwise disjoint on both the local and the remote side (lexical check); `versioningDays` must be `>= 0`. + +**Multiple mappings.** Each mapping becomes its own syncthing folder with its own direction. The first entry is the **primary** mapping: it keeps the legacy folder identity (existing sessions upgrade in place), it is the folder shared to mesh receiver pods, and it is the only remote cleared by `okdev up --reset-workspace` / `okdev sync reset-remote` — additional mappings (typically datasets or results) are never touched by resets and stay local↔hub only. Removing a mapping from the config removes its folder from both syncthing instances on the next `okdev up`. A typical split: + +```yaml +paths: + - .:/workspace # primary: code, bidirectional + - local: ./collected + remote: /data/results + direction: down # pod-generated results, pod is authority + - local: ../datasets + remote: /data/datasets + direction: up # dataset push, local is authority +``` **Sync direction.** `direction` applies to a mapping's whole synced folder — finer per-subpath direction inside one folder is impossible with Syncthing (ignore rules live in the directory itself; per-path folder types were rejected upstream). Pick the direction by what the folder carries: diff --git a/internal/cli/mesh.go b/internal/cli/mesh.go index 31b666ec..a60aeae1 100644 --- a/internal/cli/mesh.go +++ b/internal/cli/mesh.go @@ -33,8 +33,11 @@ type meshSummary struct { Receivers []meshReceiverStatus } +// meshFolderPath returns the folder the mesh shares to receiver pods: the +// primary (first) mapping's remote. Additional mappings stay local<->hub +// only — datasets and result folders do not fan out to receivers. func meshFolderPath(syncPairs []syncengine.Pair, workspaceMountPath string) string { - if len(syncPairs) == 1 && strings.TrimSpace(syncPairs[0].Remote) != "" { + if len(syncPairs) >= 1 && strings.TrimSpace(syncPairs[0].Remote) != "" { return syncPairs[0].Remote } return workspaceMountPath diff --git a/internal/cli/status_details.go b/internal/cli/status_details.go index 3d2e09bb..ffb5db9b 100644 --- a/internal/cli/status_details.go +++ b/internal/cli/status_details.go @@ -735,7 +735,7 @@ func summarizeConfiguredSyncPaths(cfg *config.DevEnvironment, cfgPath string) [] } lines := make([]string, 0, len(pairs)) for _, pair := range pairs { - lines = append(lines, fmt.Sprintf("%s -> %s", displayLocalSyncPath(pair.Local), pair.Remote)) + lines = append(lines, fmt.Sprintf("%s %s %s", displayLocalSyncPath(pair.Local), syncArrow(pairModeSymbol(pair)), pair.Remote)) } return lines } diff --git a/internal/cli/sync.go b/internal/cli/sync.go index ba67efcf..68eb78aa 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -89,12 +89,16 @@ func newSyncCmd(opts *Options) *cobra.Command { if err != nil { return err } - mode = resolveSyncMode(cmd.Flags().Changed("mode"), mode, pairs[0].Direction) + mode = resolveSyncMode(cmd.Flags().Changed("mode"), mode) if err := validateSyncMode(mode); err != nil { return err } if dryRun { - fmt.Fprintf(cmd.OutOrStdout(), "DRY RUN: sync session=%s namespace=%s engine=%s mode=%s\n", cc.sessionName, cc.namespace, engine, mode) + displayMode := mode + if displayMode == "" { + displayMode = "per-path" + } + fmt.Fprintf(cmd.OutOrStdout(), "DRY RUN: sync session=%s namespace=%s engine=%s mode=%s\n", cc.sessionName, cc.namespace, engine, displayMode) fmt.Fprintf(cmd.OutOrStdout(), "- paths: %v\n", pairs) if reset { scope := "all" @@ -145,7 +149,7 @@ func newSyncCmd(opts *Options) *cobra.Command { return nil } - if !background && mode == "bi" && os.Getenv("OKDEV_SYNCTHING_BACKGROUND_CHILD") != "1" { + if !background && (mode == "" || mode == "bi") && os.Getenv("OKDEV_SYNCTHING_BACKGROUND_CHILD") != "1" { if pidPath, err := syncthingPIDPath(cc.sessionName); err == nil { if pid, ok := readSyncthingPID(pidPath); ok && processAlive(pid) && processLooksLikeSyncthingSync(pid) { fmt.Fprintf(cmd.OutOrStdout(), "Syncthing sync already active in background for session %s\n", cc.sessionName) @@ -171,7 +175,7 @@ func newSyncCmd(opts *Options) *cobra.Command { }, } - cmd.Flags().StringVar(&mode, "mode", "bi", "Sync mode: up|down|bi (default: spec.sync.direction, or bi)") + cmd.Flags().StringVar(&mode, "mode", "", "Force a sync mode for all mappings: up|down|bi (default: each mapping's configured direction)") cmd.Flags().BoolVar(&background, "background", true, "Run syncthing sync as a detached background process") cmd.Flags().BoolVar(&foreground, "foreground", false, "Run syncthing sync in the foreground for troubleshooting") cmd.Flags().BoolVar(&reset, "reset", false, "Check sync health and reset broken components") @@ -184,20 +188,22 @@ func newSyncCmd(opts *Options) *cobra.Command { return cmd } -// resolveSyncMode picks the effective sync mode: an explicit --mode always -// wins; otherwise the configured spec.sync.direction applies. -func resolveSyncMode(flagSet bool, flagMode, configDirection string) string { +// resolveSyncMode picks the effective sync mode: an explicit --mode forces +// every folder; otherwise the empty mode means each mapping follows its own +// configured direction. +func resolveSyncMode(flagSet bool, flagMode string) string { if flagSet { return flagMode } - return configDirection + return "" } -// validateSyncMode accepts the documented modes plus the internal -// "two-phase" bootstrap mode that `okdev up` spawns for fresh sessions. +// validateSyncMode accepts the documented modes plus the internal values: +// "" (per-path directions from config) and "two-phase" (the bootstrap mode +// `okdev up` spawns for fresh sessions). func validateSyncMode(mode string) error { switch mode { - case "up", "down", "bi", "two-phase": + case "", "up", "down", "bi", "two-phase": return nil default: return fmt.Errorf("invalid sync mode %q: must be one of up, down, bi", mode) @@ -226,8 +232,8 @@ synced workspace subtree is cleared.`, if err != nil { return fmt.Errorf("parse sync paths: %w", err) } - if len(pairs) != 1 { - return fmt.Errorf("reset-remote requires exactly one sync path mapping, got %d", len(pairs)) + if len(pairs) == 0 { + return fmt.Errorf("reset-remote requires a sync path mapping") } if pairs[0].Direction == config.SyncDirectionDown { // reset-remote clears the remote and reseeds it from local; @@ -235,6 +241,9 @@ synced workspace subtree is cleared.`, // is receiveonly — the wipe would sync back as deletions. return fmt.Errorf("sync reset-remote conflicts with sync direction \"down\" (the pod is the sync authority)") } + if len(pairs) > 1 { + fmt.Fprintf(cmd.OutOrStdout(), "Note: only the primary mapping's remote (%s) is reset; %d additional mapping(s) are untouched.\n", pairs[0].Remote, len(pairs)-1) + } target, err := resolveTargetRef(cmd.Context(), cc.opts, cc.cfg, cc.namespace, cc.sessionName, cc.kube) if err != nil { @@ -448,7 +457,12 @@ func startDetachedSyncthingSync(opts *Options, mode, sessionName, namespace, cfg if opts.Verbose { args = append(args, "--verbose") } - args = append(args, "sync", "--mode", mode) + args = append(args, "sync") + if mode != "" { + // Empty mode means "follow each mapping's configured direction"; + // the child re-reads the config, so no flag is needed. + args = append(args, "--mode", mode) + } cmd := execCommand(exe, args...) cmd.Env = append(os.Environ(), "OKDEV_SYNCTHING_BACKGROUND_CHILD=1") diff --git a/internal/cli/sync_test.go b/internal/cli/sync_test.go index 7a56aa0c..5b9b45b7 100644 --- a/internal/cli/sync_test.go +++ b/internal/cli/sync_test.go @@ -13,6 +13,7 @@ import ( "github.com/acmore/okdev/internal/config" "github.com/acmore/okdev/internal/kube" + syncengine "github.com/acmore/okdev/internal/sync" ) func stubPkill(t *testing.T) { @@ -405,50 +406,60 @@ func TestSyncthingConfigChanged(t *testing.T) { func TestSyncthingSessionConfigHashTracksSyncSettings(t *testing.T) { cfg := &config.DevEnvironment{} cfg.SetDefaults() - hashA := syncthingSessionConfigHash(cfg, "/tmp/local", "/workspace") + pairs := []syncengine.Pair{{Local: "/tmp/local", Remote: "/workspace", Direction: "bi"}} + hashA := syncthingSessionConfigHash(cfg, pairs) if hashA == "" { t.Fatal("expected non-empty sync config hash") } cfg.Spec.Sync.Syncthing.WatcherDelaySeconds = 7 - hashB := syncthingSessionConfigHash(cfg, "/tmp/local", "/workspace") + hashB := syncthingSessionConfigHash(cfg, pairs) if hashA == hashB { t.Fatal("expected watcher delay change to affect sync config hash") } } -func TestSyncthingSessionConfigHashTracksDirection(t *testing.T) { +func TestSyncthingSessionConfigHashTracksDirectionAndPairs(t *testing.T) { cfg := &config.DevEnvironment{} cfg.SetDefaults() - base := syncthingSessionConfigHash(cfg, "/tmp/local", "/workspace") + base := syncthingSessionConfigHash(cfg, []syncengine.Pair{{Local: "/tmp/local", Remote: "/workspace", Direction: "bi"}}) // Explicit "bi" hashes identically to the legacy empty value so // upgrading okdev (or spelling out the default) does not force a sync // restart on existing sessions. - cfg.Spec.Sync.Paths = []config.SyncPathSpec{{Local: ".", Remote: "/workspace", Direction: "bi"}} - if got := syncthingSessionConfigHash(cfg, "/tmp/local", "/workspace"); got != base { - t.Fatal("explicit bi direction must hash like the legacy default") + if got := syncthingSessionConfigHash(cfg, []syncengine.Pair{{Local: "/tmp/local", Remote: "/workspace", Direction: "bi"}}); got != base { + t.Fatal("bi direction must hash stably") } - cfg.Spec.Sync.Paths = []config.SyncPathSpec{{Local: ".", Remote: "/workspace", Direction: "down"}} - if got := syncthingSessionConfigHash(cfg, "/tmp/local", "/workspace"); got == base { + if got := syncthingSessionConfigHash(cfg, []syncengine.Pair{{Local: "/tmp/local", Remote: "/workspace", Direction: "down"}}); got == base { t.Fatal("expected direction change to affect sync config hash") } + + // Adding a second mapping changes the hash so the next up restarts + // sync and configures the new folder. + multi := []syncengine.Pair{ + {Local: "/tmp/local", Remote: "/workspace", Direction: "bi"}, + {Local: "/tmp/results", Remote: "/data/results", Direction: "down"}, + } + if got := syncthingSessionConfigHash(cfg, multi); got == base { + t.Fatal("expected extra mapping to affect sync config hash") + } } func TestResolveSyncMode(t *testing.T) { - // Explicit --mode wins over the configured direction. - if got := resolveSyncMode(true, "up", "down"); got != "up" { + // Explicit --mode forces all mappings. + if got := resolveSyncMode(true, "up"); got != "up" { t.Fatalf("explicit flag must win, got %q", got) } - // Without the flag, spec.sync.direction applies. - if got := resolveSyncMode(false, "bi", "down"); got != "down" { - t.Fatalf("config direction must apply, got %q", got) + // Without the flag, the empty mode means each mapping follows its own + // configured direction. + if got := resolveSyncMode(false, "bi"); got != "" { + t.Fatalf("expected per-path mode, got %q", got) } } func TestValidateSyncMode(t *testing.T) { - for _, ok := range []string{"up", "down", "bi", "two-phase"} { + for _, ok := range []string{"", "up", "down", "bi", "two-phase"} { if err := validateSyncMode(ok); err != nil { t.Fatalf("mode %q should validate, got %v", ok, err) } diff --git a/internal/cli/syncthing.go b/internal/cli/syncthing.go index 9895ef9f..67b44471 100644 --- a/internal/cli/syncthing.go +++ b/internal/cli/syncthing.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "crypto/sha256" + "encoding/hex" "encoding/json" "encoding/xml" "errors" @@ -71,6 +72,7 @@ var ( markSyncthingBootstrapCompleteFn = markSyncthingBootstrapComplete runTwoPhaseInitialSyncFn = runTwoPhaseInitialSync configureSyncthingPeerFn = configureSyncthingPeer + pruneManagedSessionFoldersFn = pruneManagedSessionFolders saveSyncthingConfigHashFn = saveSyncthingConfigHash stopOwnedLocalSyncthingFn = stopOwnedLocalSyncthing runSyncHealthLoopFn = runSyncHealthLoop @@ -78,9 +80,63 @@ var ( waitForSyncthingBootstrapCompleteFn = waitForSyncthingBootstrapComplete ) +// syncFolder is one resolved syncthing folder for the session: a sync pair +// plus its stable folder ID and the mode it should run in this start. +type syncFolder struct { + id string + absLocal string + remote string + mode string // bi | up | down + twoPhase bool // run the two-phase bootstrap for this folder +} + +// resolveSyncFolders maps pairs to folders. mode semantics: "" follows each +// pair's configured direction; "up"/"down"/"bi" force every folder; +// "two-phase" bootstraps bi folders (directional folders never need the +// bootstrap — one side is already sendonly). +func resolveSyncFolders(sessionName, mode string, pairs []syncengine.Pair) ([]syncFolder, error) { + folders := make([]syncFolder, 0, len(pairs)) + for i, pair := range pairs { + absLocal, err := filepath.Abs(pair.Local) + if err != nil { + return nil, err + } + // ParsePairs normalizes Direction, but be defensive for callers + // that construct pairs directly. + folderMode := pair.Direction + if folderMode == "" { + folderMode = config.SyncDirectionBi + } + if mode != "" && mode != "two-phase" { + folderMode = mode + } + folders = append(folders, syncFolder{ + id: sessionSyncFolderID(sessionName, i, pair), + absLocal: absLocal, + remote: pair.Remote, + mode: folderMode, + twoPhase: mode == "two-phase" && folderMode == "bi", + }) + } + return folders, nil +} + +// sessionSyncFolderID returns the syncthing folder ID for a pair. The first +// (primary) mapping keeps the legacy "okdev-" ID so existing +// sessions and the mesh keep working; additional mappings get a stable +// suffix derived from the pair itself, so reordering non-primary entries +// does not reset their folders. +func sessionSyncFolderID(sessionName string, index int, pair syncengine.Pair) string { + if index == 0 { + return "okdev-" + sessionName + } + sum := sha256.Sum256([]byte(pair.Local + "\x00" + pair.Remote)) + return fmt.Sprintf("okdev-%s-%s", sessionName, hex.EncodeToString(sum[:4])) +} + func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironment, namespace, sessionName, mode string, pairs []syncengine.Pair, k *kube.Client) error { - if len(pairs) != 1 { - return fmt.Errorf("syncthing engine currently supports exactly one sync path mapping") + if len(pairs) == 0 { + return fmt.Errorf("no sync path mappings configured") } bootstrapCtx, cancelBootstrap, runtimeCtx := syncthingBootstrapAndRuntimeContexts(cmd.Context()) @@ -111,20 +167,22 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm } } - pair := pairs[0] - absLocal, err := filepath.Abs(pair.Local) + folders, err := resolveSyncFolders(sessionName, mode, pairs) if err != nil { return err } - if err := os.MkdirAll(absLocal, 0o755); err != nil { - return err - } - if err := writeLocalSTIgnore(absLocal); err != nil { - return err - } - if _, err := execInSyncthingContainerFn(bootstrapCtx, k, namespace, pod, fmt.Sprintf("mkdir -p %s", syncengine.ShellEscape(pair.Remote))); err != nil { - return err + for _, folder := range folders { + if err := os.MkdirAll(folder.absLocal, 0o755); err != nil { + return err + } + if err := writeLocalSTIgnore(folder.absLocal); err != nil { + return err + } + if _, err := execInSyncthingContainerFn(bootstrapCtx, k, namespace, pod, fmt.Sprintf("mkdir -p %s", syncengine.ShellEscape(folder.remote))); err != nil { + return err + } } + primary := folders[0] localHome, err := localSyncthingHome(sessionName) if err != nil { return err @@ -163,7 +221,6 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm return err } - folderID := "okdev-" + sessionName syncCfg := syncthingSyncConfig{ rescanInterval: cfg.Spec.Sync.Syncthing.RescanIntervalSeconds, watcherDelay: cfg.Spec.Sync.Syncthing.WatcherDelaySeconds, @@ -180,41 +237,69 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm return fmt.Errorf("mark syncthing ready: %w", err) } - if mode == "two-phase" { - // The bootstrap context has a short timeout (syncthingBootstrapTimeout). - // Two-phase initial sync may take much longer, so use a dedicated context. - twoPhaseCtx, twoPhaseCancel := context.WithTimeout(runtimeCtx, initialSyncTimeout) - defer twoPhaseCancel() - if err := runTwoPhaseInitialSyncFn(twoPhaseCtx, cmd.OutOrStdout(), localBase, localKey, localID, remoteBase, remoteKey, remoteID, localRemotePeerAddr, folderID, absLocal, pair.Remote, syncCfg); err != nil { - return fmt.Errorf("two-phase initial sync: %w", err) + ranTwoPhase := false + for _, folder := range folders { + if folder.twoPhase { + // The bootstrap context has a short timeout + // (syncthingBootstrapTimeout). Two-phase initial sync may take + // much longer, so use a dedicated context. + twoPhaseCtx, twoPhaseCancel := context.WithTimeout(runtimeCtx, initialSyncTimeout) + err := runTwoPhaseInitialSyncFn(twoPhaseCtx, cmd.OutOrStdout(), localBase, localKey, localID, remoteBase, remoteKey, remoteID, localRemotePeerAddr, folder.id, folder.absLocal, folder.remote, syncCfg) + twoPhaseCancel() + if err != nil { + return fmt.Errorf("two-phase initial sync (%s): %w", folder.id, err) + } + ranTwoPhase = true + continue } - if err := markSyncthingBootstrapCompleteFn(sessionName); err != nil { - return fmt.Errorf("mark syncthing bootstrap complete: %w", err) + folderTypeLocal, folderTypeRemote := folderTypesForMode(folder.mode) + if err := configureSyncthingPeerFn(bootstrapCtx, localBase, localKey, localID, remoteID, localRemotePeerAddr, folder.id, folder.absLocal, folderTypeLocal, syncCfg.rescanInterval, syncCfg.watcherDelay, false, syncCfg.relays, syncCfg.compression, syncCfg.versioningDays); err != nil { + return fmt.Errorf("configure local syncthing (%s): %w", folder.id, err) } - mode = "bi" - } else { - folderTypeLocal, folderTypeRemote := folderTypesForMode(mode) - if err := configureSyncthingPeerFn(bootstrapCtx, localBase, localKey, localID, remoteID, localRemotePeerAddr, folderID, absLocal, folderTypeLocal, syncCfg.rescanInterval, syncCfg.watcherDelay, false, syncCfg.relays, syncCfg.compression, syncCfg.versioningDays); err != nil { - return fmt.Errorf("configure local syncthing: %w", err) + if err := configureSyncthingPeerFn(bootstrapCtx, remoteBase, remoteKey, remoteID, localID, "", folder.id, folder.remote, folderTypeRemote, syncCfg.rescanInterval, syncCfg.watcherDelay, false, syncCfg.relays, syncCfg.compression, syncCfg.versioningDays); err != nil { + return fmt.Errorf("configure remote syncthing (%s): %w", folder.id, err) } - if err := configureSyncthingPeerFn(bootstrapCtx, remoteBase, remoteKey, remoteID, localID, "", folderID, pair.Remote, folderTypeRemote, syncCfg.rescanInterval, syncCfg.watcherDelay, false, syncCfg.relays, syncCfg.compression, syncCfg.versioningDays); err != nil { - return fmt.Errorf("configure remote syncthing: %w", err) + } + if ranTwoPhase { + if err := markSyncthingBootstrapCompleteFn(sessionName); err != nil { + return fmt.Errorf("mark syncthing bootstrap complete: %w", err) } } - if err := saveSyncthingConfigHashFn(sessionName, syncthingSessionConfigHash(cfg, absLocal, pair.Remote)); err != nil { + // Mappings removed from the config leave their syncthing folders behind + // on both sides, silently continuing to sync; drop managed folders that + // no longer correspond to a configured pair. + keepFolderIDs := make(map[string]bool, len(folders)) + for _, folder := range folders { + keepFolderIDs[folder.id] = true + } + if err := pruneManagedSessionFoldersFn(bootstrapCtx, localBase, localKey, sessionName, keepFolderIDs); err != nil { + return fmt.Errorf("prune stale local sync folders: %w", err) + } + if err := pruneManagedSessionFoldersFn(bootstrapCtx, remoteBase, remoteKey, sessionName, keepFolderIDs); err != nil { + return fmt.Errorf("prune stale remote sync folders: %w", err) + } + + if err := saveSyncthingConfigHashFn(sessionName, syncthingSessionConfigHash(cfg, pairs)); err != nil { return fmt.Errorf("persist syncthing config state: %w", err) } - fmt.Fprintf(cmd.OutOrStdout(), "Syncthing sync active (%s) for session %s\n", mode, sessionName) - fmt.Fprintf(cmd.OutOrStdout(), "Local folder: %s\n", absLocal) - fmt.Fprintf(cmd.OutOrStdout(), "Remote folder: %s\n", pair.Remote) + activeLabel := "per-path" + if mode != "" && mode != "two-phase" { + activeLabel = mode + } + fmt.Fprintf(cmd.OutOrStdout(), "Syncthing sync active (%s) for session %s\n", activeLabel, sessionName) + for _, folder := range folders { + fmt.Fprintf(cmd.OutOrStdout(), "Folder %s: %s %s %s\n", folder.id, folder.absLocal, syncArrow(modeSymbol(folder.mode)), folder.remote) + } fmt.Fprintf(cmd.OutOrStdout(), "Local binary: %s\n", localBinary) fmt.Fprintln(cmd.OutOrStdout(), "Press Ctrl+C to stop sync tunnel and local syncthing.") progressCtx, stopProgress := context.WithCancel(context.Background()) defer stopProgress() - go runSyncthingProgressReporter(progressCtx, cmd.OutOrStdout(), localBase, localKey, remoteBase, remoteKey, folderID, localID, remoteID, absLocal) + // Progress reporting covers the primary folder; additional folders + // converge in the background and surface through sync health/status. + go runSyncthingProgressReporter(progressCtx, cmd.OutOrStdout(), localBase, localKey, remoteBase, remoteKey, primary.id, localID, remoteID, primary.absLocal) // The detached sync child should survive the parent CLI process exiting. signal.Ignore(syscall.SIGHUP) @@ -2047,6 +2132,57 @@ func configureSyncthingPeer(ctx context.Context, base, key, selfID, peerID, peer return nil } +// pruneManagedSessionFolders removes this session's managed folders that are +// not in keep from a syncthing instance's config. Only IDs of the exact +// managed shapes "okdev-" and "okdev--<8 hex>" are +// touched, so user-defined folders (and other sessions' folders, whose +// session name would make a longer prefix) are never removed. +func pruneManagedSessionFolders(ctx context.Context, base, key, sessionName string, keep map[string]bool) error { + cfg, err := syncthingGetConfig(ctx, base, key) + if err != nil { + return err + } + changed, err := pruneManagedFoldersFromConfig(cfg, sessionName, keep) + if err != nil { + return err + } + if !changed { + return nil + } + return syncthingSetConfig(ctx, base, key, cfg) +} + +// pruneManagedFoldersFromConfig drops this session's managed folders that +// are not in keep, mutating cfg in place; it reports whether anything +// changed. +func pruneManagedFoldersFromConfig(cfg map[string]any, sessionName string, keep map[string]bool) (bool, error) { + folders, err := syncthingObjectArray(cfg, "folders") + if err != nil { + return false, err + } + primaryID := "okdev-" + sessionName + secondaryRe := regexp.MustCompile("^" + regexp.QuoteMeta(primaryID) + "-[0-9a-f]{8}$") + changed := false + out := make([]any, 0, len(folders)) + for _, f := range folders { + fm, err := syncthingObjectMap(f, "folders") + if err != nil { + return false, err + } + id := asString(fm["id"]) + managed := id == primaryID || secondaryRe.MatchString(id) + if managed && !keep[id] { + changed = true + continue + } + out = append(out, fm) + } + if changed { + cfg["folders"] = out + } + return changed, nil +} + func syncthingMergeFolderDevices(existingFolderDevices, devices any, selfID, peerID string) ([]any, error) { deviceEntries, err := syncthingObjectArray(map[string]any{"devices": devices}, "devices") if err != nil { @@ -2468,31 +2604,44 @@ type syncthingSessionConfigState struct { Engine string `json:"engine"` LocalPath string `json:"localPath"` RemotePath string `json:"remotePath"` - // Direction is part of the hash so editing spec.sync.direction makes + // Direction is part of the hash so editing a mapping's direction makes // the next `okdev up` restart sync with the new folder types. The - // default "bi" is hashed as the empty legacy value to avoid a - // restart on upgrade for existing sessions. - Direction string `json:"direction,omitempty"` - WatcherDelaySeconds int `json:"watcherDelaySeconds,omitempty"` - RescanIntervalSeconds int `json:"rescanIntervalSeconds,omitempty"` - RelaysEnabled bool `json:"relaysEnabled,omitempty"` - Compression bool `json:"compression,omitempty"` - VersioningDays int `json:"versioningDays,omitempty"` -} - -func syncthingSessionConfigHash(cfg *config.DevEnvironment, localPath, remotePath string) string { - if cfg == nil { + // default "bi" is hashed as the empty legacy value, and ExtraPairs is + // omitted for single-mapping sessions, so upgrading okdev does not + // force a restart on existing sessions. + Direction string `json:"direction,omitempty"` + ExtraPairs []string `json:"extraPairs,omitempty"` + WatcherDelaySeconds int `json:"watcherDelaySeconds,omitempty"` + RescanIntervalSeconds int `json:"rescanIntervalSeconds,omitempty"` + RelaysEnabled bool `json:"relaysEnabled,omitempty"` + Compression bool `json:"compression,omitempty"` + VersioningDays int `json:"versioningDays,omitempty"` +} + +func syncthingSessionConfigHash(cfg *config.DevEnvironment, pairs []syncengine.Pair) string { + if cfg == nil || len(pairs) == 0 { return "" } + absLocal := func(p syncengine.Pair) string { + if abs, err := filepath.Abs(p.Local); err == nil { + return abs + } + return p.Local + } direction := "" - if pairs, err := syncengine.ParsePairs(cfg.Spec.Sync.Paths, remotePath); err == nil && len(pairs) > 0 && pairs[0].Direction != config.SyncDirectionBi { + if pairs[0].Direction != config.SyncDirectionBi { direction = pairs[0].Direction } + var extra []string + for _, p := range pairs[1:] { + extra = append(extra, absLocal(p)+"\x00"+p.Remote+"\x00"+p.Direction) + } state := syncthingSessionConfigState{ Engine: strings.TrimSpace(cfg.Spec.Sync.Engine), - LocalPath: localPath, - RemotePath: remotePath, + LocalPath: absLocal(pairs[0]), + RemotePath: pairs[0].Remote, Direction: direction, + ExtraPairs: extra, WatcherDelaySeconds: cfg.Spec.Sync.Syncthing.WatcherDelaySeconds, RescanIntervalSeconds: cfg.Spec.Sync.Syncthing.RescanIntervalSeconds, RelaysEnabled: cfg.Spec.Sync.Syncthing.RelaysEnabled, diff --git a/internal/cli/syncthing_multifolder_test.go b/internal/cli/syncthing_multifolder_test.go new file mode 100644 index 00000000..0a623ae2 --- /dev/null +++ b/internal/cli/syncthing_multifolder_test.go @@ -0,0 +1,119 @@ +package cli + +import ( + "strings" + "testing" + + syncengine "github.com/acmore/okdev/internal/sync" +) + +func TestResolveSyncFoldersPerPathAndOverride(t *testing.T) { + pairs := []syncengine.Pair{ + {Local: ".", Remote: "/workspace", Direction: "bi"}, + {Local: "./results", Remote: "/data/results", Direction: "down"}, + {Local: "./datasets", Remote: "/data/sets", Direction: "up"}, + } + + // Empty mode: each folder follows its own direction; no bootstrap. + folders, err := resolveSyncFolders("sess", "", pairs) + if err != nil { + t.Fatalf("resolveSyncFolders: %v", err) + } + if folders[0].mode != "bi" || folders[1].mode != "down" || folders[2].mode != "up" { + t.Fatalf("unexpected per-path modes: %+v", folders) + } + for _, f := range folders { + if f.twoPhase { + t.Fatalf("no folder should bootstrap in empty mode: %+v", f) + } + } + + // two-phase bootstraps only bi folders; directional ones configure + // their final types directly. + folders, err = resolveSyncFolders("sess", "two-phase", pairs) + if err != nil { + t.Fatalf("resolveSyncFolders: %v", err) + } + if !folders[0].twoPhase || folders[1].twoPhase || folders[2].twoPhase { + t.Fatalf("two-phase must apply to bi folders only: %+v", folders) + } + if folders[1].mode != "down" || folders[2].mode != "up" { + t.Fatalf("directional folders must keep their modes under two-phase: %+v", folders) + } + + // An explicit mode forces every folder. + folders, err = resolveSyncFolders("sess", "down", pairs) + if err != nil { + t.Fatalf("resolveSyncFolders: %v", err) + } + for _, f := range folders { + if f.mode != "down" || f.twoPhase { + t.Fatalf("explicit mode must force all folders: %+v", f) + } + } + + // Defensive: an empty direction (pair built without ParsePairs) is bi. + folders, err = resolveSyncFolders("sess", "two-phase", []syncengine.Pair{{Local: ".", Remote: "/w"}}) + if err != nil { + t.Fatalf("resolveSyncFolders: %v", err) + } + if folders[0].mode != "bi" || !folders[0].twoPhase { + t.Fatalf("empty direction must default to bi: %+v", folders[0]) + } +} + +func TestSessionSyncFolderIDStability(t *testing.T) { + primary := syncengine.Pair{Local: ".", Remote: "/workspace"} + extraA := syncengine.Pair{Local: "./results", Remote: "/data/results"} + extraB := syncengine.Pair{Local: "./datasets", Remote: "/data/sets"} + + if got := sessionSyncFolderID("sess", 0, primary); got != "okdev-sess" { + t.Fatalf("primary mapping must keep the legacy folder ID, got %q", got) + } + idA := sessionSyncFolderID("sess", 1, extraA) + idB := sessionSyncFolderID("sess", 2, extraB) + if !strings.HasPrefix(idA, "okdev-sess-") || len(idA) != len("okdev-sess-")+8 { + t.Fatalf("unexpected secondary folder ID shape: %q", idA) + } + if idA == idB { + t.Fatal("different pairs must get different folder IDs") + } + // Stable under reordering: the ID depends on the pair, not the index. + if got := sessionSyncFolderID("sess", 2, extraA); got != idA { + t.Fatalf("secondary folder ID must be index-independent: %q vs %q", got, idA) + } +} + +func TestPruneManagedFoldersFromConfig(t *testing.T) { + cfg := map[string]any{ + "folders": []any{ + map[string]any{"id": "okdev-sess"}, // primary, kept + map[string]any{"id": "okdev-sess-aabbccdd"}, // managed extra, stale -> pruned + map[string]any{"id": "okdev-sess-2"}, // ANOTHER session's primary -> untouched + map[string]any{"id": "okdev-sess-2-aabbccdd"}, // another session's extra -> untouched + map[string]any{"id": "user-folder"}, // user-defined -> untouched + map[string]any{"id": "okdev-sess-not8hexnot8hex00"}, // wrong suffix shape -> untouched + }, + } + changed, err := pruneManagedFoldersFromConfig(cfg, "sess", map[string]bool{"okdev-sess": true}) + if err != nil { + t.Fatalf("prune: %v", err) + } + if !changed { + t.Fatal("expected stale managed folder to be pruned") + } + var ids []string + for _, f := range cfg["folders"].([]any) { + ids = append(ids, f.(map[string]any)["id"].(string)) + } + want := []string{"okdev-sess", "okdev-sess-2", "okdev-sess-2-aabbccdd", "user-folder", "okdev-sess-not8hexnot8hex00"} + if strings.Join(ids, ",") != strings.Join(want, ",") { + t.Fatalf("unexpected surviving folders: %v (want %v)", ids, want) + } + + // No-op when everything is kept. + changed, err = pruneManagedFoldersFromConfig(cfg, "sess", map[string]bool{"okdev-sess": true}) + if err != nil || changed { + t.Fatalf("expected no-op prune, changed=%v err=%v", changed, err) + } +} diff --git a/internal/cli/syncthing_test.go b/internal/cli/syncthing_test.go index fda5385a..408d9fb0 100644 --- a/internal/cli/syncthing_test.go +++ b/internal/cli/syncthing_test.go @@ -1536,6 +1536,9 @@ func TestRunSyncthingSyncMarksReadyBeforeTwoPhaseBootstrap(t *testing.T) { saveSyncthingConfigHashFn = func(string, string) error { return nil } stopOwnedLocalSyncthingFn = func(string, int) error { return nil } runSyncHealthLoopFn = func(<-chan os.Signal, io.Writer, syncHealthChecker) {} + origPrune := pruneManagedSessionFoldersFn + pruneManagedSessionFoldersFn = func(context.Context, string, string, string, map[string]bool) error { return nil } + t.Cleanup(func() { pruneManagedSessionFoldersFn = origPrune }) cfg := &config.DevEnvironment{} cfg.SetDefaults() @@ -1642,6 +1645,9 @@ func TestRunSyncthingSyncMarksBootstrapCompleteBeforeHealthLoop(t *testing.T) { t.Fatalf("expected bootstrap completion marker before health loop, got %v", err) } } + origPrune := pruneManagedSessionFoldersFn + pruneManagedSessionFoldersFn = func(context.Context, string, string, string, map[string]bool) error { return nil } + t.Cleanup(func() { pruneManagedSessionFoldersFn = origPrune }) cfg := &config.DevEnvironment{} cfg.SetDefaults() diff --git a/internal/cli/up.go b/internal/cli/up.go index fe24abbe..d9d40ca9 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -1079,12 +1079,8 @@ func upSetupSync(state *upState, target workload.TargetRef) (string, string, str } configHash := "" hasConfigState := false - if len(state.syncPairs) == 1 { - localPath, err := filepath.Abs(state.syncPairs[0].Local) - if err != nil { - return "", "", "", false, fmt.Errorf("resolve sync path: %w", err) - } - configHash = syncthingSessionConfigHash(state.command.cfg, localPath, state.syncPairs[0].Remote) + if len(state.syncPairs) > 0 { + configHash = syncthingSessionConfigHash(state.command.cfg, state.syncPairs) stored, err := loadSyncthingTargetSessionState(state.command.sessionName) if err != nil { return "", "", "", false, fmt.Errorf("load syncthing config state: %w", err) @@ -1099,18 +1095,18 @@ func upSetupSync(state *upState, target workload.TargetRef) (string, string, str } configChanged = changed } - direction := config.SyncDirectionBi + primaryDirection := config.SyncDirectionBi if len(state.syncPairs) > 0 { - direction = state.syncPairs[0].Direction + primaryDirection = state.syncPairs[0].Direction } active := syncthingSessionActive(state.command.sessionName) restartRequired := state.flags.resetWorkspace || configChanged || !active - startMode = syncStartMode(direction, state.flags.resetWorkspace, targetReset, hasConfigState, configChanged, active) + startMode = syncStartMode(state.flags.resetWorkspace, targetReset, hasConfigState, configChanged, active) // Signal waitForInitialSync to probe the remote folder type for an // incomplete bootstrap, reusing the port-forward it already opens. Only - // meaningful for bi: directional folders never run the two-phase - // bootstrap, so there is nothing to resume. - bootstrapResume = len(state.syncPairs) == 1 && hasConfigState && !state.flags.resetWorkspace && !targetReset && direction == config.SyncDirectionBi + // meaningful when the primary folder is bi: directional folders never + // run the two-phase bootstrap, so there is nothing to resume. + bootstrapResume = len(state.syncPairs) == 1 && hasConfigState && !state.flags.resetWorkspace && !targetReset && primaryDirection == config.SyncDirectionBi if restartRequired { if err := refreshSyncthingSessionProcesses(state.command.sessionName); err != nil { return "", "", "", false, fmt.Errorf("refresh local syncthing session state: %w", err) @@ -1118,22 +1114,28 @@ func upSetupSync(state *upState, target workload.TargetRef) (string, string, str } if state.flags.resetWorkspace { - if len(state.syncPairs) != 1 { - return "", "", "", false, fmt.Errorf("--reset-workspace requires exactly one sync path mapping, got %d", len(state.syncPairs)) + if len(state.syncPairs) == 0 { + return "", "", "", false, fmt.Errorf("--reset-workspace requires a sync path mapping") } state.ui.stepRun("sync", "resetting remote workspace") if err := resetSyncthingSessionState(state.command.sessionName); err != nil { return "", "", "", false, fmt.Errorf("reset sync state: %w", err) } + // Only the primary mapping's remote is cleared; additional mappings + // keep their data (they typically carry datasets or results). if err := resetRemoteWorkspace(state.ctx, state.command.kube, state.command.namespace, target.PodName, state.syncPairs[0].Remote, state.command.cfg.Spec.Sync.PreservePaths); err != nil { return "", "", "", false, fmt.Errorf("clear remote workspace: %w", err) } - state.ui.stepDone("sync", "remote workspace cleared") + if len(state.syncPairs) > 1 { + state.ui.stepDone("sync", fmt.Sprintf("remote workspace cleared (primary %s only; %d additional mapping(s) untouched)", state.syncPairs[0].Remote, len(state.syncPairs)-1)) + } else { + state.ui.stepDone("sync", "remote workspace cleared") + } } if !restartRequired { - modeSym = modeSymbol(startMode) - syncPathSummary := syncPairsSummary(state.syncPairs, modeSym) + modeSym = upSyncModeSymbol(state.syncPairs) + syncPathSummary := syncPairsSummary(state.syncPairs) summary = "already active (" + modeSym + ")" state.ui.stepDone("sync", fmt.Sprintf("already active (%s), %s", modeSym, syncPathSummary)) return summary, modeSym, startMode, bootstrapResume, nil @@ -1144,8 +1146,8 @@ func upSetupSync(state *upState, target workload.TargetRef) (string, string, str if err != nil { return "", "", "", false, fmt.Errorf("start syncthing background sync: %w", err) } - modeSym = modeSymbol(startMode) - syncPathSummary := syncPairsSummary(state.syncPairs, modeSym) + modeSym = upSyncModeSymbol(state.syncPairs) + syncPathSummary := syncPairsSummary(state.syncPairs) if started { summary = "active (" + modeSym + ")" state.ui.stepDone("sync", fmt.Sprintf("active (%s), %s, logs: %s", modeSym, syncPathSummary, logPath)) @@ -1787,7 +1789,7 @@ func (u *upUI) printReadyCard(sessionName, namespace, pod, sshSummary, syncSumma if len(syncPairs) > 0 { fmt.Fprintln(u.out, "sync paths:") for _, pair := range syncPairs { - fmt.Fprintf(u.out, "- %s %s %s\n", displayLocalSyncPath(pair.Local), syncArrow(syncModeSymbol), pair.Remote) + fmt.Fprintf(u.out, "- %s %s %s\n", displayLocalSyncPath(pair.Local), syncArrow(pairModeSymbol(pair)), pair.Remote) } } if len(ports) > 0 { @@ -1804,17 +1806,38 @@ func (u *upUI) printReadyCard(sessionName, namespace, pod, sshSummary, syncSumma fmt.Fprintln(u.out, "- okdev down") } -func syncPairsSummary(pairs []syncengine.Pair, syncModeSymbol string) string { +// pairModeSymbol renders a mapping's direction arrow, defaulting the empty +// direction to bidirectional (ParsePairs normalizes it, but callers may +// construct pairs directly). +func pairModeSymbol(pair syncengine.Pair) string { + direction := pair.Direction + if direction == "" { + direction = config.SyncDirectionBi + } + return modeSymbol(direction) +} + +func syncPairsSummary(pairs []syncengine.Pair) string { if len(pairs) == 0 { return "no paths" } if len(pairs) == 1 { pair := pairs[0] - return fmt.Sprintf("%s %s %s", displayLocalSyncPath(pair.Local), syncArrow(syncModeSymbol), pair.Remote) + return fmt.Sprintf("%s %s %s", displayLocalSyncPath(pair.Local), syncArrow(pairModeSymbol(pair)), pair.Remote) } return fmt.Sprintf("%d paths", len(pairs)) } +// upSyncModeSymbol renders the ready-card summary arrow: a single mapping +// shows its own direction; with multiple mappings the per-path lines carry +// the direction arrows and the summary stays bidirectional. +func upSyncModeSymbol(pairs []syncengine.Pair) string { + if len(pairs) == 1 { + return pairModeSymbol(pairs[0]) + } + return modeSymbol("bi") +} + func displayLocalSyncPath(path string) string { if strings.TrimSpace(path) == "" { return path @@ -1845,15 +1868,12 @@ func modeSymbol(mode string) string { } } -func syncStartMode(direction string, resetWorkspace, targetReset, hasConfigState, configChanged, active bool) string { - // A directional folder never needs the two-phase bootstrap: that - // protocol exists to protect the remote from the local side's initial - // emptiness before flipping both sides to sendreceive. With up/down one - // side is already sendonly, so the final folder types are safe from the - // first start. - if direction == config.SyncDirectionUp || direction == config.SyncDirectionDown { - return direction - } +// syncStartMode picks how `okdev up` starts sync: "two-phase" bootstraps +// fresh or reset sessions (applied per folder to bi mappings only — +// directional folders never need the bootstrap since one side is already +// sendonly), and the empty mode means every mapping follows its configured +// direction. +func syncStartMode(resetWorkspace, targetReset, hasConfigState, configChanged, active bool) string { switch { case resetWorkspace: return "two-phase" @@ -1861,12 +1881,8 @@ func syncStartMode(direction string, resetWorkspace, targetReset, hasConfigState return "two-phase" case !hasConfigState: return "two-phase" - case configChanged: - return "bi" - case !active: - return "bi" default: - return "bi" + return "" } } diff --git a/internal/cli/up_ui_test.go b/internal/cli/up_ui_test.go index 6bceb91e..841032c7 100644 --- a/internal/cli/up_ui_test.go +++ b/internal/cli/up_ui_test.go @@ -119,7 +119,6 @@ func TestSyncHelpers(t *testing.T) { func TestSyncStartMode(t *testing.T) { cases := []struct { name string - direction string resetWorkspace bool targetReset bool hasConfigState bool @@ -127,42 +126,50 @@ func TestSyncStartMode(t *testing.T) { active bool want string }{ - {name: "first bootstrap", direction: "bi", hasConfigState: false, active: false, want: "two-phase"}, - {name: "explicit reset", direction: "bi", resetWorkspace: true, hasConfigState: true, configChanged: true, active: true, want: "two-phase"}, - {name: "target recreated", direction: "bi", targetReset: true, hasConfigState: true, active: false, want: "two-phase"}, - {name: "config changed on active session", direction: "bi", hasConfigState: true, configChanged: true, active: true, want: "bi"}, - {name: "daemon recovery", direction: "bi", hasConfigState: true, active: false, want: "bi"}, - {name: "steady state", direction: "bi", hasConfigState: true, active: true, want: "bi"}, - // Directional folders skip the two-phase bootstrap entirely: one - // side is already sendonly, so the final types are safe from the - // first start. - {name: "up direction fresh", direction: "up", hasConfigState: false, active: false, want: "up"}, - {name: "up direction target reset", direction: "up", targetReset: true, hasConfigState: true, active: false, want: "up"}, - {name: "down direction fresh", direction: "down", hasConfigState: false, active: false, want: "down"}, - {name: "down direction steady", direction: "down", hasConfigState: true, active: true, want: "down"}, + {name: "first bootstrap", hasConfigState: false, active: false, want: "two-phase"}, + {name: "explicit reset", resetWorkspace: true, hasConfigState: true, configChanged: true, active: true, want: "two-phase"}, + {name: "target recreated", targetReset: true, hasConfigState: true, active: false, want: "two-phase"}, + // The empty mode means every mapping follows its configured + // direction (two-phase still applies per folder to bi mappings + // only, inside runSyncthingSync). + {name: "config changed on active session", hasConfigState: true, configChanged: true, active: true, want: ""}, + {name: "daemon recovery", hasConfigState: true, active: false, want: ""}, + {name: "steady state", hasConfigState: true, active: true, want: ""}, } for _, tc := range cases { - if got := syncStartMode(tc.direction, tc.resetWorkspace, tc.targetReset, tc.hasConfigState, tc.configChanged, tc.active); got != tc.want { + if got := syncStartMode(tc.resetWorkspace, tc.targetReset, tc.hasConfigState, tc.configChanged, tc.active); got != tc.want { t.Fatalf("%s: got %q want %q", tc.name, got, tc.want) } } } func TestSyncPairsSummary(t *testing.T) { - if got := syncPairsSummary(nil, "->"); got != "no paths" { + if got := syncPairsSummary(nil); got != "no paths" { t.Fatalf("unexpected empty summary %q", got) } - pairs := []syncengine.Pair{{Local: ".", Remote: "/remote"}} - if got := syncPairsSummary(pairs, "->"); !strings.Contains(got, "/remote") || !strings.Contains(got, "->") { + // A single mapping renders its own direction arrow. + pairs := []syncengine.Pair{{Local: ".", Remote: "/remote", Direction: "down"}} + if got := syncPairsSummary(pairs); !strings.Contains(got, "/remote") || !strings.Contains(got, "<-") { t.Fatalf("unexpected single-pair summary %q", got) } pairs = append(pairs, syncengine.Pair{Local: "./two", Remote: "/two"}) - if got := syncPairsSummary(pairs, "->"); got != "2 paths" { + if got := syncPairsSummary(pairs); got != "2 paths" { t.Fatalf("unexpected multi-pair summary %q", got) } } +func TestUpSyncModeSymbol(t *testing.T) { + single := []syncengine.Pair{{Local: ".", Remote: "/w", Direction: "up"}} + if got := upSyncModeSymbol(single); got != "->" { + t.Fatalf("expected single-pair direction symbol, got %q", got) + } + multi := append(single, syncengine.Pair{Local: "./r", Remote: "/r", Direction: "down"}) + if got := upSyncModeSymbol(multi); got != "<->" { + t.Fatalf("expected bidirectional summary symbol for multi, got %q", got) + } +} + func TestUpUIPrintReadyCard(t *testing.T) { var out bytes.Buffer ui := &upUI{out: &out, errOut: &out} diff --git a/internal/config/config.go b/internal/config/config.go index fab5dbb5..d62431da 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -443,9 +443,6 @@ func (d *DevEnvironment) Validate() error { if err := validateSyncPaths(d.Spec.Sync.Paths); err != nil { return err } - if len(d.Spec.Sync.Paths) > 1 { - return errors.New("spec.sync.paths must contain at most one mapping when engine is syncthing") - } if err := validatePortMappings(d.Spec.Ports); err != nil { return err } @@ -685,9 +682,49 @@ func validateSyncPaths(paths []SyncPathSpec) error { return fmt.Errorf("spec.sync.paths entry %q direction must be one of %s, %s, %s", p.Local+":"+p.Remote, SyncDirectionBi, SyncDirectionUp, SyncDirectionDown) } } + // Mappings must be disjoint on both sides. Two syncthing folders on the + // same root are impossible (.stignore lives in the directory, so both + // folders would share ignore rules), and nested roots would sync the + // nested subtree through two folders at once — reintroducing the + // overwrite hazards per-path directions exist to prevent. The check is + // lexical (no symlink resolution). + for i := 0; i < len(paths); i++ { + for j := i + 1; j < len(paths); j++ { + if syncRootsOverlap(paths[i].Local, paths[j].Local) { + return fmt.Errorf("spec.sync.paths local roots %q and %q overlap; mappings must be disjoint", paths[i].Local, paths[j].Local) + } + if syncRootsOverlap(paths[i].Remote, paths[j].Remote) { + return fmt.Errorf("spec.sync.paths remote roots %q and %q overlap; mappings must be disjoint", paths[i].Remote, paths[j].Remote) + } + } + } return nil } +// syncRootsOverlap reports whether two roots are equal or one contains the +// other, comparing cleaned paths lexically. +func syncRootsOverlap(a, b string) bool { + a = filepath.Clean(strings.TrimSpace(a)) + b = filepath.Clean(strings.TrimSpace(b)) + if a == b { + return true + } + return pathContains(a, b) || pathContains(b, a) +} + +func pathContains(parent, child string) bool { + rel, err := filepath.Rel(parent, child) + if err != nil { + // Mixed absolute/relative roots cannot be compared lexically; + // treat them as disjoint. + return false + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return false + } + return true +} + func validatePortMappings(ports []PortMapping) error { usedLocal := map[int]struct{}{} usedRemote := map[int]struct{}{} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 87adc648..ac830b17 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -422,11 +422,47 @@ func TestValidateRejectsInvalidSyncPath(t *testing.T) { } } -func TestValidateRejectsSyncthingMultiplePaths(t *testing.T) { +func TestValidateAllowsDisjointMultiplePaths(t *testing.T) { cfg := validConfig() - cfg.Spec.Sync.Paths = []SyncPathSpec{{Local: "./a", Remote: "/workspace/a"}, {Local: "./b", Remote: "/workspace/b"}} - if err := cfg.Validate(); err == nil { - t.Fatal("expected validation error") + cfg.Spec.Sync.Paths = []SyncPathSpec{ + {Local: "./a", Remote: "/workspace/a"}, + {Local: "./b", Remote: "/data/results", Direction: "down"}, + } + cfg.SetDefaults() + if err := cfg.Validate(); err != nil { + t.Fatalf("disjoint multi-path mappings should validate, got %v", err) + } +} + +func TestValidateRejectsOverlappingSyncPaths(t *testing.T) { + cases := []struct { + name string + paths []SyncPathSpec + }{ + {"equal locals", []SyncPathSpec{{Local: ".", Remote: "/a"}, {Local: "./", Remote: "/b"}}}, + {"nested locals", []SyncPathSpec{{Local: ".", Remote: "/a"}, {Local: "./results", Remote: "/b"}}}, + {"nested locals reversed", []SyncPathSpec{{Local: "./results/deep", Remote: "/a"}, {Local: "./results", Remote: "/b"}}}, + {"equal remotes", []SyncPathSpec{{Local: "./a", Remote: "/workspace"}, {Local: "./b", Remote: "/workspace/"}}}, + {"nested remotes", []SyncPathSpec{{Local: "./a", Remote: "/workspace"}, {Local: "./b", Remote: "/workspace/results"}}}, + } + for _, tc := range cases { + cfg := validConfig() + cfg.Spec.Sync.Paths = tc.paths + cfg.SetDefaults() + if err := cfg.Validate(); err == nil { + t.Fatalf("%s: expected overlap rejection", tc.name) + } + } +} + +func TestSyncRootsOverlapLexicalEdges(t *testing.T) { + // Sibling with a shared name prefix is NOT overlap; mixed abs/relative + // roots cannot be compared lexically and are treated as disjoint. + if syncRootsOverlap("/data/a", "/data/ab") { + t.Fatal("prefix sibling must not count as overlap") + } + if syncRootsOverlap(".", "/data") { + t.Fatal("mixed abs/relative must be treated as disjoint") } } diff --git a/scripts/e2e_kind_smoke.sh b/scripts/e2e_kind_smoke.sh index bf6dfde6..aed6e30f 100644 --- a/scripts/e2e_kind_smoke.sh +++ b/scripts/e2e_kind_smoke.sh @@ -600,6 +600,93 @@ if [[ "$REMOTE_RESULT_AFTER" != "result-data" ]]; then fi echo "sync direction down verified" +# --- Multiple sync mappings: disjoint folders with per-path directions ---- +# The session currently has one mapping (direction: down). Add a second +# disjoint mapping and verify: overlapping mappings are rejected by +# validation, the extra folder syncs in its own direction after `okdev up` +# (config-hash driven), and the primary mapping keeps working. + +echo "Testing overlapping sync mappings are rejected" +cp "$CFG_PATH" "$CFG_PATH.bak" +OVERLAP_ENTRY="- local: \"$SYNC_DIR\" + remote: /workspace + direction: down + - local: \"$SYNC_DIR/nested\" + remote: /data/nested" +replace_first_in_file "$CFG_PATH" "- local: \"$SYNC_DIR\" + remote: /workspace + direction: down" "$OVERLAP_ENTRY" +set +e +VALIDATE_OUTPUT=$("$OKDEV_BIN" --config "$CFG_PATH" validate 2>&1) +VALIDATE_STATUS=$? +set -e +if [[ "$VALIDATE_STATUS" -eq 0 ]] || [[ "$VALIDATE_OUTPUT" != *"overlap"* ]]; then + echo "ERROR: expected okdev validate to reject overlapping mappings (status=$VALIDATE_STATUS)" >&2 + echo "$VALIDATE_OUTPUT" >&2 + exit 1 +fi +cp "$CFG_PATH.bak" "$CFG_PATH" +echo "overlap rejection verified" + +echo "Adding a second disjoint mapping (code up, results down)" +COLLECT_DIR="$WORKDIR/collected" +mkdir -p "$COLLECT_DIR" +MULTI_ENTRY="- local: \"$SYNC_DIR\" + remote: /workspace + direction: up + - local: \"$COLLECT_DIR\" + remote: /data/results + direction: down" +replace_first_in_file "$CFG_PATH" "- local: \"$SYNC_DIR\" + remote: /workspace + direction: down" "$MULTI_ENTRY" +if ! grep -q "/data/results" "$CFG_PATH"; then + echo "ERROR: failed to add second sync mapping; config is:" >&2 + cat "$CFG_PATH" >&2 + exit 1 +fi +"$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" up --wait-timeout 5m + +echo "multi-mapping: results folder flows pod -> local" +"$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" exec --no-tty --no-prefix -- sh -lc 'mkdir -p /data/results && echo multi-result > /data/results/multi.txt' +MULTI_DOWN_OK=false +for i in $(seq 1 30); do + if [[ "$(cat "$COLLECT_DIR/multi.txt" 2>/dev/null)" == "multi-result" ]]; then + MULTI_DOWN_OK=true + break + fi + sleep 2 +done +if [[ "$MULTI_DOWN_OK" != true ]]; then + echo "ERROR: second mapping did not sync pod result to local" >&2 + exit 1 +fi + +echo "multi-mapping: primary code folder still flows local -> pod" +echo "multi-code-probe" > "$SYNC_DIR/multi-code.txt" +MULTI_UP_OK=false +for i in $(seq 1 30); do + MULTI_REMOTE=$("$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" exec --no-tty --no-prefix -- sh -lc 'cat /workspace/multi-code.txt 2>/dev/null' || true) + if [[ "$MULTI_REMOTE" == "multi-code-probe" ]]; then + MULTI_UP_OK=true + break + fi + sleep 2 +done +if [[ "$MULTI_UP_OK" != true ]]; then + echo "ERROR: primary mapping stopped syncing local code to pod" >&2 + exit 1 +fi + +echo "multi-mapping: status shows both mappings with their directions" +MULTI_DETAILS=$("$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" status --details) +if [[ "$MULTI_DETAILS" != *"-> /workspace"* ]] || [[ "$MULTI_DETAILS" != *"<- /data/results"* ]]; then + echo "ERROR: expected per-path direction arrows in status --details, got:" >&2 + echo "$MULTI_DETAILS" >&2 + exit 1 +fi +echo "multiple sync mappings verified" + echo "Sync direction tests completed" # --- Detached jobs: runtime-volume logs, process-group stop, sidecar ------- diff --git a/skills/okdev/references/multipod.md b/skills/okdev/references/multipod.md index 7db0ba6c..a7c071f6 100644 --- a/skills/okdev/references/multipod.md +++ b/skills/okdev/references/multipod.md @@ -42,7 +42,9 @@ Multi-pod sync behavior depends on workload layout: Do not assume all pods independently sync from the local machine in the same way. Check the workload pattern first. -Direction: `spec.sync.paths[].direction: bi|up|down` sets the folder direction on the local↔hub edge for that mapping (`down` = pod is the authority, local writes can never clobber pod results). It applies to the mapping's whole folder — finer per-subpath direction is impossible in Syncthing. Mesh receiver pods are always receiveonly regardless. Generated outputs belong outside the synced path either way; fetch them with `okdev cp` or `okdev jobs logs`. +Direction: `spec.sync.paths[].direction: bi|up|down` sets the folder direction on the local↔hub edge for that mapping (`down` = pod is the authority, local writes can never clobber pod results). It applies to the mapping's whole folder — finer per-subpath direction is impossible in Syncthing. Mesh receiver pods are always receiveonly regardless. + +Multiple mappings are supported when their roots are disjoint on both sides (each becomes its own folder with its own direction). Only the FIRST (primary) mapping fans out to mesh receivers; additional mappings are local↔hub only, so sync-based result collection covers the hub pod — fleet-wide results still go through `okdev jobs logs` / `okdev cp`. ## PyTorchJob From 91a476162c164837b1feea698f197b8a9217efd9 Mon Sep 17 00:00:00 2001 From: acmore Date: Sun, 5 Jul 2026 19:43:17 +0800 Subject: [PATCH 2/4] fix(sync): sidecar mirrors target volume mounts; probe shared remote roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kind e2e exposed that additional sync mappings could never work: the remote syncthing runs in the sidecar, which only mounted the workspace volume, so any extra remote root lived on the dev container's overlay — invisible to the sidecar, silently "syncing" into a directory the workload never reads. - PreparePodSpec now mirrors the target container's volume mounts into the sidecar (same paths, dedup against sidecar internals), so any volume-backed remote root is servable by the sidecar's syncthing. - Sync startup probes each additional mapping's remote root: the sidecar writes a marker and the target container must see it; otherwise startup fails with guidance to mount a volume and re-run `okdev up --reconcile` instead of silently splitting the two containers' views. - Docs call out the volume requirement, and the invalid nested-local examples ("./collected" under ".") are corrected to siblings — the disjoint rule applies to the local side too. - Kind e2e adds a results-vol emptyDir mounted at /data and reconciles before asserting the second mapping syncs. Co-Authored-By: Claude Fable 5 --- docs/config-manifest.md | 8 ++- internal/cli/syncthing.go | 39 +++++++++++++- internal/config/config.go | 6 ++- internal/config/config_test.go | 4 +- internal/kube/podspec.go | 35 +++++++++++-- internal/kube/podspec_sidecar_mounts_test.go | 55 ++++++++++++++++++++ scripts/e2e_kind_smoke.sh | 20 +++++-- 7 files changed, 152 insertions(+), 15 deletions(-) create mode 100644 internal/kube/podspec_sidecar_mounts_test.go diff --git a/docs/config-manifest.md b/docs/config-manifest.md index c77c6547..9209ae56 100644 --- a/docs/config-manifest.md +++ b/docs/config-manifest.md @@ -250,14 +250,18 @@ spec: ```yaml paths: - .:/workspace # primary: code, bidirectional - - local: ./collected - remote: /data/results + - local: ../collected # sibling of the repo — local roots must + remote: /data/results # not nest inside the primary mapping direction: down # pod-generated results, pod is authority - local: ../datasets remote: /data/datasets direction: up # dataset push, local is authority ``` +Note the disjoint rule applies to the **local** side too: a results directory *inside* the synced repo (e.g. `./collected` under `.`) is rejected — place it next to the repo instead, or narrow the primary mapping to a subdirectory. + +**Remote roots must live on a volume.** The remote side of an additional mapping must be on a volume mounted in the target container (`spec.volumes` + `podTemplate` `volumeMounts`) — okdev mirrors the target container's volume mounts into the sidecar so its syncthing can serve them. A remote root on the container's own filesystem cannot sync (the sidecar can't see it); sync startup probes this and fails with a clear error. After adding a volume for a new mapping, run `okdev up --reconcile` so the pod is recreated with the sidecar mount. + **Sync direction.** `direction` applies to a mapping's whole synced folder — finer per-subpath direction inside one folder is impossible with Syncthing (ignore rules live in the directory itself; per-path folder types were rejected upstream). Pick the direction by what the folder carries: - `bi` (default): source code you edit on both sides. Either side can overwrite the other, so **keep generated outputs (results, checkpoints, logs) out of the synced path** — write them to a non-synced location and fetch with `okdev cp` or `okdev jobs logs`. Versioning (`.stversions/`) is the safety net, not the design. diff --git a/internal/cli/syncthing.go b/internal/cli/syncthing.go index 67b44471..a1aaedd1 100644 --- a/internal/cli/syncthing.go +++ b/internal/cli/syncthing.go @@ -73,6 +73,7 @@ var ( runTwoPhaseInitialSyncFn = runTwoPhaseInitialSync configureSyncthingPeerFn = configureSyncthingPeer pruneManagedSessionFoldersFn = pruneManagedSessionFolders + verifyRemoteRootSharedFn = verifyRemoteRootShared saveSyncthingConfigHashFn = saveSyncthingConfigHash stopOwnedLocalSyncthingFn = stopOwnedLocalSyncthing runSyncHealthLoopFn = runSyncHealthLoop @@ -171,7 +172,7 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm if err != nil { return err } - for _, folder := range folders { + for i, folder := range folders { if err := os.MkdirAll(folder.absLocal, 0o755); err != nil { return err } @@ -181,6 +182,17 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm if _, err := execInSyncthingContainerFn(bootstrapCtx, k, namespace, pod, fmt.Sprintf("mkdir -p %s", syncengine.ShellEscape(folder.remote))); err != nil { return err } + // The sidecar's syncthing can only serve paths it can see: an + // additional mapping whose remote root lives on the target + // container's overlay filesystem would "sync" into a directory the + // workload never reads. Fail fast instead of silently splitting + // the two containers' views. The primary workspace mount is + // managed by okdev and needs no probe. + if i > 0 { + if err := verifyRemoteRootSharedFn(bootstrapCtx, k, namespace, pod, target.Container, folder); err != nil { + return err + } + } } primary := folders[0] localHome, err := localSyncthingHome(sessionName) @@ -2137,6 +2149,31 @@ func configureSyncthingPeer(ctx context.Context, base, key, selfID, peerID, peer // managed shapes "okdev-" and "okdev--<8 hex>" are // touched, so user-defined folders (and other sessions' folders, whose // session name would make a longer prefix) are never removed. +// verifyRemoteRootShared checks that a mapping's remote root is on a volume +// shared between the okdev sidecar and the target container: the sidecar +// writes a probe file and the target container must see it. A root on +// either container's private overlay fails the probe. +func verifyRemoteRootShared(ctx context.Context, k *kube.Client, namespace, pod, targetContainer string, folder syncFolder) error { + if k == nil { + return nil + } + marker := folder.remote + "/.okdev-sync-probe-" + folder.id + if _, err := execInSyncthingContainerFn(ctx, k, namespace, pod, fmt.Sprintf("echo shared > %s", syncengine.ShellEscape(marker))); err != nil { + return fmt.Errorf("write shared-volume probe for %s: %w", folder.remote, err) + } + defer func() { + _, _ = execInSyncthingContainerFn(ctx, k, namespace, pod, fmt.Sprintf("rm -f %s", syncengine.ShellEscape(marker))) + }() + out, err := k.ExecShInContainer(ctx, namespace, pod, targetContainer, fmt.Sprintf("cat %s 2>/dev/null || true", syncengine.ShellEscape(marker))) + if err != nil { + return fmt.Errorf("probe remote root %s from target container: %w", folder.remote, err) + } + if !strings.Contains(string(out), "shared") { + return fmt.Errorf("sync mapping remote root %s is not on a volume shared with the okdev sidecar; mount it via spec.volumes + podTemplate volumeMounts and re-run `okdev up --reconcile` so the sidecar picks it up", folder.remote) + } + return nil +} + func pruneManagedSessionFolders(ctx context.Context, base, key, sessionName string, keep map[string]bool) error { cfg, err := syncthingGetConfig(ctx, base, key) if err != nil { diff --git a/internal/config/config.go b/internal/config/config.go index d62431da..2f942012 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -161,10 +161,14 @@ type SyncSpec struct { // // paths: // - .:/workspace -// - local: ./results +// - local: ../collected // remote: /data/results // direction: down // +// Mapping roots must be pairwise disjoint on both sides (validated): note +// "../collected" above — a results directory nested inside the primary +// mapping's local root would be rejected. +// // Direction sets the syncthing folder types for this mapping's folder: // // bi — both sides sendreceive (default; either side can overwrite the diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ac830b17..2bc35358 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -275,7 +275,7 @@ func TestSyncPathSpecYAMLForms(t *testing.T) { // Compact string form and structured form must both decode; the string // form keeps direction empty (= bi). var spec SyncSpec - raw := []byte("paths:\n - .:/workspace\n - local: ./results\n remote: /data/results\n direction: down\n") + raw := []byte("paths:\n - .:/workspace\n - local: ../collected\n remote: /data/results\n direction: down\n") if err := sigsyaml.Unmarshal(raw, &spec); err != nil { t.Fatalf("unmarshal: %v", err) } @@ -285,7 +285,7 @@ func TestSyncPathSpecYAMLForms(t *testing.T) { if spec.Paths[0].Local != "." || spec.Paths[0].Remote != "/workspace" || spec.Paths[0].Direction != "" { t.Fatalf("unexpected compact entry: %+v", spec.Paths[0]) } - if spec.Paths[1].Local != "./results" || spec.Paths[1].Remote != "/data/results" || spec.Paths[1].Direction != "down" { + if spec.Paths[1].Local != "../collected" || spec.Paths[1].Remote != "/data/results" || spec.Paths[1].Direction != "down" { t.Fatalf("unexpected structured entry: %+v", spec.Paths[1]) } diff --git a/internal/kube/podspec.go b/internal/kube/podspec.go index 56cbf0ff..5d242770 100644 --- a/internal/kube/podspec.go +++ b/internal/kube/podspec.go @@ -110,11 +110,7 @@ func PreparePodSpecForTargetWithShell(podSpec corev1.PodSpec, volumes []corev1.V {ContainerPort: 8384, Name: "st-gui"}, {ContainerPort: 22000, Name: "st-sync"}, }, - VolumeMounts: []corev1.VolumeMount{ - workspaceMount, - {Name: "syncthing-home", MountPath: "/var/syncthing"}, - {Name: "okdev-runtime", MountPath: "/var/okdev"}, - }, + VolumeMounts: sidecarVolumeMounts(workspaceMount, targetIndex, spec.Containers), Env: []corev1.EnvVar{ { Name: "OKDEV_WORKSPACE", @@ -209,6 +205,35 @@ func workspaceMountForSidecar(mounts []corev1.VolumeMount, fallback corev1.Volum return fallback } +// sidecarVolumeMounts assembles the sidecar's mounts: its internal volumes, +// the workspace, plus every volume the target container mounts (at the same +// paths). Mirroring the target's mounts is what lets sync mappings target +// volume-backed remote roots outside the workspace — the sidecar's syncthing +// can only serve paths it can see; anything on the target container's +// overlay filesystem is invisible to it. +func sidecarVolumeMounts(workspaceMount corev1.VolumeMount, targetIndex int, containers []corev1.Container) []corev1.VolumeMount { + mounts := []corev1.VolumeMount{ + workspaceMount, + {Name: "syncthing-home", MountPath: "/var/syncthing"}, + {Name: "okdev-runtime", MountPath: "/var/okdev"}, + } + if targetIndex < 0 || targetIndex >= len(containers) { + return mounts + } + taken := make(map[string]bool, len(mounts)) + for _, m := range mounts { + taken[m.MountPath] = true + } + for _, m := range containers[targetIndex].VolumeMounts { + if taken[m.MountPath] { + continue + } + taken[m.MountPath] = true + mounts = append(mounts, m) + } + return mounts +} + func ensureEnvVar(envs []corev1.EnvVar, env corev1.EnvVar) []corev1.EnvVar { for i := range envs { if envs[i].Name == env.Name { diff --git a/internal/kube/podspec_sidecar_mounts_test.go b/internal/kube/podspec_sidecar_mounts_test.go new file mode 100644 index 00000000..0763e825 --- /dev/null +++ b/internal/kube/podspec_sidecar_mounts_test.go @@ -0,0 +1,55 @@ +package kube + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" +) + +func TestPreparePodSpecMirrorsTargetMountsIntoSidecar(t *testing.T) { + // Sync mappings can target volume-backed remote roots outside the + // workspace only if the sidecar mounts the same volumes: its syncthing + // cannot serve paths on the target container's overlay filesystem. + spec := corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "dev", + Image: "ubuntu:22.04", + VolumeMounts: []corev1.VolumeMount{ + {Name: "results-vol", MountPath: "/data"}, + {Name: "datasets", MountPath: "/data2", SubPath: "sets"}, + }, + }}, + } + volumes := []corev1.Volume{ + {Name: "results-vol", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + {Name: "datasets", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + } + prepared, err := PreparePodSpec(spec, volumes, "/workspace", "sidecar:test", corev1.ResourceRequirements{}, false, "") + if err != nil { + t.Fatalf("PreparePodSpec: %v", err) + } + var sidecar *corev1.Container + for i := range prepared.Containers { + if prepared.Containers[i].Name == "okdev-sidecar" { + sidecar = &prepared.Containers[i] + } + } + if sidecar == nil { + t.Fatal("sidecar container missing") + } + byPath := map[string]corev1.VolumeMount{} + for _, m := range sidecar.VolumeMounts { + if _, dup := byPath[m.MountPath]; dup { + t.Fatalf("duplicate sidecar mount path %q", m.MountPath) + } + byPath[m.MountPath] = m + } + for _, want := range []string{"/workspace", "/var/syncthing", "/var/okdev", "/data", "/data2"} { + if _, ok := byPath[want]; !ok { + t.Fatalf("expected sidecar mount at %q, got %+v", want, sidecar.VolumeMounts) + } + } + if byPath["/data2"].SubPath != "sets" { + t.Fatalf("expected subPath preserved on mirrored mount, got %+v", byPath["/data2"]) + } +} diff --git a/scripts/e2e_kind_smoke.sh b/scripts/e2e_kind_smoke.sh index aed6e30f..98109f1f 100644 --- a/scripts/e2e_kind_smoke.sh +++ b/scripts/e2e_kind_smoke.sh @@ -628,9 +628,21 @@ fi cp "$CFG_PATH.bak" "$CFG_PATH" echo "overlap rejection verified" -echo "Adding a second disjoint mapping (code up, results down)" +echo "Adding a second disjoint mapping (code up, results down) on a shared volume" COLLECT_DIR="$WORKDIR/collected" mkdir -p "$COLLECT_DIR" +# The extra mapping's remote root must live on a volume shared with the +# sidecar (its syncthing cannot see the dev container's overlay), so add a +# volume + mount for /data and reconcile the pod — the sidecar mirrors the +# target container's volume mounts. +replace_first_in_file "$CFG_PATH" " podTemplate:" " volumes: + - name: results-vol + emptyDir: {} + podTemplate:" +replace_first_in_file "$CFG_PATH" " command: [\"sleep\", \"infinity\"]" " command: [\"sleep\", \"infinity\"] + volumeMounts: + - name: results-vol + mountPath: /data" MULTI_ENTRY="- local: \"$SYNC_DIR\" remote: /workspace direction: up @@ -640,12 +652,12 @@ MULTI_ENTRY="- local: \"$SYNC_DIR\" replace_first_in_file "$CFG_PATH" "- local: \"$SYNC_DIR\" remote: /workspace direction: down" "$MULTI_ENTRY" -if ! grep -q "/data/results" "$CFG_PATH"; then - echo "ERROR: failed to add second sync mapping; config is:" >&2 +if ! grep -q "/data/results" "$CFG_PATH" || ! grep -q "results-vol" "$CFG_PATH"; then + echo "ERROR: failed to add volume or second sync mapping; config is:" >&2 cat "$CFG_PATH" >&2 exit 1 fi -"$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" up --wait-timeout 5m +"$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" up --reconcile --wait-timeout 5m echo "multi-mapping: results folder flows pod -> local" "$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" exec --no-tty --no-prefix -- sh -lc 'mkdir -p /data/results && echo multi-result > /data/results/multi.txt' From 33743126e155e1769dd337ba6b6e77bd9b01dc5e Mon Sep 17 00:00:00 2001 From: acmore Date: Sun, 5 Jul 2026 20:03:31 +0800 Subject: [PATCH 3/4] fix(sync): write bootstrap-complete marker even when no folder ran two-phase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `okdev up` waits for the bootstrap-complete handoff whenever it starts sync in two-phase mode, but with only directional mappings no folder runs the two-phase protocol, so the marker was never written and up hung until its handoff timeout (caught by the kind e2e's reconcile with up+down mappings). The marker signals "bootstrap phase concluded", not "two-phase ran" — write it whenever the start mode was two-phase. Co-Authored-By: Claude Fable 5 --- internal/cli/syncthing.go | 9 +- internal/cli/syncthing_multifolder_test.go | 125 +++++++++++++++++++++ 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/internal/cli/syncthing.go b/internal/cli/syncthing.go index a1aaedd1..ebcc9533 100644 --- a/internal/cli/syncthing.go +++ b/internal/cli/syncthing.go @@ -249,7 +249,6 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm return fmt.Errorf("mark syncthing ready: %w", err) } - ranTwoPhase := false for _, folder := range folders { if folder.twoPhase { // The bootstrap context has a short timeout @@ -261,7 +260,6 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm if err != nil { return fmt.Errorf("two-phase initial sync (%s): %w", folder.id, err) } - ranTwoPhase = true continue } folderTypeLocal, folderTypeRemote := folderTypesForMode(folder.mode) @@ -272,7 +270,12 @@ func runSyncthingSync(cmd *cobra.Command, opts *Options, cfg *config.DevEnvironm return fmt.Errorf("configure remote syncthing (%s): %w", folder.id, err) } } - if ranTwoPhase { + if mode == "two-phase" { + // The marker signals that the bootstrap phase concluded — `okdev up` + // waits on this handoff whenever it requested a bootstrap. It must + // be written even when no folder actually ran the two-phase + // protocol (all mappings directional), or the caller hangs until + // its handoff timeout. if err := markSyncthingBootstrapCompleteFn(sessionName); err != nil { return fmt.Errorf("mark syncthing bootstrap complete: %w", err) } diff --git a/internal/cli/syncthing_multifolder_test.go b/internal/cli/syncthing_multifolder_test.go index 0a623ae2..a81e255b 100644 --- a/internal/cli/syncthing_multifolder_test.go +++ b/internal/cli/syncthing_multifolder_test.go @@ -1,10 +1,19 @@ package cli import ( + "context" + "io" + "os" + "path/filepath" "strings" "testing" + "time" + "github.com/acmore/okdev/internal/config" + "github.com/acmore/okdev/internal/kube" syncengine "github.com/acmore/okdev/internal/sync" + "github.com/acmore/okdev/internal/workload" + "github.com/spf13/cobra" ) func TestResolveSyncFoldersPerPathAndOverride(t *testing.T) { @@ -117,3 +126,119 @@ func TestPruneManagedFoldersFromConfig(t *testing.T) { t.Fatalf("expected no-op prune, changed=%v err=%v", changed, err) } } + +func TestRunSyncthingSyncMarksBootstrapCompleteWithoutBiFolders(t *testing.T) { + // Regression: `okdev up` waits for the bootstrap-complete handoff + // whenever it starts sync in two-phase mode. With only directional + // mappings no folder runs the two-phase protocol, but the marker must + // still be written or the caller hangs until its handoff timeout. + home := t.TempDir() + t.Setenv("HOME", home) + + origEnsureBinary := syncthingEnsureBinaryFn + origResolveTargetRef := resolveTargetRefFn + origExec := execInSyncthingContainerFn + origPF := startSyncthingPortForwardFn + origReadKey := readRemoteSyncthingAPIKeyFn + origStartLocal := startAndWaitForLocalSyncthingFn + origWaitAPI := waitSyncthingAPIFn + origDeviceID := syncthingDeviceIDFn + origMarkReady := markSyncthingReadyFn + origMarkComplete := markSyncthingBootstrapCompleteFn + origTwoPhase := runTwoPhaseInitialSyncFn + origConfigure := configureSyncthingPeerFn + origPrune := pruneManagedSessionFoldersFn + origVerify := verifyRemoteRootSharedFn + origSaveHash := saveSyncthingConfigHashFn + origStopOwned := stopOwnedLocalSyncthingFn + origHealth := runSyncHealthLoopFn + t.Cleanup(func() { + syncthingEnsureBinaryFn = origEnsureBinary + resolveTargetRefFn = origResolveTargetRef + execInSyncthingContainerFn = origExec + startSyncthingPortForwardFn = origPF + readRemoteSyncthingAPIKeyFn = origReadKey + startAndWaitForLocalSyncthingFn = origStartLocal + waitSyncthingAPIFn = origWaitAPI + syncthingDeviceIDFn = origDeviceID + markSyncthingReadyFn = origMarkReady + markSyncthingBootstrapCompleteFn = origMarkComplete + runTwoPhaseInitialSyncFn = origTwoPhase + configureSyncthingPeerFn = origConfigure + pruneManagedSessionFoldersFn = origPrune + verifyRemoteRootSharedFn = origVerify + saveSyncthingConfigHashFn = origSaveHash + stopOwnedLocalSyncthingFn = origStopOwned + runSyncHealthLoopFn = origHealth + }) + + syncthingEnsureBinaryFn = func(context.Context, string, bool) (string, error) { return "/tmp/syncthing", nil } + resolveTargetRefFn = func(context.Context, *Options, *config.DevEnvironment, string, string, targetResolverClient) (workload.TargetRef, error) { + return workload.TargetRef{PodName: "pod-0"}, nil + } + execInSyncthingContainerFn = func(context.Context, interface { + ExecShInContainer(context.Context, string, string, string, string) ([]byte, error) + }, string, string, string) ([]byte, error) { + return nil, nil + } + startSyncthingPortForwardFn = func(context.Context, *Options, string, string) (context.CancelFunc, string, string, error) { + return func() {}, "http://remote", "tcp://127.0.0.1:22000", nil + } + readRemoteSyncthingAPIKeyFn = func(context.Context, interface { + ExecShInContainer(context.Context, string, string, string, string) ([]byte, error) + }, string, string) (string, error) { + return "remote-key", nil + } + startAndWaitForLocalSyncthingFn = func(context.Context, string, string) (string, string, int, error) { + return "http://local", "local-key", 4321, nil + } + waitSyncthingAPIFn = func(context.Context, string, string, time.Duration) error { return nil } + syncthingDeviceIDFn = func(_ context.Context, base, _ string) (string, error) { + if base == "http://local" { + return "LOCAL", nil + } + return "REMOTE", nil + } + markSyncthingReadyFn = func(string) error { return nil } + twoPhaseCalls := 0 + runTwoPhaseInitialSyncFn = func(context.Context, io.Writer, string, string, string, string, string, string, string, string, string, string, syncthingSyncConfig) error { + twoPhaseCalls++ + return nil + } + configureSyncthingPeerFn = func(context.Context, string, string, string, string, string, string, string, string, int, int, bool, bool, bool, int) error { + return nil + } + pruneManagedSessionFoldersFn = func(context.Context, string, string, string, map[string]bool) error { return nil } + verifyRemoteRootSharedFn = func(context.Context, *kube.Client, string, string, string, syncFolder) error { return nil } + saveSyncthingConfigHashFn = func(string, string) error { return nil } + stopOwnedLocalSyncthingFn = func(string, int) error { return nil } + runSyncHealthLoopFn = func(<-chan os.Signal, io.Writer, syncHealthChecker) {} + completeCalls := 0 + markSyncthingBootstrapCompleteFn = func(string) error { + completeCalls++ + return nil + } + + localA := filepath.Join(home, "code") + localB := filepath.Join(home, "collected") + cfg := &config.DevEnvironment{} + cfg.SetDefaults() + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := runSyncthingSync(cmd, &Options{}, cfg, "default", "sess-directional", "two-phase", []syncengine.Pair{ + {Local: localA, Remote: "/workspace", Direction: "up"}, + {Local: localB, Remote: "/data/results", Direction: "down"}, + }, nil) + if err != nil { + t.Fatalf("runSyncthingSync: %v", err) + } + if twoPhaseCalls != 0 { + t.Fatalf("directional folders must not run the two-phase protocol, got %d calls", twoPhaseCalls) + } + if completeCalls != 1 { + t.Fatalf("bootstrap-complete marker must be written exactly once, got %d", completeCalls) + } +} From 753f006f94f064b6f7d55f5a932c589f250bd1a1 Mon Sep 17 00:00:00 2001 From: acmore Date: Sun, 5 Jul 2026 20:44:25 +0800 Subject: [PATCH 4/4] feat(sync): auto-provision volumes for additional sync mapping remote roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring users to hand-write spec.volumes + podTemplate volumeMounts for every extra sync mapping was needless friction. okdev now provisions an emptyDir automatically at any additional remote root not already covered by a target-container volume mount (a user PVC mount at or above the root is respected and used instead), and the sidecar mirror makes it servable by the remote syncthing. Users configure mappings with paths only. The additional remote roots participate in the workload snapshot (omitempty — single-mapping sessions keep the legacy hash), so adding or removing a mapping surfaces as drift and the familiar `up --reconcile` flow recreates the pod with the new volume. The shared- volume startup probe remains as the safety net. Kind e2e drops the manual volume YAML, asserts plain `up` surfaces reconcile guidance after adding a mapping, and reconciles into a working two-direction setup. Co-Authored-By: Claude Fable 5 --- docs/config-manifest.md | 2 +- internal/cli/workload.go | 3 + internal/config/config.go | 18 ++++++ internal/config/snapshot.go | 7 +++ internal/config/snapshot_sync_roots_test.go | 30 ++++++++++ internal/kube/podspec.go | 48 +++++++++++++++ internal/kube/podspec_sidecar_mounts_test.go | 63 ++++++++++++++++++++ internal/workload/generic.go | 3 +- internal/workload/pod.go | 3 +- scripts/e2e_kind_smoke.sh | 33 +++++----- 10 files changed, 192 insertions(+), 18 deletions(-) create mode 100644 internal/config/snapshot_sync_roots_test.go diff --git a/docs/config-manifest.md b/docs/config-manifest.md index 9209ae56..464c5560 100644 --- a/docs/config-manifest.md +++ b/docs/config-manifest.md @@ -260,7 +260,7 @@ paths: Note the disjoint rule applies to the **local** side too: a results directory *inside* the synced repo (e.g. `./collected` under `.`) is rejected — place it next to the repo instead, or narrow the primary mapping to a subdirectory. -**Remote roots must live on a volume.** The remote side of an additional mapping must be on a volume mounted in the target container (`spec.volumes` + `podTemplate` `volumeMounts`) — okdev mirrors the target container's volume mounts into the sidecar so its syncthing can serve them. A remote root on the container's own filesystem cannot sync (the sidecar can't see it); sync startup probes this and fails with a clear error. After adding a volume for a new mapping, run `okdev up --reconcile` so the pod is recreated with the sidecar mount. +**Volumes are provisioned automatically.** An additional mapping's remote root must be visible to the okdev sidecar (its syncthing serves the remote side), which means it must live on a volume — a root on the container's own filesystem cannot sync. You don't need to write any volume YAML: okdev auto-provisions an emptyDir at any extra remote root not already covered by a target-container volume mount, and mirrors the target container's mounts into the sidecar. Adding or removing a mapping changes the pod spec, so the next `okdev up` asks for `--reconcile` (pod recreate). Auto-provisioned emptyDirs don't survive pod recreation — fine for both directions (`up` re-pushes from local, `down` results are mirrored locally); mount your own PVC at the root if the data must persist, and okdev will use it instead. Sync startup still probes each extra root and fails with a clear error if it is not shared with the sidecar. **Sync direction.** `direction` applies to a mapping's whole synced folder — finer per-subpath direction inside one folder is impossible with Syncthing (ignore rules live in the directory itself; per-path folder types were rejected upstream). Pick the direction by what the folder carries: diff --git a/internal/cli/workload.go b/internal/cli/workload.go index 213efae3..597f6c34 100644 --- a/internal/cli/workload.go +++ b/internal/cli/workload.go @@ -32,6 +32,7 @@ func sessionRuntime(cfg *config.DevEnvironment, cfgPath, sessionName, workloadNa snapJSON, _ := snap.JSON() snapHash, _ := snap.SHA256() + syncRemoteRoots := cfg.AdditionalSyncRemoteRoots() switch strings.TrimSpace(cfg.Spec.Workload.Type) { case "", workload.TypePod: rt := workload.NewPodRuntime( @@ -40,6 +41,7 @@ func sessionRuntime(cfg *config.DevEnvironment, cfgPath, sessionName, workloadNa tmux, cfg.Spec.SSH.Shell, preStop, targetContainer, ) rt.WorkloadNameOverride = workloadName + rt.SyncRemoteRoots = syncRemoteRoots rt.LastAppliedSpecJSON = snapJSON rt.LastAppliedSpecHash = snapHash return rt, nil @@ -57,6 +59,7 @@ func sessionRuntime(cfg *config.DevEnvironment, cfgPath, sessionName, workloadNa PreStop: preStop, TargetContainer: targetContainer, Volumes: volumes, + SyncRemoteRoots: syncRemoteRoots, Labels: labels, Annotations: annotations, Inject: cfg.EffectiveWorkloadInject(), diff --git a/internal/config/config.go b/internal/config/config.go index 2f942012..ce944897 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "regexp" + "sort" "strings" agentcatalog "github.com/acmore/okdev/internal/agent" @@ -232,6 +233,23 @@ func (p SyncPathSpec) EffectiveDirection() string { return d } +// AdditionalSyncRemoteRoots returns the remote roots of every mapping after +// the primary one, sorted for stable hashing. These roots may need +// auto-provisioned volumes so the sidecar's syncthing can serve them. +func (d *DevEnvironment) AdditionalSyncRemoteRoots() []string { + if len(d.Spec.Sync.Paths) <= 1 { + return nil + } + roots := make([]string, 0, len(d.Spec.Sync.Paths)-1) + for _, p := range d.Spec.Sync.Paths[1:] { + if root := strings.TrimSpace(p.Remote); root != "" { + roots = append(roots, root) + } + } + sort.Strings(roots) + return roots +} + type SyncthingSpec struct { Version string `yaml:"version"` AutoInstall *bool `yaml:"autoInstall"` diff --git a/internal/config/snapshot.go b/internal/config/snapshot.go index 05fcd6cb..5b3a2509 100644 --- a/internal/config/snapshot.go +++ b/internal/config/snapshot.go @@ -30,6 +30,12 @@ type LastAppliedWorkloadSpec struct { PreStop string `json:"preStop"` ManifestPath string `json:"manifestPath,omitempty"` ManifestSHA256 string `json:"manifestSHA256,omitempty"` + // SyncRemoteRoots are the remote roots of additional sync mappings. + // They shape the pod spec (auto-provisioned volumes + sidecar mounts), + // so adding or removing a mapping must surface as workload drift and + // prompt a reconcile. omitempty keeps single-mapping sessions on the + // legacy snapshot hash. + SyncRemoteRoots []string `json:"syncRemoteRoots,omitempty"` } func BuildWorkloadSnapshot(cfg *DevEnvironment, workspaceMountPath, targetContainer string, tmux bool, shell string, preStop, manifestPath, manifestResolvedPath string) LastAppliedWorkloadSpec { @@ -53,6 +59,7 @@ func BuildWorkloadSnapshot(cfg *DevEnvironment, workspaceMountPath, targetContai Shell: shell, PreStop: preStop, ManifestPath: manifestPath, + SyncRemoteRoots: cfg.AdditionalSyncRemoteRoots(), } if manifestResolvedPath != "" { hash, err := ComputeManifestSHA256(manifestResolvedPath) diff --git a/internal/config/snapshot_sync_roots_test.go b/internal/config/snapshot_sync_roots_test.go new file mode 100644 index 00000000..1c2000c9 --- /dev/null +++ b/internal/config/snapshot_sync_roots_test.go @@ -0,0 +1,30 @@ +package config + +import "testing" + +func TestWorkloadSnapshotTracksAdditionalSyncRoots(t *testing.T) { + cfg := &DevEnvironment{} + cfg.SetDefaults() + cfg.Spec.Sync.Paths = []SyncPathSpec{{Local: ".", Remote: "/workspace"}} + baseSnap := BuildWorkloadSnapshot(cfg, "/workspace", "dev", false, "", "", "", "") + base, _ := baseSnap.SHA256() + + // Adding a second mapping shapes the pod spec (auto-provisioned + // volume), so the snapshot must change and trigger the reconcile + // prompt. + cfg.Spec.Sync.Paths = append(cfg.Spec.Sync.Paths, SyncPathSpec{Local: "../collected", Remote: "/data/results", Direction: "down"}) + multiSnap := BuildWorkloadSnapshot(cfg, "/workspace", "dev", false, "", "", "", "") + multi, _ := multiSnap.SHA256() + if base == multi { + t.Fatal("expected extra sync mapping to change the workload snapshot") + } + + // Single-mapping sessions keep the legacy snapshot (omitempty), so + // upgrading okdev does not force a reconcile. + cfg.Spec.Sync.Paths = cfg.Spec.Sync.Paths[:1] + againSnap := BuildWorkloadSnapshot(cfg, "/workspace", "dev", false, "", "", "", "") + again, _ := againSnap.SHA256() + if again != base { + t.Fatal("single-mapping snapshot must stay on the legacy hash") + } +} diff --git a/internal/kube/podspec.go b/internal/kube/podspec.go index 5d242770..9bc13e00 100644 --- a/internal/kube/podspec.go +++ b/internal/kube/podspec.go @@ -1,6 +1,8 @@ package kube import ( + "crypto/sha256" + "encoding/hex" "fmt" "regexp" "strings" @@ -19,6 +21,16 @@ func PreparePodSpecForTarget(podSpec corev1.PodSpec, volumes []corev1.Volume, wo } func PreparePodSpecForTargetWithShell(podSpec corev1.PodSpec, volumes []corev1.Volume, workspaceMountPath, sidecarImage string, sidecarResources corev1.ResourceRequirements, tmux bool, preStop string, targetContainer string, shell string) (corev1.PodSpec, error) { + return PreparePodSpecForTargetWithShellAndSyncRoots(podSpec, volumes, workspaceMountPath, sidecarImage, sidecarResources, tmux, preStop, targetContainer, shell, nil) +} + +// PreparePodSpecForTargetWithShellAndSyncRoots additionally auto-provisions +// volumes for sync mapping remote roots: any root not already covered by a +// target-container volume mount gets an emptyDir mounted at that exact path +// (and mirrored into the sidecar), so users configure extra sync mappings +// without writing any volume YAML. Roots covered by an existing mount (for +// example a user PVC) are left alone. +func PreparePodSpecForTargetWithShellAndSyncRoots(podSpec corev1.PodSpec, volumes []corev1.Volume, workspaceMountPath, sidecarImage string, sidecarResources corev1.ResourceRequirements, tmux bool, preStop string, targetContainer string, shell string, syncRemoteRoots []string) (corev1.PodSpec, error) { if strings.TrimSpace(sidecarImage) == "" { return corev1.PodSpec{}, fmt.Errorf("sidecar image cannot be empty") } @@ -94,6 +106,24 @@ func PreparePodSpecForTargetWithShell(podSpec corev1.PodSpec, volumes []corev1.V } } + if targetIndex >= 0 { + for _, root := range syncRemoteRoots { + root = strings.TrimSpace(root) + if root == "" || syncRootCoveredByMounts(root, spec.Containers[targetIndex].VolumeMounts) { + continue + } + name := syncAutoVolumeName(root) + spec.Volumes = ensureVolume(spec.Volumes, corev1.Volume{ + Name: name, + VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}, + }) + spec.Containers[targetIndex].VolumeMounts = ensureVolumeMount(spec.Containers[targetIndex].VolumeMounts, corev1.VolumeMount{ + Name: name, + MountPath: root, + }) + } + } + InjectPreStopForTarget(spec, preStop, targetContainer) if !hasContainer(spec.Containers, "okdev-sidecar") { @@ -196,6 +226,24 @@ func ensureVolumeMount(mounts []corev1.VolumeMount, vm corev1.VolumeMount) []cor return append(mounts, vm) } +// syncRootCoveredByMounts reports whether root already lives under one of +// the container's volume mounts. +func syncRootCoveredByMounts(root string, mounts []corev1.VolumeMount) bool { + for _, m := range mounts { + if root == m.MountPath || strings.HasPrefix(root, m.MountPath+"/") { + return true + } + } + return false +} + +// syncAutoVolumeName derives a stable volume name for an auto-provisioned +// sync remote root. +func syncAutoVolumeName(root string) string { + sum := sha256.Sum256([]byte(root)) + return "okdev-sync-" + hex.EncodeToString(sum[:4]) +} + func workspaceMountForSidecar(mounts []corev1.VolumeMount, fallback corev1.VolumeMount) corev1.VolumeMount { for _, mount := range mounts { if mount.Name == fallback.Name && mount.MountPath == fallback.MountPath { diff --git a/internal/kube/podspec_sidecar_mounts_test.go b/internal/kube/podspec_sidecar_mounts_test.go index 0763e825..055ed684 100644 --- a/internal/kube/podspec_sidecar_mounts_test.go +++ b/internal/kube/podspec_sidecar_mounts_test.go @@ -1,6 +1,7 @@ package kube import ( + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -53,3 +54,65 @@ func TestPreparePodSpecMirrorsTargetMountsIntoSidecar(t *testing.T) { t.Fatalf("expected subPath preserved on mirrored mount, got %+v", byPath["/data2"]) } } + +func TestPreparePodSpecAutoProvisionsSyncRemoteRoots(t *testing.T) { + // Users configure extra sync mappings with paths only; okdev provisions + // an emptyDir at any remote root not already covered by a volume, and + // the sidecar mirror makes it servable by the remote syncthing. + spec := corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "dev", + Image: "ubuntu:22.04", + VolumeMounts: []corev1.VolumeMount{ + {Name: "shared-pvc", MountPath: "/shared"}, + }, + }}, + } + volumes := []corev1.Volume{ + {Name: "shared-pvc", VolumeSource: corev1.VolumeSource{PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: "shared"}}}, + } + prepared, err := PreparePodSpecForTargetWithShellAndSyncRoots(spec, volumes, "/workspace", "sidecar:test", corev1.ResourceRequirements{}, false, "", "dev", "", []string{ + "/data/results", // uncovered -> emptyDir auto-provisioned + "/shared/results", // under the user PVC -> untouched + "/workspace/nested", // under workspace -> untouched (also overlap-rejected upstream) + }) + if err != nil { + t.Fatalf("prepare: %v", err) + } + var dev, sidecar *corev1.Container + for i := range prepared.Containers { + switch prepared.Containers[i].Name { + case "dev": + dev = &prepared.Containers[i] + case "okdev-sidecar": + sidecar = &prepared.Containers[i] + } + } + devPaths := map[string]string{} + for _, m := range dev.VolumeMounts { + devPaths[m.MountPath] = m.Name + } + if name := devPaths["/data/results"]; name == "" || !strings.HasPrefix(name, "okdev-sync-") { + t.Fatalf("expected auto-provisioned mount at /data/results, got %+v", dev.VolumeMounts) + } + if _, exists := devPaths["/shared/results"]; exists { + t.Fatalf("covered root must not get its own mount, got %+v", dev.VolumeMounts) + } + if _, exists := devPaths["/workspace/nested"]; exists { + t.Fatalf("workspace-nested root must not get its own mount, got %+v", dev.VolumeMounts) + } + volNames := map[string]bool{} + for _, v := range prepared.Volumes { + volNames[v.Name] = true + } + if !volNames[devPaths["/data/results"]] { + t.Fatalf("expected emptyDir volume %q in pod spec", devPaths["/data/results"]) + } + sidecarPaths := map[string]bool{} + for _, m := range sidecar.VolumeMounts { + sidecarPaths[m.MountPath] = true + } + if !sidecarPaths["/data/results"] { + t.Fatalf("sidecar must mirror the auto-provisioned mount, got %+v", sidecar.VolumeMounts) + } +} diff --git a/internal/workload/generic.go b/internal/workload/generic.go index 7566ad6e..c166ea91 100644 --- a/internal/workload/generic.go +++ b/internal/workload/generic.go @@ -33,6 +33,7 @@ type GenericRuntime struct { PreStop string TargetContainer string Volumes []corev1.Volume + SyncRemoteRoots []string Labels map[string]string Annotations map[string]string Inject []config.WorkloadInjectSpec @@ -120,7 +121,7 @@ func (r *GenericRuntime) Apply(ctx context.Context, k ApplyClient, namespace str } else { templateLabels["okdev.io/mesh-role"] = "receiver" } - template.Spec, err = kube.PreparePodSpecForTargetWithShell(template.Spec, r.Volumes, r.WorkspaceMountPath, r.SidecarImage, r.SidecarResources, r.Tmux, r.PreStop, r.interactiveContainer(), r.Shell) + template.Spec, err = kube.PreparePodSpecForTargetWithShellAndSyncRoots(template.Spec, r.Volumes, r.WorkspaceMountPath, r.SidecarImage, r.SidecarResources, r.Tmux, r.PreStop, r.interactiveContainer(), r.Shell, r.SyncRemoteRoots) if err != nil { return err } diff --git a/internal/workload/pod.go b/internal/workload/pod.go index c65d2fdd..209ab747 100644 --- a/internal/workload/pod.go +++ b/internal/workload/pod.go @@ -23,6 +23,7 @@ type PodRuntime struct { Tmux bool Shell string PreStop string + SyncRemoteRoots []string TargetContainer string LastAppliedSpecJSON string LastAppliedSpecHash string @@ -61,7 +62,7 @@ func (r *PodRuntime) WorkloadRef() (string, string, string, error) { } func (r *PodRuntime) Apply(ctx context.Context, k ApplyClient, namespace string) error { - prepared, err := kube.PreparePodSpecForTargetWithShell(r.PodSpec, r.Volumes, r.WorkspaceMountPath, r.SidecarImage, r.SidecarResources, r.Tmux, r.PreStop, r.effectiveTargetContainer(), r.Shell) + prepared, err := kube.PreparePodSpecForTargetWithShellAndSyncRoots(r.PodSpec, r.Volumes, r.WorkspaceMountPath, r.SidecarImage, r.SidecarResources, r.Tmux, r.PreStop, r.effectiveTargetContainer(), r.Shell, r.SyncRemoteRoots) if err != nil { return err } diff --git a/scripts/e2e_kind_smoke.sh b/scripts/e2e_kind_smoke.sh index 98109f1f..b0c9d03d 100644 --- a/scripts/e2e_kind_smoke.sh +++ b/scripts/e2e_kind_smoke.sh @@ -628,21 +628,12 @@ fi cp "$CFG_PATH.bak" "$CFG_PATH" echo "overlap rejection verified" -echo "Adding a second disjoint mapping (code up, results down) on a shared volume" +echo "Adding a second disjoint mapping (code up, results down)" COLLECT_DIR="$WORKDIR/collected" mkdir -p "$COLLECT_DIR" -# The extra mapping's remote root must live on a volume shared with the -# sidecar (its syncthing cannot see the dev container's overlay), so add a -# volume + mount for /data and reconcile the pod — the sidecar mirrors the -# target container's volume mounts. -replace_first_in_file "$CFG_PATH" " podTemplate:" " volumes: - - name: results-vol - emptyDir: {} - podTemplate:" -replace_first_in_file "$CFG_PATH" " command: [\"sleep\", \"infinity\"]" " command: [\"sleep\", \"infinity\"] - volumeMounts: - - name: results-vol - mountPath: /data" +# No volume YAML needed: okdev auto-provisions an emptyDir at the extra +# mapping's remote root and mirrors it into the sidecar. Adding the mapping +# changes the pod spec, so plain `up` must first surface drift guidance. MULTI_ENTRY="- local: \"$SYNC_DIR\" remote: /workspace direction: up @@ -652,11 +643,23 @@ MULTI_ENTRY="- local: \"$SYNC_DIR\" replace_first_in_file "$CFG_PATH" "- local: \"$SYNC_DIR\" remote: /workspace direction: down" "$MULTI_ENTRY" -if ! grep -q "/data/results" "$CFG_PATH" || ! grep -q "results-vol" "$CFG_PATH"; then - echo "ERROR: failed to add volume or second sync mapping; config is:" >&2 +if ! grep -q "/data/results" "$CFG_PATH"; then + echo "ERROR: failed to add second sync mapping; config is:" >&2 cat "$CFG_PATH" >&2 exit 1 fi + +echo "multi-mapping: plain up surfaces reconcile guidance for the new volume" +set +e +MULTI_DRIFT_OUTPUT=$("$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" up --wait-timeout 5m 2>&1) +MULTI_DRIFT_STATUS=$? +set -e +if [[ "$MULTI_DRIFT_STATUS" -eq 0 ]] || [[ "$MULTI_DRIFT_OUTPUT" != *"--reconcile"* ]]; then + echo "ERROR: expected drift guidance when adding a sync mapping (status=$MULTI_DRIFT_STATUS)" >&2 + echo "$MULTI_DRIFT_OUTPUT" >&2 + exit 1 +fi + "$OKDEV_BIN" --config "$CFG_PATH" --session "$SESSION_NAME" up --reconcile --wait-timeout 5m echo "multi-mapping: results folder flows pod -> local"