issue: 4207451 Move from env params to config file #312

tomerdbz · 2025-02-23T14:05:51Z

MVP: Move from env params to config file.

This change introduces a new configuration file that will be used to configure the system.

The configuration file is a JSON file that contains all the configuration parameters that were previously passed as environment variables.

In the case of a failure to load the configuration file, the system will fall back to the old environment variables method.

See HLD and LLD for more information.

This patch also adds unit-tests for the new configuration system.

Change type

What kind of change does this PR introduce?

Check list

Code follows the style de facto guidelines of this project
Comments have been inserted in hard to understand places
Documentation has been updated (if necessary)
Test has been added (if possible)

tomerdbz · 2025-02-25T10:00:54Z

A.I. - before merging open ticket to devops to build and run unit_tests by:
cd libxlio/tests/unit_tests
make -j
./unit_tests

tomerdbz · 2025-03-31T09:40:56Z

bot:retest

MVP: Move from env params to config file. This change introduces a new configuration file that will be used to configure the system. The configuration file is a JSON file that contains all the configuration parameters that were previously passed as environment variables. In the case of a failure to load the configuration file, the system will fall back to the old environment variables method. See HLD and LLD for more information. This patch also adds unit-tests for the new configuration system. Signed-off-by: Tomer Cabouly <[email protected]>

Complete transformation of the XLIO configuration descriptor to a standard JSON Schema (draft-07) format, preserving the full hierarchical structure, data types, constraints, and default values from the original configuration. Key improvements: - Implemented proper oneOf schema handling for properties - Added complete descriptions for all properties and objects This schema enables formal validation of configuration files and supports tooling for IDE integration, documentation generation, and configuration validation. Signed-off-by: Tomer Cabouly <[email protected]>

Change the transport_control property from a string to an array of objects, where each object contains: - id: String identifier for the transport control - name: String name for the application - actions: Array of string actions To support this change, add full array handling to the config system: - Update JSON descriptor - define array types and nested properties - Implement array type recognition and default value handling - Modify JSON loader to process arrays instead of throwing an exception - Add comprehensive unit tests for arrays This enables more structured configuration for transport control rules and provides general array support throughout the configuration system. Signed-off-by: Tomer Cabouly <[email protected]>

galnoam · 2025-03-31T17:20:47Z

config/m4/json.m4

@@ -0,0 +1,48 @@
+#
+# SPDX-FileCopyrightText: NVIDIA CORPORATION & AFFILIATES


We have a new license format, please change in all new files

galnoam · 2025-03-31T17:21:06Z

configure.ac

@@ -413,6 +415,7 @@ AC_CONFIG_FILES([
 		src/state_machine/Makefile
 		tests/Makefile
 		tests/gtest/Makefile
+        tests/unit_tests/Makefile


indentation

galnoam · 2025-03-31T17:32:44Z

src/core/util/sys_vars.h

@@ -513,6 +514,9 @@ struct mce_sys_var {
    } m_ioctl;

 private:
+    void get_app_name();
+    void legacy_get_env_params();
+    void new_get_params(const config_manager &config_manager);


avoid using new in the names, better to change the deprecated to legacy or something

galnoam · 2025-03-31T17:33:11Z

src/core/util/sys_vars.h

@@ -527,11 +531,11 @@ struct mce_sys_var {

    // prevent unautothrized creation of objects
    mce_sys_var()
-        : sysctl_reader(sysctl_reader_t::instance())
+        : sysctl_reader(sysctl_reader_t::instance()) // TODO - Bashar


remove TODO, we have task on it

galnoam · 2025-03-31T17:47:52Z

src/core/util/sys_vars.cpp

+            "---------------------------------------------------------------------------\n");
+        legacy_get_env_params();
+    } else {
+        new_get_params(config_manager());


Why do we need to get it as param, can be created inside the function.

galnoam · 2025-03-31T17:56:41Z

src/core/util/sys_vars.cpp

+        config_manager.get_value<int64_t>("xlio.ring.tx.alloc_logic");
+    ring_allocation_logic_tx = static_cast<ring_logic_t>(ring_tx_alloc_logic);
+
+    const int64_t ring_rx_alloc_logic =


Why int64_t, you are casting to enum which i think its int basically.
probably you will not need casting.
relevant to all params casting to enum if you think it should be changed.

galnoam · 2025-03-31T18:00:53Z

src/core/util/sys_vars.cpp

+    //	exception_handling is handled by its CTOR
+
+    wait_after_join_msec =
+        MCE_DEFAULT_WAIT_AFTER_JOIN_MSEC; // TODO - not in use - should be deleted


can you explain?

galnoam · 2025-03-31T18:07:43Z

src/core/util/sys_vars.cpp

+    }
+}
+
+void mce_sys_var::new_get_params(const config_manager &config_manager)


I know you got this from original code. but lets try to break it down to subfunctions

galnoam · 2025-03-31T18:08:18Z

src/core/util/sys_vars.cpp

