DAOS-15655 control: fix support for non default system name #14170

mchaarawi · 2024-04-16T18:20:18Z

Features: control

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:

Features: control Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

github-actions · 2024-04-16T18:58:35Z

Ticket title is 'Using a different system name (other than daos_server) does not work.'
Status is 'Open'
Labels: 'triaged'
https://daosio.atlassian.net/browse/DAOS-15655

daosbuild1 · 2024-04-17T00:44:31Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14170/1/execution/node/1198/log

daosbuild1 · 2024-04-18T15:03:54Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14170/2/execution/node/701/log

daosbuild1 · 2024-04-18T17:00:35Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14170/2/execution/node/747/log

mchaarawi · 2024-04-18T22:15:24Z

i would still like a review from a @daos-stack/metadata-owners before requesting this to land. @liw or @kccain please :-)

liw · 2024-04-19T08:37:16Z

src/include/daos/pool.h

@@ -78,6 +78,9 @@
 */
 #define DAOS_POOL_GLOBAL_VERSION 3

+/** Default system name used in srv_pool.c */
+extern char daos_sysname[];
+


Since daos_sysname is defined in engine, its declaration should be in daos_engine.h instead, no?

im not sure that it matters? we only use it in the the server client pool operations, but i will move it, which would require though including daos_engine.h

actually pool/srv_cli.c already includes that, so that actually works better. thanks ill update.

liw · 2024-04-19T08:39:05Z

src/engine/init.c

@@ -46,7 +44,7 @@ static char		modules[MAX_MODULE_OPTIONS + 1];
 static unsigned int	nr_threads;

 /** DAOS system name (corresponds to crt group ID) */
-static char	       *daos_sysname = DAOS_DEFAULT_SYS_NAME;
+char                    daos_sysname[DAOS_SYS_NAME_MAX + 1] = DAOS_DEFAULT_SYS_NAME;


[Question] Why is this change to array necessary?

to allow for a sys name that might be larger than DAOS_DEFAULT_SYS_NAME and could be up to DAOS_SYS_NAME_MAX

daos_sysname was a pointer, it could be assigned the address of a string longer than DAOS_DEFAULT_SYS_NAME, is that not so?

yea, i'm not sure then why i made that change :-)
I'll revert it back on my follow on PR to the attach info optimization.

liw · 2024-04-19T08:41:31Z

src/engine/init.c

@@ -1032,7 +1030,7 @@ parse(int argc, char **argv)
 				rc = -DER_INVAL;
 				break;
 			}
-			daos_sysname = optarg;
+			strcpy(daos_sysname, optarg);


[Nit?] For this to be safe, we have to assume that optarg won't be too long. Although that may indeed be true, it's not obvious---we might want to check the length (either by strlen or strncpy).

yes, but if you look at the check right before this strcpy, we already use strnlen for the check and error if it's larger. so this strcpy is safe.

I will update it though to use memcpy to avoid coverity warnings in case it's not smart enough

Required-githooks: true

Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

kjacque

I found a few other areas where changes are needed to fully support a non-default system name:

System property always displays the default instead of the configured name: https://github.com/daos-stack/daos/blob/master/src/control/lib/daos/system_prop.go#L502
If the request doesn't supply a system name, the agent uses the built-in default system name to cache system GetAttachInfo responses. This isn't very important -- right now we don't support multi-system access for a single agent. But it should be a fairly simple fix: https://github.com/daos-stack/daos/blob/master/src/control/cmd/daos_agent/infocache.go#L348

mchaarawi · 2024-04-19T20:13:36Z

I found a few other areas where changes are needed to fully support a non-default system name:

System property always displays the default instead of the configured name: https://github.com/daos-stack/daos/blob/master/src/control/lib/daos/system_prop.go#L502

I tried:

dmg system get-prop -o=/home/mschaara/config/daos.yml
Name                                             Value   
----                                             -----   
Checksum scrubbing threshold (pool_scrub_thresh) 0       
DAOS system name (daos_system)                   my_srv  
DAOS version (daos_version)                      2.5.101 
Checksum scrubbing mode (pool_scrub_mode)        off

so it might be taken into account already?

If the request doesn't supply a system name, the agent uses the built-in default system name to cache system GetAttachInfo responses. This isn't very important -- right now we don't support multi-system access for a single agent. But it should be a fairly simple fix: https://github.com/daos-stack/daos/blob/master/src/control/cmd/daos_agent/infocache.go#L348

we gave up on the idea of 1 agent supporting multiple systems i believe.. but in this case attach info would return the system name anyway so it should not hit that part of the code that you indicated?

kjacque · 2024-04-19T21:23:20Z

I tried:

dmg system get-prop -o=/home/mschaara/config/daos.yml
Name                                             Value   
----                                             -----   
Checksum scrubbing threshold (pool_scrub_thresh) 0       
DAOS system name (daos_system)                   my_srv  
DAOS version (daos_version)                      2.5.101 
Checksum scrubbing mode (pool_scrub_mode)        off

so it might be taken into account already?

Oops, yes, I found the place where this is updated. Disregard.

we gave up on the idea of 1 agent supporting multiple systems i believe.. but in this case attach info would return the system name anyway so it should not hit that part of the code that you indicated?

The agent case is looking for a locally-stored value based on the system name requested by the client. If no system name was requested, it tries the built-in default. But yeah, I agree, the single-agent-multi-system case isn't on the table for the foreseeable future, so this isn't important. I'm OK approving as-is.

daosbuild1 · 2024-04-21T03:13:05Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14170/3/execution/node/1405/log

