Fix node-recycler filtering with cached layout and live-node retries

[Rui Gao (hippogr)]

Summary

This PR hardens node-recycler node selection and query scoping while keeping behavior safe under transient dependency failures.

What this PR changes

1) Endpoint-scoped Kusto queries

Adds Endpoint == self.endpoint filters to key node action/status query paths so data access is isolated to the current cluster endpoint.

• src/kusto-sdk/ltp_kusto_sdk/features/node_action/client.py
  • update_node_action duplicate check query
  • get_node_actions
  • get_latest_node_action
  • get_latest_action_by_state
• src/kusto-sdk/ltp_kusto_sdk/features/node_status/client.py
  • get_node_status
  • get_nodes_by_status

This prevents cross-cluster contamination when hostnames overlap.

2) Policy-based filtering in node-recycler

Introduces centralized candidate filtering for OFR/operate/validate flows in:

• src/alert-manager/src/node-recycler/recycler.py

Filtering can enforce:

• membership in layout (layout.yaml)
• membership in current live Kubernetes nodes
• readiness (Ready=True) for validation-stage filtering

3) Load layout once per process

Adds one-time layout initialization and caching:

• _initialize_layout_nodes()
• _layout_nodes_cache, _layout_nodes_loaded, _layout_nodes_load_success

layout.yaml is parsed once and reused to reduce repeated file I/O.

4) Live-node retry with fail-fast stage skip

Adds live-node retrieval retries with exponential backoff:

• _load_live_nodes_with_retry(stage)
• defaults via env:
  • LIVE_NODES_RETRY_ATTEMPTS (default 3)
  • LIVE_NODES_RETRY_INTERVAL_SECONDS (default 1)

If all retries fail, that stage is skipped and naturally retried in the next loop iteration.

5) Graceful fallback for layout issues

If layout load fails or layout is empty:

• log warning
• continue processing without layout-based filtering

This avoids hard-stopping the pipeline solely due to layout data issues.

6) Deployment/runtime support

• Mounts pai-configuration into node-recycler container at /pai-cluster-config:
  • src/alert-manager/deploy/alert-manager-deployment.yaml.template
• Adds YAML parsing dependency:
  • src/alert-manager/src/node-recycler/requirements.txt (PyYAML)

7) Test coverage improvements

Extended node-recycler tests:

• src/alert-manager/src/node-recycler/tests/test_recycler.py

Added/updated coverage for:

• layout cache load-once behavior
• no-layout-required path
• live-node retry success and retry exhaustion
• fallback behavior when layout load fails
• fallback behavior when layout is empty
• stabilized VMSS pipeline tests by replacing brittle ordered side effects with argument-based side effects

Also updated query assertion for endpoint filtering:

• src/kusto-sdk/tests/test_node_status_client.py

Validation

Executed:

• pytest -q src/alert-manager/src/node-recycler/tests/test_recycler.py

Result:

• 29 passed

Notes

src/kusto-sdk/tests/test_node_status_client.py was not runnable in this environment due to missing test dependency azure.kusto.ingest during collection.

[Copilot]

Pull request overview

This PR hardens node selection and Kusto query scoping to avoid cross-endpoint contamination, and makes node-recycler more resilient by introducing centralized filtering with cached layout parsing and retrying live-node discovery.

Changes:

• Adds endpoint-scoped filters (Endpoint == self.endpoint) to node status/action Kusto queries.
• Introduces policy-based candidate filtering in node-recycler, including one-time layout caching and live-node retries with backoff.
• Updates deployment/runtime inputs (mount cluster layout config, add PyYAML) and extends tests for the new filtering/retry behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/kusto-sdk/ltp_kusto_sdk/features/node_status/client.py	Adds endpoint scoping to node status queries and adjusts the “latest status” query shape.
src/kusto-sdk/ltp_kusto_sdk/features/node_action/client.py	Adds endpoint scoping to node action queries (duplicate check + retrieval paths).
src/kusto-sdk/tests/test_node_status_client.py	Updates assertions to require endpoint filtering in generated queries.
src/alert-manager/src/node-recycler/recycler.py	Adds cached layout parsing, live-node loading with retry/backoff, and centralized policy filtering for OFR/operate/validate.
src/alert-manager/src/node-recycler/tests/test_recycler.py	Extends test coverage for caching, fallback behavior, and retry exhaustion/success paths; stabilizes VMSS test side effects.
src/alert-manager/src/node-recycler/requirements.txt	Adds PyYAML dependency for parsing layout.yaml.
src/alert-manager/deploy/alert-manager-deployment.yaml.template	Mounts pai-configuration into the node-recycler container for layout access.

