Skip to content

Backport ccm main #1296

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

Lorak-mmk
Copy link
Collaborator

@Lorak-mmk Lorak-mmk commented Mar 25, 2025

@muzarski @wprzytula This is a draft of the CCM backport into main branch.

I mostly took the current state from branch-hackathon. One important change I made is to remove separate ccm-integration test target. Instead ccm tests are now a module in integration.
This avoids the problem with sharing utils. It should also be quicker to compile - no need to link 2 separate binaries.

Apart from that I did not really modify CCM integration. Now the question is: what do we do with it.
Are we satisfied with the API? Probably not.
If not, what should the API be like?

This is fully internal to the crate, so we can change it freely, so there is no need to spend too much time on it - we can always improve later.
Still, we should retain some reasonable level of quality, so I'd like to discuss this a bit.

I did not yet pick up @muzarski 's changes that integrated auth and TLS workflows into CCM. I can do that after we agree to the rest of this PR.

On the above matter @muzarski : How should I adapt CCM given that we now support multiple TLS backends?
I see we have in mod.rs DB_TLS_CERT_PATH, DB_TLS_KEY_PATH and CA_TLS_CERT_PATH (which I removed for now because it was guarded by old feature name).
I also see that on branch-hackathon you wrote a TLS test. We should probably make a test per backend, right?
What about those vars? Do they make sense for all the tests? In that case we just need to change feature guard on CA_TLS_CERT_PATH to be activated when any backend is active - or to even make it always active because why not.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

Copy link

github-actions bot commented Mar 25, 2025

cargo semver-checks found no API-breaking changes in this PR.
Checked commit: a939123

@Lorak-mmk Lorak-mmk force-pushed the backport-ccm-main branch 3 times, most recently from ad0c3aa to 6e14276 Compare March 25, 2025 15:20
@Lorak-mmk
Copy link
Collaborator Author

