Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(registry): disable replacement of nodes that are active in subnets #3935

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

sasa-tomic
Copy link
Member

@sasa-tomic sasa-tomic commented Feb 13, 2025

Direct node replacements of nodes that are active in a subnet may result in unexpected behavior and potential problems in the current Consensus code.
So to be on the safe side we need to disable the functionality on the Registry side until the rest of the core protocol can handle it safely.

@sasa-tomic sasa-tomic self-assigned this Feb 13, 2025
@github-actions github-actions bot added the test label Feb 13, 2025
@sasa-tomic sasa-tomic changed the title test(node-management): disable node replacement tests fix(registry): disable replacement of nodes that are active in subnets Feb 13, 2025
@github-actions github-actions bot added fix and removed test labels Feb 13, 2025
@aterga aterga added the CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 label Feb 13, 2025
### Summary

- Disabled tests related to node replacement in subnet.

### Details

- Marked `should_add_node_and_replace_existing_node_in_subnet` as ignored in `do_add_node.rs` pending Consensus team support.
- Marked `should_replace_node_in_subnet_and_update_allowance` as ignored in `do_remove_node_directly.rs` pending Consensus team support.

### Additional Changes

- Introduced `replacements_of_nodes_in_subnets_enabled` flag set to `false` in `do_remove_node_directly.rs` to conditionally control node replacement logic.
@aterga aterga force-pushed the disable-node-subnet-replacement-tests branch from 7d4eccf to 8c1e08c Compare February 13, 2025 12:18
@sasa-tomic sasa-tomic marked this pull request as ready for review February 13, 2025 12:24
@sasa-tomic sasa-tomic requested a review from a team as a code owner February 13, 2025 12:24
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).

To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:

  1. Done.

  2. No canister behavior changes.

@basvandijk basvandijk disabled auto-merge February 13, 2025 14:06
@basvandijk basvandijk merged commit 4f60863 into master Feb 13, 2025
22 checks passed
@basvandijk basvandijk deleted the disable-node-subnet-replacement-tests branch February 13, 2025 14:06
marko-k0 pushed a commit that referenced this pull request Feb 14, 2025
#3935)

Direct node replacements of nodes that are active in a subnet may result
in unexpected behavior and potential problems in the current Consensus
code.
So to be on the safe side we need to disable the functionality on the
Registry side until the rest of the core protocol can handle it safely.

---------

Co-authored-by: CI Automation <[email protected]>
Co-authored-by: Arshavir Ter-Gabrielyan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 fix @idx @nns-team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants