Fix: Persist scylla_mode to cluster.conf for ccm add to remember build mode
#691
Conversation
ccm add to remember build mode
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #164 where the ccm add command would incorrectly use the release build mode instead of the original build mode (e.g., dev or debug) specified during cluster creation. The root cause is that when a cluster is created with --install-dir=/path/to/scylla/build/dev, the install directory is normalized to /path/to/scylla and the build mode information is lost when the cluster is reloaded.
Key Changes
- Persists the
scylla_modefield tocluster.confinScyllaCluster._update_config() - Restores
scylla_modewhen loading clusters viaClusterFactory.load() - Adds a unit test to verify YAML serialization/deserialization of
scylla_mode
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ccmlib/scylla_cluster.py | Saves scylla_mode to cluster.conf following the same pattern as scylla_version |
| ccmlib/cluster_factory.py | Restores scylla_mode attribute after loading ScyllaCluster instances |
| tests/test_common.py | Adds unit test for YAML persistence of scylla_mode field |
| def test_scylla_mode_persistence(): | ||
| """Test that scylla_mode is properly saved to and loaded from cluster.conf. | ||
| This test verifies the fix for issue #164 where `ccm add` command would forget | ||
| which build mode was used during cluster creation (e.g., dev vs release). | ||
| """ | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| # Create a mock cluster.conf file with scylla_mode | ||
| cluster_conf = os.path.join(tmpdir, 'cluster.conf') | ||
| config_data = { | ||
| 'name': 'test_cluster', | ||
| 'nodes': ['node1'], | ||
| 'seeds': ['node1'], | ||
| 'partitioner': None, | ||
| 'install_dir': '/path/to/scylla', | ||
| 'config_options': {}, | ||
| 'log_level': 'INFO', | ||
| 'use_vnodes': False, | ||
| 'id': 0, | ||
| 'ipprefix': None, | ||
| 'scylla_mode': 'dev', # This is what we're testing | ||
| } | ||
|
|
||
| with open(cluster_conf, 'w') as f: | ||
| YAML().dump(config_data, f) | ||
|
|
||
| # Read back and verify scylla_mode is preserved | ||
| with open(cluster_conf, 'r') as f: | ||
| loaded_data = YAML().load(f) | ||
|
|
||
| assert 'scylla_mode' in loaded_data | ||
| assert loaded_data['scylla_mode'] == 'dev' | ||
|
|
||
| # Also test with 'debug' mode | ||
| config_data['scylla_mode'] = 'debug' | ||
| with open(cluster_conf, 'w') as f: | ||
| YAML().dump(config_data, f) | ||
|
|
||
| with open(cluster_conf, 'r') as f: | ||
| loaded_data = YAML().load(f) | ||
|
|
||
| assert loaded_data['scylla_mode'] == 'debug' | ||
|
|
||
| # Test with 'release' mode | ||
| config_data['scylla_mode'] = 'release' | ||
| with open(cluster_conf, 'w') as f: | ||
| YAML().dump(config_data, f) | ||
|
|
||
| with open(cluster_conf, 'r') as f: | ||
| loaded_data = YAML().load(f) | ||
|
|
||
| assert loaded_data['scylla_mode'] == 'release' |
There was a problem hiding this comment.
This test only verifies that YAML can serialize/deserialize the scylla_mode field, but doesn't test the actual issue reported in #164. The test should verify:
- That
ScyllaCluster._update_config()actually savesscylla_modetocluster.conf - That
ClusterFactory.load()correctly restores thescylla_modewhen loading the cluster - That when a new node is added to the cluster (the actual use case from
addcommand forgets which build mode we use #164), it uses the correct build mode
Consider adding an integration test that:
- Creates a ScyllaCluster instance with a specific scylla_mode (e.g., 'dev')
- Calls
_update_config()to persist it - Uses
ClusterFactory.load()to reload the cluster - Verifies that the loaded cluster has the correct scylla_mode set
|
I've tried to compile scylla and try it out, but with zero luck (it's OOMing on linking) |
…ild mode When creating a cluster with `--install-dir=path/to/build/dev`, the build mode (dev/debug/release) was being extracted but not saved to cluster.conf. This caused nodes added later via `ccm add` to use the wrong build mode (defaulting to release). This fix: 1. Saves `scylla_mode` to cluster.conf in ScyllaCluster._update_config() 2. Restores `scylla_mode` from cluster.conf in ClusterFactory.load() 3. Adds a unit test to verify scylla_mode persistence Fixes #164 Co-authored-by: fruch <[email protected]>
fruch
force-pushed
the
copilot/fix-ccm-add-build-mode
branch
from
98986ce to
c65e0d5
Compare
|
I'm not able to test: $ ./ccm start
Error starting node node1: unable to connect to scylla-jmx port 127.0.0.1:7199There's no jmx, so I don't see why it tries to connect. |
./ccm node1 showlog ? |
$ ./ccm node1 showlog
Traceback (most recent call last):
File "/home/avi/scylla-ccm/./ccm", line 6, in <module>
main()
~~~~^^
File "/home/avi/scylla-ccm/ccmlib/bin/__init__.py", line 75, in main
cmd.run()
~~~~~~~^^
File "/home/avi/scylla-ccm/ccmlib/cmds/node_cmds.py", line 105, in run
os.execvp(pager, (pager, log))
~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
File "<frozen os>", line 604, in execvp
File "<frozen os>", line 646, in _execvpe
File "<frozen os>", line 637, in _execvpe
FileNotFoundError: [Errno 2] No such file or directorGuessing the problem is that I do have a tools/jmx directory. Need to improve detection. |
|
I reproduced the original complaint, and confirm that it is solved with the patch. |
at least that something, |
When creating a cluster with
--install-dir=path/to/build/dev, the build mode was extracted but not persisted. Nodes added later viaccm adddefaulted toreleasemode instead of the originaldevmode.Changes
ccmlib/scylla_cluster.py: Savescylla_modetocluster.confin_update_config()ccmlib/cluster_factory.py: Restorescylla_modewhen loading cluster viaClusterFactory.load()tests/test_common.py: Add unit test forscylla_modepersistenceFixes #164
Original prompt
addcommand forgets which build mode we use #164💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.