Skip to content

DTEST-1 Infer datacenter from existing nodes when adding without explicit datacenter #704

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.

Sign up for GitHub

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

Jump to bottom
Open
Copilot wants to merge 7 commits into
base: master
Choose a base branch
from
Open

DTEST-1 Infer datacenter from existing nodes when adding without explicit datacenter #704

Copilot wants to merge 7 commits into from
+171 −0

Conversation

Copy link
Contributor

Copilot AI commented Dec 24, 2025 •
edited
Loading

  • Understand the issue: When adding a new node without explicitly specifying data_center, the cassandra-rackdc.properties file is not properly updated
  • Create a test to reproduce the issue
  • Implement datacenter inference from existing nodes
  • Update tests to verify the new behavior
  • Refactor tests to use pytest style with fixtures (separate commit)
  • Add pytest style guidelines to copilot instructions (separate commit)

Summary

Fixed the issue by making the add() method infer datacenter from existing nodes when not explicitly specified. Refactored tests to use pytest best practices with fixtures instead of class-based tests. Added pytest style guidelines to copilot instructions in separate commits as requested.

Commit Structure

  1. Infer datacenter from existing nodes when not specified
  2. Revert combined refactoring commit
  3. Refactor tests to use pytest style with fixtures (tests only)
  4. Add pytest style guidelines to copilot instructions (instructions only)

Changes

  • ccmlib/cluster.py: Add logic to infer datacenter and rack from existing nodes
  • tests/test_add_node_without_datacenter.py: Refactored to use pytest fixtures
  • .github/copilot-instructions.md: Added Test Style Guidelines section

Tests now follow pytest best practices with function-based tests and reusable fixtures.

Original prompt

This section details on the original issue you should resolve

<issue_title>Can't add new node to the cluster if I don't specify data-center explicitly.</issue_title>
<issue_description>In materialized_views_test.py I wanted to add a test doing something like:

        session = self.prepare(rf=rf)
        session.execute("CREATE TABLE t (id int PRIMARY KEY, v int)")
        node4 = new_node(self.cluster)
        node4.start(wait_other_notice=True, wait_for_binary_proto=True)

This did not work... The fourth Scylla instance failed to start up. The problem was that the file
/home/nyh/.dtest/dtest-kawO4S/test/node4/conf/cassandra-rackdc.properties instead of containing

dc=dc1
rack=RAC1

as was expected, it contain a comment with explanation, but no content, apparently copied from the default Cassandra configuration.

If I change the code to explicitly specify the data center:

        node4 = new_node(self.cluster, data_center='dc1')

Everything works.</issue_description>

<agent_instructions>First add a test to check if this is still an issue, and suggest a fix and wait</agent_instructions>

Comments on the Issue (you are @copilot in this section)

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

github-code-quality[bot]
github-code-quality bot found potential problems Dec 24, 2025
View reviewed changes
Copilot AI changed the title [WIP] Fix issue with adding new node to cluster without data center Fix topology file creation when adding nodes without explicit datacenter Dec 24, 2025
Copilot AI requested a review from fruch December 24, 2025 20:47
fruch
fruch requested changes Dec 24, 2025
View reviewed changes
Copilot AI changed the title Fix topology file creation when adding nodes without explicit datacenter Infer datacenter from existing nodes when adding without explicit datacenter Dec 24, 2025
Copilot AI requested a review from fruch December 24, 2025 21:35
fruch
fruch reviewed Dec 25, 2025
View reviewed changes
Copilot AI requested a review from fruch December 25, 2025 15:57
@fruch fruch force-pushed the copilot/fix-new-node-data-center branch from c4908d5 to 6d9f35c Compare December 29, 2025 18:54
@fruch fruch marked this pull request as ready for review December 29, 2025 18:55
@fruch fruch force-pushed the copilot/fix-new-node-data-center branch from 6d9f35c to d2be848 Compare January 15, 2026 14:40
fruch
fruch approved these changes Jan 15, 2026
View reviewed changes
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

@dani-tweig dani-tweig changed the title Infer datacenter from existing nodes when adding without explicit datacenter DTEST-1 Infer datacenter from existing nodes when adding without explicit datacenter Jan 18, 2026
Copilot AI and others added 7 commits January 18, 2026 11:09
@fruch
Add test to reproduce the datacenter issue
379493f 
Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
@fruch
Fix: Always update topology files when adding nodes
12185cb 
Previously, __update_topology_files() was only called when data_center was
explicitly provided. This caused issues when adding nodes without specifying
data_center, as the cassandra-rackdc.properties file would not be created
with default values (dc1, RAC1).

The fix moves __update_topology_files() outside the data_center check,
ensuring topology files are always updated. The method itself handles None
values by using defaults.

Fixes issue: "Can't add new node to the cluster if I don't specify data-center explicitly."

Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
@fruch
Infer datacenter from existing nodes when not specified
170dbd8 
Instead of always defaulting to dc1, the add() method now looks at existing
nodes and infers the datacenter from them when not explicitly specified.
This ensures that new nodes added to a cluster inherit the datacenter of
existing nodes, which is more intuitive behavior.

- If datacenter is not specified and there are existing nodes with a datacenter,
  use theirs
- Also infer rack from existing nodes if not specified
- Updated tests to verify the new inference behavior

Addresses review feedback from @fruch

Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
@fruch
Refactor tests to use pytest style with fixtures
ad069c7 
Switch from class-based tests to function-based tests using pytest fixtures
to minimize code duplication and follow pytest best practices:

- Use @pytest.fixture for setup (temp_dir, mock_cluster, mock_node)
- Convert class methods to standalone test functions
- Remove manual try/finally cleanup in favor of fixture teardown
- Create factory fixture (mock_node) for creating multiple mock nodes

This makes tests more readable and maintainable while keeping the same
test coverage and assertions.

Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
@fruch
Revert "Refactor tests to use pytest style with fixtures"
0bf3a68 
This reverts commit 8e79170.
@fruch
Refactor tests to use pytest style with fixtures
07311da 
Switch from class-based tests to function-based tests using pytest fixtures
to minimize code duplication and follow pytest best practices:

- Use @pytest.fixture for setup (temp_dir, mock_cluster, mock_node)
- Convert class methods to standalone test functions
- Remove manual try/finally cleanup in favor of fixture teardown
- Create factory fixture (mock_node) for creating multiple mock nodes

This makes tests more readable and maintainable while keeping the same
test coverage and assertions.
@fruch
Add pytest style guidelines to copilot instructions
27f6eb1 
Document best practices for writing tests in scylla-ccm:
- Use function-based tests with pytest fixtures
- Create reusable fixtures to minimize code duplication
- Use @pytest.fixture decorator for setup/teardown
- Prefer fixtures over manual try/finally blocks

This provides guidance for contributors to write tests that follow
the project's conventions and pytest best practices.
@fruch fruch force-pushed the copilot/fix-new-node-data-center branch from d2be848 to 27f6eb1 Compare January 18, 2026 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@github-code-quality github-code-quality[bot] github-code-quality[bot] left review comments

@fruch fruch fruch approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@fruch fruch

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't add new node to the cluster if I don't specify data-center explicitly.

2 participants

@fruch