DAOS-16304 tools: Add daos health net-test command #14980

mjmac · 2024-08-21T21:15:06Z

Wrap self_test to provide a simplified network test
to detect obvious client/server connectivity and
performance problems.

Features: control daos_cmd
Required-githooks: true
Change-Id: I607596acca28ee2524ddeb973bf3dfe9ee960c48
Signed-off-by: Michael MacDonald mjmac@google.com

Allow these types to be used without importing all of the Control API. Required-githooks: true Change-Id: I6858cd2bd4dadfa745ff6dce4cf41cece3b8f5f0 Signed-off-by: Michael MacDonald <mjmac@google.com>

Wrap self_test to provide a simplified network test to detect obvious client/server connectivity and performance problems. Features: control daos_cmd Required-githooks: true Change-Id: I607596acca28ee2524ddeb973bf3dfe9ee960c48 Signed-off-by: Michael MacDonald <mjmac@google.com>

github-actions · 2024-08-21T21:15:26Z

Ticket title is 'Add daos health net-test command'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-16304

daosbuild1 · 2024-08-22T07:09:45Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14980/1/execution/node/1416/log

mjmac · 2024-08-22T12:36:33Z

Hardware-large failure is DAOS-16393.

kjacque

Looks good! Nice work! My only comments are on the unit testing, but I wouldn't block on it.

src/control/common/test/utils.go

kjacque · 2024-08-24T00:31:14Z

src/control/lib/daos/api/libdaos_stubs.go

+	daos_mgmt_get_sys_info_RC         C.int            = 0
+)
+
+func daos_mgmt_get_sys_info(group *C.char, sys_info_out **C.struct_daos_sys_info) C.int {


This is an interesting approach for the Go tests, very similar to the link-time mocking we do in the C tests. I wonder, if instead of having functions here, and using globals to inject the values, if it might make sense to use a struct with methods to represent the API, so each instance gets its own mock variables. The same build flag approach could still be used to decide which one is defined for tests, or it could be defined as an interface and injected somehow to allow for mocking without need for build flags.

I am definitely open to ideas on how to make these better... The main thing that I went around and around on over the past year was how to make most of the logic around the bindings testable without contorting the production code too much in service of the tests.

In another patch that adds a lot more to the bindings, I experimented with an interface-based approach and it wound up feeling pretty gross, with a giant interface definition that could be implemented by either the real thing or a mock. Even if we could somehow break it down into smaller interfaces that were specific to each API call, it seems like we're just shifting the maintenance mess around a bit.

I guess the thing that bothers me more than the approach itself is that the layers are a bit jumbled... There's Provider, api, and then a wrapper layer directly above the actual C functions to allow the stubbing. Sometimes Provider calls api, and sometimes it calls the wrapper functions directly. Straightening that relationship out would make me feel a bit better about it. As I was playing around with it, I kept wanting to make api the simple wrapper layer, and do everything else (including the extra init/fini bookkeeping) in the Provider.

There's Provider, api, and then a wrapper layer directly above the actual C functions to allow the stubbing.

Yeah, those are all artifacts from a couple of different approaches to implementing the bindings. My inclination at this point is to continue down the path that I've started with this PR, i.e. the stubs, and gradually eliminate the other stuff. As I've said, though, I'm open to other ideas about how to do this. I'd like to settle on something we can live with for the long term though, because I don't want to get into mass rewrites later.

I'm fine with landing as-is and making incremental improvements to this implementation. As we discussed elsewhere, it would be great to have a more explicit form of dependency injection, but we don't need to have it 100% perfect on the first go-round.

src/control/lib/daos/api/libdaos_stubs.go

src/control/lib/daos/api/libdaos_selftest_stubs.go

Change-Id: I4ecadf2009522526d7793ebc9f42b07617314dc9

* Make stubs panic instead of handling errors Features: daos_cmd Required-githooks: true Change-Id: I215b040bc9f473f57d6e315f18b1f7529881eda2 Signed-off-by: Michael MacDonald <mjmac@google.com>

mjmac · 2024-08-28T14:56:29Z

@tanabarr, @knard-intel: Nudge... Would really like to get this landed so I can backport it to our release branch, thanks.

pavelgritsai

This is a huge PR, Mike :) breaking it up would allow for simpler review - I only got to a part of the changes - but still wanted to share my thoughts.

pavelgritsai · 2024-08-27T19:57:56Z

