DAOS-10028 client: Add Go bindings for libdaos (Pool) #15659

mjmac · 2024-12-22T01:04:55Z

Start the work of converting the raw cgo in the daos
tool into proper Go bindings for libdaos. This patch
covers pool functionality and adds some new infrastructure
common to both pools and containers.

Features: daos_cmd pool
Required-githooks: true
Signed-off-by: Michael MacDonald [email protected]

github-actions · 2024-12-22T01:05:16Z

Ticket title is 'Update golang binding for the DAOS API'
Status is 'In Progress'
Labels: 'SODACODE2022'
https://daosio.atlassian.net/browse/DAOS-10028

daosbuild1 · 2024-12-22T01:14:27Z

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

daosbuild1 · 2024-12-22T01:15:52Z

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-15659/2/execution/node/346/log

daosbuild1 · 2024-12-22T01:16:28Z

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-15659/2/execution/node/337/log

daosbuild1 · 2024-12-22T01:20:46Z

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-15659/2/execution/node/340/log

daosbuild1 · 2024-12-22T01:22:52Z

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

daosbuild1 · 2024-12-22T01:27:06Z

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15659/2/execution/node/508/log

daosbuild1 · 2024-12-24T19:05:15Z

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

daosbuild1 · 2024-12-25T02:29:14Z

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

daosbuild1 · 2024-12-25T09:03:36Z

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

daosbuild1 · 2024-12-28T00:49:33Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15659/7/testReport/

daosbuild1 · 2024-12-29T05:38:06Z

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

daosbuild1 · 2024-12-29T11:28:30Z

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

Start the work of converting the raw cgo in the daos tool into proper Go bindings for libdaos. This patch covers pool functionality and adds some new infrastructure common to both pools and containers. Features: daos_cmd Required-githooks: true Signed-off-by: Michael MacDonald <[email protected]>

mjmac · 2025-01-18T16:41:06Z

Note to reviewers... Yeah, this is big. It's the first of several patches, though, and contains a lot of the infrastructure needed to start the change over to using the client API instead of raw cgo in the daos tool. There is no new functionality here, and my focus will be on maintaining parity with the existing implementation for now. Most of the "new" code is just the existing cgo moved over into the API, along with extensive unit tests. Imagine that, unit tests!

For context, this is the first follow-on PR (still a WIP): #15721

Features: daos_cmd Required-githooks: true Signed-off-by: Michael MacDonald <[email protected]>

daosbuild1 · 2025-01-26T21:34:48Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15659/23/testReport/

mjmac · 2025-01-29T13:02:06Z

If it's helpful, it may be easiest to focus on the changes to the daos command specifically, e.g.

https://github.com/daos-stack/daos/pull/15659/files#diff-a2bbc3ab4e19cd5d6f398e18eb8713a348289cac5bf983930db913e1462cf0a6

Starting with those changes should give reviewers a sense of how the new API works, e.g.

	-poolInfo, err := queryPool(cmd.cPoolHandle, queryMask)
	+poolInfo, err := cmd.pool.Query(cmd.MustLogCtx(), queryMask)

The code under lib/daos/api is mostly not new, just moved from the tool and made unit-testable via a mocking framework.

Signed-off-by: Michael MacDonald <[email protected]>

kjacque

A nice cleanup overall. I do find myself worrying about the rapidly-expanding stubs, though. I'm also curious how people are supposed to mock the API without the stubs, given that they are functions and not interfaces.

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

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

kjacque · 2025-02-01T01:26:07Z

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

+// Long-term, this should be phased out as API users should mock
+// the API instead of relying on the stubs.


IMO we probably need a task to change any existing code at the higher level so people don't imitate it.

Yeah, good point. I see these as temporary scaffolding for the construction of the API. I'll update the comment to make that more explicit.

kjacque · 2025-02-01T01:34:03Z

src/control/lib/daos/api/util.h

+/*static inline uint32_t
+get_rebuild_state(struct daos_rebuild_status *drs)
+{
+	if (drs == NULL)
+		return 0;
+
+	return drs->rs_state;
+}*/


Should this be commented out?

Hmm, I think that's cruft that I missed. I'll remove it if I need to make changes to this PR, or in the next one.

mjmac · 2025-02-01T17:51:26Z

A nice cleanup overall. I do find myself worrying about the rapidly-expanding stubs, though.

I really don't see a better way to make the code testable, but I'm open to suggestions. Personally, I think it's fine for the long term because all of the stub stuff should eventually be hidden behind the API, so API users and external tests don't need to deal with them.

I'm also curious how people are supposed to mock the API without the stubs, given that they are functions and not interfaces.

I was thinking about this the other day. I started playing around with the api.Provider thing when I added the health check stuff. One option would be to expand that to include methods for all of the API functions. At the moment, though, I think that would be a maintenance headache.

I think a better way to do this is to delegate mocking to users of the API. So, e.g. out in the daos tool we can define interfaces for the parts of the API that we want to stub, and just create wrappers to implement those interfaces. This seems like a more maintainable and cleaner solution. I think it will be clearer as more of the tool code is moved into the API, and the code that remains under cmd/daos is mostly presentation layer, which means that the tests and stubs should be pretty simple.