+{
+    const bool core_append_pid_to_path = config_manager.get_value<bool>("core.append_pid_to_path");
+
+    const std::string net_offload_transport_control =


move all param loading to help function

galnoam · 2025-03-31T18:09:00Z

src/core/util/sys_vars.cpp

+    }
+#endif // DEFINED_NGINX
+
+    switch (mce_spec) {


each profile can call to help function to set its params

tomerdbz · 2025-04-03T08:00:34Z

add to description of json_descriptor.json - what's the schema purpose
add comments for the variable created in compile time
auto loaded_data = // remove the auto

galnoam · 2025-04-03T08:23:17Z

src/core/config/config_manager.h

+#include <memory>
+#include <queue>
+
+class config_manager : public config {


config_manager_interface?

galnoam · 2025-04-03T08:30:49Z

src/core/config/config_manager.h

+    std::experimental::any get_value_as_any(const std::string &key) const override;
+
+private:
+    config_bundle m_config_bundle;


Usualy we do this when we want to pass params to function or return from function.
but here its feels like hiding the config members. i think better without it

galnoam · 2025-04-03T08:36:47Z

src/core/config/config_manager.cpp

+    // 2) Load or build the descriptor structure
+    m_config_bundle.m_config_descriptor = descriptor_provider->load_descriptors();
+
+    // 3) Validate all settings on construction (fail fast if invalid)


avoid comments that can understood from the code itself.

galnoam · 2025-04-03T08:37:12Z

src/core/config/config_manager.cpp

+
+    m_config_bundle.m_config_data = std::move(aggregated_config_data);
+
+    // 2) Load or build the descriptor structure


avoid comments that can be understood from the code itself

galnoam · 2025-04-03T08:37:26Z

src/core/config/config_manager.cpp

+        throw_xlio_exception("loader/descriptor_provider cannot be null");
+    }
+
+    // 1) Load raw config data - first in queue means higher priority


remove index in comment

galnoam · 2025-04-03T11:45:50Z

src/core/config/descriptor_providers/json_descriptor_provider.h

+
+private:
+    const char *m_json_string;
+    void validate_schema(json_object *schema);


add new line between members and functions

galnoam · 2025-04-03T11:46:27Z

src/core/config/descriptor_providers/json_descriptor_provider.h

+    explicit json_descriptor_provider();
+    explicit json_descriptor_provider(const char *json_string);
+    ~json_descriptor_provider() override = default;
+    config_descriptor load_descriptors() override;


add new line between constructors and member functions

galnoam · 2025-04-03T11:49:16Z

src/core/config/descriptors/parameter_descriptor.h

+
+class parameter_descriptor {
+public:
+    explicit parameter_descriptor() = default;


why so mant constructors?
can we join?

because parameter_descriptor is in a container we have to supply empty, copy and move ctors. the copy ctor is not trivial but the move ctor is so made it default and spared some code

galnoam · 2025-04-03T11:50:00Z

src/core/config/descriptors/parameter_descriptor.h

+
+    void add_string_mapping(const std::string &str, const std::experimental::any &val);
+
+    // Validates the given value against:


cnage the name on the function and avoid the comments

galnoam · 2025-04-03T11:50:28Z

src/core/config/descriptors/parameter_descriptor.h

+    std::experimental::any default_value() const;
+
+    using constraint_t = std::function<bool(const std::experimental::any &)>;
+    void add_constraint(constraint_t c);


c not a good name

tomerdbz requested review from pasis, AlexanderGrissik, BasharRadya and galnoam February 23, 2025 14:06

galnoam removed request for AlexanderGrissik and BasharRadya February 23, 2025 18:14

tomerdbz force-pushed the new_config branch 10 times, most recently from 8d82cb0 to 73ee4a9 Compare February 24, 2025 16:54

tomerdbz requested review from igor-ivanov, AlexanderGrissik and BasharRadya February 24, 2025 16:55

tomerdbz force-pushed the new_config branch 2 times, most recently from a18146f to ef448be Compare February 24, 2025 16:59

tomerdbz removed request for igor-ivanov, AlexanderGrissik and BasharRadya February 25, 2025 07:06

tomerdbz force-pushed the new_config branch from ef448be to afc8566 Compare February 25, 2025 07:45

tomerdbz force-pushed the new_config branch 4 times, most recently from e59a4a0 to 1beb24a Compare February 26, 2025 09:39

tomerdbz force-pushed the new_config branch from 1beb24a to b687054 Compare April 1, 2025 09:23

tomerdbz force-pushed the new_config branch from 579eed9 to fa9718d Compare April 2, 2025 07:57

galnoam reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue: 4207451 Move from env params to config file #312

issue: 4207451 Move from env params to config file #312

tomerdbz commented Feb 23, 2025

tomerdbz commented Feb 25, 2025

tomerdbz commented Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

galnoam Mar 31, 2025

tomerdbz commented Apr 3, 2025 •

edited

Loading

galnoam Apr 3, 2025

galnoam Apr 3, 2025

galnoam Apr 3, 2025

galnoam Apr 3, 2025

galnoam Apr 3, 2025

galnoam Apr 3, 2025

galnoam Apr 3, 2025

galnoam Apr 3, 2025

tomerdbz Apr 3, 2025 •

edited

Loading

galnoam Apr 3, 2025

galnoam Apr 3, 2025

		@@ -0,0 +1,48 @@
		#
		# SPDX-FileCopyrightText: NVIDIA CORPORATION & AFFILIATES


		m_config_bundle.m_config_data = std::move(aggregated_config_data);

		// 2) Load or build the descriptor structure


		void add_string_mapping(const std::string &str, const std::experimental::any &val);

		// Validates the given value against:

issue: 4207451 Move from env params to config file #312

Are you sure you want to change the base?

issue: 4207451 Move from env params to config file #312

Conversation

tomerdbz commented Feb 23, 2025

Change type

Check list

tomerdbz commented Feb 25, 2025

tomerdbz commented Mar 31, 2025

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

tomerdbz commented Apr 3, 2025 • edited Loading

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

tomerdbz Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerdbz commented Apr 3, 2025 •

edited

Loading

tomerdbz Apr 3, 2025 •

edited

Loading