-
Notifications
You must be signed in to change notification settings - Fork 5
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
Deploy Bootstrap and add Metrics #38
Conversation
9a55b94
to
f6c17c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just small things to address
``` | ||
cd scripts/ | ||
./init-metrics -c <database-name> <metrics-username> | ||
# enter password when promted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit typo
# enter password when promted | |
# enter password when prompted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely doesn't need to be done in this PR, but would it be easier for maintenance for this to be done through a separate little rust binary in this repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya i can do that. what is the benefit? Just less bash code? and since it doesn't change we can just use a binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was wondering where we draw the line between RIIR and Keep It In Bash (KIIB??)... This could definitely stay as a bash script, which is why my comment was a question. Feel free to disregard!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh yes fair enough! i do like the idea of putting it in rust just to keep it somewhat uniform.
let metrics_secret = match kub_controller.create_metrics_secret() { | ||
Ok(secret) => secret, | ||
Err(err) => { | ||
error!("Failed to create metrics secret! {err}"); | ||
return; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if you have the main function call a function returning a Result<(), Box<dyn std::error::Error>>
, and printing the error, most of these can be reduced with ?
s, ie
let metrics_secret = match kub_controller.create_metrics_secret() { | |
Ok(secret) => secret, | |
Err(err) => { | |
error!("Failed to create metrics secret! {err}"); | |
return; | |
} | |
}; | |
let metrics_secret = kub_controller.create_metrics_secret()?; | |
info!("something good happened"!); |
"replica set: {} not ready...", | ||
bootstrap_validator.replica_set_name() | ||
); | ||
std::thread::sleep(std::time::Duration::from_secs(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal here since you're not doing anything concurrently, but if you're in async world, and you sleep the whole thread, that thread can't pick up any other work, so instead you should do an async sleep with tokio::time::sleep
https://docs.rs/tokio/latest/tokio/time/fn.sleep.html
while { | ||
match kub_controller | ||
.check_replica_set_ready(bootstrap_validator.replica_set_name().as_str()) | ||
.await | ||
{ | ||
Ok(ok) => !ok, // Continue the loop if replica set is not ready: Ok(false) | ||
Err(_) => panic!("Error occurred while checking replica set readiness"), | ||
} | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: As another point for making this function return a Result
, with ?
this can be come much more readable as:
while { | |
match kub_controller | |
.check_replica_set_ready(bootstrap_validator.replica_set_name().as_str()) | |
.await | |
{ | |
Ok(ok) => !ok, // Continue the loop if replica set is not ready: Ok(false) | |
Err(_) => panic!("Error occurred while checking replica set readiness"), | |
} | |
} { | |
while !kub_controller | |
.check_replica_set_ready(bootstrap_validator.replica_set_name().as_str()) | |
.await? { |
pub async fn check_replica_set_ready( | ||
&self, | ||
replica_set_name: &str, | ||
) -> Result<bool, kube::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
micro-nit: the word check
doesn't make it clear what this returns, how about changing this to is_replica_set_ready
? Otherwise, you can have this return an enum with Ready
and NotReady
as the states, to force people to match on it
* address jon nits from #38 * chido comment: refactor known validator from: #10 * rewrite init-metrics.sh in rust * Create non-bootstrap, voting validator accounts * build and push validator docker image * create and deploy validator secret * add validator selectors * create validator replica sets. need shred_version * add in get shred version from genesis * deploy validator replica set * deploy validator service * refactor buildtype skip. will skip release channel pull/extract as well
Summary of Changes
A series of PRs that will build out the monogon testing framework for deploying validator clusters on Kubernetes