Possible improvements / API changes after a brief glance at the code:

  • Should NodeStartOptions be a struct? In other words, does it make sense to enable e.g. no_wait and wait_other_notice at the same time? We need to know the exact semantics of the wait-related flags to know that (cc @fruch because I don't think this is documented anywhere in this cursed software).
  • Same for NodeStopOptions
  • We hold nodes in Arc<RwLock<>>, and methods that give user nodes return that. Maybe we could return refs / mut refs and get rid of Arc<RwLock<>>? I'm not sure.
  • In the future when we have custom test runner, we could make new struct (ClusterPreferences + NodePreferences) or extend *Options structs. Why? Tests may not care about some parameters, which could help test runner to provide less clusters.

@muzarski
Copy link
Contributor

On the above matter @muzarski : How should I adapt CCM given that we now support multiple TLS backends? I see we have in mod.rs DB_TLS_CERT_PATH, DB_TLS_KEY_PATH and CA_TLS_CERT_PATH (which I removed for now because it was guarded by old feature name). I also see that on branch-hackathon you wrote a TLS test. We should probably make a test per backend, right? What about those vars? Do they make sense for all the tests? In that case we just need to change feature guard on CA_TLS_CERT_PATH to be activated when any backend is active - or to even make it always active because why not.

Notice that I implemented all of this when we had old certificates in the repository. We had to update them, because of the errors thrown by rustls.

Why old certs worked for openssl but did not for rustls?

It's because rustls supports hostname verification by default, while openssl does not. The CN (common name) in db certificate was not matching the hostname, thus rustls was throwing an error.

What changed in the certificates, compared to the previous version?

I generated the db certificates assigned to static IP (172.44.0.2 - one we currently use in CI for TLS single-node cluster). In other words, the extensions to certificate request in openssl config looked like:

[v3_req]
# Extensions to add to a certificate request
basicConstraints = CA:FALSE
subjectAltName = IP:172.44.0.2
keyUsage = nonRepudiation, digitalSignature, keyEncipherment

Thanks to that, rustls is able to verify the hostname using node's IP. It checks whether the node that we try to connect to has the same IP as the one defined in certificate (under subjectAltName).

Current state

Currently, our SSL "tests" are limited to simply running the tls-openssl and tls-rustls examples in SSL CI workflow.
On hackathon-branch, however, I removed the SSL workflow and migrated the example test to ccm. It obviously did it for openssl only, as rustls was not supported back then.

Implementing the corresponding ccm test for rustls backed and removing SSL CI workflow.

Well, this is a bit tricky. While, it was not an issue with openssl, for the reasons stated above (no hostname verification), it won't work for rustls. This is because we use dynamic IPs in ccm tests.

The temporary solution I see: migrate the tests from SSL workflow to ccm only for openssl and limit rustls "tests" to just running the example against the global cluster (as it is currently done in CI).

If we ever decide that we want to have ccm tests for rustls backend, I list the possible solutions to the dynamic IP problem:

  • Generate certificates on the fly, during the test. We would firstly receive some IP from the IpAllocator (or some other mechanism in the future) and then we could generate the self-signed cert for this IP. Then we could ccm updateconf and provide the path to generated certificate. There are some crates we could use, e.g. https://docs.rs/rcgen/latest/rcgen/.
  • Disable hostname verification during the tests. I think this can be configured via the trait: rustls::client::danger::ServerCertVerifier. I'm not entirely sure, though. This needs some research - I think this is placed in danger module for a reason. OTOH, we already use it in scylla::cloud::config and implement it for our NoCertificateVerification struct.

@Lorak-mmk
Copy link
Collaborator Author

Moving such workflows to CCM has 2 advantages:

  • We get rid of custom images
  • We get rid of a GHA workflow

If we move only part of it, we get neither. So I'll cherry pick commits that move auth, and skip TLS for now. We can do that in the future.

Btw is it possible to use domain names instead of ip addresses with scylla? In other words, can we have domain names instead of ip addresses in system.peers in driver-relevant columns?
If it was possible, we could use certs with hostnames instead of ips.

@fruch
Copy link

fruch commented Mar 26, 2025

Moving such workflows to CCM has 2 advantages:

  • We get rid of custom images
  • We get rid of a GHA workflow

If we move only part of it, we get neither. So I'll cherry pick commits that move auth, and skip TLS for now. We can do that in the future.

Btw is it possible to use domain names instead of ip addresses with scylla? In other words, can we have domain names instead of ip addresses in system.peers in driver-relevant columns? If it was possible, we could use certs with hostnames instead of ips.

scylla can use hostnames, but then you need a dns server to map them.

I think generating certs as needed is the best approach, and also give the flexibility to try more variants as needed.
that's what we are doing in dtest, and in SCT.

@Lorak-mmk
Copy link
Collaborator Author

scylla can use hostnames, but then you need a dns server to map them.

I think generating certs as needed is the best approach, and also give the flexibility to try more variants as needed. that's what we are doing in dtest, and in SCT.

Is there functionality in CCM to generate certs? Or do we have to do it other way?

If Scylla can use hostnames, then we should test it too.

@fruch one other question for you. Could you describe (or point to documentation if such exists) what exactly wait-related flags do in CCM, and how do they interact if I specify more than one?

@fruch
Copy link

fruch commented Mar 26, 2025

scylla can use hostnames, but then you need a dns server to map them.
I think generating certs as needed is the best approach, and also give the flexibility to try more variants as needed. that's what we are doing in dtest, and in SCT.

Is there functionality in CCM to generate certs? Or do we have to do it other way?

If Scylla can use hostnames, then we should test it too.

@fruch one other question for you. Could you describe (or point to documentation if such exists) what exactly wait-related flags do in CCM, and how do they interact if I specify more than one?

you are more then welcome to document it.

@Lorak-mmk
Copy link
Collaborator Author

you are more then welcome to document it.

I'd be happy to make a PR that improves descriptions, but I would have to first understand those options myself.
I don't know ccm's codebase at all, and it is not really friendly to new contributors, that's why I asked you to explain those options.

@Lorak-mmk
Copy link
Collaborator Author

I backported the commits that move auth to CCM. I also removed TLS support from CCM for now.

@Lorak-mmk
Copy link
Collaborator Author

Marking as ready. The way I see it the only improvement I can make here is better CCM API - which needs input from others, which is basically a review.

@Lorak-mmk Lorak-mmk marked this pull request as ready for review March 26, 2025 13:45
@Lorak-mmk Lorak-mmk requested review from wprzytula and muzarski and removed request for wprzytula March 26, 2025 13:45
Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We hold nodes in Arc<RwLock<>>, and methods that give user nodes return that. Maybe we could return refs / mut refs and get rid of Arc<RwLock<>>? I'm not sure.

As of now, there is no use case for Arc<RwLock<>> - I think we can return refs/mutrefs for now. We could always revert this in the future. It also simplifies the API - I believe NodeList is no longer necessary then. Instead, we can expose nodes_iter_[mut]() and get_node_[mut]_by_id methods on Cluster.

Currently, append_node() and add_node() methods return Arc<RwLock<>>. They could return node id instead.

@Lorak-mmk
Copy link
Collaborator Author

Lorak-mmk commented Mar 28, 2025

  • Addressed @muzarski 's comments
  • Removed all the Arc<Mutex> stuff, now we just operate on Nodes.
  • I decided to retain NodeList because I welcome any kind of separation and structure in this code. I made its method simpler using iterator methods.
  • Method that adds node return mut reference to this node. I think it is more useful than id.

@Lorak-mmk
Copy link
Collaborator Author

I have one more idea: we can split off another file from cluster.rs, I would call it ccm_cmd.rs.
It would be a simple wrapper over CCM, providing builder-style commands.
The purpose of this module would be to provide convenient way to call CCM, and encode all its commands and flags into Rust types.
cluster.rs would be responsible for providing user-facing API, handling config dirs etc. Its code would hopefully become cleaner.

@Lorak-mmk
Copy link
Collaborator Author

I did this for 2 commands as an experiment, in additional commit. I like the new version, so unless anyone has different opinion I'll convert the rest of the command to this.

@dkropachev I see that both ccm create and ccm populate accept ipprefix argument. Why? What are their respective semantics?

@Lorak-mmk Lorak-mmk force-pushed the backport-ccm-main branch 3 times, most recently from b605f97 to a02254c Compare March 28, 2025 20:51
@muzarski
Copy link
Contributor

I did this for 2 commands as an experiment, in additional commit. I like the new version, so unless anyone has different opinion I'll convert the rest of the command to this.

I love the idea. The code in cluster.rs looks much cleaner.

Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cluster.rs contains a lot of functions which are not used and whose goal is not always clear to me. Are we going to merge them as-is, only later wondering about their intended use and possibly adding documentation?

@Lorak-mmk Lorak-mmk force-pushed the backport-ccm-main branch 2 times, most recently from db70e01 to 8253f0d Compare May 10, 2025 15:58
@Lorak-mmk
Copy link
Collaborator Author

New version of the PR. Finalized the move to a separate command builders and made a lot of other changes.
I think there is a lot of room for improvement still, but I'd like to put this up for review now anyway - it will be easier to get it to something acceptable together.

@Lorak-mmk Lorak-mmk requested review from wprzytula and muzarski May 10, 2025 16:02
Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better.

Comment on lines +279 to +294
fn append_node(&mut self, node_options: NodeOptions) -> &mut Node {
let node_name = node_options.name();
let node = Node::new(node_options, self.ccm_cmd.for_node(node_name));

self.nodes.push(node);
self.nodes.0.last_mut().unwrap()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we return &mut Node here. (previously that was Arc<RwLock<_>>, correct?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure tbh.

@Lorak-mmk Lorak-mmk force-pushed the backport-ccm-main branch from 8253f0d to 3e4b32a Compare May 13, 2025 15:11
@Lorak-mmk
Copy link
Collaborator Author

Addressed Mikolaj's comment.

Comment on lines +103 to +118
.PHONY: use_cargo_lock_msrv
use_cargo_lock_msrv:
mv Cargo.lock Cargo.lock.bak
mv Cargo.lock.msrv Cargo.lock

.PHONY: restore_cargo_lock
restore_cargo_lock:
mv Cargo.lock Cargo.lock.msrv
mv Cargo.lock.bak Cargo.lock

.PHONY: test_cargo_lock_msrv
test_cargo_lock_msrv: use_cargo_lock_msrv check restore_cargo_lock
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Can you please add comments explaining use case of these commands?

Comment on lines +165 to +177
pub(crate) fn wait_options(mut self, options: Option<NodeStartOptions>) -> Self {
self.wait_opts = options;
self
}
pub(crate) fn scylla_smp(mut self, smp: u16) -> Self {
self.scylla_smp = Some(smp);
self
}

pub(crate) fn scylla_mem_megabytes(mut self, mem_megabytes: u32) -> Self {
self.scylla_mem_megabytes = Some(mem_megabytes);
self
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Why does wait_options() accept Options and the other two functions accept non-Options?

Comment on lines +114 to +123
let mut ip_port = ip_hex.split(':');
if let Some(ip_hex) = ip_port.next() {
if let Ok(ip) = u32::from_str_radix(ip_hex, 16) {
let first_octet = ip as u8;
if first_octet == 127 {
used_ips
.insert(LocalSubnetIdentifier((ip >> 8) as u8, (ip >> 16) as u8));
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely be ip_addr, not ip_port.

Comment on lines +104 to +128
/// The constructor scans /proc/net/tcp for busy local subnets (127.x.x.x/24). The subnet is busy,
/// if there is at least one listener on any port and any address in this network.
pub(super) fn new() -> Result<Self, Error> {
let mut used_ips: BTreeSet<LocalSubnetIdentifier> = BTreeSet::new();
let file = File::open("/proc/net/tcp").context("Failed to open /proc/net/tcp file")?;
let lines = BufReader::new(file).lines();
for line_res in lines {
let line = line_res.context("Failed to read a line from /proc/net/tcp")?;
let parts: Vec<&str> = line.split_whitespace().collect();
if let Some(ip_hex) = parts.get(1) {
let mut ip_port = ip_hex.split(':');
if let Some(ip_hex) = ip_port.next() {
if let Ok(ip) = u32::from_str_radix(ip_hex, 16) {
let first_octet = ip as u8;
if first_octet == 127 {
used_ips
.insert(LocalSubnetIdentifier((ip >> 8) as u8, (ip >> 16) as u8));
}
}
}
}
}

Ok(Self { used_ips })
}
Copy link
Collaborator

@wprzytula wprzytula May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored the whole function:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The constructor scans /proc/net/tcp for busy local subnets (127.x.x.x/24). The subnet is busy,
/// if there is at least one listener on any port and any address in this network.
pub(super) fn new() -> Result<Self, Error> {
let mut used_ips: BTreeSet<LocalSubnetIdentifier> = BTreeSet::new();
let file = File::open("/proc/net/tcp").context("Failed to open /proc/net/tcp file")?;
let lines = BufReader::new(file).lines();
for line_res in lines {
let line = line_res.context("Failed to read a line from /proc/net/tcp")?;
let parts: Vec<&str> = line.split_whitespace().collect();
if let Some(ip_hex) = parts.get(1) {
let mut ip_port = ip_hex.split(':');
if let Some(ip_hex) = ip_port.next() {
if let Ok(ip) = u32::from_str_radix(ip_hex, 16) {
let first_octet = ip as u8;
if first_octet == 127 {
used_ips
.insert(LocalSubnetIdentifier((ip >> 8) as u8, (ip >> 16) as u8));
}
}
}
}
}
Ok(Self { used_ips })
}
/// The constructor scans /proc/net/tcp for busy local subnets (127.x.y.z/24). The subnet is busy
/// if there is at least one listener on any port and any address in this network.
pub(super) fn new() -> Result<Self, Error> {
let mut used_ips: BTreeSet<LocalSubnetIdentifier> = BTreeSet::new();
let file = File::open("/proc/net/tcp").context("Failed to open /proc/net/tcp file")?;
let mut lines = BufReader::new(file).lines();
let _header_line = lines
.next()
.context("Failed to read the header line from /proc/net/tcp")?;
for line_res in lines {
let line = line_res.context("Failed to read a line from /proc/net/tcp")?;
line.split_whitespace()
.skip(1 /* ordinal number */)
.next() // Get second column - the local address.
.and_then(|ip_addr_hex| ip_addr_hex.split_once(':'))
.and_then(|(addr_hex, _port_hex)| u32::from_str_radix(addr_hex, 16).ok())
.inspect(|&ip| {
let first_octet = ip as u8;
if first_octet == 127 {
used_ips.insert(LocalSubnetIdentifier((ip >> 8) as u8, (ip >> 16) as u8));
}
});
}
Ok(Self { used_ips })
}

Comment on lines +153 to +161
_ => return Err(anyhow::anyhow!("Ipv6 addresses are not yet supported!")),
};
let subnet_id: LocalSubnetIdentifier = ipv4.into();

if !self.used_ips.remove(&subnet_id) {
return Err(anyhow::anyhow!(
"IP prefix {} was not allocated - something gone wrong!",
ip_prefix
));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ consider using anyhow::bail! as an idiomatic way.

Comment on lines +82 to +86
tracing::info!(
"{:15} -> failed to wait on child process: = {}",
format!("exited[{}]", run_id),
e
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Just to make sure I undestand correctly: would the following be equivalent?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracing::info!(
"{:15} -> failed to wait on child process: = {}",
format!("exited[{}]", run_id),
e
);
tracing::info!(
"exited[{:7}] -> failed to wait on child process: = {}",
run_id
e
);

@Lorak-mmk Lorak-mmk force-pushed the backport-ccm-main branch from 3e4b32a to 1564879 Compare May 14, 2025 21:45
Lorak-mmk and others added 8 commits May 14, 2025 23:49
This allows us to use `expect` lint level.
Cleanup unused dependancies
ccm module will contain tests that require ccm.
It's lib submodule will contain the ccm integration.
Why do it this way - which is different than what we did during a
hackathon?
- Old way required ugly hacks to share test utils between integration
test targets, and those hacks did not work well with rust-analyzer.
- One target means better compilation time

CCM tests will be guardded by a cfg, so we will still be able to run the
subset that we want:
- All tests: run integration tests with the required cfg
- Only CCM tests: as above, but filter by ccm folder
- Only non-ccm test: run without the cfg
Co-authored-by: Mikołaj Uzarski <mikolaj.uzarski@scylladb.com>
Co-authored-by: Dmitry Kropachev <dmitry.kropachev@gmail.com>
For now it will run on each PR. If at some point it becomes too slow
we can switch it to running manually and before release.
@Lorak-mmk Lorak-mmk force-pushed the backport-ccm-main branch from 1564879 to b183deb Compare May 14, 2025 21:49
Lorak-mmk and others added 3 commits May 14, 2025 23:49
Co-authored-by: Mikołaj Uzarski <mikolaj.uzarski@scylladb.com>
Auth tests are now run as a part of CCM test suite.
@Lorak-mmk Lorak-mmk force-pushed the backport-ccm-main branch 2 times, most recently from 7c6d135 to a939123 Compare May 14, 2025 22:05
@Lorak-mmk Lorak-mmk mentioned this pull request May 15, 2025
8 tasks
@wprzytula wprzytula added this to the 1.3.0 milestone May 28, 2025
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.

5 participants