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 9 commits into
base: main
Choose a base branch
from
Open
123 changes: 123 additions & 0 deletions scylla/tests/integration/ccm/lib/ccm_cmd.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
use std::path::Path;
use std::process::ExitStatus;
use std::sync::Arc;

use crate::ccm::lib::ip_allocator::NetPrefix;

use super::logged_cmd::LoggedCmd;
use super::logged_cmd::RunOptions;

#[derive(Debug, Clone, Copy, PartialEq)]
pub(crate) enum DBType {
Scylla,
#[allow(dead_code)]
Cassandra,
}

pub(crate) struct Ccm {
config_dir: String,
cmd: Arc<LoggedCmd>,
}

impl Ccm {
pub(crate) fn new(config_dir: &Path, cmd: Arc<LoggedCmd>) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

⛏️

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
pub(crate) fn new(config_dir: &Path, cmd: Arc<LoggedCmd>) -> Self {
pub(crate) fn new(config_dir: impl AsRef<Path>, cmd: Arc<LoggedCmd>) -> Self {

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is customary in fs-related APIs to accept impl AsRef<Path>.

Self {
config_dir: config_dir
.to_str()
.expect("Config dir not representable as str")
.to_string(),
cmd,
}
}
}

pub(crate) struct ClusterCreate<'ccm> {
ccm: &'ccm mut Ccm,
name: String,
version: String,
ip_prefix: String,
db_type: DBType,
}

impl Ccm {
pub(crate) fn cluster_create(
&mut self,
name: String,
version: String,
ip_prefix: NetPrefix,
db_type: DBType,
) -> ClusterCreate<'_> {
ClusterCreate {
ccm: self,
name,
version,
ip_prefix: ip_prefix.to_string(),
db_type,
}
}
}

impl ClusterCreate<'_> {
pub(crate) async fn run(self) -> Result<ExitStatus, anyhow::Error> {
let mut args: Vec<&str> = vec![
"create",
self.name.as_str(),
"--version",
self.version.as_str(),
"--ipprefix",
self.ip_prefix.as_str(),
"--config-dir",
self.ccm.config_dir.as_str(),
];
if self.db_type == DBType::Scylla {
args.push("--scylla");
}
self.ccm
.cmd
.run_command("ccm", &args, RunOptions::new())
.await
}
}
Comment on lines +34 to +80
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯 🤩 What a great, clean design! Congrats!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now all parameters are accepted in the constructor of the command. I'll switch it to builder pattern soon, and of course move other commands too.


pub(crate) struct ClusterPopulate<'ccm> {
ccm: &'ccm mut Ccm,
nodes: String,
ip_prefix: String,
}

impl Ccm {
pub(crate) fn cluster_populate(
&mut self,
nodes: &[u8],
ip_prefix: NetPrefix,
) -> ClusterPopulate<'_> {
let nodes_string = nodes
.iter()
.map(|node| node.to_string())
.collect::<Vec<String>>()
.join(":");
Comment on lines +94 to +98
Copy link
Collaborator

Choose a reason for hiding this comment

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

⛏️ Let's use Itertools::join. Unnecessary collect() pains my eyes even though I know these are just tests...

ClusterPopulate {
ccm: self,
nodes: nodes_string,
ip_prefix: ip_prefix.to_string(),
}
}
}

impl ClusterPopulate<'_> {
pub(crate) async fn run(self) -> Result<ExitStatus, anyhow::Error> {
let args: Vec<&str> = vec![
"populate",
"--ipprefix",
self.ip_prefix.as_str(),
"--nodes",
self.nodes.as_str(),
"--config-dir",
self.ccm.config_dir.as_str(),
];
self.ccm
.cmd
.run_command("ccm", &args, RunOptions::new())
.await
}
}
69 changes: 22 additions & 47 deletions scylla/tests/integration/ccm/lib/cluster.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use super::ccm_cmd::Ccm;
use super::ccm_cmd::DBType;
use super::ip_allocator::NetPrefix;
use super::logged_cmd::{LoggedCmd, RunOptions};
use super::{IP_ALLOCATOR, ROOT_CCM_DIR};
Expand All @@ -15,13 +17,6 @@ use std::path::{Path, PathBuf};
use std::process::Command;
use std::sync::Arc;

#[derive(Debug, Clone, PartialEq)]
pub(crate) enum DBType {
Scylla,
#[allow(dead_code)]
Cassandra,
}

