Repository navigation
Add the upgrade flag to skupper system install command - #2597
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:
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 16 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: adding the upgrade flag to the Skupper system install command. Linked Issues check Issue #2390requests a CLI path to update the System controller and controlled router sites. The PR addsskupper system install --upgrade, validates its flag combination, detects controller and rout…Out of Scope Changes check The production changes implement the issue #2390upgrade flow. Backup restoration, stopped-container cleanup, tar extraction truncation, and isolated non-Kubernetes test data support that flow or its …
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
27b7c91b-3675-49a0-848c-a4f611a6eb05
📒 Files selected for processing (9)
internal/cmd/skupper/common/flags.gointernal/cmd/skupper/system/nonkube/system_install.gointernal/cmd/skupper/system/nonkube/system_install_test.gointernal/cmd/skupper/system/system.gointernal/cmd/skupper/system/system_test.gointernal/nonkube/bootstrap/image_check.gointernal/nonkube/bootstrap/image_check_test.gointernal/nonkube/bootstrap/upgrade.gointernal/nonkube/bootstrap/upgrade_test.go
Included review availability: This review used your included allowance. 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: Advanced
- Run ID:
78fb89b3-db34-48de-876d-d9d6db5fc679
📒 Files selected for processing (8)
internal/cmd/skupper/connector/nonkube/connector_create_test.gointernal/cmd/skupper/connector/nonkube/connector_update_test.gointernal/cmd/skupper/listener/nonkube/listener_create_test.gointernal/cmd/skupper/listener/nonkube/listener_update_test.gointernal/cmd/skupper/site/nonkube/site_create_test.gointernal/cmd/skupper/system/nonkube/system_install.gointernal/cmd/skupper/system/nonkube/system_install_test.gointernal/nonkube/bootstrap/upgrade.go
Included review availability: This review used your included allowance. 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: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
798b666f-1597-4cfa-8ab6-ac39a2c53367
📒 Files selected for processing (4)
internal/nonkube/bootstrap/upgrade.gointernal/nonkube/bootstrap/upgrade_test.gointernal/nonkube/compat/site_state_renderer.gointernal/utils/tarball.go
Included review availability: This review used your included allowance. 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
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Propagate operational errors from router inspection. · upgrade.go:107-120
internal/nonkube/bootstrap/upgrade.go:107-120
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate operational errors from router inspection.
Upgradechecks router drift, then inspects each router again during backup. If the secondContainerInspectreturns an operational error,backupRouterContainersskips that namespace.upgradeNamespacesthen has no entry to upgrade and returns success. Preserve the existing not-found handling, but return other inspection errors.Suggested fix
import ( "errors" "fmt" "log/slog" "os" "os/user" + "strings" @@ c, err := cli.ContainerInspect(containerName) - if err != nil || c == nil { + if err != nil { + if strings.Contains(err.Error(), "not found") { + continue + } + return backups, backupContainerNames, fmt.Errorf("namespace %q: failed to inspect router container: %w", ns, err) + } + if c == nil { continue }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9157acd1-165a-445d-b285-bbcccccd5a4a
📒 Files selected for processing (1)
internal/nonkube/bootstrap/upgrade.go
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
|
|
||
| // Remove the partially created controller if Install created it before | ||
| // failing, so the rename below does not hit a name collision. | ||
| if existing, inspectErr := cli.ContainerInspect(originalName); inspectErr == nil && existing != nil { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle controller inspection errors before rollback.
If the replacement controller still has originalName and ContainerInspect returns an error, the current condition skips ContainerRemove. The subsequent backup rename can fail because the replacement still owns the name, while the original controller remains stopped. Distinguish a missing container from other inspection errors. Preserve the backup and return an error that identifies the failed inspection.
…cking the image drift.
9060f7a to
a5b9363
Compare
fgiorgetti
left a comment
fgiorgetti
left a comment
There was a problem hiding this comment.
One last comment. If the system controller is running with docker and the reload-type is set to auto. The upgrade is not working due to permission issues.
… don't show an error if there are no namespaces to upgrade
| Platform: types.Platform(nsPlatform), | ||
| InputPath: sourcesPath, | ||
| } | ||
| siteState, err := w.bootstrap(cfg) |
There was a problem hiding this comment.
I am seeing one issue when the controller reloads a site due to a drifted router (reload-type=auto).
If a site is started with --enable-link-access, the allocated router access port is 55671 and 45671.
When the router image drifts, the new container is allocating 55672 and 45672, therefore existing incoming links stop working.
I believe we might need a change to detect currently allocated router access ports if enable-link-access has been defined and reuse them.
There was a problem hiding this comment.
Actually this is also happening when reload-type is manual.
There was a problem hiding this comment.
Thank you! I tested this before adding the backup part for the upgrade!
Resolves #2390
Summary by CodeRabbit
--upgradeoption toskupper system installto update the system controller and routers to their configured images.--upgradeoption cannot be combined with--reload-type.