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

Add clients #41

Merged
merged 9 commits into from
May 22, 2024
Merged

Add clients #41

merged 9 commits into from
May 22, 2024

Conversation

gregcusack
Copy link
Collaborator

Summary of Changes

  1. Create client accounts
  2. Create and deploy client secrets, services, replica sets
  3. Fix nits from jon: fix bug, add rpc nodes #40

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looking really good! Just small things

@@ -191,15 +193,20 @@ USER solana
COPY --chown=solana:solana {startup_script_directory} /home/solana/k8s-cluster-scripts
RUN chmod +x /home/solana/k8s-cluster-scripts/*
COPY --chown=solana:solana ./config-k8s/bootstrap-validator /home/solana/ledger
COPY --chown=solana:solana ./{solana_build_directory}/bin/ /home/solana/.cargo/bin/
COPY --chown=solana:solana ./{solana_build_directory}/bin/ /home/solana/bin/

Choose a reason for hiding this comment

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

Woo!

src/docker.rs Outdated
Comment on lines 205 to 208
let dockerfile = format!(
"{dockerfile}\n{}",
self.insert_client_accounts_if_present(solana_root_path, validator_type)?,
);

Choose a reason for hiding this comment

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

nit: how about tacking on this bit into the previous call to format!? Ie.

let dockerfile = format!(r#"
.....
{}
"#, self.base_image, self.insert_client_accounts_if_present(solana_root_path, validator_type)?);

src/docker.rs Show resolved Hide resolved
src/docker.rs Show resolved Hide resolved
src/genesis.rs Outdated
Comment on lines 99 to 100
in_file: &PathBuf, //bench-tps-i.yml
out_file: &PathBuf, //client-accounts.yml

Choose a reason for hiding this comment

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

nit: up to you, but if the commented names are needed, may as well change the parameter names in the function!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great point lol

src/main.rs Outdated
.to_string(),
client_to_run: matches
.value_of("client_to_run")
.unwrap_or_default()

Choose a reason for hiding this comment

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

same here, I think unwrap() will be better

src/main.rs Outdated
Comment on lines 380 to 386
target_node: match matches.value_of("target_node") {
Some(s) => match s.parse::<Pubkey>() {
Ok(pubkey) => Some(pubkey),
Err(e) => return Err(format!("failed to parse pubkey in target_node: {e}").into()),
},
None => None,
},

Choose a reason for hiding this comment

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

Feel free to use clap_utils::input_parsers::pubkey_of to avoid this annoyance

src/main.rs Outdated
@@ -558,7 +658,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
kub_controller
.deploy_secret(bootstrap_validator.secret())
.await?;
info!("Deployed Bootstrap Secret");
info!("Bootstrap Secret deployed");

Choose a reason for hiding this comment

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

micro-nit: I actually prefer the new active style more that you're using in the other logs, but it's up to you

Suggested change
info!("Bootstrap Secret deployed");
info!("Deployed Bootstrap Secret");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahh ya i went back and forth on this. will update

for rpc_name in &rpc_nodes {
if kub_controller.is_replica_set_ready(rpc_name).await? {
ready = true;
break;
break 'outer;

Choose a reason for hiding this comment

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

💪

if num_validators == 0 {
info!("No validators to deploy. Returning");
return Ok(());
if num_validators > 0 {

Choose a reason for hiding this comment

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

Highly recommend "hide whitespace" to review this part 😅

Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

sorry for the very late review 😢 looks good to me! let's do it!

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Just a couple of last things

src/docker.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/startup_scripts.rs Outdated Show resolved Hide resolved
@gregcusack gregcusack requested a review from joncinque May 22, 2024 15:47
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for addressing everything, looks great!

@gregcusack gregcusack merged commit c72446d into anza-xyz:main May 22, 2024
2 checks passed
@gregcusack gregcusack deleted the add-clients branch May 22, 2024 16:14
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.

3 participants