core: Add basic system-config support #8572

ooststep · 2023-02-27T16:49:12Z

Add support for reading environment variables from a config file on the system. The config file supports a single definition per line, defined as KEY=VALUE any whitespaces (other than '\n') will be preserved.

When loading environment variables, check this file for definitions as a fallback

Runtime definitions will overwrite system definitions

This targets an initial solution for #8308

ooststep

adding some notes to take care of before merge - avoiding cycling CI before further review

src/var.c

ooststep · 2023-02-27T17:31:20Z

oops, need to use some OS abstraction to open/read the file.

src/var.c

ooststep · 2023-02-27T18:14:30Z

oops, need to use some OS abstraction to open/read the file.

I misunderstood the CI error at first. I think how I access the file should work, but the SYSCONFDIR preprocessor define seems undefined for Windows build.

src/var.c

shefty · 2023-02-28T17:51:10Z

Please update the commit message title to something like "core: Add support for system-config files". I'm mostly focused on the 'core' part, so I can identify what component the patch applies to when running git log --oneline 6 months from now.

shefty

I'm thinking we may want to add an option to the config file to force it to override the system values in case of a conflict.

src/var.c

ooststep · 2023-02-28T18:43:20Z

I'm thinking we may want to add an option to the config file to force it to override the system values in case of a conflict.

what system values? environment defined values will override config defined values

shefty · 2023-02-28T19:18:31Z

I'm suggesting a separate change that allows the config file to override environment variables

j-xiong · 2023-03-01T20:39:51Z

src/var.c

+	if (ofi_prefer_sysconfig) {
 		conf = find_conf_entry(param->env_var_name);
 		if (!conf) {
-			FI_INFO(provider, FI_LOG_CORE,
-				"variable %s=<not set>\n", param_name);
-			ret = -FI_ENODATA;
-			goto out;
+			FI_DBG(provider, FI_LOG_CORE,
+			       "variable %s=<not set>, checking environment\n",
+			       param_name);
+			str_value = getenv(param->env_var_name);
+			if (!str_value) {
+				FI_INFO(provider, FI_LOG_CORE,
+					"variable %s=<not set>\n", param_name);
+				ret = -FI_ENODATA;
+				goto out;
+			}
+		} else {
+			str_value = conf->value;
+		}
+	} else {
+		str_value = getenv(param->env_var_name);
+		if (!str_value) {
+			FI_DBG(provider, FI_LOG_CORE,
+			       "variable %s=<not set>, checking system\n",
+			       param_name);
+			conf = find_conf_entry(param->env_var_name);
+			if (!conf) {
+				FI_INFO(provider, FI_LOG_CORE,
+					"variable %s=<not set>\n", param_name);
+				ret = -FI_ENODATA;
+				goto out;
+			}
+			str_value = conf->value;
 		}
-		str_value = conf->value;
 	}


To reduce code duplication, you can read both conf and env first, and choose the right one based on preference. Something like:

if (ofi_prefer_sysconfig) str_value = conf->value ? conf->value : env_value; else str_value = env_value ? env_value : conf->value; if (!str_value) FI_DBG(.....);

I did think about this and can move to this if preferred. my concern was adding extra calls to query the environment or looping through the config cache. I suppose the expectation is the cache should be pretty small so querying the environment would be the bigger concern, but we're doing that anyway today so maybe my concern was misplaced...

updated to remove duplication!

shefty · 2023-03-01T22:07:34Z

src/var.c

+			break;
+		}
+
+		dlist_insert_tail(&conf->entry, &conf_list);


It would be nice to add FI_INFO() here using the core to print all read name-value pairs. This can be added as a separate change.

the logging interface requires an instance of a provider, so we can either dump this late (maybe during getinfo?) or we need to add a logging function with that requirement removed.

Use 'core_prov', which is for this purpose.

ah, tmyk...

Other issue is that this takes place prior to fi_log_init, so the logging system hasn't been initialized. The logging system relies on these variables, so it seems a catch 22