There's already some precedent for this kind of thing in the http.HandlerFunc pattern. Basically you can wrap a function in a struct with a method to implement an interface. We use it in the server: https://github.com/daos-stack/daos/blob/master/src/control/server/server_utils.go#L692

tanabarr

I wasn't able to do as in-depth a review as I would have liked given the sheer volume of changes so whilst I haven't seen any obvious problems it's possible I might have missed something subtle. No blocking issues but I really dislike the "shoehorning" of target-indexes into RankSetFlag just because it's a numerical list.

tanabarr · 2025-02-03T11:50:25Z

src/control/cmd/dmg/pool.go

-	Rank    uint32 `long:"rank" required:"1" description:"Engine rank of the targets to be queried"`
-	Targets string `long:"target-idx" description:"Comma-separated list of target idx(s) to be queried"`
+	Rank    uint32         `long:"rank" required:"1" description:"Engine rank of the target(s) to be queried"`
+	Targets ui.RankSetFlag `long:"target-idx" description:"Comma-separated list of target index(es) to be queried (default: all)"`


I don't really like this, it's confusing. Either rename RankSetFlag or explicitly use ParseNumberList. Shoehorning targets into RankSetFlag just because it's a numerical list isn't pretty IMO

Yeah, I agree that it would probably be better to create a new flag type. The nice thing about RankSetFlag is that it handles individual numbers, comma-separated numbers, or ranges (with or without enclosing brackets). There are also helpers already for converting rank slices to uint32 slices and vice-versa. Probably better to do that cleanup in a standalone PR, though.

src/control/cmd/dmg/pool.go

kjacque · 2025-02-03T21:50:34Z

I really don't see a better way to make the code testable, but I'm open to suggestions. Personally, I think it's fine for the long term because all of the stub stuff should eventually be hidden behind the API, so API users and external tests don't need to deal with them.

I think that's the piece that's missing for me. The stub usage climbs up out of the layer where it should be isolated.

To be honest I'd prefer the stubbing to be done at runtime in the test cases themselves, similar to what we do elsewhere in the code, by having a thin wrapper type that can have interfaces written around it, passing around functions as parameters, setting them as members of structs, etc. Any of those methods seem better to me than having build-time stubs similar to what we are forced to do in the C code.

For the time being I think it's fine, any coverage is better than nothing. But philosophically, I do think tests are more readable if you have most of the information in the test itself, regardless of the method used.

I think a better way to do this is to delegate mocking to users of the API. So, e.g. out in the daos tool we can define interfaces for the parts of the API that we want to stub, and just create wrappers to implement those interfaces. This seems like a more maintainable and cleaner solution.

I'm inclined toward the API package defining the wrapper struct itself, but otherwise I agree. One big interface seems guaranteed to be a maintenance nightmare (and leads to more bloated mock structures that try to be everything for every test, and become hard to understand). Makes sense to have the callers define their own interfaces, and mock those small interfaces in their tests.

All that is to say I think this is fine to land and continue to iterate. We can debate (or not) about approaches, but it's all an improvement over the current untestable tool. Nice work.

kjacque

Any remaining issues can be improved upon in the coming follow-on patches. Nice work.

knard38

Mostly OK for me for what I understand.
If my two comments are relevant, I have no issues that they will be fix in a followup PR.

knard38 · 2025-02-04T15:44:30Z

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

+get_rebuild_state(struct daos_rebuild_status *drs)
+{
+	if (drs == NULL)
+		return 0;


Returning DRS_IN_PROGRESS when drs is NULL sounds strange to me.

Hmm, this was copied from the tool code: https://github.com/daos-stack/daos/blame/master/src/control/cmd/daos/util.h#L119

I agree, though... Seems like it would be better return DRS_NOT_STARTED? I'll fix it in the next PR.

knard38 · 2025-02-05T08:51:12Z

src/control/cmd/daos/pool.go

-}
+		pLabel := C.CString(cmd.pool.Label)
+		defer freeString(pLabel)
+		C.strncpy(&ap.pool_str[0], pLabel, C.DAOS_PROP_LABEL_MAX_LEN)


but could be useful to add '\0' at the end of ap.pool_str as strncpy() is not safe.

Suggested change

C.strncpy(&ap.pool_str[0], pLabel, C.DAOS_PROP_LABEL_MAX_LEN)

C.strncpy(&ap.pool_str[0], pLabel, C.DAOS_PROP_LABEL_MAX_LEN)

ap.pool_str[C.DAOS_PROP_LABEL_MAX_LEN] = '\0'

OK, I'll add that in the next PR.

mjmac force-pushed the mjmac/DAOS-10028-pool branch from 20e923c to 68a15e1 Compare December 22, 2024 01:09

mjmac force-pushed the mjmac/DAOS-10028-pool branch 2 times, most recently from 785f5c7 to d94cc38 Compare December 22, 2024 16:02

