DAOS-16300 cart: Parse domain properly for multi-interface case #14864

frostedcmos · 2024-08-01T23:14:00Z

parse out domain similarly to interface for multi-interface case
fix issue with multi-interface and cxi workaround
treat not set and set to empty as the same for domain/interface
update test
Required-githooks: true

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

- parse out domain similarly to interface for multi-interface case Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

github-actions · 2024-08-01T23:14:18Z

Ticket title is 'cart: multi-interface case does not parse out D_DOMAIN correctly'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-16300

daosbuild1 · 2024-08-01T23:24:31Z

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14864/1/execution/node/304/log

For cxi we use domain names and not interface names. Create context on interface was not accounting for this, causing wrong handling. Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

daosbuild1 · 2024-08-02T18:30:14Z

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14864/2/execution/node/357/log

daosbuild1 · 2024-08-02T18:30:20Z

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14864/2/execution/node/347/log

daosbuild1 · 2024-08-02T18:30:21Z

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14864/2/execution/node/356/log

Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

for purposes of get_info string generation Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

frostedcmos · 2024-08-03T06:15:47Z

src/cart/crt_init.c


 	if (interface) {
-		if (provider == CRT_PROV_OFI_CXI) {
-			D_INFO("Interface '%s' ignored for CXI. Using domain '%s' instead\n",
-			       interface, domain);
-
-			/* Note: crt_provider_iface_str_get() returns interface name  */


Previously with CXI (as it uses domain names not interface names) we would overwrite interface with domain values, but this causes issues with multi-interface feature that uses interface names as identifiers.
Instead crt_get_info_string() now special case handles CXI provider and only uses domain names there for it.

Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

daltonbohning

ftest changes LGTM

mchaarawi · 2024-08-05T21:45:51Z

I have tested those changes on aurora and with that PR, it works as expected.

soumagne · 2024-08-05T22:03:51Z

src/cart/crt_hg.c

+	/* treat not set and set to empty as the same */
+	no_iface  = (iface_str == NULL || *iface_str == '\0') ? true : false;
+	no_domain = (domain_str == NULL || *domain_str == '\0') ? true : false;
+


I think we'll have to simplify that, I don't really like that we have all this if (no_iface) and if (no_domain) etc :) it's just hard to understand.

sure. some of a mess here is due to us having to form a string from 4 parts, 3 of which are optional. It would have been simpler if we could just fill in 4 fields (provider, domain, interface, port) in a structure as an alternate way of specifying info to mercury:) i feel most providers map well to this 4, and for outliers like 'sm' users can still use string version.

soumagne · 2024-08-05T22:09:44Z

src/cart/crt_hg.c

+	/* CXI provider uses domain names for info string */
+	if (provider == CRT_PROV_OFI_CXI)
+		iface_str = NULL;
+	else


maybe we should improve that by separating init formats, cxi has its own format (cxi:) whereas other providers have IP format (<hostname/IP>:).

maybe we should improve that by separating init formats, cxi has its own format (cxi:) whereas other providers have IP format (<hostname/IP>:).

yeah there are few things we can improve here, lets discuss next cart wg meeting

soumagne · 2024-08-05T22:13:16Z

src/cart/crt_internal_types.h

@@ -34,10 +34,12 @@ struct crt_grp_gdata;
 struct crt_na_config {
 	int32_t		 noc_port;
 	int		 noc_iface_total;
+	int               noc_domain_total;


does not seem aligned

does not seem aligned

yeah, i think whole struct is badly aligned and clangformat is now forcing the right formatting on 1 field only:) will reformat next patch

- parse out domain similarly to interface for multi-interface case, as a coma-separated list of domains. - Bugfix: new interface-specific context APIs segfaulted with CXI provider specified, due to interface being previously discarded (cxi uses domain names instead). test scenario added to no_pmix_multi_ctx test case as well. - treat not set and 'set to empty' as the same for purposes of info_string generation. - add sanity check to make sure domain count matches interface count Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