bah, i guess we'll figure it out when the variables are assigned.

Add support for reading environment variables from a config file on the system. The config file supports a single definition per line, defined as KEY=VALUE any whitespaces (other than '\n') will be preserved. When loading environment variables, check this file for definitions as a fallback Runtime definitions will overwrite system definitions Signed-off-by: Stephen Oost <stephen.oost@intel.com>

There are some situations where the user or administrator will want to force system-configured values for certain variables. This switch enables the config file to select such an option. Signed-off-by: Stephen Oost <stephen.oost@intel.com>

Signed-off-by: Stephen Oost <stephen.oost@intel.com>

ooststep requested a review from shefty February 27, 2023 16:49

ooststep force-pushed the main branch from 01dd0ba to 50ff47a Compare February 27, 2023 16:54

ooststep commented Feb 27, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

j-xiong reviewed Feb 27, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

ooststep force-pushed the main branch 2 times, most recently from 200049c to d498ead Compare February 27, 2023 22:49

shefty reviewed Feb 27, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

ooststep force-pushed the main branch from d498ead to 1d25992 Compare February 27, 2023 23:25

j-xiong reviewed Feb 28, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

j-xiong reviewed Feb 28, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

j-xiong reviewed Feb 28, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

j-xiong reviewed Feb 28, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

ooststep force-pushed the main branch 2 times, most recently from 2ec31e5 to 28a0c6b Compare February 28, 2023 15:42

j-xiong approved these changes Feb 28, 2023

View reviewed changes

shefty mentioned this pull request Feb 28, 2023

src/hmem_cuda: Check FI_HMEM_CUDA_ENABLE_XFER in cuda_get_xfer_setting() #8529

Closed

shefty reviewed Feb 28, 2023

View reviewed changes

src/var.c Outdated Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

src/var.c Show resolved Hide resolved

src/var.c Outdated Show resolved Hide resolved

ooststep force-pushed the main branch 2 times, most recently from 2de3f0c to f6fb106 Compare February 28, 2023 18:42

ooststep changed the title ~~Add basic system-config support~~ core: Add basic system-config support Feb 28, 2023

ooststep force-pushed the main branch from f6fb106 to 116e7d9 Compare March 1, 2023 19:16

j-xiong reviewed Mar 1, 2023

View reviewed changes

shefty approved these changes Mar 1, 2023

View reviewed changes

ooststep added 2 commits March 2, 2023 06:32

ooststep force-pushed the main branch 2 times, most recently from a683510 to b9c6b7c Compare March 2, 2023 14:58

shefty approved these changes Mar 2, 2023

View reviewed changes

core: list variables read from config file

2013561

Signed-off-by: Stephen Oost <stephen.oost@intel.com>

shefty merged commit 2355120 into ofiwg:main Mar 3, 2023

shefty mentioned this pull request Mar 3, 2023

core: Allow external control over provider ordering #8308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Add basic system-config support #8572

core: Add basic system-config support #8572

ooststep commented Feb 27, 2023

ooststep left a comment

ooststep commented Feb 27, 2023

ooststep commented Feb 27, 2023

shefty commented Feb 28, 2023

shefty left a comment

ooststep commented Feb 28, 2023

shefty commented Feb 28, 2023

j-xiong Mar 1, 2023

ooststep Mar 1, 2023

ooststep Mar 2, 2023

shefty Mar 1, 2023

ooststep Mar 2, 2023

shefty Mar 2, 2023

ooststep Mar 2, 2023

shefty Mar 2, 2023

core: Add basic system-config support #8572

core: Add basic system-config support #8572

Conversation

ooststep commented Feb 27, 2023

ooststep left a comment

Choose a reason for hiding this comment

ooststep commented Feb 27, 2023

ooststep commented Feb 27, 2023

shefty commented Feb 28, 2023

shefty left a comment

Choose a reason for hiding this comment

ooststep commented Feb 28, 2023

shefty commented Feb 28, 2023

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