Create bootstrap replicaset by gregcusack · Pull Request #10 · anza-xyz/validator-lab

gregcusack · 2024-04-01T23:11:24Z

Summary of Changes

create bootstrap validator replicaset
Update token-2022 to v1.0.0
address a few nits from PR: add bootstrap kubernetes labels and validator management structs #9
Library is now ClusterImages and its structure is slightly changed

10th PR in a series of PRs that will build out the monogon testing framework for deploying validator clusters on Kubernetes

… and update struct

joncinque

Looks great overall! Almost entirely nits

joncinque · 2024-04-26T18:23:11Z

+    pub tpu_enable_udp: bool,
+    pub tpu_disable_quic: bool,


nit: I'm wondering if these should be totally omitted. They were useful during the transition from UDP to QUIC, but since the transition has been complete for awhile, we should never want to enable UDP and never want to disable QUIC

joncinque · 2024-04-26T18:25:31Z

+impl std::fmt::Display for ValidatorConfig {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        let known_validators = match &self.known_validators {
+            Some(validators) => validators
+                .iter()
+                .map(|v| v.to_string())
+                .collect::<Vec<_>>()
+                .join(", "),
+            None => "None".to_string(),
+        };
+        write!(
+            f,
+            "Runtime Config\n\
+             tpu_enable_udp: {}\n\
+             tpu_disable_quic: {}\n\
+             max_ledger_size: {:?}\n\
+             skip_poh_verify: {}\n\
+             no_snapshot_fetch: {}\n\
+             require_tower: {}\n\
+             enable_full_rpc: {}\n\
+             known_validators: {:?}",
+            self.tpu_enable_udp,
+            self.tpu_disable_quic,
+            self.max_ledger_size,
+            self.skip_poh_verify,
+            self.no_snapshot_fetch,
+            self.require_tower,
+            self.enable_full_rpc,
+            known_validators,
+        )
+    }
+}


nit: is this needed? I'm wondering if it would be easier to just #[derive(Debug)] and let people use the debug output if needed

joncinque · 2024-04-26T18:26:07Z

        .to_string();

-    let mut validator_library = Library::default();
+    let mut validator_library = ClusterImages::default();


nit: maybe rename this variable to cluster_images?

joncinque · 2024-04-26T21:41:43Z

+                error!(
+                    "The provided --limit-ledger-size value was too small, the minimum value is {DEFAULT_MIN_MAX_LEDGER_SHREDS}"
+                );
+                return;


nit: I just found out about this, but a nice option if there's an issue on arg parsing that can't be covered by clap solana-labs/solana-program-library#6550 (comment)

Doesn't need to be done here, of course

joncinque · 2024-04-26T21:43:15Z

+            Arg::with_name("limit_ledger_size")
+                .long("limit-ledger-size")
+                .takes_value(true)
+                .help("Validator Config. The `--limit-ledger-size` parameter allows you to specify how many ledger
+                shreds your node retains on disk. If you do not
+                include this parameter, the validator will keep the entire ledger until it runs
+                out of disk space. The default value attempts to keep the ledger disk usage
+                under 500GB. More or less disk usage may be requested by adding an argument to
+                `--limit-ledger-size` if desired. Check `agave-validator --help` for the
+                default limit value used by `--limit-ledger-size`. More information about
+                selecting a custom limit value is at : https://github.com/solana-labs/solana/blob/583cec922b6107e0f85c7e14cb5e642bc7dfb340/core/src/ledger_cleanup_service.rs#L15-L26"),


nit: how about using a default value of DEFAULT_MAX_LEDGER_SHREDS.to_string()? that way you can just unwrap() fetching this later

joncinque · 2024-04-26T21:47:22Z

+            Arg::with_name("memory_requests")
+                .long("memory-requests")
+                .takes_value(true)
+                .default_value("70Gi") // 70 Gigabytes


micro-nit

Suggested change

.default_value("70Gi") // 70 Gigabytes

.default_value("70Gi") // 70 Gibibytes

this one always gets me

joncinque · 2024-04-26T21:50:09Z

 }
