graceful restart #23
graceful restart #23
Conversation
WalkthroughRemoves error propagation from config validation in cmd/apply.go. Adds per-image debug logging in internal/config/loader.go. Overhauls restart/control flow in internal/exporter/ssh/ssh.go: introduces restartGracefully (SIGHUP via podman/systemd), bootc-aware restart deferral, refined container label parsing, and minor messaging changes. Adds HostManager.Diff() and noValuePlaceholder (unexported). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as cmd/apply
participant Lint as config_lint
U->>CLI: run apply
CLI->>Lint: Validate(cfg)
note right of Lint: No error returned to caller
Lint-->>CLI: validation result (non-fatal)
CLI-->>U: continue apply flow
sequenceDiagram
autonumber
participant Orchestrator as SSH Exporter Apply
participant Host as HostManager
participant Podman as podman
participant Systemd as systemd
Orchestrator->>Host: get BootcStatus
alt bootc updating
Orchestrator-->>Host: skip immediate restarts
note right of Orchestrator: Defer restarts during update
else normal
Orchestrator->>Podman: inspect running container labels (formatted)
Podman-->>Orchestrator: labels (normalized)
Orchestrator->>Host: check container version vs desired
alt service running and needs reload
rect rgba(200,255,200,0.2)
Orchestrator->>Podman: kill -s SIGHUP <container>
Podman-->>Orchestrator: ok or error
end
opt fallback
Orchestrator->>Systemd: systemctl kill -s SIGHUP <service>
Systemd-->>Orchestrator: ok or error
end
else service not running
Orchestrator->>Host: attempt restart (or log dry-run)
Host-->>Orchestrator: result
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/exporter/ssh/ssh.go (1)
17-17: Import relies on internal/container (see config.go comment).Same build-break risk applies here. Refer to the critical comment in internal/config/config.go Lines 7-8.
internal/config/loader.go (1)
12-12: Import relies on internal/container (see config.go comment).Same build-break risk; refer to critical comment in internal/config/config.go Lines 7-7.
🧹 Nitpick comments (4)
internal/exporter/ssh/ssh.go (2)
252-254: Make service-running check robust using exit code.Relying on exact stdout "active\n" is brittle. Prefer
--quietwith exit code.Apply this diff:
- statusResult, err := m.runCommand("systemctl is-active " + fmt.Sprintf("%q", svcName)) - serviceRunning := err == nil && statusResult.Stdout == "active\n" + statusResult, err := m.runCommand("systemctl is-active --quiet " + fmt.Sprintf("%q", svcName)) + serviceRunning := err == nil && statusResult.ExitCode == 0
336-385: Parse labels via JSON instead of space-splitting.
strings.Fieldsbreaks if a label contains spaces and depends on the "" placeholder. Prefer{{json .Config.Labels}}and decode.Apply this diff to the function:
func (m *SSHHostManager) getRunningContainerLabels(serviceName string) (*container.ImageLabels, error) { - // Try jumpstarter labels first, then fall back to OCI standard labels - result, err := m.runCommand(fmt.Sprintf("podman inspect --format '{{index .Config.Labels \"jumpstarter.version\"}} {{index .Config.Labels \"jumpstarter.revision\"}} {{index .Config.Labels \"org.opencontainers.image.version\"}} {{index .Config.Labels \"org.opencontainers.image.revision\"}}' %s", serviceName)) + // Inspect labels as JSON and parse reliably + result, err := m.runCommand(fmt.Sprintf("podman inspect --format '{{json .Config.Labels}}' %q", serviceName)) if err != nil { return nil, fmt.Errorf("failed to inspect container %s: %w", serviceName, err) } - parts := strings.Fields(strings.TrimSpace(result.Stdout)) - // Pad with empty strings if we got fewer parts - for len(parts) < 4 { - parts = append(parts, "") - } - - jumpstarterVersion := parts[0] // jumpstarter.version - jumpstarterRevision := parts[1] // jumpstarter.revision - ociVersion := parts[2] // org.opencontainers.image.version - ociRevision := parts[3] // org.opencontainers.image.revision - - // Clean up "<no value>" to empty string - if jumpstarterVersion == noValuePlaceholder { - jumpstarterVersion = "" - } - if jumpstarterRevision == noValuePlaceholder { - jumpstarterRevision = "" - } - if ociVersion == noValuePlaceholder { - ociVersion = "" - } - if ociRevision == noValuePlaceholder { - ociRevision = "" - } + labelsJSON := strings.TrimSpace(result.Stdout) + var labels map[string]string + if labelsJSON != "" { + if err := json.Unmarshal([]byte(labelsJSON), &labels); err != nil { + return nil, fmt.Errorf("failed to parse container labels for %s: %w", serviceName, err) + } + } else { + labels = map[string]string{} + } + + jumpstarterVersion := strings.TrimSpace(labels["jumpstarter.version"]) + jumpstarterRevision := strings.TrimSpace(labels["jumpstarter.revision"]) + ociVersion := strings.TrimSpace(labels["org.opencontainers.image.version"]) + ociRevision := strings.TrimSpace(labels["org.opencontainers.image.revision"]) // Use jumpstarter labels if both exist, otherwise fall back to OCI labels var version, revision string if jumpstarterVersion != "" && jumpstarterRevision != "" { version = jumpstarterVersion revision = jumpstarterRevision } else { version = ociVersion revision = ociRevision } // Return labels even if only partially available (version OR revision can be empty) return &container.ImageLabels{ Version: version, Revision: revision, }, nil }Add import:
// at top of file import "encoding/json"internal/config/loader.go (2)
309-314: Attaching ContainerVersions at load-time: OK; consider gating.Fetching registry metadata during config load can be slow. Consider lazy retrieval, caching, or a flag to skip in performance-sensitive paths.
318-349: Consider parallel fetch with timeout and basic caching.To improve UX:
- Use context with timeout per image (e.g., 5–10s).
- Fetch in parallel (errgroup with bounded concurrency).
- Cache results across runs to avoid re-querying unchanged images.
I can draft this if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/apply.go(1 hunks)internal/config/config.go(2 hunks)internal/config/loader.go(2 hunks)internal/exporter/ssh/ssh.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursorrules)
**/*.go: Run golangci-lint viamake lintand fix all linting errors before proceeding
Runmake buildto ensure the binary compiles successfully and fix any build errors
Runmake fmtto ensure code formatting (go fmt) before finalizing changes
Files:
internal/exporter/ssh/ssh.gointernal/config/config.gocmd/apply.gointernal/config/loader.go
🧬 Code graph analysis (1)
internal/config/config.go (1)
internal/config/loader.go (1)
LoadedLabConfig(24-37)
🪛 GitHub Check: Run jumpstarter-lab-config lint
internal/config/config.go
[failure] 7-7:
no required module provides package github.com/jumpstarter-dev/jumpstarter-lab-config/internal/container; to add it:
🔇 Additional comments (6)
internal/exporter/ssh/ssh.go (3)
33-35: Good: normalize Podman’s "" placeholder.
189-201: Graceful restart flow LGTM.Clear SIGHUP signaling with podman/systemd fallback and bootc-upgrade skip.
Also applies to: 223-230, 248-248
279-334: Version-aware restart logic is sensible; confirm image pull semantics.Flow is conservative (non-fatal on errors) and restarts only when versions differ. Ensure the systemd container unit or Quadlet uses an “pull on restart/auto-update” policy; SIGHUP alone won’t fetch newer images unless configured (e.g., ImagePullPolicy=always or podman auto-update).
Can you confirm the container unit ensures fresh image on restart?
cmd/apply.go (1)
63-64: No-op formatting change. LGTM.internal/config/config.go (2)
13-18: Config.ContainerVersions addition looks good.Properly non-serialized (yaml:"-") surfacing version data to callers.
47-53: Load error handling is correct.Returning nil on LoadAllResources error avoids partially-initialized configs.
| "os" | ||
| "path/filepath" | ||
|
|
||
| "github.com/jumpstarter-dev/jumpstarter-lab-config/internal/container" |
There was a problem hiding this comment.
🧩 Analysis chain
Build break: missing internal/container package or wrong import path.
CI shows: “no required module provides package github.com/jumpstarter-dev/jumpstarter-lab-config/internal/container”. Ensure the package exists in-repo and exports ImageLabels, IsEmpty/Matches/String, and GetImageLabelsFromRegistry, or fix the import path/module name.
Run this to verify presence and expected symbols:
Also, please run:
- make fmt
- make build
- make lint
As per coding guidelines.
🏁 Script executed:
#!/bin/bash
# Check package directory exists
fd -t d '^internal/container$' -H
# Confirm package declaration and key symbols
rg -n --type=go -C1 '^\s*package\s+container\b'
rg -n --type=go '\btype\s+ImageLabels\b'
rg -n --type=go '\bGetImageLabelsFromRegistry\s*\('
# Also ensure module path matches repo path (go.mod)
rg -n '^module\s+'Length of output: 274
Invalid import: missing internal/container package
The import github.com/jumpstarter-dev/jumpstarter-lab-config/internal/container doesn’t exist—add the directory with its package container and expected symbols or fix the import path.
🧰 Tools
🪛 GitHub Check: Run jumpstarter-lab-config lint
[failure] 7-7:
no required module provides package github.com/jumpstarter-dev/jumpstarter-lab-config/internal/container; to add it:
🤖 Prompt for AI Agents
internal/config/config.go around line 7: the import
"github.com/jumpstarter-dev/jumpstarter-lab-config/internal/container" is
invalid because that package path does not exist; either create an
internal/container directory with a Go file declaring "package container" and
exporting the symbols referenced by config.go (types/functions/constants), or
correct the import to the proper existing module path; after adding or fixing
the package, run go mod tidy and update any references in config.go to match the
exported names from package container.
just sending SIGHUP to trigger restart only once it's not leased. Expecting the service configuration is set up to pull a newer image on startup (i.e. upgrade)
michalskrivanek
force-pushed
the
graceful
branch
from
354103e to
6b8b003
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exporter/ssh/ssh.go (1)
40-40: Document or remove the unimplemented Diff() stubNo callers reference
SSHHostManager.Diff()and it returns a placeholder; either remove it from the interface until it’s implemented or add a clear// TODOand documentation marking it as pending functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/apply.go(0 hunks)internal/config/loader.go(1 hunks)internal/exporter/ssh/ssh.go(4 hunks)
💤 Files with no reviewable changes (1)
- cmd/apply.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/loader.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursorrules)
**/*.go: Run golangci-lint viamake lintand fix all linting errors before proceeding
Runmake buildto ensure the binary compiles successfully and fix any build errors
Runmake fmtto ensure code formatting (go fmt) before finalizing changes
Files:
internal/exporter/ssh/ssh.go
🔇 Additional comments (8)
internal/exporter/ssh/ssh.go (8)
33-35: LGTM!The
noValuePlaceholderconstant is well-defined and improves code maintainability by avoiding magic strings when handling missing container labels.
248-248: LGTM!The call to
restartGracefullyis consistent with the function rename and maintains the same parameters.
257-266: LGTM! Logic is correct given the bootc guard.The direct restart approach for non-running services is appropriate since the bootc update check (lines 223-230) already gates this code path. When the service isn't running, a full restart is more suitable than a graceful signal.
269-269: LGTM!Passing
restartGracefullytocheckContainerVersionis consistent with the function rename and maintains the correct signature.
339-339: LGTM! More efficient label retrieval.The consolidated format string efficiently retrieves all four labels in a single
podman inspectcall, reducing command overhead while maintaining correct handling of missing labels.
344-348: LGTM! Correct parsing approach.Using
strings.Fieldswith padding is the right approach for parsing space-separated label values from the consolidated format string. This handles edge cases where fewer than four labels are present.
356-367: LGTM! Proper placeholder normalization.The normalization of
noValuePlaceholderto empty strings ensures consistent handling of missing labels throughout the codebase.
189-201: Fix typo and align podman kill with container names
- Replace “wating” with “waiting” in the function comment.
- Podman kill requires a container name, not a systemd service. Confirm that your container names match
serviceName(or look up the actual container name before invoking podman); otherwise, usesystemctl killfor both.
2 participants
Summary by CodeRabbit