…) (#14880) - parse out domain similarly to interface for multi-interface case, as a coma-separated list of domains. - Bugfix: new interface-specific context APIs segfaulted with CXI provider specified, due to interface being previously discarded (cxi uses domain names instead). test scenario added to no_pmix_multi_ctx test case as well. - treat not set and 'set to empty' as the same for purposes of info_string generation. - add sanity check to make sure domain count matches interface count Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

DAOS-16300 cart: Parse domain properly for multi-interface case

7d6649b

- parse out domain similarly to interface for multi-interface case Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

- CXI Fix:

ab31b18

For cxi we use domain names and not interface names. Create context on interface was not accounting for this, causing wrong handling. Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

frostedcmos force-pushed the aaoganez/DAOS-16300 branch from e65cecb to ab31b18 Compare August 2, 2024 18:21

frostedcmos added 4 commits August 2, 2024 18:41

- fix for build to stop complaining

9286701

Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

- style

ffcf3cc

Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

- treat not set, and set to empty as the same for interface & domain

a511e64

for purposes of get_info string generation Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

style

a0acf83

Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

frostedcmos commented Aug 3, 2024

View reviewed changes

- add sanity check to make sure domain count matches interface count

9cdb29a

Required-githooks: true Signed-off-by: Alexander A Oganezov <alexander.a.oganezov@intel.com>

frostedcmos marked this pull request as ready for review August 3, 2024 06:49

frostedcmos requested review from a team as code owners August 3, 2024 06:49

frostedcmos requested review from mchaarawi, soumagne and jgmoore-or August 3, 2024 06:49

daltonbohning reviewed Aug 5, 2024

View reviewed changes

jolivier23 approved these changes Aug 5, 2024

View reviewed changes

soumagne approved these changes Aug 5, 2024

View reviewed changes

frostedcmos merged commit 9221e7f into master Aug 6, 2024
55 checks passed

frostedcmos deleted the aaoganez/DAOS-16300 branch August 6, 2024 08:31

mjmac mentioned this pull request Aug 7, 2024

Merge upstream/release/2.6 into upstream/google/2.6 #14891

Merged

mjmac mentioned this pull request Nov 13, 2024

mjmac/DAOS 16787 google 2.6 #15498

Closed

mjmac mentioned this pull request Mar 18, 2025

dev/mjmac/b403616653 #16115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-16300 cart: Parse domain properly for multi-interface case #14864

DAOS-16300 cart: Parse domain properly for multi-interface case #14864

frostedcmos commented Aug 1, 2024 •

edited

Loading

github-actions bot commented Aug 1, 2024 •

edited

Loading

daosbuild1 commented Aug 1, 2024

daosbuild1 commented Aug 2, 2024

daosbuild1 commented Aug 2, 2024

daosbuild1 commented Aug 2, 2024

frostedcmos Aug 3, 2024 •

edited

Loading

daltonbohning left a comment

mchaarawi commented Aug 5, 2024

soumagne Aug 5, 2024

frostedcmos Aug 6, 2024

soumagne Aug 5, 2024

frostedcmos Aug 6, 2024

soumagne Aug 5, 2024

frostedcmos Aug 6, 2024

DAOS-16300 cart: Parse domain properly for multi-interface case #14864

DAOS-16300 cart: Parse domain properly for multi-interface case #14864

Conversation

frostedcmos commented Aug 1, 2024 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Aug 1, 2024 • edited Loading

daosbuild1 commented Aug 1, 2024

daosbuild1 commented Aug 2, 2024

daosbuild1 commented Aug 2, 2024

daosbuild1 commented Aug 2, 2024

frostedcmos Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

daltonbohning left a comment

Choose a reason for hiding this comment

mchaarawi commented Aug 5, 2024

soumagne Aug 5, 2024

Choose a reason for hiding this comment

frostedcmos Aug 6, 2024

Choose a reason for hiding this comment

soumagne Aug 5, 2024

Choose a reason for hiding this comment

frostedcmos Aug 6, 2024

Choose a reason for hiding this comment

soumagne Aug 5, 2024

Choose a reason for hiding this comment

frostedcmos Aug 6, 2024

Choose a reason for hiding this comment

frostedcmos commented Aug 1, 2024 •

edited

Loading

github-actions bot commented Aug 1, 2024 •

edited

Loading

frostedcmos Aug 3, 2024 •

edited

Loading