fix: Disallow ISO sync on shared storage within a cluster.

[gyptazy]

fix: Disallow ISO sync on shared storage within a cluster.
ISO sync might end up in confusions when using shared
storage in a cluster where all data is already present
for all nodes (unless explicitly excluded).

Details:
This simply doesn't show up for storage types that are declared by shared: 1, as all data is already available for all nodes.

Result in UI:
ISOs on storages that are defined as shared should not be visible, as there isn't any need for a sync. Attached, an example of an ISO of 'griml' on a shared NFS which won't get shown anymore while the local ISOs on the nodes LVM are still present:

Sponsored-by: credativ GmbH https://credativ.de

[qodo-code-review]

Review Summary by Qodo

(Agentic_describe updated until commit cc64118)

Disallow ISO/template sync on shared storage in clusters
🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Prevent ISO/template sync on shared storage within clusters
• Add validation for content_type parameter in sync endpoints
• Implement _get_syncable_storage() method to check storage eligibility
• Filter shared storage from sync status matrix calculations

Diagram

flowchart LR
  A["Sync Request"] --> B["Validate content_type"]
  B --> C["Check Storage Eligibility"]
  C --> D{Is Shared?}
  D -->|Yes| E["Reject Sync"]
  D -->|No| F["Proceed with Sync"]
  G["Sync Status Matrix"] --> H["Filter Shared Storage"]
  H --> I["Return Non-Shared Only"]

Loading

File Changes

1. pegaprox/api/storage.py 🐞 Bug fix +8/-1

Add storage validation to sync endpoints

• Added content_type validation to reject invalid types (only 'iso' or 'vztmpl' allowed)
• Added call to _get_syncable_storage() in iso_sync_trigger() to validate storage before sync
• Added same validation to iso_sync_all() endpoint
• Removed trailing blank line from iso_sync_last_result()

pegaprox/api/storage.py

---

2. pegaprox/core/manager.py 🐞 Bug fix +35/-2

Implement shared storage detection and validation

• Added _storage_is_shared() static method to detect shared storage entries
• Added _get_syncable_storage() method to validate storage eligibility for sync operations
• Modified get_content_sync_status() to exclude shared storage from sync matrix
• Added storage validation checks in sync_content_to_nodes() for both source and target nodes

pegaprox/core/manager.py

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

1. Misleading shared-storage error 🐞 Bug ⚙ Maintainability ⭐ New

Description

_get_syncable_storage() returns an error message that always says “ISO sync” even when the caller is
syncing templates (content_type='vztmpl'), which can confuse users and UI error displays. This is
reachable because the API explicitly allows content_type='vztmpl' for ISO sync endpoints.

Code

pegaprox/core/manager.py[R10668-10670]

+            if self._storage_is_shared(storage):
+                return None, f"Storage {storage_name} is shared; ISO sync is only available for non-shared storage"
+            if not storage.get('active'):

Evidence

The manager hard-codes “ISO sync” in the shared-storage error path, while the API layer allows
template sync (vztmpl) and passes that content_type through to the manager, making the misleading
message observable to users.

pegaprox/core/manager.py[10661-10671]
pegaprox/api/storage.py[2467-2477]
pegaprox/api/storage.py[2494-2498]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When syncing templates (`content_type='vztmpl'`), `_get_syncable_storage()` can return an error string that still says “ISO sync”, which is misleading.

## Issue Context
The storage sync API supports both `iso` and `vztmpl`, and `_get_syncable_storage()` is called with the user-provided `content_type`.

## Fix Focus Areas
- pegaprox/core/manager.py[10666-10671]

## Suggested fix
Update the shared-storage error to be generic (e.g., “content sync”) or parameterized based on `content_type` (e.g., `f"...; {content_type} sync is only available ..."`), so template sync errors do not mention ISO sync.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Wrong error when offline 🐞 Bug ☼ Reliability

Description

iso_sync_trigger() now calls _get_syncable_storage() before starting the sync thread; if the manager
is disconnected/unreachable, get_storage_list() returns an empty list and the API returns HTTP 400
"Storage … not found" instead of a connectivity error. This is a behavior regression (previously it
always returned success and the background job failed with "Not connected"), and it can cause
clients/UI to treat an infra issue as a user input error (no retry).

Code

pegaprox/api/storage.py[R2474-2477]

