Prevent config rendering on every process #3011

gememma · 2025-01-10T14:40:59Z

…config

…date_env() instead

meowjesty

Just some small stuff.

So if we save the config to an env var, we don't get the templating breaking on get_env secondary calls? Since we only do it once, is that what this PR is solving?

mirrord/config/Cargo.toml

mirrord/config/src/feature/network/incoming.rs

meowjesty · 2025-01-14T14:30:46Z

mirrord/config/src/lib.rs

@@ -327,15 +331,50 @@ pub struct LayerConfig {
 }

 impl LayerConfig {
+    /// Given an encoded complete config from the [`MIRRORD_RESOLVED_CONFIG_ENV`]
+    /// env var, attempt to decode it into [`LayerConfig`].
+    /// Intended to avoid re-resolving the config in every process mirrord in loaded into


Suggested change

/// Intended to avoid re-resolving the config in every process mirrord in loaded into

/// Intended to avoid re-resolving the config in every process mirrord is loaded into.

meowjesty · 2025-01-14T14:37:21Z

mirrord/config/src/lib.rs

+        Ok(BASE64_STANDARD.encode(serialized))
+    }
+
+    pub fn update_env(&self) -> Result<(), ConfigError> {


Suggested change

pub fn update_env(&self) -> Result<(), ConfigError> {

pub fn update_env_var(&self) -> Result<(), ConfigError> {

Nit, since the others are *_env_var.

meowjesty · 2025-01-14T14:39:32Z

mirrord/config/src/lib.rs

+    /// checks that resolved config written to [`MIRRORD_RESOLVED_CONFIG_ENV`] can be
+    /// transformed back into a [`LayerConfig`]
+    #[test]
+    fn encode_and_decode_config() {


I wonder if we should have a test value with a filled config too, instead of just default values.

Wdyt?

good idea, i can hit some more unusual cases 🫡

meowjesty · 2025-01-14T14:40:40Z

mirrord/config/src/lib.rs

+        Ok(serde_json::from_str::<Self>(serialized)?)
+    }
+
+    pub fn to_env_var(&self) -> Result<String, ConfigError> {


Docs for this new friend.

meowjesty · 2025-01-14T14:40:49Z

mirrord/config/src/lib.rs

+        Ok(BASE64_STANDARD.encode(serialized))
+    }
+
+    pub fn update_env(&self) -> Result<(), ConfigError> {


Docs for this new friend.

meowjesty · 2025-01-14T14:41:04Z

mirrord/config/src/lib.rs

+    /// Given an encoded complete config from the [`MIRRORD_RESOLVED_CONFIG_ENV`]
+    /// env var, attempt to decode it into [`LayerConfig`].
+    /// Intended to avoid re-resolving the config in every process mirrord in loaded into
+    pub fn from_env_var(encoded_value: String) -> Result<Self, ConfigError> {


Do we need this to be pub?

meowjesty · 2025-01-14T14:41:09Z

mirrord/config/src/lib.rs

+        Ok(serde_json::from_str::<Self>(serialized)?)
+    }
+
+    pub fn to_env_var(&self) -> Result<String, ConfigError> {


Do we need this to be pub?

meowjesty · 2025-01-14T14:46:16Z

mirrord/cli/src/execution.rs

-            MIRRORD_CONNECT_TCP_ENV.to_string(),
-            format!("127.0.0.1:{}", address.port()),
-        );
+        let mut config = config.clone();


Instead of clone to mut here, why not take this as &mut in the function args?

gememma · 2025-01-14T15:06:33Z

So if we save the config to an env var, we don't get the templating breaking on get_env secondary calls? Since we only do it once, is that what this PR is solving?

@meowjesty That's correct - the config template variables get resolved when the config file is rendered by Tera, which previously happened on every process. This causes an issue when a process tries to, for example, call get_env() to render the config but the process doesnt have access to the same env as previous ones - doing the get_env() call only once will solve this :) plus I think this way is more sensible in terms of reducing pointless recalculation and having fewer sources of truth

DmitryDodzin

I would check mirrord container due to changes to MIRRORD_CONNECT_TCP_ENV in exec, it should work but I would double check

DmitryDodzin · 2025-01-14T15:10:29Z

mirrord/config/src/agent.rs

@@ -354,7 +354,7 @@ pub struct AgentConfig {
    /// Create an agent that returns an error after accepting the first client. For testing
    /// purposes. Only supported with job agents (not with ephemeral agents).
    #[cfg(all(debug_assertions, not(test)))] // not(test) so that it's not included in the schema json.
-    #[serde(skip_serializing)]
+    #[serde(skip_serializing, skip_deserializing)]


Suggested change

#[serde(skip_serializing, skip_deserializing)]

#[serde(skip)]

DmitryDodzin · 2025-01-14T15:13:13Z

mirrord/cli/src/execution.rs

-            MIRRORD_CONNECT_TCP_ENV.to_string(),
-            format!("127.0.0.1:{}", address.port()),
-        );
+        let mut config = config.clone();


Im a bit worried for container feature behaviour where we split up the env variables for sidecar and main container relying on MIRRORD_CONNECT_TCP_ENV as env value

DmitryDodzin · 2025-01-14T15:21:56Z

mirrord/config/src/lib.rs

+    }
+
+    pub fn update_env(&self) -> Result<(), ConfigError> {
+        std::env::set_var(MIRRORD_RESOLVED_CONFIG_ENV, self.to_env_var()?);


There might be a problem with using env variables due to size (especially if tls certs are included),my local test macos can handle ~512mb but can possibly cause a problem on some linux distribution

Good point. Think we could save it to /tmp and refresh it from there?

this honestly didnt even occur to me 🙃 @aviramha @Razz4780 any opinions on this? i feel like there was maybe a reason to avoid using /tmp ?

gememma added 6 commits January 10, 2025 15:00

Add detecting encoded config from env var

44e7351

[WIP] check for empty env var value

8726677

Skip test_error config field when deserializing

938c27e

Add unit test to check if encoding and decoding matches the original …

8c13deb

…config

Changelog

da63bc7

Fix clippy

16d7df1

gememma force-pushed the gemma/config-resolution-hang branch from 64265cd to 16d7df1 Compare January 10, 2025 15:00

Remove direct interactions with env vars for fields in config, use up…

2aa9588

…date_env() instead

gememma force-pushed the gemma/config-resolution-hang branch from 1affb5f to 2aa9588 Compare January 14, 2025 13:45

meowjesty self-assigned this Jan 14, 2025

Razz4780 requested a review from DmitryDodzin January 14, 2025 14:43

meowjesty requested changes Jan 14, 2025

View reviewed changes

DmitryDodzin reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent config rendering on every process #3011

Prevent config rendering on every process #3011

gememma commented Jan 10, 2025

meowjesty left a comment

meowjesty Jan 14, 2025

meowjesty Jan 14, 2025

meowjesty Jan 14, 2025

gememma Jan 14, 2025

meowjesty Jan 14, 2025

meowjesty Jan 14, 2025

meowjesty Jan 14, 2025

meowjesty Jan 14, 2025

meowjesty Jan 14, 2025

gememma commented Jan 14, 2025

DmitryDodzin left a comment

DmitryDodzin Jan 14, 2025

DmitryDodzin Jan 14, 2025

DmitryDodzin Jan 14, 2025

meowjesty Jan 14, 2025

gememma Jan 14, 2025

	/// Intended to avoid re-resolving the config in every process mirrord in loaded into
	/// Intended to avoid re-resolving the config in every process mirrord is loaded into.

	pub fn update_env(&self) -> Result<(), ConfigError> {
	pub fn update_env_var(&self) -> Result<(), ConfigError> {

	#[serde(skip_serializing, skip_deserializing)]
	#[serde(skip)]

Prevent config rendering on every process #3011

Are you sure you want to change the base?

Prevent config rendering on every process #3011

Conversation

gememma commented Jan 10, 2025

meowjesty 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

gememma commented Jan 14, 2025

DmitryDodzin 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