Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UCS/CONFIG: Filter out not loaded config tables for unused vars message #8557

Merged
merged 4 commits into from
Oct 30, 2022
Merged

UCS/CONFIG: Filter out not loaded config tables for unused vars message #8557

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27ac58b
UCS/CONFIG: filter out not loaded config tables for unused vars message
shasson5 Oct 25, 2022
81c00ba
UCS/CONFIG: Fixed test to support setups with no IB devices
shasson5 Oct 27, 2022
801c1ef
UCS/CONFIG: CR fixes
shasson5 Oct 28, 2022
0ad5005
UCS/CONFIG: CR fixes
shasson5 Oct 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/ucp/core/ucp_context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,8 +592,9 @@ ucs_status_t ucp_config_read(const char *env_prefix, const char *filename,
UCS_DEFAULT_ENV_PREFIX);
}

status = ucs_config_parser_fill_opts(config, ucp_config_table,
config->env_prefix, NULL, 0);
status = ucs_config_parser_fill_opts(config,
UCS_CONFIG_GET_TABLE(ucp_config_table),
config->env_prefix, 0);
if (status != UCS_OK) {
goto err_free_prefix;
}
Expand Down
12 changes: 7 additions & 5 deletions src/ucs/config/global_opts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,16 +424,18 @@ void ucs_global_opts_init()
UCS_CONFIG_ADD_TABLE(ucs_global_opts_read_only_table,
&ucs_config_global_list);

status = ucs_config_parser_fill_opts(&ucs_global_opts,
ucs_global_opts_read_only_table,
UCS_DEFAULT_ENV_PREFIX, NULL, 1);
status = ucs_config_parser_fill_opts(
&ucs_global_opts,
UCS_CONFIG_GET_TABLE(ucs_global_opts_read_only_table),
UCS_DEFAULT_ENV_PREFIX, 1);
if (status != UCS_OK) {
ucs_fatal("failed to parse global runtime read-only configuration");
}

status = ucs_config_parser_fill_opts(&ucs_global_opts,
ucs_global_opts_table,
UCS_DEFAULT_ENV_PREFIX, NULL, 1);
UCS_CONFIG_GET_TABLE(
ucs_global_opts_table),
UCS_DEFAULT_ENV_PREFIX, 1);
if (status != UCS_OK) {
ucs_fatal("failed to parse global configuration");
}
Expand Down
31 changes: 17 additions & 14 deletions src/ucs/config/parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,17 +1468,16 @@ void ucs_config_parse_config_files()
ucs_config_parse_config_file(".", UCX_CONFIG_FILE_NAME, 1);
}