daosbuild1 · 2024-04-21T14:15:17Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14170/3/execution/node/1503/log

daosbuild1 · 2024-04-22T09:08:47Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14170/4/execution/node/1492/log

liw · 2024-04-22T11:38:33Z

src/engine/init.c

@@ -46,7 +44,7 @@ static char		modules[MAX_MODULE_OPTIONS + 1];
 static unsigned int	nr_threads;

 /** DAOS system name (corresponds to crt group ID) */
-static char	       *daos_sysname = DAOS_DEFAULT_SYS_NAME;
+char                    daos_sysname[DAOS_SYS_NAME_MAX + 1] = DAOS_DEFAULT_SYS_NAME;


daos_sysname was a pointer, it could be assigned the address of a string longer than DAOS_DEFAULT_SYS_NAME, is that not so?

mchaarawi · 2024-04-22T16:12:22Z

https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-14170/4/pipeline/267/
was the last test result before accidently restarting.
the failures there are known: DAOS-15431. DAOS-15662

Signed-off-by: Mohamad Chaarawi <[email protected]>

…stem (#14318) improve the daos_init() and pool_connect() process to reuse the attach info instead of doing agent drpc upcalls multiple times. Also includes: DAOS-15655 control: fix support for non default system name (#14170) Signed-off-by: Mohamad Chaarawi <[email protected]>

DAOS-15655 control: fix support for non default system name

48d54f7

Features: control Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

mchaarawi marked this pull request as ready for review April 18, 2024 15:11

mchaarawi requested review from a team as code owners April 18, 2024 15:11

mchaarawi requested review from liw, tanabarr and kjacque April 18, 2024 15:11

mjmac previously approved these changes Apr 18, 2024

View reviewed changes

tanabarr previously approved these changes Apr 18, 2024

View reviewed changes

mchaarawi requested a review from kccain April 18, 2024 15:48

mchaarawi requested a review from johannlombardi April 18, 2024 21:48

liw reviewed Apr 19, 2024

View reviewed changes

mchaarawi added 2 commits April 19, 2024 16:54

Merge branch 'master' into mschaara/15655

db0897e

Required-githooks: true

address review feedback

785702a

Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

mchaarawi dismissed stale reviews from tanabarr and mjmac via 785702a April 19, 2024 17:05

kjacque reviewed Apr 19, 2024

View reviewed changes

mchaarawi requested review from liw, tanabarr, mjmac and kjacque April 19, 2024 20:40

kjacque approved these changes Apr 19, 2024

View reviewed changes

Merge branch 'master' into mschaara/15655

63df4b2

johannlombardi approved these changes Apr 22, 2024

View reviewed changes

liw approved these changes Apr 22, 2024

View reviewed changes

tanabarr approved these changes Apr 22, 2024

View reviewed changes

mchaarawi requested a review from a team April 22, 2024 16:06

mchaarawi merged commit 5bf0d4f into master Apr 22, 2024
49 of 51 checks passed

mchaarawi deleted the mschaara/15655 branch April 22, 2024 16:12

mjmac pushed a commit that referenced this pull request May 6, 2024

DAOS-15655 control: fix support for non default system name (#14170)

a518c0e

Signed-off-by: Mohamad Chaarawi <[email protected]>

mjmac mentioned this pull request May 6, 2024

DAOS-13151 client: cache and reuse the attach info for the default system #14318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-15655 control: fix support for non default system name #14170

DAOS-15655 control: fix support for non default system name #14170

mchaarawi commented Apr 16, 2024

github-actions bot commented Apr 16, 2024 •

edited

Loading

daosbuild1 commented Apr 17, 2024

daosbuild1 commented Apr 18, 2024

daosbuild1 commented Apr 18, 2024

mchaarawi commented Apr 18, 2024 •

edited

Loading

liw Apr 19, 2024

mchaarawi Apr 19, 2024

mchaarawi Apr 19, 2024

liw Apr 19, 2024

mchaarawi Apr 19, 2024

liw Apr 22, 2024

mchaarawi Apr 22, 2024 •

edited

Loading

liw Apr 19, 2024

mchaarawi Apr 19, 2024

mchaarawi Apr 19, 2024 •

edited

Loading

kjacque left a comment

mchaarawi commented Apr 19, 2024

kjacque commented Apr 19, 2024

daosbuild1 commented Apr 21, 2024

daosbuild1 commented Apr 21, 2024

daosbuild1 commented Apr 22, 2024

liw Apr 22, 2024

mchaarawi commented Apr 22, 2024

DAOS-15655 control: fix support for non default system name #14170

DAOS-15655 control: fix support for non default system name #14170

Conversation

mchaarawi commented Apr 16, 2024

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Apr 16, 2024 • edited Loading

daosbuild1 commented Apr 17, 2024

daosbuild1 commented Apr 18, 2024

daosbuild1 commented Apr 18, 2024

mchaarawi commented Apr 18, 2024 • 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

mchaarawi Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchaarawi Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

kjacque left a comment

Choose a reason for hiding this comment

mchaarawi commented Apr 19, 2024

kjacque commented Apr 19, 2024

daosbuild1 commented Apr 21, 2024

daosbuild1 commented Apr 21, 2024

daosbuild1 commented Apr 22, 2024

Choose a reason for hiding this comment

mchaarawi commented Apr 22, 2024

github-actions bot commented Apr 16, 2024 •

edited

Loading

mchaarawi commented Apr 18, 2024 •

edited

Loading

mchaarawi Apr 22, 2024 •

edited

Loading

mchaarawi Apr 19, 2024 •

edited

Loading