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

tools/scylla-sstable: always read scylla.yaml #16174

Closed

Conversation

denesb
Copy link
Contributor

@denesb denesb commented Nov 24, 2023

Currently, scylla.yaml is read conditionally, if either the user provided --scylla-yaml-file command line parameter, or if deducing the data dir location from the sstable path failed.
We want the scylla.yaml file to be always read, so that when working with encrypted file (enterprise), scylla-sstable can pick up the configuration for the encryption.
This patch makes scylla-sstable always attempt to read the scylla-yaml file, whether the user provided a location for it or not. When not, the default location is used (also considering the SCYLLA_CONF and SCYLLA_HOME environment variables.
Failing to find the scylla.yaml file is not considered an error. The rational is that the user will discover this if they attempt to do an operation that requires this anyway.
There is a debug-level log about whether it was successfully read or not.

Fixes: #16132

Currently, scylla.yaml is read conditionally, if either the user
provided `--scylla-yaml-file` command line parameter, or if deducing the
data dir location from the sstable path failed.
We want the scylla.yaml file to be always read, so that when working
with encrypted file (enterprise), scylla-sstable can pick up the
configuration for the encryption.
This patch makes scylla-sstable always attempt to read the scylla-yaml
file, whether the user provided a location for it or not. When not, the
default location is used (also considering the `SCYLLA_CONF` and
`SCYLLA_HOME` environment variables.
Failing to find the scylla.yaml file is not considered an error. The
rational is that the user will discover this if they attempt to do an
operation that requires this anyway.
There is a debug-level log about whether it was successfully read or
not.

Fixes: scylladb#16132
@denesb denesb requested a review from nyh as a code owner November 24, 2023 14:03
@scylladb-promoter
Copy link
Contributor

🟢 CI State: SUCCESS

✅ - Build
✅ - Sanity Tests
✅ - Unit Tests

Build Details:

@denesb denesb requested a review from elcallio December 5, 2023 08:49
@avikivity
Copy link
Member

How does this impact running offline?

@denesb
Copy link
Contributor Author

denesb commented Dec 5, 2023

How does this impact running offline?

The tool always runs offline. I think you mean running outside a scylla-node? This is still fully supported and we have regression tests to make sure of this. Note that reading the scylla.yaml file is still optional, I just made the tool more eager to try to find and read it, to make using the tool with enterprise versions more seamless. With enterprise EAR, dumping sstables does require a scylla.yaml file, otherwise the encryption will not be set-up.

@avikivity
Copy link
Member

How does this impact running offline?

The tool always runs offline. I think you mean running outside a scylla-node?

Yes

This is still fully supported and we have regression tests to make sure of this. Note that reading the scylla.yaml file is still optional, I just made the tool more eager to try to find and read it, to make using the tool with enterprise versions more seamless. With enterprise EAR, dumping sstables does require a scylla.yaml file, otherwise the encryption will not be set-up.

Ok, I'm worried we'll drift into requiring it more and more, but I understand the reasoning.

@denesb
Copy link
Contributor Author

denesb commented Dec 5, 2023

Ok, I'm worried we'll drift into requiring it more and more, but I understand the reasoning.

I share your worry and for me personally the biggest wart of sstabledump was that it only worked on a scylla node. So I will work hard to keep the support for running on downloaded sstables, on a laptop.
For now, the tool even supports providing the schema via a text file (schema.cql) and I see no reason why we couldn't keep supporting this for the foreseeable future. At least for non-encrypted sstables.

@avikivity
Copy link
Member

Ok, I'm worried we'll drift into requiring it more and more, but I understand the reasoning.

I share your worry and for me personally the biggest wart of sstabledump was that it only worked on a scylla node. So I will work hard to keep the support for running on downloaded sstables, on a laptop. For now, the tool even supports providing the schema via a text file (schema.cql) and I see no reason why we couldn't keep supporting this for the foreseeable future. At least for non-encrypted sstables.

We could make sstables more self-contained by adding their schema to Scylla.db.

I want to add scylla --config inline-yaml-snippet (to replace --some-random-config-option=value`), this would allow copy/paste of just the encryption stuff.

dgarcia360 pushed a commit to dgarcia360/scylla that referenced this pull request Apr 30, 2024
Currently, scylla.yaml is read conditionally, if either the user
provided `--scylla-yaml-file` command line parameter, or if deducing the
data dir location from the sstable path failed.
We want the scylla.yaml file to be always read, so that when working
with encrypted file (enterprise), scylla-sstable can pick up the
configuration for the encryption.
This patch makes scylla-sstable always attempt to read the scylla-yaml
file, whether the user provided a location for it or not. When not, the
default location is used (also considering the `SCYLLA_CONF` and
`SCYLLA_HOME` environment variables.
Failing to find the scylla.yaml file is not considered an error. The
rational is that the user will discover this if they attempt to do an
operation that requires this anyway.
There is a debug-level log about whether it was successfully read or
not.

Fixes: scylladb#16132

Closes scylladb#16174
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.

scylla-sstable ignores scylla.yaml unless it is explicitely provided
5 participants