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

ccmlib/scylla_node.py: use native scylla nodetool when it's available #573

Merged
merged 1 commit into from
May 2, 2024

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented May 1, 2024

in b18f85d, we use the native nodetool if JMX is not available, but when running dtest from the local build, the jmx repo is still around, so the java-based nodetool is picked. but the java-based nodetool does not support tablets, and it would be great to always test the native implementation built locally when testing using dtest without building a relocatable package.

so, in this change, we go a step further by checking if the command line of scylla nodetool help works, if it succeeds, we go ahead and use scylla nodetool, otherwise, we fall back to the java-based nodetool. we could instead use scylla nodetool --help instead of calling a certain command of nodetool, but seastar returns 1 if --help is used. a pull request was created to address it . see scylladb/seastar#2213.

@mykaul
Copy link
Contributor

mykaul commented May 1, 2024

That reminds me of the functionality of the wrapper script we've had. Why not use it?

@fruch
Copy link
Contributor

fruch commented May 1, 2024

That reminds me of the functionality of the wrapper script we've had. Why not use it?

cause it's not there anymore ?

@tchaikov
Copy link
Contributor Author

tchaikov commented May 1, 2024

@mykaul hi Yaniv, as @fruch put, it was dropped. see https://github.com/scylladb/scylla-tools-java/pull/386/files

@tchaikov tchaikov requested a review from denesb May 1, 2024 10:10
@tchaikov

This comment was marked as resolved.

in b18f85d, we use the native nodetool if JMX is not available, but
when running dtest from the local build, the jmx repo is still around,
so the java-based nodetool is picked. but the java-based nodetool does
not support tablets, and it would be great to always test the native
implementation built locally when testing using dtest without building
a relocatable package.

so, in this change, we go a step further by checking if the command line
of `scylla nodetool help` works, if it succeeds, we go ahead and use
`scylla nodetool`, otherwise, we fall back to the java-based nodetool.
we could instead use `scylla nodetool --help` instead of calling a
certain command of nodetool, but seastar returns 1 if `--help` is
used. a pull request was created to address it . see
scylladb/seastar#2213.

also, the native nodetool expect `-h` and `--port` as parameters of
the subcommand, not the parameters of `scylla nodetool`, it complains
like:

```
E               ccmlib.node.ToolError: Subprocess /jenkins/workspace/scylla-master/gating-dtest-release/scylla/.dtest/dtest-7r2n368l/test/node1/bin/scylla nodetool -h 127.0.17.1 -p 10000 snapshot exited with non-zero status; exit status: 100;
E               stderr: error: unrecognized operation argument: expected one of ({cleanup, compact, disablebackup, disablebinary, disablegossip, enablebackup, enablebinary, enablegossip, flush, gettraceprobability, help, settraceprobability, statusbackup, statusbinary, statusgossip, version}), got --host
```

but the java-based tool looks for these options before the subcommand,
so put them conditionally.

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov tchaikov marked this pull request as ready for review May 2, 2024 04:46
@tchaikov
Copy link
Contributor Author

tchaikov commented May 2, 2024

@tchaikov
Copy link
Contributor Author

tchaikov commented May 2, 2024

@denesb and @fruch could you help review this change?

@tchaikov
Copy link
Contributor Author

tchaikov commented May 2, 2024

it's a resurrection of #572. which was queued but then dequeued due to a regression. but the regression was addressed by scylladb/scylladb#18470.

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

@fruch fruch merged commit 7d067c0 into scylladb:next May 2, 2024
4 checks passed
@tchaikov tchaikov deleted the use-nodetool-from-source branch May 2, 2024 05:21
@bhalevy
Copy link
Member

bhalevy commented May 2, 2024

@frush, was 7d067c0 dequeued?

@bhalevy
Copy link
Member

bhalevy commented May 6, 2024

@tchaikov please retest all next_gating tests and resubmit

@tchaikov
Copy link
Contributor Author

tchaikov commented May 6, 2024

@tchaikov please retest all next_gating tests and resubmit

i am actually testing it at https://jenkins.scylladb.com/job/scylla-staging/job/tchaikov/job/gating-dtest-release/16/. but some tests are failing even without this patch. see https://jenkins.scylladb.com/job/scylla-staging/job/tchaikov/job/gating-dtest-release/17/ . still looking.

@bhalevy
Copy link
Member

bhalevy commented May 6, 2024

@tchaikov please retest all next_gating tests and resubmit

i am actually testing it at https://jenkins.scylladb.com/job/scylla-staging/job/tchaikov/job/gating-dtest-release/16/. but some tests are failing even without this patch. see https://jenkins.scylladb.com/job/scylla-staging/job/tchaikov/job/gating-dtest-release/17/ . still looking.

Apparently it's due to the https://github.com/scylladb/scylla-dtest/pull/4186 fiasco.
The test doesn't recognize that consistent-topology-changes is on (now by default).

I think that the reason test passes now in next-gating is: https://github.com/scylladb/scylla-dtest/pull/4179/files#diff-ae5a46b3aa3bae6d415117bd5efb9b347ef844d4bdf4c9ef899445c8f77edc6aR13
which is extremely confusing...

@bhalevy
Copy link
Member

bhalevy commented May 6, 2024

@tchaikov please also test rolling_upgrade_test.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants