Add nodetool interposer script

[denesb]

This script is intended as a wrapper for nodetool, first trying to route commands to scylla-nodetool, and if the command is unimplemented, it is routed to the java nodetool. Whether a command is implemented or not is determined by checking the exit-code of scylla-nodetool. An unimplemented command uses a unique exit-code of 100. When this is seen, the command is routed to Java nodetool. install.sh is adjusted to install nodetool as nodetool.java and nodetool-wrapper as nodetool. If scylla-nodetool is misbehaving, users can manually fall-back to the legacy nodetool, by invoking nodetool.java.

Tests: build both the unified tarball as well as the tools-java one and verified that the script works as intended at the installed location. This will also be checked by Scylla CI.

Refs: https://github.com/scylladb/scylladb/issues/15588

[nyh]

I'm curious, why did you want this script in scylla-tools-java.git and not scylla.git? The benefit of putting it in scylla.git is that you can later improve it (or maybe even remove it!) in the same patch with a Scylla patch, and won't need those ugly two-repository dances. But I don't insist, if you like it this way.

[nyh]

I'm curious, why did you want this script in scylla-tools-java.git and not scylla.git? The benefit of putting it in scylla.git is that you can later improve it (or maybe even remove it!) in the same patch with a Scylla patch, and won't need those ugly two-repository dances. But I don't insist, if you like it this way.

Oh, I misunderstood that you wanted the new script to hide the old nodetool, and other projects (e.g., dtest) which used the old nodetool will start to use yours. Then I guess I understand why it needs to be in the nodetool repository. So sorry about the spam.

[tchaikov]

for the posterity, this change was previously implemented in scylladb, see https://github.com/scylladb/scylladb/pull/16087

[denesb]

New in v2:

• install.sh:
  • make style more consistent with rest of the file (then on same line as if).
  • install_tool_bin(): use more descriptive variable names
  • more quoting to make bash-linter happy
• nodetool-wrapper: use scylla nodetool $COMMAND --help to poll for command implementation, this way we don't need games with output capture.

[denesb]

Note to maintainers: I want a CI pass before merge. I will open a special PR in scylla.git to get a CI run. After that passes, we can merge, I will keep this PR up-to-date w.r.t this. First, I'd like a consensus from reviewers.

[denesb]

CI PR: https://github.com/scylladb/scylladb/pull/16366

[mykaul]

Instead of running --help you could have run it once on installation, save it somewhere and read from there. I think it's a micro-optimization that isn't worth it though.

[avikivity]

Instead of running --help you could have run it once on installation, save it somewhere and read from there. I think it's a micro-optimization that isn't worth it though.

It's a micro-optimization but adds macro-complexity. Where do you store it? How do you know when to refresh it? Do you have write permissions? And in any case the whole thing disappears once we figure out what N is in 5/N.

This is just a way to get advance testing before the whole thing is complete.

[avikivity]

Please test with ./install.sh --prefix ~/tmp123 from the tarball, just to be sure. I think it will work with no changes (well it might not work if "tmp123" has spaces injected).

[denesb]

Please test with ./install.sh --prefix ~/tmp123 from the tarball, just to be sure. I think it will work with no changes (well it might not work if "tmp123" has spaces injected).

I already did test it like this. I guess I should have mentioned this in the patch log.

[denesb]

New in v3:

• nodetool-wrapper:
  • more quoting
  • check if $SCYLLA_PATH exists, fall-back to java nodetool if not
• s/nodetool.java/nodetool-java/
• dist: add nodetool-java to rpm and deb specs

[denesb]

New in v3.1:

• nodetool-wrapper: don't use realpath on the SCYLLA_PATH until we don't know it exists. It results in an ugly error message printed.

[mykaul]

Instead of running --help you could have run it once on installation, save it somewhere and read from there. I think it's a micro-optimization that isn't worth it though.

It's a micro-optimization but adds macro-complexity. Where do you store it? How do you know when to refresh it? Do you have write permissions? And in any case the whole thing disappears once we figure out what N is in 5/N.

This is just a way to get advance testing before the whole thing is complete.

%pre of the RPM should handle it all. I think it's few lines, but may be wrong. Again, unsure if it's worth it.

[denesb]

We have green light from CI: https://github.com/scylladb/scylladb/pull/16366#issuecomment-1851894236.

[nyh]

I merged this patch to scylla-tools-java, but we also need to do the submodule update in Scylla (that's https://github.com/scylladb/scylladb/pull/16366, I guess?).

[avikivity]

@nyh don't half-merge things, if you merge into the submodule then also update the submodule reference

[nyh]

@avikivity as I said, the second half of the merge was https://github.com/scylladb/scylladb/pull/16366 - a PR that @denesb already opened. If he hadn't, I would have to do it myself (to test the submodule update before merging it), but he already did.

[denesb]

@avikivity as I said, the second half of the merge was scylladb/scylladb#16366 - a PR that @denesb already opened. If he hadn't, I would have to do it myself (to test the submodule update before merging it), but he already did.

Said PR was opened for CI only. I wanted to get CI feedback while review was ongoing, as I was expecting some problems. My idea was that once we have CI pass, there is no need for the submodule update for this PR to go through another scylla.git PR.

Sorry for the confusion.

[avikivity]

@denesb your pull request clearly stated it's for CI only, no problem there

@nyh it's not generally possible to open pull requests for submodule updates, because submodule history is linear and does not tolerate merges. The maintainer should perform the merge and submodule update close together to avoid another merge from updating the submodule under its feet.

[nyh]

@nyh it's not generally possible to open pull requests for submodule updates, because submodule history is linear and does not tolerate merges. The maintainer should perform the merge and submodule update close together to avoid another merge from updating the submodule under its feet.

The latter is true, but PRs are still the best way to go (even if you have to merge them quickly), especially in submodules like Seastar where a submodule refresh can bring in a lot of unrelated changes, and they should all be tested to not break next.

[avikivity]

So far the procedure is for the maintainer to generate the submodule update.