add checking of running container on each exporter host #21
Conversation
WalkthroughAdds registry-based image label retrieval to config loading, surfaces labels on Config.ContainerVersions, enforces config validation in the apply command, implements container-version comparison and restart logic in SSH HostManager, and adds unit tests for host exporter filtering behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as apply cmd
participant Loader as LoadConfig / LoadAllResources
participant Registry as Container Registry (skopeo)
participant Lint as config_lint.ValidateWithError
participant Applier as Template Applier
User->>CLI: jumpstarter apply
CLI->>Loader: LoadConfig()
Loader->>Registry: GetImageLabelsFromRegistry(image) [for each unique image]
Registry-->>Loader: ImageLabels / error
Loader-->>CLI: cfg (with ContainerVersions)
CLI->>Lint: ValidateWithError(cfg)
alt validation ok
CLI->>Applier: New(cfg) and proceed with apply
else validation failed
Lint-->>CLI: error -> CLI returns wrapped error
end
sequenceDiagram
autonumber
participant SSH as SSHHostManager
participant Podman as Remote Podman
participant Registry as Config.ContainerVersions (cached)
note right of SSH: During Apply for container-based exporter
SSH->>Podman: GetRunningContainerLabels(serviceName)
Podman-->>SSH: running ImageLabels (or empty)
SSH->>Registry: lookup = cfg.ContainerVersions[image]
Registry-->>SSH: registry ImageLabels (from loader)
SSH->>SSH: compare running vs registry labels
alt labels differ -> restart needed
SSH->>SSH: print actions (dry-run vs real) and restart service
else labels match or unknown -> no restart
SSH-->>SSH: skip restart
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (5)
internal/exporter/ssh/ssh.go (2)
287-299: Avoid duplicate registry lookups; reuse pre-fetched labelsThis unconditionally fetches labels from the registry again during Apply. You already populate cfg.ContainerVersions during load. Plumb expected labels to this call (or add a small cache in the container package) to avoid repeated remote calls per host.
191-197: Align log messages with action (restart vs start)The helper performs a restart, but messages say “start”. Adjust for clarity.
Apply:
- fmt.Printf(" ❌ Failed to start service %s: %v\n", serviceName, enableErr) + fmt.Printf(" ❌ Failed to restart service %s: %v\n", serviceName, enableErr) @@ - fmt.Printf(" ✅ Service %s started\n", serviceName) + fmt.Printf(" ✅ Service %s restarted\n", serviceName)internal/config/loader.go (3)
309-314: Avoid network I/O during config load or gate itFetching container labels during LoadAllResources increases startup latency and couples loading with external calls. Consider:
- Deferring to the validation phase (and handle its errors) or
- Making it optional via a flag or
- Using a cache so apply-time checks reuse these results.
318-348: Parallelize label retrieval for multiple imagesRetrieval is fully serial. For many images, this will be slow. Use a bounded worker pool with context cancellation, and collect results into the map.
I can provide a goroutine-based worker implementation if desired.
332-344: Prefer structured logging over stdout prints in library codeEmojis/prints are fine for CLI, but loader functions are library-like. Consider using a logger with levels or returning the info to the caller to print.
📜 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:
cmd/apply.gointernal/config/config.gointernal/exporter/ssh/ssh.gointernal/config/loader.go
🧬 Code graph analysis (2)
internal/config/config.go (2)
internal/vars/vars.go (1)
Variables(11-14)internal/config/loader.go (1)
LoadedLabConfig(24-37)
internal/exporter/ssh/ssh.go (1)
api/v1alpha1/exporterconfigtemplate_types.go (1)
ExporterConfigTemplate(74-80)
🪛 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:
🪛 GitHub Actions: Tests
internal/config/config.go
[error] 7-7: No required module provides package 'github.com/jumpstarter-dev/jumpstarter-lab-config/internal/container'.
🔇 Additional comments (3)
internal/config/config.go (2)
7-7: Import references missing internal/container package (build break)Same issue as flagged in ssh.go; the package isn’t present per CI. Ensure the package is added or adjust types to remove this dependency.
See the verification script in the ssh.go comment. As per coding guidelines
48-53: LGTM: corrected LoadConfig return handlingReturning (&cfg, nil) on success after an explicit error check is correct.
internal/config/loader.go (1)
12-12: Import references missing internal/container package (build break)This is another occurrence; add the package or refactor to avoid it.
Use the same repository scan script provided in ssh.go. As per coding guidelines
| config_lint.Validate(cfg) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Handle validation errors; fail fast on invalid config
The return value from config_lint.Validate(cfg) is ignored. This likely violates linting and allows applying invalid configs.
Apply:
- config_lint.Validate(cfg)
+ if err := config_lint.Validate(cfg); err != nil {
+ return fmt.Errorf("config validation failed: %w", err)
+ }If Validate returns a slice of errors, aggregate them. To confirm signature:
As per coding guidelines
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '(?s)^package\s+config_lint\b.*?func\s+Validate\s*\(' -C2 --type=goLength of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cmd/apply.go (first 100 lines) ==="
head -n 100 cmd/apply.go || true
echo
echo "=== config_lint.Validate call context (±2 lines) ==="
rg -nC2 'config_lint\.Validate' cmd/apply.go || true
echo
echo "=== Files declaring package config_lint ==="
rg -l 'package config_lint' -g '*.go' || true
echo
echo "=== All Validate function signatures in repo ==="
rg -n 'func Validate' --type=go || trueLength of output: 3904
Use ValidateWithError to handle config validation errors
Replace the call to config_lint.Validate(cfg) with:
if err := config_lint.ValidateWithError(cfg); err != nil {
return fmt.Errorf("config validation failed: %w", err)
}This ensures invalid configs cause a fast failure.
🤖 Prompt for AI Agents
In cmd/apply.go around lines 62-63, the current call to
config_lint.Validate(cfg) ignores validation failures; replace it with a call to
config_lint.ValidateWithError(cfg), check the returned error, and if non-nil
return a wrapped error using fmt.Errorf("config validation failed: %w", err) so
invalid configs cause an immediate failure.
michalskrivanek
force-pushed
the
labels
branch
from
f97fac9 to
1c036f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/apply.go (1)
62-64: Handle config validation errors.
config_lint.Validate(cfg)still discards failures, so invalid configs proceed to apply. Please switch to the error-returning helper and fail fast.- config_lint.Validate(cfg) + if err := config_lint.ValidateWithError(cfg); err != nil { + return fmt.Errorf("config validation failed: %w", err) + }As per coding guidelines
internal/exporter/ssh/ssh.go (1)
330-332: QuoteserviceNamein the podman inspect command.The unquoted placeholder lets spaces or shell metacharacters break the command (or worse, allows injection). Please wrap it with
%qbefore passing torunCommand.- 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)) + 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\"}}' %q", serviceName))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/apply.go(1 hunks)internal/config/config.go(2 hunks)internal/config/loader.go(2 hunks)internal/container/version.go(1 hunks)internal/exporter/host/host_test.go(1 hunks)internal/exporter/ssh/ssh.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/loader.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/container/version.gointernal/exporter/ssh/ssh.gointernal/config/config.gocmd/apply.gointernal/exporter/host/host_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursorrules)
Run the full test suite via
make testand ensure all tests pass before considering the iteration complete
Files:
internal/exporter/host/host_test.go
🧬 Code graph analysis (3)
internal/exporter/ssh/ssh.go (1)
internal/container/version.go (2)
GetImageLabelsFromRegistry(17-57)ImageLabels(11-14)
internal/config/config.go (2)
internal/config/loader.go (1)
LoadedLabConfig(24-37)internal/container/version.go (1)
ImageLabels(11-14)
internal/exporter/host/host_test.go (1)
api/v1alpha1/exporterinstance_types.go (1)
ExporterInstance(71-77)
michalskrivanek
force-pushed
the
labels
branch
from
1c036f7 to
40233fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/apply.go(1 hunks)internal/config/config.go(2 hunks)internal/config/loader.go(2 hunks)internal/container/version.go(1 hunks)internal/exporter/host/host_test.go(1 hunks)internal/exporter/ssh/ssh.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/container/version.go
- internal/exporter/host/host_test.go
- cmd/apply.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.gointernal/config/loader.gointernal/config/config.go
🧠 Learnings (1)
📚 Learning: 2025-10-03T16:50:32.459Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter-lab-config#21
File: internal/exporter/ssh/ssh.go:330-333
Timestamp: 2025-10-03T16:50:32.459Z
Learning: In internal/exporter/ssh/ssh.go, the podman inspect command for serviceName should use %s (not %q) formatting, as adding quotes breaks the command execution. The serviceName is passed directly to podman as a container name argument and does not require shell quoting.
Applied to files:
internal/exporter/ssh/ssh.go
🧬 Code graph analysis (3)
internal/exporter/ssh/ssh.go (1)
internal/container/version.go (2)
GetImageLabelsFromRegistry(17-57)ImageLabels(11-14)
internal/config/loader.go (1)
internal/container/version.go (2)
ImageLabels(11-14)GetImageLabelsFromRegistry(17-57)
internal/config/config.go (2)
internal/config/loader.go (1)
LoadedLabConfig(24-37)internal/container/version.go (1)
ImageLabels(11-14)
relies on labels jumpstarter.version and jumpstarter.revision on image, or standard OCI labels org.opencontainers.image.version and org.opencontainers.image.revision
michalskrivanek
force-pushed
the
labels
branch
from
40233fa to
77c725c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/apply.go(1 hunks)internal/config/config.go(2 hunks)internal/config/loader.go(2 hunks)internal/container/version.go(1 hunks)internal/exporter/host/host_test.go(1 hunks)internal/exporter/ssh/ssh.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/apply.go
- internal/container/version.go
- internal/config/loader.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/config/config.gointernal/exporter/ssh/ssh.gointernal/exporter/host/host_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursorrules)
Run the full test suite via
make testand ensure all tests pass before considering the iteration complete
Files:
internal/exporter/host/host_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-03T16:50:32.459Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter-lab-config#21
File: internal/exporter/ssh/ssh.go:330-333
Timestamp: 2025-10-03T16:50:32.459Z
Learning: In internal/exporter/ssh/ssh.go, the podman inspect command for serviceName should use %s (not %q) formatting, as adding quotes breaks the command execution. The serviceName is passed directly to podman as a container name argument and does not require shell quoting.
Applied to files:
internal/exporter/ssh/ssh.go
🧬 Code graph analysis (3)
internal/config/config.go (2)
internal/config/loader.go (1)
LoadedLabConfig(24-37)internal/container/version.go (1)
ImageLabels(11-14)
internal/exporter/ssh/ssh.go (1)
internal/container/version.go (2)
GetImageLabelsFromRegistry(17-57)ImageLabels(11-14)
internal/exporter/host/host_test.go (1)
api/v1alpha1/exporterinstance_types.go (1)
ExporterInstance(71-77)
🔇 Additional comments (9)
internal/config/config.go (3)
7-7: LGTM!The import of
internal/containeris correct and the package exists per the relevant code snippets.
17-17: LGTM!The new
ContainerVersionsfield is well-typed and properly documented with the yaml:"-" tag to exclude it from serialization.
48-52: LGTM! Error handling fix.The change from returning
(&cfg, err)to returning(nil, err)on error is correct. Previously, returning a partially loaded config alongside an error was confusing for callers. This change ensures consistent error handling semantics.internal/exporter/host/host_test.go (1)
130-224: LGTM with caveat!The test logic correctly validates host skipping behavior. However, this test will inherit the same semantic issue flagged in the previous comment regarding how the "dead" annotation is interpreted (presence vs. value).
internal/exporter/ssh/ssh.go (5)
33-35: LGTM!The
noValuePlaceholderconstant correctly captures the Podman/Docker placeholder for missing labels.
253-268: LGTM! Container version checking integrated correctly.The logic appropriately checks container versions only when the service is running, avoiding unnecessary checks during service restarts. Error handling is correct with non-fatal errors for version checks.
270-284: LGTM!The function correctly limits version checking to container-based exporters only, with proper early returns for non-container exporters.
286-325: LGTM! Robust error handling.The detailed version comparison logic is well-structured:
- Non-fatal error handling (warnings only) prevents version check failures from blocking operations
- Clear user feedback for version mismatches in both dry-run and real execution modes
- Appropriate conditional restart based on version comparison
327-375: LGTM! Past review issues resolved.The implementation correctly addresses all previous feedback:
- Uses
%s(not%q) for serviceName formatting (as per learnings)- Uses
strings.Spliton newlines instead ofstrings.Fields, preserving<no value>placeholder intact- Normalizes
<no value>to empty string correctly (lines 346-358)- Prioritizes jumpstarter labels over OCI labels, matching the logic in
internal/container/version.goBased on learnings
| return fmt.Errorf("error loading config file %s: %w", configFilePath, err) | ||
| } | ||
|
|
||
| config_lint.Validate(cfg) |
There was a problem hiding this comment.
| config_lint.Validate(cfg) |
Should we remove this line since we have the other?
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
just one nit not worth keeping this PR waiting, we can do it later.
2 participants
relies on labels jumpstarter.version and jumpstarter.revision on image, or standard OCI labels org.opencontainers.image.version and org.opencontainers.image.revision
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests