Streamline restatectl connection to cluster #2592

muhamadazmy · 2025-01-30T17:21:29Z

Streamline restatectl connection to cluster

Summary:
Make it easier and cleaner to run the restatectl command
by accepting the restate nodes addresses via a unified argument --address

Support passing multiple addresses
The command will try to fetch the nodes configuration from the first reachable node
All other subcommands can then use the connection info to use the nodes configuration directly
or to connect to a specific node role

tillrohrmann

Really nice work @muhamadazmy. I like how the ConnectionInfo now contains the logic to retrieve the available nodes :-)

I left a few smaller comments. The one thing which is potentially a bit dangerous is how the cluster-provision works right now. It would be great if we can ensure that it behaves idempotently when retrying it.

tillrohrmann · 2025-01-31T13:03:41Z

tools/restatectl/src/commands/cluster/provision.rs

+    let Some((_, channel)) = connection_info.channels().next() else {
+        anyhow::bail!("At least one node address is required");
+    };


What we have to make sure for the cluster-provision command is that the call is idempotent. So if the user retries, then it shouldn't result into another node being provisioned (unless he explicitly ask us to do so). Accidentally provisioning multiple nodes is a very tricky situation to be in. Moreover, if the user is using the embedded metadata store, then he needs to hit a node that has the metadata-server role. If he uses an external metadata store, then he can hit any node.

@tillrohrmann I understand. This is why I am only taking the first address provided by the command line. This should be idempotent unless the user changed the params they are providing to the command line.

An improvement maybe to fail if multiple addresses are given and ask the user to only pass one and to stick to it maybe?

I'm not quite following this discussion - this calls into NodeCtlSvcHandler::provision_cluster, from where we make a series of updates to cluster metadata using the metadata client configuration on the node on which we land. The updates themselves are conditional updates to versioned metadata values, and we appear to cater for retries. In particular, writing the initial nodes config is conditional on Precondition::DoesNotExist. What am I missing?

tillrohrmann · 2025-01-31T13:08:27Z

tools/restatectl/src/commands/log/find_tail.rs

-                chain_table.add_row(vec![
-                    Cell::new(log_id),
-                    Cell::new(err.code()).fg(Color::DarkRed),
-                    Cell::new(err.message()).fg(Color::DarkRed),
-                ]);
-                continue;


Is it intended to remove this logic if cannot find the tail for a given log_id?

It is intended. I can partially restore it

tillrohrmann · 2025-01-31T13:09:54Z

tools/restatectl/src/commands/log/mod.rs

+    let mut response = connection
+        .try_each(None, |channel| async {
+            let mut client = NodeCtlSvcClient::new(channel)
+                .accept_compressed(CompressionEncoding::Gzip)
+                .send_compressed(CompressionEncoding::Gzip);
+
+            client.get_metadata(req).await
+        })
+        .await?
+        .into_inner();


Can we make sure to get the latest Logs if we can obtain it from multiple nodes?

tools/restatectl/src/commands/node/list_nodes.rs

tillrohrmann · 2025-01-31T13:14:33Z

tools/restatectl/src/commands/replicated_loglet/digest.rs

+        .try_each(None, |channel| async {
+            let mut client = NodeCtlSvcClient::new(channel)
+                .accept_compressed(CompressionEncoding::Gzip)
+                .send_compressed(CompressionEncoding::Gzip);
+
+            client.get_metadata(req).await
+        })
+        .await?


Same question here regarding what is the latest Logs version.

tools/restatectl/src/connection.rs

tillrohrmann · 2025-01-31T13:19:23Z

tools/restatectl/src/connection.rs

