fix: skip non-HA cinder-volume deploy when HA app exists#797
Conversation
ahmad-can
force-pushed
the
fix/skip-cinder-volume-deploy-when-ha-exists
branch
from
88128b1 to
4202f08
Compare
There was a problem hiding this comment.
Pull request overview
Prevents duplicate cinder-volume deployments by adding is_skip logic on both sides of the HA/non-HA split, and rewrites the non-HA backend's principal to the HA app at deploy time if the HA app already exists.
Changes:
- Add
DeployCinderVolumeApplicationStep.is_skipthat skips the HA deploy when every registered storage backend'sprincipalis non-HA. - Add
DeploySpecificCinderVolumeStep.is_skipand arun()fixup that skip / re-target non-HA deploys when the HAcinder-volumeapp already exists in the model. - Add unit tests for the new skip/proceed branches in both files.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sunbeam-python/sunbeam/steps/cinder_volume.py | New is_skip on the HA deploy step, gated by registered storage backends. |
| sunbeam-python/sunbeam/storage/steps.py | New ApplicationNotFoundException import; run() override of principal_application when HA app exists; new is_skip HA-app-exists branch for the specific (non-HA) deploy step. |
| sunbeam-python/tests/unit/sunbeam/steps/test_cinder_volume.py | Unit tests for the new HA is_skip branches. |
| sunbeam-python/tests/unit/sunbeam/storage/test_steps.py | Unit tests for the new specific cinder-volume is_skip branches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ahmad-can
force-pushed
the
fix/skip-cinder-volume-deploy-when-ha-exists
branch
2 times, most recently
from
b966564 to
1d386b8
Compare
hemanthnakkina
left a comment
hemanthnakkina
left a comment
There was a problem hiding this comment.
minor inline comments
| try: | ||
| self.jhelper.get_application(APPLICATION, self.model) | ||
| backends[backend_key]["principal_application"] = APPLICATION | ||
| actual_principal = APPLICATION |
There was a problem hiding this comment.
Can you add log to mention the principal is overrided
| self.jhelper.get_application(APPLICATION, self.model) | ||
| backends[backend_key]["principal_application"] = APPLICATION | ||
| actual_principal = APPLICATION | ||
| except ApplicationNotFoundException: |
There was a problem hiding this comment.
Need to handle JujuException here.. its done in other class
| deployment_with_tfhelpers, "telemetry" | ||
| ) | ||
|
|
||
| def test_is_skip_no_backends_proceeds( |
There was a problem hiding this comment.
Add test where there is HA and non-HA backend exists
Prevent duplicate cinder-volume applications by checking for existing HA app before deploying non-HA, and skipping HA deploy when only non-HA backends exist. Signed-off-by: Ahmad Hassan <ahmad.hassan@canonical.com>
ahmad-can
force-pushed
the
fix/skip-cinder-volume-deploy-when-ha-exists
branch
from
1d386b8 to
61b695a
Compare
|
I haven't looked at the content yet, but I don't understand Do you mean you want to deploy non-HA app on the HA instance when it exists? |
|
@gboutry Not deploying a non-HA app on the HA instance.Preventing the non-HA app from being deployed at all — the non-HA backend reuses the existing HA app as its principal instead. |
I still don't get it. We want the You don't want the non-ha to be attached to multi-units deployments until we support active/passive HA. |
|
closing as this is the expected behaviour to have cinder-volume-noha running for non-ha storage backends and cinder-volume co exist for ceph. |
Prevent duplicate cinder-volume applications by checking for existing HA app before deploying non-HA, and skipping HA deploy when only non-HA backends exist.