+
+#[allow(clippy::too_many_arguments)]
+pub fn create_replica_set(


I have no idea if this works, so I'll assume it's all correct 😅

haha it does. but i could change create_replica_set() to accept a struct instead of all the args.

joncinque · 2024-04-26T21:52:45Z

+    namespace: &str,
+    label_selector: &BTreeMap<String, String>,
+    image_name: &DockerImage,
+    environment_variables: Vec<EnvVar>,
+    command: &[String],


nit: since the refs here all end up getting cloned, you may as well take them by value instead of ref here, and leave the burden of copies on the caller

joncinque · 2024-04-26T21:54:39Z

+            requests: vec![
+                ("cpu".to_string(), Quantity(cpu_requests)),
+                ("memory".to_string(), Quantity(memory_requests)),
+            ]
+            .into_iter()
+            .collect(),


nit: you should be able to use BTreeMap::from, ie

Suggested change

requests: vec![

("cpu".to_string(), Quantity(cpu_requests)),

("memory".to_string(), Quantity(memory_requests)),

]

.into_iter()

.collect(),

requests: BTreeMap::from([

("cpu".to_string(), Quantity(cpu_requests)),

("memory".to_string(), Quantity(memory_requests)),

])

https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#impl-From%3C%5B(K,+V);+N%5D%3E-for-BTreeMap%3CK,+V%3E

joncinque · 2024-04-26T21:57:16Z

+    pub no_snapshot_fetch: bool,
+    pub require_tower: bool,
+    pub enable_full_rpc: bool,
+    pub known_validators: Option<Vec<Pubkey>>,


nit: The logic later of initializing it in add_known_validator is a bit confusing, could this just be a straight-up Vec?

yes will fix. i think i got addicted to using Option early on. Will try to avoid when not needed lol

yihau · 2024-04-29T07:32:57Z

-        let mut secrets = HashMap::new();
+        let bootstrap_keypair = read_keypair_file(&identity_key_path)
+            .expect("Failed to read bootstrap validator keypair file");
+        self.add_known_validator(bootstrap_keypair.pubkey());


nit: just out of curiosity, do we add this one for the rest of validator scripts 🤔 also, it seems that this action is not quite related to the create_bootstrap_secret

you are def right that this is in the wrong spot. not related to create_bootstrap_secret. will fix. I added the bootstrap to known_validators because for all other validators deployed, I will pass in --known-validators <bootstrap-identity> to the other validators' startup scripts

* 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

gregcusack mentioned this pull request Apr 2, 2024

Create Validator struct and refactor #11

Closed

gregcusack force-pushed the create-bootstrap-replicaset branch from b5d9dfb to 80c7496 Compare April 10, 2024 04:50

gregcusack added 3 commits April 23, 2024 16:09

wip. adding replicaset but need validator config flags

c0e1a97

add validator config

fa63fc1

add pod requests. need for scheduling

e6d5867

gregcusack force-pushed the create-bootstrap-replicaset branch from 80c7496 to b9ef043 Compare April 23, 2024 16:55

create bootstrap validator replicaset

5c3067d

gregcusack force-pushed the create-bootstrap-replicaset branch from b9ef043 to 5c3067d Compare April 23, 2024 16:59

update token-2022 to v1.0.0: anza-xyz/agave#995

e01a864

gregcusack requested review from joncinque and yihau April 23, 2024 20:56

gregcusack added a commit to gregcusack/validator-lab that referenced this pull request Apr 25, 2024

address jon nits in anza-xyz#10 part 1.

3647d4a

gregcusack added 2 commits April 25, 2024 23:53

address jon nits in anza-xyz#9 part 1.

6b69de3

jon comments from: anza-xyz#9 part 2. rename Library to ClusterImages…

3351061

… and update struct

gregcusack force-pushed the create-bootstrap-replicaset branch from 9f72e80 to 3351061 Compare April 25, 2024 23:54

gregcusack requested review from joncinque and yihau and removed request for joncinque and yihau April 25, 2024 23:57

joncinque reviewed Apr 26, 2024

View reviewed changes

address Jon nits

da5377e

yihau approved these changes Apr 29, 2024

View reviewed changes

joncinque approved these changes Apr 29, 2024

View reviewed changes

gregcusack merged commit 576851d into anza-xyz:main Apr 29, 2024

gregcusack deleted the create-bootstrap-replicaset branch April 29, 2024 17:10

gregcusack mentioned this pull request Apr 29, 2024

Deploy Bootstrap and add Metrics #38

Merged

gregcusack added a commit to gregcusack/validator-lab that referenced this pull request May 1, 2024

chido comment: refactor known validator from: anza-xyz#10

dfd5fb3

	.default_value("70Gi") // 70 Gigabytes
	.default_value("70Gi") // 70 Gibibytes

Conversation

gregcusack commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gregcusack commented Apr 1, 2024 •

edited

Loading