Suppressed comments (2)

src/kusto-sdk/ltp_kusto_sdk/features/node_status/client.py:150

• The endpoint value is interpolated directly into the Kusto query string without escaping. If CLUSTER_ID/endpoint ever contains a single quote, the query will become invalid (and this pattern can open up query-injection risks). Escaping single quotes makes the query construction more robust.

        query = (
            f"{self.table_name}"
            f" | where HostName == '{hostname}'"
            f" | where Endpoint == '{self.endpoint}'"
        )

src/kusto-sdk/ltp_kusto_sdk/features/node_status/client.py:218

• This query also interpolates endpoint into the Kusto string without escaping. Escaping single quotes here avoids malformed queries if endpoint contains unexpected characters and keeps query construction consistent with other string sanitization in this client.

            query = f"""
            {self.table_name}
            | where Endpoint == '{self.endpoint}'
            {timestamp_condition}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Rui Gao (hippogr)]

I found two additional issues that should be considered before merging:

1. Endpoint isolation is still incomplete: In src/kusto-sdk/ltp_kusto_sdk/features/node_action/client.py:279-290, neither the next_available_ts subquery nor the final query in find_triaged_failure() filters on Endpoint == self.endpoint. If different clusters contain the same hostname, an action from another endpoint can incorrectly truncate the current cluster's time window, and triaged actions from another endpoint can be returned. Please scope both table scans by endpoint and add a cross-endpoint test.

2. A transient layout load failure permanently disables layout filtering: In src/alert-manager/src/node-recycler/recycler.py:61-72, _layout_nodes_loaded is set to True regardless of whether loading succeeds. If the ConfigMap is temporarily unreadable, invalid, or empty at startup, the process never reloads it after the configuration recovers, leaving layout filtering in fail-open mode for the lifetime of the process. Please cache only successful, non-empty results, or retry/refresh failed and empty loads.

[Rui Gao (hippogr)]

Follow-up on my earlier layout comment: after reconsidering the deployment model, I recommend fail-fast startup validation instead of retrying or falling back.

This PR already mounts the pai-configuration ConfigMap at /pai-cluster-config, and NodeRecycler reads /pai-cluster-config/layout.yaml by default. The layout should therefore be treated as a required local startup dependency:

• Load and validate layout.yaml once during startup, before entering the pipeline loop.
• If the file is missing, unreadable, malformed, or contains no usable node names, raise a clear error and terminate the process.
• Remove the fail-open behavior that disables layout filtering when loading fails or returns an empty set.
• Keep the successfully parsed node set immutable for the process lifetime; no layout retry or refresh logic is needed. Kubernetes can restart the container when the mounted configuration is unavailable or invalid.

The retry environment variables should follow the same startup-validation principle. Parse LIVE_NODES_RETRY_ATTEMPTS and LIVE_NODES_RETRY_INTERVAL_SECONDS with explicit validation and a clear error message for invalid or out-of-range values, rather than silently falling back. Runtime retries should remain limited to transient REST Server failures.

[Rui Gao (hippogr)]

Addressed review feedback in commit 9e08edc:\n\n1) Endpoint isolation in \n- Added endpoint scoping to both table scans in the KQL (the subquery and the final triaged-actions query).\n- Also escaped endpoint when embedding it in KQL ().\n- Files:\n - \n - (added assertion test for endpoint scoping in this query path)\n\n2) Layout loading behavior\n- Switched to fail-fast startup/dependency behavior for layout initialization:\n - raise if layout load fails\n - raise if parsed node set is empty\n - raise if cached layout state is unexpectedly empty after successful init\n- Removed fail-open fallback in layout filtering.\n- Files:\n - \n - (updated/added tests for runtime-error behavior)\n\nValidation run:\n- .............................. [100%]
30 passed in 0.27s -> \n\nNote: tests in this environment are limited by missing local test dependencies, but query-level coverage was added for the changed path.

[Rui Gao (hippogr)]

Correction note (previous comment had shell-escaping artifacts). Below is the intended update.

Addressed review feedback in commit 9e08edc:

1. Endpoint isolation in find_triaged_failure()

• Added endpoint scoping to both table scans in the KQL (the next_available_ts subquery and the final triaged-actions query).
• Also escaped endpoint when embedding it in KQL (self.endpoint.replace("'", "''")).
• Files:
  • src/kusto-sdk/ltp_kusto_sdk/features/node_action/client.py
  • src/kusto-sdk/tests/test_node_action_client.py (added assertion test for endpoint scoping in this query path)

2. Layout loading behavior

• Switched to fail-fast startup/dependency behavior for layout initialization:
  • raise if layout load fails
  • raise if parsed node set is empty
  • raise if cached layout state is unexpectedly empty after successful init
• Removed fail-open fallback in layout filtering.
• Files:
  • src/alert-manager/src/node-recycler/recycler.py
  • src/alert-manager/src/node-recycler/tests/test_recycler.py (updated/added tests for runtime-error behavior)

Validation run:

• pytest -q src/alert-manager/src/node-recycler/tests/test_recycler.py -> 30 passed

Note: kusto-sdk tests in this environment are limited by missing local test dependencies, but query-level coverage was added for the changed find_triaged_failure() path.

[Rui Gao (hippogr)]

Thanks for the update. The endpoint scoping in find_triaged_failure() and the fail-fast layout initialization now look correct.

There are three remaining review items to check:

1. Retry environment variables are not explicitly validated yet. LIVE_NODES_RETRY_ATTEMPTS and LIVE_NODES_RETRY_INTERVAL_SECONDS are still parsed with int() at class definition time. A non-integer fails with a generic import-time ValueError, while negative values are silently normalized later by max(). To match the agreed fail-fast configuration model, please validate them explicitly at startup with clear errors and enforce attempts >= 1 and interval >= 0.

2. Endpoint escaping is only applied in find_triaged_failure(). The endpoint filters newly added to update_node_action(), get_node_actions(), get_latest_node_action(), get_latest_action_by_state(), get_node_status(), and get_nodes_by_status() still interpolate self.endpoint directly. Please consider a shared KQL string-literal escaping helper and use it consistently across all affected queries. A test with an endpoint containing a single quote would verify the escaping behavior.

3. The get_node_status() return annotation still does not match its behavior. It returns None when no record exists, so the return type should be Optional[NodeStatusRecord].

The first two are recommended before merging; the annotation fix is low priority but straightforward.

[Rui Gao (hippogr)]

There is still a PostgreSQL backend compatibility issue in the narrowed implementation.

alert-parser, node-issue-classifier, and node-recycler obtain their clients through ltp_storage.factory, so the returned object can be either a Kusto client or a PostgreSQL client depending on LTP_STORAGE_BACKEND_DEFAULT. The updated callers now invoke methods such as:

client.get_node_status(node, use_current_endpoint=True)
client.get_latest_node_action(node, use_current_endpoint=True)

The new keyword is supported by the Kusto implementations, but the corresponding PostgreSQL methods still have their original signatures and do not accept use_current_endpoint. When one of these services is configured with LTP_STORAGE_BACKEND_DEFAULT=postgresql, the first affected call will fail with:

TypeError: ... got an unexpected keyword argument 'use_current_endpoint'

Please avoid passing this Kusto-specific option through backend-neutral callers. Since the goal is to keep the SDK impact narrow, one approach is to remove the changes from unrelated services and have node-recycler pass the option only when its configured backend is Kusto; PostgreSQL calls should retain their existing signatures. A backend-parameterized test through ltp_storage.factory would catch this regression.