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 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds a host-side systemd service enabler for automatic reloads. It synchronizes site service units, supports root and non-root systemd paths, reports installation and removal errors, and returns container state. ChangesSite service enabler
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds and removes a host-side systemd helper, but current uninstall ordering can remove it for the wrong platform, while failure paths may leave configuration unapplied or report successful removal when the unit remains installed. These bounded service-management risks require follow-up before merge. 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)
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. 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.
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) | |
| } |
| for _, container := range containers { | ||
| if container.Name == containerName { | ||
| return true | ||
| return container.Running |
There was a problem hiding this comment.
I think this change ends up changing how other parts of the install work. Not sure which behavior is better - neither of them feel totally correct to me. Maybe we need a function here that returns true when the container is found AND the container state instead of just a basic IsContainerRunning func.
from main:
[christian@rh skupper] (main)$ go run ./cmd/skupper/ system install -p podman
Pulled system-controller image: quay.io/skupper/system-controller:v2-dev
Platform podman is now configured for Skupper
[christian@rh skupper] (main)$ podman stop christian-skupper-controller
christian-skupper-controller
[christian@rh skupper] (main)$ go run ./cmd/skupper/ system install -p podman
Warning: The system controller container "christian-skupper-controller" is already running in Podman.
[christian@rh skupper] (main)$ ./skupper system uninstall -p podman --force
Unable to uninstall.
Error: failed to stop system-controller container: error stopping container christian-skupper-controller:
[christian@rh skupper] (main)$ podman start christian-skupper-controller
christian-skupper-controller
[christian@rh skupper] (main)$ go run ./cmd/skupper/ system uninstall -p podman --force
System-controller has been removed
Platform podman infrastructure for Skupper is now uninstalled
from this PR:
[christian@rh skupper] (add-systmd-service-per-router)$ go run ./cmd/skupper/ system install -p podman
Pulled system-controller image: quay.io/skupper/system-controller:v2-dev
Platform podman is now configured for Skupper
[christian@rh skupper] (add-systmd-service-per-router)$ go run ./cmd/skupper/ system install -p podman
Warning: The system controller container "christian-skupper-controller" is already running in Podman.
[christian@rh skupper] (add-systmd-service-per-router)$ podman stop christian-skupper-controller
christian-skupper-controller
[christian@rh skupper] (add-systmd-service-per-router)$ go run ./cmd/skupper/ system install -p podman
Pulled system-controller image: quay.io/skupper/system-controller:v2-dev
failed to configure the environment : failed to create system-controller container: error creating container christian-skupper-controller: container create: creating container storage: the container name "christian-skupper-controller" is already in use by 15be017357be68663d643f69400d652ecec647be5505eacc72a94f1b4689ac15. You have to remove that container to be able to reuse that name: that name is already in use
exit status 1
[christian@rh skupper] (add-systmd-service-per-router)$ podman start christian-skupper-controller
christian-skupper-controller
[christian@rh skupper] (add-systmd-service-per-router)$ ./skupper system uninstall -p podman --force
System-controller has been removed
Platform podman infrastructure for Skupper is now uninstalled
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/nonkube/bootstrap/uninstall.go (1)
18-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove helper removal after platform-mismatch checks.
At Line 18,
Remove()runs beforeUninstallchecks whether the controller belongs to another engine. If a Podman controller exists and the selected platform is Docker, Lines 31-34 return without removing that controller, but the host-side enabler is already gone. The same occurs for Docker when the selected platform is Podman.Check both platform mismatches first. Remove the helper only after those checks, while keeping removal before the selected-platform container-absent return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bade2805-de12-42e6-a4e7-e3aa7b1c12dc
📒 Files selected for processing (4)
internal/nonkube/bootstrap/install.gointernal/nonkube/bootstrap/uninstall.gointernal/nonkube/client/compat/container.gopkg/container/client.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
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