+            None => Either::Right(nodes_config.iter()),
+        }
+        .sorted_by(|left, right| {
+            // nodes that we already have channels open for, gets higher presence.


Suggested change

// nodes that we already have channels open for, gets higher presence.

// nodes that we already have channels open for get higher presence.

tillrohrmann · 2025-01-31T13:20:58Z

tools/restatectl/src/connection.rs

+
+#[derive(Debug, thiserror::Error)]
+pub enum ConnectionInfoError {
+    #[error("Could not retrieve nodes configuration. Possible un provisioned cluster!")]


Suggested change

#[error("Could not retrieve nodes configuration. Possible un provisioned cluster!")]

#[error("Could not retrieve nodes configuration. Has the cluster been provisioned yet?")]

tillrohrmann · 2025-01-31T13:22:17Z

tools/restatectl/src/connection.rs

+#[derive(Debug, thiserror::Error)]
+pub enum ConnectionInfoError {
+    #[error("Could not retrieve nodes configuration. Possible un provisioned cluster!")]
+    MissingNodesConfiguration,


Follow-up: Wondering whether a CodedError with more details and a short description how to provision the cluster could be helpful here.

tillrohrmann · 2025-01-31T13:24:22Z

tools/restatectl/src/connection.rs

+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        writeln!(
+            f,
+            "Encountered multiple errors trying to retrieve nodes configurations:"


Suggested change

"Encountered multiple errors trying to retrieve nodes configurations:"

"Encountered multiple errors trying to retrieve nodes' configuration:"

We seem to use NodesErrors also for the try_each call where we are not necessarily trying to retrieve the NodesConfiguration. Maybe this needs an update of the Display implementation.

Yes, makes sense

pcholakov

Nice improvement, @muhamadazmy! Some nitpicks but overall looks good to me.

One question I had was whether it's useful to report the node from which we obtained the response reported by a particular invocation? The node sorting should make that somewhat repeatable but it may help operators in case different nodes have diverged in their views.

pcholakov · 2025-01-31T13:09:53Z

tools/restatectl/src/connection.rs

+            None => Either::Right(nodes_config.iter()),
+        }
+        .sorted_by(|left, right| {
+            // nodes that we already have channels open for, gets higher presence.


Maybe?

Suggested change

// nodes that we already have channels open for, gets higher presence.

// nodes for which we already have open channels get higher precedence.

pcholakov · 2025-01-31T13:11:06Z

tools/restatectl/src/connection.rs

@@ -0,0 +1,267 @@
+// Copyright (c) 2023 - 2025 Restate Software, Inc., Restate GmbH.


Suggested change

// Copyright (c) 2023 - 2025 Restate Software, Inc., Restate GmbH.

// Copyright (c) 2025 Restate Software, Inc., Restate GmbH.

pcholakov · 2025-01-31T13:16:09Z

tools/restatectl/src/connection.rs

+    pub async fn try_each<F, T, Fut>(
+        &self,
+        role: Option<Role>,
+        mut closure: F,
+    ) -> Result<Response<T>, ConnectionInfoError>


Since we only ever construct either ClusterCtrlSvcClient or NodeCtlSvcClient maybe it would be useful to just have two convenience wrappers that pass the closure a pre-instantiated client? That would eliminate a fair amount of duplicated code across the various commands.

I actually tried that but it's much harder than it sounds because of the compiler complaining about captured variable cannot escape FnMut closure body, etc.. so I let it go for now. I will definitely try to do this later.

pcholakov · 2025-01-31T13:30:59Z

tools/restatectl/src/connection.rs

+            Some(role) => Either::Left(nodes_config.iter_role(role)),
+            None => Either::Right(nodes_config.iter()),
+        }
+        .sorted_by(|left, right| {


Nit: The left/right naming threw me for a second because the stream is of type Either - I'd maybe just use a/b here instead :-)

pcholakov · 2025-01-31T13:33:26Z

tools/restatectl/src/connection.rs

+
+#[derive(Debug, thiserror::Error)]
+pub enum ConnectionInfoError {
+    #[error("Could not retrieve nodes configuration. Possible un provisioned cluster!")]


Unprovisioned sounds a bit awkward, maybe something like this is more consistent with the tone we take elsewhere?

Suggested change

#[error("Could not retrieve nodes configuration. Possible un provisioned cluster!")]

#[error("Could not retrieve nodes configuration. Have you provisioned this cluster?")]

pcholakov · 2025-01-31T13:35:11Z

tools/restatectl/src/connection.rs

+        }
+
+        if !answer {
+            // all nodes has errored


Nit: *have! :-)

Suggested change

// all nodes has errored

// all nodes have returned error

pcholakov · 2025-01-31T13:58:25Z

tools/restatectl/src/connection.rs

+            };
+
+            let mut response = match client.get_metadata(req).await {
+                Ok(response) => response.into_inner(),


I'm wondering if we should somehow inform the operator about what node was used to render the response? Maybe not by default but only if debug logging / verbose mode is active?

Summary: Make it easier and cleaner to run the restatectl command by accepting the restate nodes addresses via a unified argument `--address` - Support passing multiple addresses - The command will try to fetch the nodes configuration from the first reachable node - All other subcommands can then use the connection info to use the nodes configuration directly or to connect to a specific node role

muhamadazmy · 2025-02-01T09:34:46Z

Thank you @pcholakov for your very helpful review. I think I processed all your comments. Thanks again :)

muhamadazmy · 2025-02-01T09:37:38Z

Thank you so much @tillrohrmann for your insightful review as always. I already processed all your comments, specially the ones regarding Logs version. I did some refactoring on how we fetch the logs.

for the provision command. I made sure to restore the logic regarding the addresses and i also made sure the user can only provide one address. Although before I only used the first but it's clearer to error if multiple given so there is no confusion over which one was provisioned.

Would love if you can do another pass over the code. Thanks again

muhamadazmy force-pushed the pr2592 branch 2 times, most recently from c68d2ad to 512163f Compare January 31, 2025 09:20

muhamadazmy marked this pull request as ready for review January 31, 2025 09:22

muhamadazmy force-pushed the pr2592 branch from 512163f to 87c62f3 Compare January 31, 2025 09:29

muhamadazmy requested a review from pcholakov January 31, 2025 09:30

tillrohrmann reviewed Jan 31, 2025

View reviewed changes

pcholakov approved these changes Jan 31, 2025

View reviewed changes

muhamadazmy force-pushed the pr2592 branch 3 times, most recently from 16571cc to d3e4791 Compare February 1, 2025 08:58

muhamadazmy force-pushed the pr2592 branch from d3e4791 to dbe67fe Compare February 1, 2025 09:13

muhamadazmy requested a review from tillrohrmann February 1, 2025 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streamline restatectl connection to cluster #2592

Streamline restatectl connection to cluster #2592

muhamadazmy commented Jan 30, 2025

tillrohrmann left a comment

tillrohrmann Jan 31, 2025

muhamadazmy Jan 31, 2025

pcholakov Jan 31, 2025

tillrohrmann Jan 31, 2025

muhamadazmy Jan 31, 2025

tillrohrmann Jan 31, 2025

tillrohrmann Jan 31, 2025

tillrohrmann Jan 31, 2025

tillrohrmann Jan 31, 2025

tillrohrmann Jan 31, 2025

tillrohrmann Jan 31, 2025

tillrohrmann Jan 31, 2025

muhamadazmy Jan 31, 2025

pcholakov left a comment

pcholakov Jan 31, 2025

pcholakov Jan 31, 2025

pcholakov Jan 31, 2025

muhamadazmy Feb 1, 2025

pcholakov Jan 31, 2025

pcholakov Jan 31, 2025

pcholakov Jan 31, 2025

pcholakov Jan 31, 2025

muhamadazmy commented Feb 1, 2025

muhamadazmy commented Feb 1, 2025

	// nodes that we already have channels open for, gets higher presence.
	// nodes that we already have channels open for get higher presence.

	#[error("Could not retrieve nodes configuration. Possible un provisioned cluster!")]
	#[error("Could not retrieve nodes configuration. Has the cluster been provisioned yet?")]

	"Encountered multiple errors trying to retrieve nodes configurations:"
	"Encountered multiple errors trying to retrieve nodes' configuration:"

	// nodes that we already have channels open for, gets higher presence.
	// nodes for which we already have open channels get higher precedence.

		@@ -0,0 +1,267 @@
		// Copyright (c) 2023 - 2025 Restate Software, Inc., Restate GmbH.

	// Copyright (c) 2023 - 2025 Restate Software, Inc., Restate GmbH.
	// Copyright (c) 2025 Restate Software, Inc., Restate GmbH.

Streamline restatectl connection to cluster #2592

Are you sure you want to change the base?

Streamline restatectl connection to cluster #2592

Conversation

muhamadazmy commented Jan 30, 2025

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcholakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy commented Feb 1, 2025

muhamadazmy commented Feb 1, 2025