src/control/cmd/daos/health.go

 	"github.com/daos-stack/daos/src/control/logging"
 )

 type healthCmds struct {
-	Check healthCheckCmd `command:"check" description:"Perform DAOS system health checks"`
+	Check   healthCheckCmd `command:"check" description:"Perform DAOS system health checks"`
+	NetTest netTestCmd     `command:"net-test" description:"Perform non-destructive DAOS networking tests"`


Is "non-destructive" necessary here? we don't qualify the above health check command at all

I wanted to convey to the user that the test won't overwrite any data, even though it's doing writes. Simpler to just write "non-destructive" to allay any concerns, otherwise the concerned user has to go read documentation to figure out whether or not they should be worried.

pavelgritsai · 2024-08-27T20:00:27Z

src/control/cmd/daos/health.go

@@ -68,7 +72,7 @@ func (cmd *healthCheckCmd) Execute([]string) error {
 		return err
 	}

-	sysInfo, err := cmd.apiProvider.GetSystemInfo()
+	sysInfo, err := cmd.apiProvider.GetSystemInfo(cmd.MustLogCtx())


Curious why this change? seems a bit cumbersome to pass a thing from the same object (cmd) into a submethos onto itself. Should we consider adding a method to cmd instead? cmd.GetSysInfor() to take care of all other call sites?

As discussed in another conversation with @kjacque, I'm experimenting with a couple of different approaches to implementing the bindings, hence the provider thing here. This change specifically was just to establish a precedent that the API calls should have a context.

As for why we're getting a context from the command object and giving it to the API call, that's a whole other conversation that boils down to patterns encouraged by the use of go-flags, which is what we settled on years ago as the CLI framework for these commands.

pavelgritsai · 2024-08-27T20:03:55Z

src/control/cmd/daos/main.go

@@ -182,7 +183,7 @@ or query/manage an object inside a container.`
 	// Initialize the daos debug system first so that


Is this comment still accurate with change below?

Yeah, because initializing logging also initializes debug, and there's no way to initialize debug without initializing logging.

pavelgritsai · 2024-08-28T21:55:17Z

src/control/cmd/daos/pretty/selftest.go

+	summary = append(summary, txtfmt.TableRow{
+		"Average Latency": printLatencyVal(float64(result.MasterLatency.Average()), ms),
+	})
+	if l, found := masterBuckets[95]; found {


95 is a magic number - consider defining a variable or const with a descriptive name of what it represents. same for 99 below

pavelgritsai · 2024-08-28T21:56:40Z

src/control/cmd/daos/pretty/selftest.go

+	fmt.Fprintln(out, ef.Format(summary))
+
+	if !verbose {
+		return nil


consider alternatively moving the code below into a separate method? addVerboseDetails() or something like this - otherwise this is a very long method.

pavelgritsai · 2024-08-28T21:57:11Z

src/control/cmd/daos/pretty/selftest.go

+			"Max":    printLatencyVal(float64(el.Max), dispUnit),
+			"Failed": fmt.Sprintf("%.01f%%", float64(el.FailCount)/float64(el.TotalRPCs)*100),
+		}
+		if verbose {


this is always true, right?

mjmac · 2024-08-29T21:23:31Z

This is a huge PR, Mike :) breaking it up would allow for simpler review - I only got to a part of the changes - but still wanted to share my thoughts.

Thanks for taking a look, Pavel! Yes, it is large. Unfortunately, breaking these kinds of feature landings into smaller PRs is kind of a nightmare with the way github works. You get into a mess of branch management (each PR is its own branch), and the Intel convention is to avoid rebasing once reviews have started, so then you have to merge from lower branches into higher branches, etc.

Anyhow, I did already break out #14950 into its own PR. ;)

knard-intel

Mostly looks good to me for what I understand.
Very large PR not so easy to understand.

src/control/cmd/daos/health.go

knard-intel

LGTM for what I understand.

* DAOS-16484 test: Exclude local host in default interface selection (#15049) When including the local host in the default interface selection a difference in ib0 speeds will cause the logic to select eth0 and then the tcp provider. Signed-off-by: Phil Henderson <phillip.henderson@intel.com> * DAOS-15800 client: create cart context on specific interface (#14804) Cart has added the ability to select network interface on context creation. The daos_agent also added a numa-fabric map that can be queried at init time. Update the DAOS client to query from the agent a map of numa to network interface on daos_init(), and on EQ creation, select the best interface for the network context based on the numa of the calling thread. Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com> * DAOS-16445 client: Add function to cycle OIDs non-sequentially (#14999) We've noticed that with sequential order, object placement is poor. We get 40% fill for 8GiB files with 25 ranks and 16 targets per rank with EC_2P1G8. With this patch, we get a much better distribution. This patch adds the following: 1. A function for cycling oid.hi incrementing by a large prime 2. For DFS, randomize the starting value 3. Modify DFS to cycle OIDs using the new function. Signed-off-by: Jeff Olivier <jeffolivier@google.com> * DAOS-16251 dtx: Fix dtx_req_send user-after-free (#15035) In dtx_req_send, since the crt_req_send releases the req reference, din may have been freed when dereferenced for the DL_CDEBUG call. Signed-off-by: Li Wei <wei.g.li@intel.com> * DAOS-16304 tools: Add daos health net-test command (#14980) Wrap self_test to provide a simplified network test to detect obvious client/server connectivity and performance problems. Signed-off-by: Michael MacDonald <mjmac@google.com> * DAOS-16272 dfs: fix get_info returning incorrect oclass (#15048) If user creates a container without --file-oclass, the get_info call was returning the default oclass of a directory on daos fs get-attr. Fix that to properly use the enum types for default scenario. Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com> * DAOS-15863 container: fix a race for container cache (#15038) * DAOS-15863 container: fix a race for container cache while destroying a container, cont_child_destroy_one() releases its own refcount before waiting, if another ULT releases its refcount, which is the last one, wakes up the waiting ULT and frees it ds_cont_child straightaway, because no one else has refcount. When the waiting ULT is waken up, it will try to change the already freed ds_cont_child. This patch changes the LRU eviction logic and fixes this race. Signed-off-by: Liang Zhen <liang.zhen@intel.com> Signed-off-by: Jeff Olivier <jeffolivier@google.com> Co-authored-by: Jeff Olivier <jeffolivier@google.com> * DAOS-16471 test: Reduce targets for ioctl_pool_handles.py (#15063) The dfuse/ioctl_pool_handles.py test is overloading the VM so reduce the number of engine targets. Signed-off-by: Phil Henderson <phillip.henderson@intel.com> * DAOS-16483 vos: handle empty DTX when vos_tx_end (#15053) It is possible that the DTX modified nothing when stop currnet backend transaction. Under such case, we may not generate persistent DTX entry. Then need to bypass such case before checking on-disk DTX entry status. The patch makes some clean and removed redundant metrics for committed DTX entries. Enhance vos_dtx_deregister_record() to handle GC case. Signed-off-by: Fan Yong <fan.yong@intel.com> * DAOS-16271 mercury: Add patch to avoid seg fault in key resolve. (#15067) Signed-off-by: Joseph Moore <joseph.moore@intel.com> * DAOS-16484 test: Support mixed speeds when selecting a default interface (#15050) Allow selecting a default interface that is running at a different speed on different hosts. Primarily this is to support selecting the ib0 interface by default when the launch node has a slower ib0 interface than the cluster hosts. Signed-off-by: Phil Henderson <phillip.henderson@intel.com> * DAOS-16446 test: HDF5-VOL test - Set object class and container prope… (#15004) In HDF5, DFS, MPIIO, or POSIX, object class and container properties are defined during the container create. If it’s DFS, object class is also set to the IOR parameter. However, in HDF5-VOL, object class and container properties are defined with the following environment variables of mpirun. HDF5_DAOS_OBJ_CLASS (Object class) HDF5_DAOS_FILE_PROP (Container properties) The infrastructure to set these variables are already there in run_ior_with_pool(). In file_count_test_base.py, pass in the env vars to run_ior_with_pool(env=env) as a dictionary. Object class is the oclass variable. Container properties can be obtained from container -> properties field in the test yaml. This fix is discussed in PR #14964. Signed-off-by: Makito Kano <makito.kano@intel.com> * DAOS-16447 test: set D_IL_REPORT per test (#15012) set D_IL_REPORT per test instead of setting defaults values in utilities. This allows running without it set. Signed-off-by: Dalton Bohning <dalton.bohning@intel.com> * DAOS-16450 test: auto run dfs tests when dfs is modified (#15017) Automatically include dfs tests when dfs files are modified in PRs. Signed-off-by: Dalton Bohning <dalton.bohning@intel.com> * DAOS-16510 cq: update pylint to 3.2.7 (#15072) update pylint to 3.2.7 Signed-off-by: Dalton Bohning <dalton.bohning@intel.com> * DAOS-16509 test: replace IorTestBase.execute_cmd with run_remote (#15070) replace usage of IorTestBase.execute_cmd with run_remote Signed-off-by: Dalton Bohning <dalton.bohning@intel.com> * DAOS-16458 object: fix invalid DRAM access in obj_bulk_transfer (#15026) For EC object update via CPD RPC, when calculate the bitmap to skip some iods for current EC data shard, we may input NULL for "*skips" parameter. It may cause the old logic in obj_get_iods_offs_by_oid() to generate some undefined DRAM for "skips" bitmap. Such bitmap may be over-written by others, as to subsequent obj_bulk_transfer() may be misguided. The patch also fixes a bug inside obj_bulk_transfer() that cast any input RPC as UPDATE/FETCH by force. Signed-off-by: Fan Yong <fan.yong@intel.com> * DAOS-16486 object: return proper error on stale pool map (#15064) Client with stale pool map may try to send RPC to a DOWN target, if the target was brought DOWN due to faulty NVMe device, the ds_pool_child could have been stopped on the NVMe faulty reaction, We'd ensure proper error code is returned for such case. Signed-off-by: Niu Yawei <yawei.niu@intel.com> * DAOS-16514 vos: fix coverity issue (#15083) Fix coverity 2555843 explict null dereferenced. Signed-off-by: Niu Yawei <yawei.niu@intel.com> * DAOS-16467 rebuild: add DAOS_POOL_RF ENV for massive failure case (#15037) * DAOS-16467 rebuild: add DAOS_PW_RF ENV for massive failure case Allow user to set DAOS_PW_RF as pw_rf (pool wise RF). If SWIM detected engine failure is going to break pw_rf, don't change pool map, also don't trigger rebuild. With critical log message to ask administrator to bring back those engines in top priority (just "system start --ranks=xxx", need not to reintegrate those engines). a few functions renamed to avoid confuse - pool_map_find_nodes() -> pool_map_find_ranks() pool_map_find_node_by_rank() -> pool_map_find_dom_by_rank() pool_map_node_nr() -> pool_map_rank_nr() Signed-off-by: Xuezhao Liu <xuezhao.liu@intel.com> * DAOS-16508 csum: retry a few times on checksum mismatch on update (#15069) Unlike fetch, we return DER_CSUM on update (turned into EIO by dfs) without any retry. We should retry a few times in case it is a transient error. The patch also prints more information about the actual checksum mismatch. Signed-off-by: Johann Lombardi <johann.lombardi@gmail.com> * DAOS-10877 vos: gang allocation for huge SV (#14790) To avoid allocation failure on a fragmented system, huge SV allocation will be split into multiple smaller allocations, each allocation size is capped to 8MB (the DMA chunk size, that could avoid huge DMA buffer allocation). The address of such scattered SV payload is represented by 'gang address'. Removed io_allocbuf_failure() vos unit test, it's not applicable in gang SV mode now. Signed-off-by: Niu Yawei <yawei.niu@intel.com> * DAOS-16304 tools: Adjust default RPC size for net-test (#15091) The previous default of 1MiB isn't helpful at large scales. Use a default of 1KiB to get faster results and a better balance between raw latency and bandwidth. Also include calculated rpc throughput and bandwidth in JSON output. Signed-off-by: Michael MacDonald <mjmac@google.com> * SRE-2408 ci: Increase timeout (to 15 minutes) for system restore (#14926) Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com> * DAOS-16251 object: Fix obj_ec_singv_split overflow (#15045) It has been seen that obj_ec_singv_split may read beyond the end of sgl->sg_iovs[0].iov_buf: iod_size=8569 c_bytes=4288 id_shard=0 tgt_off=1 iov_len=8569 iov_buf_len=8569 The memmove read 4288 bytes from offset 4288, whereas the buffer only had 8569 - 4288 = 4281 bytes from offset 4288. This patch fixes the problem by adding the min(...) expression. Signed-off-by: Li Wei <wei.g.li@intel.com> --------- Signed-off-by: Phil Henderson <phillip.henderson@intel.com> Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com> Signed-off-by: Jeff Olivier <jeffolivier@google.com> Signed-off-by: Li Wei <wei.g.li@intel.com> Signed-off-by: Michael MacDonald <mjmac@google.com> Signed-off-by: Liang Zhen <liang.zhen@intel.com> Signed-off-by: Fan Yong <fan.yong@intel.com> Signed-off-by: Joseph Moore <joseph.moore@intel.com> Signed-off-by: Makito Kano <makito.kano@intel.com> Signed-off-by: Dalton Bohning <dalton.bohning@intel.com> Signed-off-by: Niu Yawei <yawei.niu@intel.com> Signed-off-by: Xuezhao Liu <xuezhao.liu@intel.com> Signed-off-by: Johann Lombardi <johann.lombardi@gmail.com> Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com> Co-authored-by: Phil Henderson <phillip.henderson@intel.com> Co-authored-by: Jeff Olivier <jeffolivier@google.com> Co-authored-by: Li Wei <wei.g.li@intel.com> Co-authored-by: Michael MacDonald <mjmac@google.com> Co-authored-by: Liang Zhen <liang.zhen@intel.com> Co-authored-by: Nasf-Fan <fan.yong@intel.com> Co-authored-by: Joseph Moore <26410038+jgmoore-or@users.noreply.github.com> Co-authored-by: Makito Kano <makito.kano@intel.com> Co-authored-by: Dalton Bohning <dalton.bohning@intel.com> Co-authored-by: Niu Yawei <yawei.niu@intel.com> Co-authored-by: Liu Xuezhao <xuezhao.liu@intel.com> Co-authored-by: Johann Lombardi <johann.lombardi@gmail.com> Co-authored-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>

mjmac added 2 commits August 21, 2024 20:53

DAOS-16304 control: Move metric types into lib/daos

687a402

Allow these types to be used without importing all of the Control API. Required-githooks: true Change-Id: I6858cd2bd4dadfa745ff6dce4cf41cece3b8f5f0 Signed-off-by: Michael MacDonald <mjmac@google.com>

mjmac marked this pull request as ready for review August 22, 2024 21:01

mjmac requested review from a team as code owners August 22, 2024 21:01

kjacque previously approved these changes Aug 24, 2024

View reviewed changes

mjmac requested review from tanabarr and knard-intel August 25, 2024 15:24

mjmac added 2 commits August 25, 2024 15:44

Merge branch 'master' into mjmac/DAOS-16304

467a25a

Change-Id: I4ecadf2009522526d7793ebc9f42b07617314dc9

Address review feedback

b8ccad8

* Make stubs panic instead of handling errors Features: daos_cmd Required-githooks: true Change-Id: I215b040bc9f473f57d6e315f18b1f7529881eda2 Signed-off-by: Michael MacDonald <mjmac@google.com>

mjmac dismissed kjacque’s stale review via b8ccad8 August 25, 2024 15:53

mjmac requested a review from kjacque August 25, 2024 19:40

kjacque approved these changes Aug 27, 2024

View reviewed changes

pavelgritsai reviewed Aug 29, 2024

View reviewed changes

knard-intel reviewed Aug 30, 2024

View reviewed changes

src/control/cmd/daos/health.go Show resolved Hide resolved

knard-intel approved these changes Sep 2, 2024

View reviewed changes

mjmac merged commit e6be2a6 into master Sep 2, 2024
55 checks passed

mjmac deleted the mjmac/DAOS-16304 branch September 2, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-16304 tools: Add daos health net-test command #14980

DAOS-16304 tools: Add daos health net-test command #14980

mjmac commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

daosbuild1 commented Aug 22, 2024

mjmac commented Aug 22, 2024

kjacque left a comment

kjacque Aug 24, 2024 •

edited

Loading

mjmac Aug 25, 2024

kjacque Aug 26, 2024

mjmac Aug 27, 2024

kjacque Aug 27, 2024

mjmac commented Aug 28, 2024

pavelgritsai left a comment

pavelgritsai Aug 27, 2024

mjmac Aug 29, 2024

pavelgritsai Aug 27, 2024

mjmac Aug 29, 2024

pavelgritsai Aug 27, 2024

mjmac Aug 29, 2024

pavelgritsai Aug 28, 2024

pavelgritsai Aug 28, 2024

pavelgritsai Aug 28, 2024

mjmac commented Aug 29, 2024

knard-intel left a comment

knard-intel left a comment

		@@ -182,7 +183,7 @@ or query/manage an object inside a container.`
		// Initialize the daos debug system first so that

DAOS-16304 tools: Add daos health net-test command #14980

DAOS-16304 tools: Add daos health net-test command #14980

Conversation

mjmac commented Aug 21, 2024 • edited Loading

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

daosbuild1 commented Aug 22, 2024

mjmac commented Aug 22, 2024

kjacque left a comment

Choose a reason for hiding this comment

kjacque Aug 24, 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

mjmac commented Aug 28, 2024

pavelgritsai left a 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

Choose a reason for hiding this comment

mjmac commented Aug 29, 2024

knard-intel left a comment

Choose a reason for hiding this comment

knard-intel left a comment

Choose a reason for hiding this comment

mjmac commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

kjacque Aug 24, 2024 •

edited

Loading