Doris-Breakwater commented on issue #68120: URL: https://github.com/apache/doris/issues/68120#issuecomment-5709888909
Breakwater-GitHub-Analysis-Slot: slot_489c0b76c69f ## Initial assessment **Verdict: confirmed high-impact cloud scale-in correctness bug.** The code-level race is present in both `master` and `branch-3.1`. The reported production sequence is consistent with the verified code paths, although the incident-specific timeline has not been independently validated from logs. ### Verified facts - `handle_decommission_node` and `handle_notify_decommissioned` each call `ResourceManager::modify_nodes` twice: once to delete the selected nodes and once to append them with the new status. - Every `modify_nodes` invocation creates, reads, writes, and commits its own transaction. Therefore, after the first successful commit and before the second successful commit, the persisted `InstanceInfoPB` really does not contain those nodes. This is not merely an in-memory cache artifact. - `get_cluster` and `get_instance` read that same instance record through independent transactions, so a reader in this interval can observe the node as absent. - FE `CloudClusterChecker` runs at `cloud_cluster_check_interval_second` (default 10 seconds), diffs nodes by endpoint, removes endpoints absent from the meta-service response, and creates a new `Backend` with a newly allocated ID when the endpoint reappears. This validates the mechanism behind the reported remove/re-add behavior. - The two-step implementation also has a failure-atomicity problem: if the re-add transaction fails after deletion commits, the node remains absent. Conversely, an error from the first call can be overwritten by the result of the second call because the handler does not stop between calls. ### Assessment of PR #68119 The proposed architectural direction is correct: `ResourceManager::update_cluster` reads and writes the instance in one transaction, and mutating the existing `NodeInfoPB` in place removes both the observable missing-node interval and the partial delete/re-add state. However, the current PR should address these points before merge: 1. **Preserve node identity checks.** The old path first scopes candidates with `get_node(cloud_unique_id)` and then matches the endpoint. The new helper scans the requested cluster and matches only `ip/host + heartbeat_port`. A stale or incorrect `cloud_unique_id` can therefore update a different node at the same endpoint. Match `cloud_unique_id` as well as the endpoint, or otherwise retain the old unique-ID scoping explicitly. 2. **Do not return success for zero or partial matches.** `flip_nodes_status_in_place` currently only logs when `matched_cnt != req_nodes.size()`, after which `update_cluster` commits and the RPC returns `OK`. Make matching one-to-one and return a non-empty error before commit unless every requested node is found exactly once. Add negative tests for an unknown unique ID, an endpoint mismatch, duplicate request entries, and a partially matched multi-node request. 3. **Strengthen the regression coverage.** The added test usefully proves final node count, order, and status, and it fails the current delete/append implementation because the order changes. It does not directly exercise a concurrent `get_cluster` reader. A deterministic sync-point/concurrency test would better lock down the stated invariant; at minimum, retain the order/count assertions and add the identity/error cases above plus both IP and host matching paths. 4. **Backport deliberately.** PR #68119 currently targets `master`, while the same two-transaction code is present on `branch-3.1`. After the master fix is validated, prepare and test the branch-specific backport rather than assuming identical cache-update internals. ### Missing incident evidence No additional evidence is required to confirm the code defect. To validate the exact production impact and define affected release scope, it would still be useful to attach: - the exact FE and meta-service build commits; - timestamp-correlated meta-service logs for both `modify_nodes` commits and the later `DROP_NODE` failure; - FE `CloudClusterChecker` logs showing the endpoint removal/re-add and old/new backend IDs; - the failed `DROP_NODE` response code/message and the compute-group state transition timeline. The issue currently has no labels, assignee, or milestone. Maintainers should add the project-standard cloud/meta-service bug and release/backport labels, assign a reviewer, amend the PR as above, run the focused cloud meta-service unit tests plus normal CI, and verify the fix on both `master` and `branch-3.1`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