mjmac force-pushed the mjmac/DAOS-10028-pool branch 2 times, most recently from 5a76de4 to 628f3ce Compare December 28, 2024 00:03

mjmac force-pushed the mjmac/DAOS-10028-pool branch 2 times, most recently from 510d64b to 90ef6d7 Compare December 28, 2024 18:58

mjmac force-pushed the mjmac/DAOS-10028-pool branch from 90ef6d7 to ad55ebd Compare December 31, 2024 12:32

mjmac force-pushed the mjmac/DAOS-10028-pool branch 7 times, most recently from 6d9b688 to 5f28018 Compare January 13, 2025 23:04

mjmac force-pushed the mjmac/DAOS-10028-pool branch 2 times, most recently from 7a1ccde to aae584f Compare January 17, 2025 21:24

mjmac force-pushed the mjmac/DAOS-10028-pool branch from aae584f to cf8ef8a Compare January 17, 2025 22:59

mjmac force-pushed the mjmac/DAOS-10028-pool branch from cf8ef8a to 9b0d824 Compare January 17, 2025 23:01

mjmac marked this pull request as ready for review January 18, 2025 16:36

mjmac requested review from a team as code owners January 18, 2025 16:36

mjmac self-assigned this Jan 18, 2025

Merge branch 'master' into mjmac/DAOS-10028-pool

9a42d11

Features: daos_cmd Required-githooks: true Signed-off-by: Michael MacDonald <[email protected]>

mjmac requested review from tanabarr, kjacque and knard38 January 29, 2025 12:53

Merge branch 'master' into mjmac/DAOS-10028-pool

5565afe

Signed-off-by: Michael MacDonald <[email protected]>

kjacque reviewed Feb 1, 2025

View reviewed changes

tanabarr approved these changes Feb 3, 2025

View reviewed changes

kjacque approved these changes Feb 3, 2025

View reviewed changes

knard38 approved these changes Feb 5, 2025

View reviewed changes

mjmac merged commit 7d02111 into master Feb 7, 2025
57 checks passed

mjmac deleted the mjmac/DAOS-10028-pool branch February 7, 2025 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-10028 client: Add Go bindings for libdaos (Pool) #15659

DAOS-10028 client: Add Go bindings for libdaos (Pool) #15659

mjmac commented Dec 22, 2024 •

edited

Loading

github-actions bot commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 24, 2024

daosbuild1 commented Dec 25, 2024

daosbuild1 commented Dec 25, 2024

daosbuild1 commented Dec 28, 2024

daosbuild1 commented Dec 29, 2024

daosbuild1 commented Dec 29, 2024

mjmac commented Jan 18, 2025 •

edited

Loading

daosbuild1 commented Jan 26, 2025

mjmac commented Jan 29, 2025

kjacque left a comment

kjacque Feb 1, 2025

mjmac Feb 1, 2025

kjacque Feb 1, 2025

mjmac Feb 1, 2025

mjmac commented Feb 1, 2025 •

edited

Loading

tanabarr left a comment •

edited

Loading

tanabarr Feb 3, 2025

mjmac Feb 3, 2025

kjacque commented Feb 3, 2025

kjacque left a comment

knard38 left a comment

knard38 Feb 4, 2025

mjmac Feb 5, 2025

knard38 Feb 5, 2025 •

edited

Loading

mjmac Feb 5, 2025

		// Long-term, this should be phased out as API users should mock
		// the API instead of relying on the stubs.

	C.strncpy(&ap.pool_str[0], pLabel, C.DAOS_PROP_LABEL_MAX_LEN)
	C.strncpy(&ap.pool_str[0], pLabel, C.DAOS_PROP_LABEL_MAX_LEN)
	ap.pool_str[C.DAOS_PROP_LABEL_MAX_LEN] = '\0'

DAOS-10028 client: Add Go bindings for libdaos (Pool) #15659

DAOS-10028 client: Add Go bindings for libdaos (Pool) #15659

Conversation

mjmac commented Dec 22, 2024 • edited Loading

github-actions bot commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 22, 2024

daosbuild1 commented Dec 24, 2024

daosbuild1 commented Dec 25, 2024

daosbuild1 commented Dec 25, 2024

daosbuild1 commented Dec 28, 2024

daosbuild1 commented Dec 29, 2024

daosbuild1 commented Dec 29, 2024

mjmac commented Jan 18, 2025 • edited Loading

daosbuild1 commented Jan 26, 2025

mjmac commented Jan 29, 2025

kjacque 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

mjmac commented Feb 1, 2025 • edited Loading

tanabarr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjacque commented Feb 3, 2025

kjacque left a comment

Choose a reason for hiding this comment

knard38 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard38 Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjmac commented Dec 22, 2024 •

edited

Loading

mjmac commented Jan 18, 2025 •

edited

Loading

mjmac commented Feb 1, 2025 •

edited

Loading

tanabarr left a comment •

edited

Loading

knard38 Feb 5, 2025 •

edited

Loading