scylla_node: ignore scylla-tools config files if scylla-tools is missing

[tchaikov]

this change is created in the same spirit of 3a88640, so that we don't assume the existence of scylla-tools. otherwise we could have following failure:

07:51:05  Traceback (most recent call last):
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/bin/ccm", line 74, in <module>
07:51:05      cmd.run()
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/cmds/cluster_cmds.py", line 269, in run
07:51:05      cluster.populate(self.nodes, self.options.debug, use_vnodes=self.options.vnodes, ipprefix=self.options.ipprefix, ipformat=self.options.ipformat)
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/cluster.py", line 330, in populate
07:51:05      self.new_node(i, debug=debug, initial_token=tk, data_center=dc, rack=rack)
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/cluster.py", line 337, in new_node
07:51:05      node = self.create_node(name=f'node{i}',
07:51:05             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/scylla_cluster.py", line 94, in create_node
07:51:05      return ScyllaNode(name, self, auto_bootstrap, None,
07:51:05             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/scylla_node.py", line 53, in __init__
07:51:05      super().__init__(name, cluster, auto_bootstrap,
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/node.py", line 128, in __init__
07:51:05      self.import_config_files()
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/scylla_node.py", line 979, in import_config_files
07:51:05      self.__copy_logback_files()
07:51:05    File "/jenkins/workspace/scylla-master/gating-dtest-release/scylla/.local/lib/python3.12/site-packages/ccmlib/scylla_node.py", line 1003, in __copy_logback_files
07:51:05      shutil.copy(os.path.join(self.get_tools_java_dir(), 'conf', 'logback-tools.xml'),

Fixes #617
Signed-off-by: Kefu Chai [email protected]

[tchaikov]

• testing with syuu1228/scylla.git:drop_tools_from_unified_package: https://jenkins.scylladb.com/job/scylla-master/job/byo/job/byo_build_tests_dtest/2507/
• testing without this branch: https://jenkins.scylladb.com/job/scylla-master/job/releng-testing/job/gating-dtest-release-with-consistent-topology-changes/148/

[tchaikov]

@fruch @syuu1228 @yaronkaikov hii everyone, i'd appreciate a review of this change. There are still failing tests in the Jenkins build . however, I believe this change addresses some aspects of the issue and brings us closer to resolving the failing tests.

[fruch]

@fruch @syuu1228 @yaronkaikov hii everyone, i'd appreciate a review of this change. There are still failing tests in the Jenkins build . however, I believe this change addresses some aspects of the issue and brings us closer to resolving the failing tests.

we are still left with ~500 tests failing with that removal change.
merging this one doesn't really get scylladb/scylla-cluster-tests#8799 and closer to being finalized

[tchaikov]

as you put, there are two issues.

• we assume the existence of scylla-tools. this is tracked by ccmlib should work when scylla-tools not installed #617
• we do not use the native nodetool or the native alternatives are missing. this is tracked by nonroot tests should call native nodetool scylla-cluster-tests#8799

but i i don't think the later is a dependency of the fix of #617.

[fruch]

as you put, there are two issues.

• we assume the existence of scylla-tools. this is tracked by ccmlib should work when scylla-tools not installed #617
• we do not use the native nodetool or the native alternatives are missing. this is tracked by nonroot tests should call native nodetool scylla-cluster-tests#8799

but i i don't think the later is a dependency of the fix of #617.

it was a mistake copying the wrong item scylladb/scylladb#20740 is still breaking 500 dtests, so having this in doesn't address all of those.

[tchaikov]

fair enough. i explained this change does not address all of them.

[fruch]

fair enough. i explained this change does not address all of them.

I don't know if we should give it up, but we are probably gonna have more changes needed for this effort.

[tchaikov]

fair enough. i explained this change does not address all of them.

I don't know if we should give it up, but we are probably gonna have more changes needed for this effort.

i asked for review , and explained why this change should be included. but failed to get the review or the acknowledgment or any comment regarding the change made in this PR, so that i could improve it. what i have is

having this in doesn't address all of those.

in my opinion, it's was a "NO". and it was not constructive. so i didn't know what else i can do, but to close it.

[fruch]

fair enough. i explained this change does not address all of them.

I don't know if we should give it up, but we are probably gonna have more changes needed for this effort.

i asked for review , and explained why this change should be included. but failed to get the review or the acknowledgment or any comment regarding the change made in this PR, so that i could improve it. what i have is

having this in doesn't address all of those.

in my opinion, it's was a "NO". and it was not constructive. so i didn't know what else i can do, but to close it.

You got a review, and a question about it. not the review you wanted.

My concern is putting in multiple changes that we can't later track for backporting into pushing forward a change that can't be merged.

I.e. that doesn't seem to be the last of it, if there are 500 failing tests.

[tchaikov]

@fruch what was the question? i am confused. i read all your comments again. but cannot find a question. and if you have concern on tracking multiple changes. how is it related to the change i proposed? i never claim that this change is the last one. but i wanted to limit the scope of a certain change.

[fruch]

@fruch what was the question? i am confused. i read all your comments again. but cannot find a question. and if you have concern on tracking multiple changes. how is it related to the change i proposed? i never claim that this change is the last one. but i wanted to limit the scope of a certain change.

#618

Was to limit the scope as well wasn't it, but was solving all of what it's intended.

If there are changes needed for a PR to come into scylla, I would rather collect all what is required from one repo or other, then have a long long list of small changes.

Regardless, at any point I didn't decline this PR, I was asking questions, even if you didn't find the question marks.

[roydahan]

It seems like this PR is a step in the correct direction.
Most (or even all) of the failing tests are failing for 3 main tools that are missing:

• sstableloader
• sstabledump
• cqlsh (which shouldn't be taken from scylla-tools anyway).

[fruch]

also look like the cqlsh related tests were missing cause of of using os.path.join with None, and this should fix it:

diff --git a/ccmlib/scylla_node.py b/ccmlib/scylla_node.py
index 96fc5d6..7627c4b 100644
--- a/ccmlib/scylla_node.py
+++ b/ccmlib/scylla_node.py
@@ -153,9 +153,9 @@ class ScyllaNode(Node):
             os.path.join(self.node_install_dir, 'share', 'cassandra', BIN_DIR),
             os.path.join(self.get_cqlsh_dir(), BIN_DIR),
         ]
-        tools_java_dir = os.path.join(self.get_tools_java_dir(), BIN_DIR),
+        tools_java_dir = self.get_tools_java_dir()
         if tools_java_dir:
-            candidate_dirs.append(tools_java_dir)
+            candidate_dirs.append( os.path.join(tools_java_dir, BIN_DIR))
         for candidate_dir in candidate_dirs:
             candidate = shutil.which(toolname, path=candidate_dir)
             if candidate:

[roydahan]

@tchaikov Can you please resubmit this PR?
I think it's needed.

[tchaikov]

@tchaikov Can you please resubmit this PR? I think it's needed.

@roydahan hi Roy, reopened.

[fruch]

also look like the cqlsh related tests were missing cause of of using os.path.join with None, and this should fix it:

diff --git a/ccmlib/scylla_node.py b/ccmlib/scylla_node.py
index 96fc5d6..7627c4b 100644
--- a/ccmlib/scylla_node.py
+++ b/ccmlib/scylla_node.py
@@ -153,9 +153,9 @@ class ScyllaNode(Node):
             os.path.join(self.node_install_dir, 'share', 'cassandra', BIN_DIR),
             os.path.join(self.get_cqlsh_dir(), BIN_DIR),
         ]
-        tools_java_dir = os.path.join(self.get_tools_java_dir(), BIN_DIR),
+        tools_java_dir = self.get_tools_java_dir()
         if tools_java_dir:
-            candidate_dirs.append(tools_java_dir)
+            candidate_dirs.append( os.path.join(tools_java_dir, BIN_DIR))
         for candidate_dir in candidate_dirs:
             candidate = shutil.which(toolname, path=candidate_dir)
             if candidate:

@tchaikov check this part, I think it's missing for cqlsh related tests to work

[tchaikov]

tools_java_dir = self.get_tools_java_dir()

incorporated.

[fruch]

@syuu1228 want to take it for a ride with you PR ?

[fruch]

taking it for a ride with master dtest/scylla:

• 🟢 https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/605/ (Gossip - OSS)
• 🟢 https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/606/ (Gossip - Enterprise)
• 🟢 https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/607/ (Raft - OSS)
• 🟢 https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/608/ (Raft - Enterprise)
• 🟡 https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/609/ (Tablets - OSS)
• 🟢 https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/610/ (Tablets - Enterprise)

[fruch]

LGTM

and passed the gating dtests