Add helper that enables systemd services created by the system-controller container - #2548
Add helper that enables systemd services created by the system-controller container#2548nluaces wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThe change adds a host-side systemd service enabler for automatic reloads. It synchronizes site service units, handles root and non-root systemd paths, reports installation and removal errors, and returns the actual container running state. ChangesSite service enabler
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds and removes a systemd helper, but uninstall may report success while leaving the helper installed when the unit file cannot be inspected, and a transient restart failure may leave updated configuration unapplied. These failure paths should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant Install
participant SiteServiceEnablerInstaller
participant systemd
participant SiteServiceEnablerScript
Install->>SiteServiceEnablerInstaller: install automatic reload support
SiteServiceEnablerInstaller->>SiteServiceEnablerScript: render and write wrapper
SiteServiceEnablerInstaller->>systemd: reload, enable, and start enabler
SiteServiceEnablerScript->>systemd: install and synchronize site units
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fcea0e92-8ae3-4ba5-83bd-e45997ba52b1
📒 Files selected for processing (9)
internal/cmd/skupper/system/nonkube/site_service_enabler.gointernal/cmd/skupper/system/system.gointernal/nonkube/bootstrap/install.gointernal/nonkube/bootstrap/site_service_enabler_installer.gointernal/nonkube/bootstrap/site_service_enabler_installer_test.gointernal/nonkube/bootstrap/site_service_enabler_service.templateinternal/nonkube/bootstrap/uninstall.gointernal/nonkube/enabler/enabler.gointernal/nonkube/enabler/enabler_test.go
|
@coderabbitai generate sequence diagram |
Sequence Diagram for PRsequenceDiagram
participant User
participant SkupperCLI as skupper CLI
participant Installer as Site Service Enabler Installer
participant HostFS as Host filesystem
participant Systemd as systemd
participant Wrapper as site-service-enabler.service
participant Daemon as _site-service-enabler
participant Watcher as Namespace script watcher
participant Controller as System controller
participant Enabler as Service enabler
User->>SkupperCLI: system install --reload-type auto
SkupperCLI->>Installer: Install site service enabler
Installer->>HostFS: Install wrapper and unit template
Installer->>HostFS: Select root or user systemd paths
Installer->>Systemd: Reload unit configuration
Installer->>Systemd: Enable and start helper service
Systemd->>Wrapper: Start service
Wrapper->>Daemon: Launch hidden _site-service-enabler command
Daemon->>Watcher: Watch namespaces directory
Watcher-->>Daemon: Watchers active
Controller->>HostFS: Create namespace/internal/scripts/skupper-{namespace}.service
HostFS-->>Watcher: Script directory or service file event
Watcher->>Enabler: Forward service file change
Enabler->>HostFS: Copy service into systemd unit directory
Enabler->>Systemd: Reload systemd units
Enabler->>Systemd: Enable skupper-{namespace}.service
Enabler->>Systemd: Start or restart service
Controller->>HostFS: Modify or remove service file
HostFS-->>Watcher: File change or removal event
Watcher->>Enabler: Forward update
Enabler->>HostFS: Synchronize or remove installed unit
Enabler->>Systemd: Reload systemd units
Enabler->>Systemd: Disable or stop removed service
Daemon-->>Daemon: Receive SIGINT or SIGTERM
Daemon->>Watcher: Stop filesystem watchers
Daemon-->>Wrapper: Exit
Wrapper-->>Systemd: Service stopped
User->>SkupperCLI: system uninstall --force
SkupperCLI->>Installer: Remove site service enabler
Installer->>Systemd: Stop and disable helper service
Installer->>HostFS: Remove wrapper and unit files
Installer->>Systemd: Reload unit configuration
|
✅ Action performedSequence diagram generated. |
| Use: "_site-service-enabler", | ||
| Hidden: true, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| slog.Info("Starting site-service-enabler", slog.String("version", version.Version)) |
There was a problem hiding this comment.
Worth checking if another instance is running or not.
| } | ||
|
|
||
| if reloadType == types.SystemReloadTypeAuto { | ||
| enabler := newSiteServiceEnablerInstaller() |
There was a problem hiding this comment.
Safer to let it just run when not running in a container.
If someone, for example, installs the controller using Ansible (which runs the cli image in a container), then it should not be installed.
| After=network.target | ||
|
|
||
| [Service] | ||
| Type=simple |
There was a problem hiding this comment.
I thought I heard @hash-d suggest something like this:
What if instead of a long-lived service we had two units: one path unit watching a file and then a service unit with type=oneshot that syncs all unit files, enables/disables skupper services and exits. The system controller would just have to touch the path file on change, and the path unit would kick the service unit.
I think that regardless of which way we go on the skupper CLI dependency this works out better:
- If we require
skupperis installed, which seems fine to me, we don't have some hidden one-off daemon mode in the CLI. - If we accommodate installs without the
skupperdependency, I think we probably could write this service in bash.
7c16669 to
3adcc22
Compare
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c23868f-5366-4aa7-8c7d-7bb0dfa0f4e0
📒 Files selected for processing (7)
internal/kube/certificates/mgr_test.gointernal/nonkube/bootstrap/install.gointernal/nonkube/bootstrap/site_service_enabler_installer.gointernal/nonkube/bootstrap/site_service_enabler_installer_test.gointernal/nonkube/bootstrap/site_service_enabler_script.templateinternal/nonkube/bootstrap/site_service_enabler_service.templateinternal/nonkube/bootstrap/uninstall.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
…rker to systemd services
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c90ce887-4a47-4326-8daf-4f044bf0e85c
📒 Files selected for processing (5)
internal/nonkube/bootstrap/install.gointernal/nonkube/bootstrap/site_service_enabler_installer.gointernal/nonkube/bootstrap/site_service_enabler_installer_test.gointernal/nonkube/bootstrap/site_service_enabler_script.templateinternal/nonkube/bootstrap/uninstall.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| if [ "$owned" != "$existing" ]; then | ||
| printf '%s' "$owned" > "$dst" | ||
| systemctl_run daemon-reload || true | ||
| changed=1 | ||
| fi | ||
| systemctl_run enable --now "$svc" || true | ||
| if [ "$changed" -eq 1 ]; then | ||
| systemctl_run restart "$svc" || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- candidate files ---'
git ls-files | grep -E '(^|/)(site_service_enabler_script\.template|.*site.*service.*enabl.*|.*bootstrap.*test.*|.*nonkube.*test.*)' || true
echo '--- template outline ---'
if [ -f internal/nonkube/bootstrap/site_service_enabler_script.template ]; then
ast-grep outline internal/nonkube/bootstrap/site_service_enabler_script.template || true
wc -l internal/nonkube/bootstrap/site_service_enabler_script.template
cat -n internal/nonkube/bootstrap/site_service_enabler_script.template
fi
echo '--- relevant symbols and references ---'
rg -n -S 'systemctl_run|site_service_enabler_script|daemon-reload|enable --now|restart "\$svc"|restart.*svc' internal .github 2>/dev/null | head -300 || true
echo '--- related test files and relevant sections ---'
for f in $(git ls-files | grep -E '(^|/)(test|tests|.*_test|.*_spec).*\.(sh|bash|py|js|ts|go|rb)$' || true); do
if rg -q -S 'site_service_enabler|systemctl_run|daemon-reload|enable --now' "$f"; then
echo "### $f"
rg -n -C 8 -S 'site_service_enabler|systemctl_run|daemon-reload|enable --now' "$f"
fi
done
echo '--- read-only state transition probe ---'
python3 - <<'PY'
# Model only the control flow shown in the review comment.
states = [
{"owned": "new", "existing": "old", "restart_rc": 1, "enable_now_rc": 0},
{"owned": "new", "existing": "new", "restart_rc": 0, "enable_now_rc": 0},
]
changed = 0
for poll, state in enumerate(states, 1):
if state["owned"] != state["existing"]:
changed = 1
print(f"poll {poll}: write unit; daemon-reload; enable --now; restart (rc={state['restart_rc']})")
else:
print(f"poll {poll}: no write; enable --now only (rc={state['enable_now_rc']})")
print(f"poll {poll}: changed={changed}")
PYRepository: skupperproject/skupper
Length of output: 17112
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- focused bootstrap test sections ---'
sed -n '1,210p' internal/nonkube/bootstrap/site_service_enabler_installer_test.go
echo '--- related Go enabler implementation outline ---'
ast-grep outline internal/nonkube/enabler/enabler.go || true
sed -n '1,135p' internal/nonkube/enabler/enabler.go
echo '--- related Go enabler tests ---'
sed -n '1,125p' internal/nonkube/enabler/enabler_test.go
echo '--- all references to the generated helper and restart behavior ---'
rg -n -C 5 -S 'site-service-enabler|site_service_enabler|restart.*service|enableService\(' internal/nonkube README.md docs 2>/dev/null | head -400 || true
echo '--- shell semantic probe: per-invocation changed state ---'
python3 - <<'PY'
# Model the exact function-local reset in the template.
def enable_service(owned, existing, restart_rc, calls):
changed = 0
if owned != existing:
calls.append("write")
calls.append("daemon-reload")
changed = 1
calls.append("enable --now")
if changed == 1:
calls.append(("restart", restart_rc))
return calls
calls = []
enable_service("new", "old", 1, calls)
enable_service("new", "new", 0, calls)
print(calls)
assert calls == ["write", "daemon-reload", "enable --now", ("restart", 1),
"enable --now"]
print("unchanged poll does not issue restart")
PYRepository: skupperproject/skupper
Length of output: 32602
🌐 Web query:
systemd systemctl enable --now active service does it restart or reload unit documentation
💡 Result:
When you use the command systemctl enable --now [unit], it does not restart or reload the service; instead, it performs two distinct actions: enabling the unit and starting the unit [1][2][3]. The enable component creates the necessary symlinks based on the [Install] section of the unit file, which ensures the service will start automatically at boot [1][3][4]. By adding the --now switch, you instruct systemctl to immediately start the unit in the current session as well [1][3]. The command does not trigger a restart or a reload of an already running service [1][2]. Enabling and starting are orthogonal operations: - Enable: Configures the persistent boot behavior [5][3]. - Start: Initiates the process for the current session [5][3]. If you need to ensure a service uses the latest configuration, you would typically use systemctl daemon-reload (to reload unit files) or systemctl reload [unit] (to signal the service to reload its configuration), which are separate operations from enabling or starting [6][3].
Citations:
- 1: https://www.freedesktop.org/software/systemd/man/252/systemctl.html
- 2: https://man7.org/linux/man-pages/man1/systemctl.1.html
- 3: https://linuxize.com/post/systemctl-command-in-linux/
- 4: https://www.redhat.com/en/blog/linux-systemctl-manage-services
- 5: https://manpages.debian.org/trixie/systemd/systemctl.1.en.html
- 6: https://www.freedesktop.org/software/systemd/man/systemctl.html
Retry failed service restarts.
When systemctl_run restart "$svc" fails after the unit file update, the next poll sets changed=0. systemctl_run enable --now "$svc" does not restart an active service, so the updated configuration remains unapplied. Persist pending restart state or retry until success. Add a test for a failed restart followed by an unchanged poll.
There was a problem hiding this comment.
I will discard this suggestion for now.
There was a problem hiding this comment.
@nluaces, acknowledged. The review finding remains open.
🐇
You are interacting with an AI system.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b80af336-1df8-4c46-9646-849bd55c6c68
📒 Files selected for processing (2)
internal/nonkube/bootstrap/site_service_enabler_installer.gointernal/nonkube/bootstrap/site_service_enabler_installer_test.go
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| if _, err := os.Stat(unitFile); err == nil { | ||
| if err := s.systemctl("stop", siteServiceEnablerServiceFile); err != nil { | ||
| return fmt.Errorf("failed to stop %s: %w", siteServiceEnablerServiceFile, err) | ||
| } | ||
| if err := s.systemctl("disable", siteServiceEnablerServiceFile); err != nil { | ||
| return fmt.Errorf("failed to disable %s: %w", siteServiceEnablerServiceFile, err) | ||
| } | ||
| if err := os.Remove(unitFile); err != nil && !os.IsNotExist(err) { | ||
| return fmt.Errorf("failed to remove unit file: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return errors that prevent inspection of the unit file.
At Line 91, a permission or I/O error from os.Stat is handled as if the unit were absent. Remove can then return success after removing the wrapper and reloading systemd while the helper unit remains installed. Return errors other than os.IsNotExist(err).
Proposed fix
- if _, err := os.Stat(unitFile); err == nil {
+ if _, err := os.Stat(unitFile); err == nil {
if err := s.systemctl("stop", siteServiceEnablerServiceFile); err != nil {
return fmt.Errorf("failed to stop %s: %w", siteServiceEnablerServiceFile, err)
}
if err := s.systemctl("disable", siteServiceEnablerServiceFile); err != nil {
return fmt.Errorf("failed to disable %s: %w", siteServiceEnablerServiceFile, err)
}
if err := os.Remove(unitFile); err != nil && !os.IsNotExist(err) {
return fmt.Errorf("failed to remove unit file: %w", err)
}
+ } else if !os.IsNotExist(err) {
+ return fmt.Errorf("failed to inspect unit file: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, err := os.Stat(unitFile); err == nil { | |
| if err := s.systemctl("stop", siteServiceEnablerServiceFile); err != nil { | |
| return fmt.Errorf("failed to stop %s: %w", siteServiceEnablerServiceFile, err) | |
| } | |
| if err := s.systemctl("disable", siteServiceEnablerServiceFile); err != nil { | |
| return fmt.Errorf("failed to disable %s: %w", siteServiceEnablerServiceFile, err) | |
| } | |
| if err := os.Remove(unitFile); err != nil && !os.IsNotExist(err) { | |
| return fmt.Errorf("failed to remove unit file: %w", err) | |
| } | |
| } | |
| if _, err := os.Stat(unitFile); err == nil { | |
| if err := s.systemctl("stop", siteServiceEnablerServiceFile); err != nil { | |
| return fmt.Errorf("failed to stop %s: %w", siteServiceEnablerServiceFile, err) | |
| } | |
| if err := s.systemctl("disable", siteServiceEnablerServiceFile); err != nil { | |
| return fmt.Errorf("failed to disable %s: %w", siteServiceEnablerServiceFile, err) | |
| } | |
| if err := os.Remove(unitFile); err != nil && !os.IsNotExist(err) { | |
| return fmt.Errorf("failed to remove unit file: %w", err) | |
| } | |
| } else if !os.IsNotExist(err) { | |
| return fmt.Errorf("failed to inspect unit file: %w", err) | |
| } |
3 participants
Resolves #2490
A new skupper-site-service-enabler.service unit is installed when running
skupper system install --reload-type autoand removed when runningskupper system uninstall --force.Summary by CodeRabbit
New Features
Bug Fixes