ucs_status_t ucs_config_parser_fill_opts(void *opts, ucs_config_field_t *fields,
const char *env_prefix,
const char *table_prefix,
int ignore_errors)
ucs_status_t
ucs_config_parser_fill_opts(void *opts, ucs_config_global_list_entry_t *entry,
const char *env_prefix, int ignore_errors)
{
const char *sub_prefix = NULL;
static ucs_init_once_t config_file_parse = UCS_INIT_ONCE_INITIALIZER;
ucs_status_t status;

/* Set default values */
status = ucs_config_parser_set_default_values(opts, fields);
status = ucs_config_parser_set_default_values(opts, entry->table);
if (status != UCS_OK) {
goto err;
}
Expand All @@ -1495,24 +1494,26 @@ ucs_status_t ucs_config_parser_fill_opts(void *opts, ucs_config_field_t *fields,

/* Apply environment variables */
if (sub_prefix != NULL) {
status = ucs_config_apply_config_vars(opts, fields, sub_prefix,
table_prefix, 1, ignore_errors);
status = ucs_config_apply_config_vars(opts, entry->table, sub_prefix,
entry->prefix, 1, ignore_errors);
if (status != UCS_OK) {
goto err_free;
}
}

/* Apply environment variables with custom prefix */
status = ucs_config_apply_config_vars(opts, fields, env_prefix,
table_prefix, 1, ignore_errors);
status = ucs_config_apply_config_vars(opts, entry->table, env_prefix,
entry->prefix, 1, ignore_errors);
if (status != UCS_OK) {
goto err_free;
}

entry->flags |= UCS_CONFIG_TABLE_FLAG_LOADED;
return UCS_OK;

err_free:
ucs_config_parser_release_opts(opts, fields); /* Release default values */
ucs_config_parser_release_opts(opts,
entry->table); /* Release default values */
err:
return status;
}
Expand Down Expand Up @@ -1841,7 +1842,7 @@ void ucs_config_parser_print_all_opts(FILE *stream, const char *prefix,
ucs_config_print_flags_t flags,
ucs_list_link_t *config_list)
{
const ucs_config_global_list_entry_t *entry;
ucs_config_global_list_entry_t *entry;
ucs_status_t status;
char title[64];
void *opts;
Expand All @@ -1859,8 +1860,7 @@ void ucs_config_parser_print_all_opts(FILE *stream, const char *prefix,
continue;
}

status = ucs_config_parser_fill_opts(opts, entry->table, prefix,
entry->prefix, 0);
status = ucs_config_parser_fill_opts(opts, entry, prefix, 0);
if (status != UCS_OK) {
ucs_free(opts);
continue;
Expand Down Expand Up @@ -1910,7 +1910,10 @@ static void ucs_config_parser_append_similar_vars_message(

ucs_list_for_each(entry, &ucs_config_global_list, list) {
if ((entry->table == NULL) ||
ucs_config_field_is_last(&entry->table[0])) {
ucs_config_field_is_last(&entry->table[0]) ||
/* Show warning only for loaded tables (we only want to display
the relevant env vars) */
!(entry->flags & UCS_CONFIG_TABLE_FLAG_LOADED)) {
continue;
}

Expand Down
28 changes: 21 additions & 7 deletions src/ucs/config/parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,22 @@ typedef struct ucs_range_spec {
} ucs_range_spec_t;


/**
yosefe marked this conversation as resolved.
Show resolved Hide resolved
* Configuration table flags
*/
typedef enum {
/* Table has been already loaded by config parser */
UCS_CONFIG_TABLE_FLAG_LOADED = UCS_BIT(0)
} ucs_config_table_flags_t;
yosefe marked this conversation as resolved.
Show resolved Hide resolved


typedef struct ucs_config_global_list_entry {
const char *name; /* configuration table name */
const char *prefix; /* configuration prefix */
ucs_config_field_t *table; /* array of configuration fields */
size_t size; /* size of config structure */
ucs_list_link_t list; /* entry in global list */
uint8_t flags; /* config table flags */
} ucs_config_global_list_entry_t;


Expand Down Expand Up @@ -127,9 +137,14 @@ typedef struct ucs_config_bw_spec {
.table = _table, \
.name = _name, \
.prefix = _prefix, \
.size = sizeof(_type) \
.size = sizeof(_type), \
.flags = 0 \
};


#define UCS_CONFIG_GET_TABLE(_table) &_table##_config_entry
gleon99 marked this conversation as resolved.
Show resolved Hide resolved


#define UCS_CONFIG_ADD_TABLE(_table, _list) \
ucs_list_add_tail(_list, &(_table##_config_entry).list)

Expand Down Expand Up @@ -423,17 +438,16 @@ void ucs_config_parse_config_files();
* Fill existing opts structure.
*
* @param opts User-defined options structure to fill.
* @param fields Array of fields which define how to parse.
* @param entry Global configuration list entry which contains
* relevant fields (eg. parser info, prefix etc).
* @param env_prefix Prefix to add to all environment variables,
* env_prefix may consist of multiple sub prefixes
* @param table_prefix Optional prefix to add to the variables of top-level table.
* @param ignore_errors Whether to ignore parsing errors and continue parsing
* other fields.
*/
ucs_status_t ucs_config_parser_fill_opts(void *opts, ucs_config_field_t *fields,
const char *env_prefix,
const char *table_prefix,
int ignore_errors);
ucs_status_t
gleon99 marked this conversation as resolved.
Show resolved Hide resolved
ucs_config_parser_fill_opts(void *opts, ucs_config_global_list_entry_t *entry,
const char *env_prefix, int ignore_errors);

/**
* Perform deep copy of the options structure.
Expand Down
7 changes: 4 additions & 3 deletions src/ucs/config/ucm_opts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,10 @@ void ucs_init_ucm_opts()

UCS_CONFIG_ADD_TABLE(ucm_global_config_table, &ucs_config_global_list);

(void)ucs_config_parser_fill_opts(&ucm_opts, ucm_global_config_table,
UCS_DEFAULT_ENV_PREFIX, UCM_CONFIG_PREFIX,
0);
(void)ucs_config_parser_fill_opts(&ucm_opts,
UCS_CONFIG_GET_TABLE(
ucm_global_config_table),
UCS_DEFAULT_ENV_PREFIX, 0);
ucm_set_global_opts(&ucm_opts);
}

Expand Down
4 changes: 1 addition & 3 deletions src/uct/base/uct_cm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ ucs_status_t uct_cm_config_read(uct_component_h component,
uct_config_bundle_t *bundle = NULL;
ucs_status_t status;

status = uct_config_read(&bundle, component->cm_config.table,
component->cm_config.size, env_prefix,
component->cm_config.prefix);
status = uct_config_read(&bundle, &component->cm_config, env_prefix);
if (status != UCS_OK) {
ucs_error("failed to read CM configuration");
return status;
Expand Down
18 changes: 9 additions & 9 deletions src/uct/base/uct_component.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,19 +124,19 @@ ucs_status_t uct_component_query(uct_component_h component,
}

ucs_status_t uct_config_read(uct_config_bundle_t **bundle,
ucs_config_field_t *config_table,
size_t config_size, const char *env_prefix,
const char *cfg_prefix)
ucs_config_global_list_entry_t *entry,
const char *env_prefix)
{
char full_prefix[128] = UCS_DEFAULT_ENV_PREFIX;
uct_config_bundle_t *config_bundle;
ucs_status_t status;

if (config_table == NULL) {
if (entry->table == NULL) {
return UCS_ERR_INVALID_PARAM;
}

config_bundle = ucs_calloc(1, sizeof(*config_bundle) + config_size, "uct_config");
config_bundle = ucs_calloc(1, sizeof(*config_bundle) + entry->size,
"uct_config");
if (config_bundle == NULL) {
status = UCS_ERR_NO_MEMORY;
goto err;
Expand All @@ -148,14 +148,14 @@ ucs_status_t uct_config_read(uct_config_bundle_t **bundle,
env_prefix, UCS_DEFAULT_ENV_PREFIX);
}

status = ucs_config_parser_fill_opts(config_bundle->data, config_table,
full_prefix, cfg_prefix, 0);
status = ucs_config_parser_fill_opts(config_bundle->data, entry,
full_prefix, 0);
if (status != UCS_OK) {
goto err_free_bundle;
}

config_bundle->table = config_table;
config_bundle->table_prefix = ucs_strdup(cfg_prefix, "uct_config");
config_bundle->table = entry->table;
config_bundle->table_prefix = ucs_strdup(entry->prefix, "uct_config");
if (config_bundle->table_prefix == NULL) {
status = UCS_ERR_NO_MEMORY;
goto err_free_bundle;
Expand Down
5 changes: 2 additions & 3 deletions src/uct/base/uct_component.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,8 @@ struct uct_component {


ucs_status_t uct_config_read(uct_config_bundle_t **bundle,
ucs_config_field_t *config_table,
size_t config_size, const char *env_prefix,
const char *cfg_prefix);
ucs_config_global_list_entry_t *entry,
const char *env_prefix);

void uct_component_register(uct_component_t *component);

Expand Down
7 changes: 2 additions & 5 deletions src/uct/base/uct_md.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,7 @@ ucs_status_t uct_md_iface_config_read(uct_md_h md, const char *tl_name,
return status;
}

status = uct_config_read(&bundle, tl->config.table, tl->config.size,
env_prefix, tl->config.prefix);
status = uct_config_read(&bundle, &tl->config, env_prefix);
if (status != UCS_OK) {
ucs_error("Failed to read iface config");
return status;
Expand Down Expand Up @@ -300,9 +299,7 @@ ucs_status_t uct_md_config_read(uct_component_h component,
uct_config_bundle_t *bundle = NULL;
ucs_status_t status;

status = uct_config_read(&bundle, component->md_config.table,
component->md_config.size, env_prefix,
component->md_config.prefix);
status = uct_config_read(&bundle, &component->md_config, env_prefix);
if (status != UCS_OK) {
ucs_error("Failed to read MD config");
return status;
Expand Down
27 changes: 19 additions & 8 deletions test/apps/test_fuzzy_match.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,23 @@ def __init__(self, ucx_info, verbose):
self.ucx_info = ucx_info
if verbose:
logging.basicConfig(level=logging.DEBUG)

def ib_available(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_ib_available or has_ib

status, output = commands.getstatusoutput('ibv_devinfo')
if status != 0:
return False

return 'No IB devices found' not in output

def run(self, expected):
with Environment(expected.keys()):
if expected['ib_required'] and not self.ib_available():
# Skip test if IB transport is required but not available.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excessive commenting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean it's not needed at all? or it should just be shorter?

return

with Environment(expected['results'].keys()):
matches = self.get_fuzzy_matches()

if matches != expected:
if matches != expected['results']:
raise Exception('Wrong fuzzy list: got: %s, expected: %s' % (matches, expected))

logging.info('found all expected matches: %s' % expected)
Expand Down Expand Up @@ -86,12 +97,12 @@ def get_fuzzy_matches(self):

try:
runner = TestRunner(args.ucx_info, args.verbose)
expected_list = [{'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL']},
{'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL'], 'UCX_MOFULE_D' : ['UCX_MODULE_DIR', 'UCX_MODULES']},
{'UCX_SOME_VAR' : [], 'UCX_SOME_VAR2' : [], 'UCX_SOME_VAR3' : [], 'UCX_SOME_VAR4' : []},
{'UCX_SOME_VAR' : [], 'UCX_MOFULE_D' : ['UCX_MODULE_DIR', 'UCX_MODULES'], 'UCX_SOME_VAR2' : [], 'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL']},
{'UCX_RC_VERBS_RX_MAX_BUF' : ['UCX_RC_VERBS_TX_MAX_BUFS', 'UCX_RC_VERBS_RX_MAX_BUFS', 'UCX_UD_VERBS_RX_MAX_BUFS']},
{'UCX_RLS' : ['UCX_TLS']}]
expected_list = [{'results' : {'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL']}, 'ib_required': False},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of the following refactoring (simplify).

  1. Rename expected_list to something like test_cases or fuzzy_matches (at least remove _list).
  2. Revert to the "old" structure of the dicts in the list
  3. Split to 2 lists, e.g test_cases, ib_test_cases.
  4. Then can have:
def run(self, test_case, needs_ib=False):
    ...

for test_case in test_cases:
    runner.run(test_case)

for test_case in ib_test_cases:
    runner.run(test_case, needs_ib=True)

{'results' : {'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL'], 'UCX_MOFULE_D' : ['UCX_MODULE_DIR', 'UCX_MODULES']}, 'ib_required': False},
{'results' : {'UCX_SOME_VAR' : [], 'UCX_SOME_VAR2' : [], 'UCX_SOME_VAR3' : [], 'UCX_SOME_VAR4' : []}, 'ib_required': False},
{'results' : {'UCX_SOME_VAR' : [], 'UCX_MOFULE_D' : ['UCX_MODULE_DIR', 'UCX_MODULES'], 'UCX_SOME_VAR2' : [], 'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL']}, 'ib_required': False},
{'results' : {'UCX_RC_VERBS_RX_MAX_BUF' : ['UCX_RC_VERBS_TX_MAX_BUFS', 'UCX_RC_VERBS_RX_MAX_BUFS', 'UCX_UD_VERBS_RX_MAX_BUFS']}, 'ib_required': True},
{'results' : {'UCX_RLS' : ['UCX_TLS']}, 'ib_required': False}]

for expected in expected_list:
runner.run(expected)
Expand Down
29 changes: 20 additions & 9 deletions test/gtest/ucs/test_config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,11 +339,14 @@ class test_config : public ucs::test {

ucs_config_parse_config_files();

status = ucs_config_parser_fill_opts(&tmp,
car_opts_table,
env_prefix,
table_prefix,
0);
static ucs_config_global_list_entry_t entry;
entry.table = car_opts_table;
entry.name = "cars";
entry.prefix = table_prefix;
entry.size = sizeof(car_opts_t);
entry.flags = 0;

status = ucs_config_parser_fill_opts(&tmp, &entry, env_prefix, 0);
ASSERT_UCS_OK(status);
return tmp;
}
Expand Down Expand Up @@ -702,14 +705,22 @@ UCS_TEST_F(test_config, test_config_file_parse_files) {
ucs_status_t status;

ucs_config_parse_config_file(TEST_CONFIG_DIR, TEST_CONFIG_FILE, 1);
status = ucs_config_parser_fill_opts(&opts, car_opts_table,
UCS_DEFAULT_ENV_PREFIX, NULL, 0);

ucs_config_global_list_entry_t entry;
entry.table = car_opts_table;
entry.name = "cars";
entry.prefix = NULL;
entry.size = sizeof(car_opts_t);
entry.flags = 0;

status = ucs_config_parser_fill_opts(&opts, &entry, UCS_DEFAULT_ENV_PREFIX,
0);
EXPECT_EQ(200, opts.price);
ucs_config_parser_release_opts(&opts, car_opts_table);

ucs_config_parse_config_files();
status = ucs_config_parser_fill_opts(&opts, car_opts_table,
UCS_DEFAULT_ENV_PREFIX, NULL, 0);
status = ucs_config_parser_fill_opts(&opts, &entry, UCS_DEFAULT_ENV_PREFIX,
0);
ASSERT_UCS_OK(status);

/* Verify ucs_config_parse_config_files() overrides config */
Expand Down