+    if hasattr(mgr, '_get_syncable_storage'):
+        _, storage_err = mgr._get_syncable_storage(source, storage, content_type)
+        if storage_err:
+            return jsonify({'error': storage_err}), 400

Evidence

The endpoint returns HTTP 400 directly from the new pre-check; _get_syncable_storage() depends on
get_storage_list(), which returns an empty list when it cannot connect, leading to a misleading
"not found" error. The repo already provides get_connected_manager() for correct 503 handling but
the endpoint bypasses it.

pegaprox/api/storage.py[2461-2478]
pegaprox/core/manager.py[10306-10321]
pegaprox/core/manager.py[10661-10673]
pegaprox/api/helpers.py[257-269]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`iso_sync_trigger()` pre-validates the requested storage via `mgr._get_syncable_storage(...)`. When the cluster manager is disconnected, `get_storage_list()` returns `[]`, which makes `_get_syncable_storage()` report `"Storage … not found"` and the endpoint returns HTTP 400. This misclassifies an offline/disconnected cluster as a client error.
### Issue Context
There is already a common API helper `get_connected_manager()` that returns a standardized 503 response for offline clusters. Using it here prevents misleading errors and keeps client-side retry logic correct.
### Fix Focus Areas
- pegaprox/api/storage.py[2455-2484]
- pegaprox/api/helpers.py[257-269]
- pegaprox/core/manager.py[10306-10321]
- pegaprox/core/manager.py[10661-10673]
### Suggested fix
1. In `iso_sync_trigger()`, replace direct `mgr = cluster_managers[cluster_id]` usage with `mgr, err = get_connected_manager(cluster_id)` and return `err` when present (503 for offline).
2. Optionally (defense-in-depth), update `_get_syncable_storage()` to explicitly return a `"Not connected"` error when it cannot fetch storages due to connectivity, instead of falling through to `"not found"`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit cc64118

Results up to commit cc64118

🐞 Bugs (1) 📘 Rule violations (0)

1. Wrong error when offline 🐞 Bug ☼ Reliability

Description

iso_sync_trigger() now calls _get_syncable_storage() before starting the sync thread; if the manager
is disconnected/unreachable, get_storage_list() returns an empty list and the API returns HTTP 400
"Storage … not found" instead of a connectivity error. This is a behavior regression (previously it
always returned success and the background job failed with "Not connected"), and it can cause
clients/UI to treat an infra issue as a user input error (no retry).

Code

pegaprox/api/storage.py[R2474-2477]

+    if hasattr(mgr, '_get_syncable_storage'):
+        _, storage_err = mgr._get_syncable_storage(source, storage, content_type)
+        if storage_err:
+            return jsonify({'error': storage_err}), 400

Evidence

The endpoint returns HTTP 400 directly from the new pre-check; _get_syncable_storage() depends on
get_storage_list(), which returns an empty list when it cannot connect, leading to a misleading
"not found" error. The repo already provides get_connected_manager() for correct 503 handling but
the endpoint bypasses it.

pegaprox/api/storage.py[2461-2478]
pegaprox/core/manager.py[10306-10321]
pegaprox/core/manager.py[10661-10673]
pegaprox/api/helpers.py[257-269]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`iso_sync_trigger()` pre-validates the requested storage via `mgr._get_syncable_storage(...)`. When the cluster manager is disconnected, `get_storage_list()` returns `[]`, which makes `_get_syncable_storage()` report `"Storage … not found"` and the endpoint returns HTTP 400. This misclassifies an offline/disconnected cluster as a client error.

### Issue Context
There is already a common API helper `get_connected_manager()` that returns a standardized 503 response for offline clusters. Using it here prevents misleading errors and keeps client-side retry logic correct.

### Fix Focus Areas
- pegaprox/api/storage.py[2455-2484]
- pegaprox/api/helpers.py[257-269]
- pegaprox/core/manager.py[10306-10321]
- pegaprox/core/manager.py[10661-10673]

### Suggested fix
1. In `iso_sync_trigger()`, replace direct `mgr = cluster_managers[cluster_id]` usage with `mgr, err = get_connected_manager(cluster_id)` and return `err` when present (503 for offline).
2. Optionally (defense-in-depth), update `_get_syncable_storage()` to explicitly return a `"Not connected"` error when it cannot fetch storages due to connectivity, instead of falling through to `"not found"`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Persistent review updated to latest commit cc64118