#[derive(Debug, Clone)]
pub(crate) struct ClusterOptions {
/// Cluster Name
Expand Down Expand Up @@ -93,9 +88,9 @@ impl NodeOptions {
NodeOptions {
id: 0,
datacenter_id: 1,
db_type: value.db_type.clone(),
db_type: value.db_type,
version: value.version.clone(),
ip_prefix: value.ip_prefix.clone(),
ip_prefix: value.ip_prefix,
smp: value.smp,
memory: value.memory,
}
Expand Down Expand Up @@ -445,6 +440,7 @@ impl NodeList {
}

pub(crate) struct Cluster {
ccm_cmd: Ccm,
nodes: NodeList,
destroyed: bool,
logged_cmd: Arc<LoggedCmd>,
Expand Down Expand Up @@ -566,12 +562,13 @@ impl Cluster {
}
}

let lcmd = LoggedCmd::new().await;
let lcmd = Arc::new(LoggedCmd::new().await);

let mut cluster = Cluster {
ccm_cmd: Ccm::new(config_dir.path(), Arc::clone(&lcmd)),
destroyed: false,
nodes: NodeList::new(),
logged_cmd: Arc::new(lcmd),
logged_cmd: Arc::clone(&lcmd),
opts: opts.clone(),
config_dir,
};
Expand All @@ -588,43 +585,21 @@ impl Cluster {
pub(crate) async fn init(&mut self) -> Result<(), Error> {
let config_dir = self.config_dir();
debug!("Init cluster, config_dir: {:?}", config_dir);
let mut args: Vec<String> = vec![
"create".to_string(),
self.opts.name.clone(),
"-v".to_string(),
self.opts.version.clone(),
"-i".to_string(),
self.opts.ip_prefix.to_string(),
"--config-dir".to_string(),
config_dir.to_string_lossy().into_owned(),
];
if self.opts.db_type == DBType::Scylla {
args.push("--scylla".to_string());
}
self.logged_cmd
.run_command("ccm", &args, RunOptions::new())
.await?;
let nodes_str = self
.opts
.nodes
.iter()
.map(|node| node.to_string())
.collect::<Vec<String>>()
.join(":");

let args = vec![
"populate".to_string(),
"-i".to_string(),
self.opts.ip_prefix.to_string(),
"-n".to_string(),
nodes_str,
"--config-dir".to_string(),
config_dir.to_string_lossy().into_owned(),
];
self.logged_cmd
.run_command("ccm", &args, RunOptions::new())
self.ccm_cmd
.cluster_create(
self.opts.name.clone(),
self.opts.version.clone(),
self.opts.ip_prefix,
self.opts.db_type,
)
.run()
.await?;
Ok(())

self.ccm_cmd
.cluster_populate(&self.opts.nodes, self.opts.ip_prefix)
.run()
.await
.map(|_| ())
}

/// Executes `ccm updateconf` and applies it for all nodes in the cluster.
Expand Down
6 changes: 3 additions & 3 deletions scylla/tests/integration/ccm/lib/ip_allocator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::str::FromStr;
use anyhow::{Context, Error};

/// A subnet prefix for local network (127.x.x.x/24).
#[derive(Debug, Clone)]
#[derive(Debug, Clone, Copy)]
pub(crate) struct NetPrefix(IpAddr);

impl NetPrefix {
Expand All @@ -23,7 +23,7 @@ impl NetPrefix {
Ok(IpAddr::from_str(&value)?.into())
}

pub(super) fn to_str(&self) -> String {
pub(super) fn to_str(self) -> String {
match self.0 {
IpAddr::V4(v4) => {
let octets = v4.octets();
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 I doubted to_* functions should accept self by value, even in case of Copy types.
However, a quick look at i64::to_be() convinced me. TIL.

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 would like to change this name anyway, but not because of to_ part. The str part is the problem imo, as the function returns String, not &str.

Expand All @@ -39,7 +39,7 @@ impl NetPrefix {
}
}

pub(super) fn to_ipaddress(&self, id: u16) -> IpAddr {
pub(super) fn to_ipaddress(self, id: u16) -> IpAddr {
match self.0 {
IpAddr::V4(v4) => {
let mut octets = v4.octets();
Expand Down
1 change: 1 addition & 0 deletions scylla/tests/integration/ccm/lib/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod ccm_cmd;
pub(crate) mod cluster;
mod ip_allocator;
mod logged_cmd;
Expand Down