DAOS-10028 control: Move core types into daos package #14174

mjmac · 2024-04-16T21:33:26Z

The lib/daos package should be the main repository for
types (e.g. PoolInfo) used throughout the codebase. It
should be possible to import these types without pulling
in extra dependencies (e.g. gRPC, protobufs, etc).

Also reverts an inadvertant change to the system
database's serialization that resulted in 2.4/2.6
incompatibilities.

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

github-actions · 2024-04-16T21:34:12Z

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

The lib/daos package should be the main repository for types (e.g. PoolInfo) used throughout the codebase. It should be possible to import these types without pulling in extra dependencies (e.g. gRPC, protobufs, etc). Also reverts an inadvertant change to the system database's serialization that resulted in 2.4/2.6 incompatibilities. Features: pool Required-githooks: true Change-Id: I152742e5d10e689da391fc941f7a9d1afa4e3874 Signed-off-by: Michael MacDonald <[email protected]>

daltonbohning

ftest LGTM

daosbuild1 · 2024-04-16T23:17:14Z

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14174/2/execution/node/1143/log

kjacque

Nothing I'd block on. Nice work!

kjacque · 2024-04-16T22:34:01Z

src/control/lib/daos/pool.go

+	if pss < PoolServiceStateCreating || pss > PoolServiceStateUnknown {
+		return fmt.Sprintf("Invalid pool service state %d", pss)
+	}


I would probably just call this unknown

Yeah, I thought about that... Was thinking that this would make it a bit more obvious if someone added a new state and forgot to update the stringer, though.

As I was looking at these stringers and marshallers, I realized that we were in an in-between state, where some of the types were backed by the protobuf values, and some were just using native values. I started to go down the road of getting rid of all of the protobuf stuff, with the thought that the lib/daos package shouldn't depend on the protobufs. I backed away from that position though, because I realized that I was adding maintenance burden to keeping everything in sync.

I still think there's value in revisiting whether or not we should have protobuf dependencies in lib/daos, but I don't think this PR is the place to do that.

src/control/system/raft/raft_recovery_test.go

src/control/lib/daos/pool.go

tanabarr

Thanks for taking the time to rework this!

tanabarr · 2024-04-17T17:31:12Z

src/control/cmd/dmg/pretty/pool.go

 		"SvcReps":        svcReps,
-		"Disabled":       fmt.Sprintf("%d/%d", pool.TargetsDisabled, pool.TargetsTotal),
+		"Disabled":       fmt.Sprintf("%d/%d", pool.DisabledTargets, pool.TotalTargets),


why the change from to the other way round? thought we tended to stay with the former?

Had to choose one or the other when doing the merge, and the PoolInfo names had been around longest.

src/control/lib/daos/pool.go

tanabarr · 2024-04-17T17:41:31Z

src/control/lib/daos/pool.go

+	if pss < PoolServiceStateCreating || pss > PoolServiceStateUnknown {
+		return fmt.Sprintf("Invalid pool service state %d", pss)
+	}
+	return [...]string{


just out of curiosity, why use an array literal rather than just a slice? is there a performance/storage benefit?

I backed this out in the latest commit in favor of using the protobuf names as in the other stringer methods. As I indicated in my other comment to Kris, I had initially taken the position that we shouldn't have any protobuf dependencies in the lib/daos package, but I'm not so sure now. It is certainly less of a maintenance headache to accept the dependency.

tanabarr · 2024-04-17T17:42:46Z

src/control/lib/daos/pool.go

+	if !ok {
+		// Try converting the string to an int32, to handle the
+		// conversion from protobuf message using convert.Types().
+		si, err := strconv.ParseInt(stateStr, 0, 32)


nice, approve of using this method that we've used elsewhere, handles the multiple cases well

Move PoolQueryTarget and related types into lib/daos. Features: pool Required-githooks: true Change-Id: Ibb90c44d681402b699e023cc78adb4b7f21b0910 Signed-off-by: Michael MacDonald <[email protected]>

daosbuild1 · 2024-04-17T20:03:40Z

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14174/3/execution/node/1152/log

tanabarr · 2024-04-18T09:37:05Z

src/control/lib/daos/pool.go

 }

 func (pss PoolServiceState) MarshalJSON() ([]byte, error) {
 	return []byte(`"` + pss.String() + `"`), nil
 }

+func unmarshalStrVal(inStr string, name2Vals map[string]int32, val2Names map[int32]string) (int32, error) {


Given this is essentially the same approach we are using with NvmeDevState and LedState do we want to formalise it somehow with a helper so we can update any improvements to be approach in one place?

Probably not a bad idea to create a helper in the common/proto package. I'd like to do that as a follow-on, though.

Features: pool Change-Id: Ia619442c1624b10593155291df217d7151ec0fe5

daosbuild1 · 2024-04-18T17:13:37Z

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14174/4/execution/node/1152/log

daosbuild1 · 2024-04-18T17:30:13Z

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14174/5/execution/node/412/log

Some responses used "leader", and others used "svc_ldr". As we already have a "svc_reps" precedent, and much of the code was expecting "svc_ldr" from the old control.Pool type, just use that. Features: pool Required-githooks: true Change-Id: Iee8bda89de8be47a0c4884014de352340e5a032f Signed-off-by: Michael MacDonald <[email protected]>

kjacque

LGTM

src/proto/mgmt/pool.proto

daosbuild1 · 2024-04-20T11:00:44Z

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

Change-Id: I457c40d99bea78795dd63cd2ee25b5ea25302d38

Features: pool Required-githooks: true Change-Id: Iba398a2d986e4d6e668a9f6195698e4074eab402 Signed-off-by: Michael MacDonald <[email protected]>

daosbuild1 · 2024-04-22T00:24:02Z

Test stage Functional Hardware Medium Verbs Provider completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14174/7/testReport/

daltonbohning

It seems we need to update this test as well?

daos/src/tests/ftest/control/dmg_pool_query_test.py

Line 69 in 7861ebb

"leader": self.params.get("leader", path="/run/exp_vals/*"),

And we should probably add a pool tag, which would have helped you catch this.
These lines:

daos/src/tests/ftest/control/dmg_pool_query_test.py

Line 38 in 7861ebb

:avocado: tags=dmg,pool_query,basic,control

daos/src/tests/ftest/control/dmg_pool_query_test.py

Line 108 in 7861ebb

:avocado: tags=dmg,pool_query,basic,control

daos/src/tests/ftest/control/dmg_pool_query_test.py

Line 155 in 7861ebb

:avocado: tags=dmg,pool_query,basic,control

mjmac · 2024-05-02T15:19:31Z

It seems we need to update this test as well?

daos/src/tests/ftest/control/dmg_pool_query_test.py

Line 69 in 7861ebb

"leader": self.params.get("leader", path="/run/exp_vals/*"),

Bah. Sigh, will do.

And we should probably add a pool tag, which would have helped you catch this.
These lines:

daos/src/tests/ftest/control/dmg_pool_query_test.py

Line 38 in 7861ebb

:avocado: tags=dmg,pool_query,basic,control

I'll add it.

Required-githooks: true Change-Id: I380e559f6dd6e1ec52e59b141af45af447ddf445

Use svc_ldr instead of leader. Also adds pool tags so that it's included in Features: pool testing. Features: pool Required-githooks: true Change-Id: I66a146f748699e54e61b7f8e5c5e4aa44a2ace55 Signed-off-by: Michael MacDonald <[email protected]>

daltonbohning

ftest LGTM

kjacque · 2024-05-03T00:57:56Z

src/control/lib/control/pool.go

+	piJSON[0] = ','
+	return append(auxJSON[:len(auxJSON)-1], piJSON...), nil


I'm a little confused. If PoolInfo marshals cleanly, why is it necessary to separate it out and tack it on, rather than including it as part of the aux struct?

It's due to one of the quirks of using embedded types. The MarshalJSON method on daos.PoolInfo is promoted, so the logic in PoolQueryResp's MarshalJSON is never used.

daosbuild1 · 2024-05-03T16:46:15Z

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

Required-githooks: true Change-Id: Ib692576b6ce1c79c26e3e289fabe5dfb7e32ff21

Features: pool Required-githooks: true Change-Id: Ia552ad8d2c10005ad607c03bd785d5cae4176de2 Signed-off-by: Michael MacDonald <[email protected]>

daosbuild1 · 2024-05-04T20:19:53Z

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

daosbuild1 · 2024-05-05T05:49:59Z

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-14174/16/execution/node/1205/log

Features: pool Required-githooks: true Change-Id: Ia76cdf30d8e1ad91677e0a00b0128e0e9648e969 Signed-off-by: Michael MacDonald <[email protected]>

Required-githooks: true Change-Id: I1ea908bb41fe71ec082fbf614549e3158db772e4

daosbuild1 · 2024-05-06T09:49:41Z

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14174/17/testReport/

Required-githooks: true Change-Id: Icc08b67739586c328d944959991ddb564164a791 Signed-off-by: Michael MacDonald <[email protected]>

Change-Id: Id09db85bfe2f27ac91d2fdfd6f46935dc9011d47

The lib/daos package should be the main repository for types (e.g. PoolInfo) used throughout the codebase. It should be possible to import these types without pulling in extra dependencies (e.g. gRPC, protobufs, etc). Also reverts an inadvertent change to the system database's serialization that resulted in 2.4/2.6 incompatibilities. Required-githooks: true Change-Id: I2e198a57cc5ae7fe76875c59f2f21eda745ce3e7 Signed-off-by: Michael MacDonald <[email protected]>

…4414) As a convenience, provide a "streamlined" version of the pool query that only performs the minimum amount of work to query the pool's health. Practically speaking, this means that it will query for disabled ranks and omit the space query, which is expensive. Includes the following PR backports: DAOS-9953: Usability improvements to dmg pool list and query. (DAOS-9953: Usability improvements to dmg pool list and query. #12511) DAOS-14243 test: pool/list_verbose.py:test_fields_basic - missing rebuild_state (DAOS-14243 test: pool/list_verbose.py:test_fields_basic - missing rebuild_state #12983) DAOS-10028 control: Move core types into daos package (DAOS-10028 control: Move core types into daos package #14174) Signed-off-by: Michael MacDonald <[email protected]> Co-authored-by: Samir Raval <[email protected]> Co-authored-by: Ding Ho <[email protected]>

mjmac requested review from a team as code owners April 16, 2024 21:33

mjmac force-pushed the mjmac/DAOS-10028-pool branch from f6d9d8d to d73d0dd Compare April 16, 2024 21:40

daltonbohning reviewed Apr 16, 2024

View reviewed changes

kjacque previously approved these changes Apr 16, 2024

View reviewed changes

mjmac requested a review from tanabarr April 17, 2024 15:28

tanabarr previously approved these changes Apr 17, 2024

View reviewed changes

Address review feedback

33f1d61

Move PoolQueryTarget and related types into lib/daos. Features: pool Required-githooks: true Change-Id: Ibb90c44d681402b699e023cc78adb4b7f21b0910 Signed-off-by: Michael MacDonald <[email protected]>

mjmac dismissed stale reviews from tanabarr and kjacque via 33f1d61 April 17, 2024 18:43

mjmac requested review from tanabarr and kjacque April 17, 2024 18:52

tanabarr previously approved these changes Apr 18, 2024

View reviewed changes

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

44e9b9c

Features: pool Change-Id: Ia619442c1624b10593155291df217d7151ec0fe5

mjmac dismissed tanabarr’s stale review via 8ef0b8d April 18, 2024 19:01

kjacque previously approved these changes Apr 19, 2024

View reviewed changes

src/proto/mgmt/pool.proto Outdated Show resolved Hide resolved

mjmac added 2 commits April 20, 2024 16:19

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

f8637bb

Change-Id: I457c40d99bea78795dd63cd2ee25b5ea25302d38

fix a few more broken tests

7774a48

Features: pool Required-githooks: true Change-Id: Iba398a2d986e4d6e668a9f6195698e4074eab402 Signed-off-by: Michael MacDonald <[email protected]>

mjmac dismissed kjacque’s stale review via 7774a48 April 20, 2024 17:02

daltonbohning reviewed May 2, 2024

View reviewed changes

mjmac added 2 commits May 2, 2024 15:21

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

19c8644

Required-githooks: true Change-Id: I380e559f6dd6e1ec52e59b141af45af447ddf445

Update control/dmg_pool_query_test.py

f6d3e7b

Use svc_ldr instead of leader. Also adds pool tags so that it's included in Features: pool testing. Features: pool Required-githooks: true Change-Id: I66a146f748699e54e61b7f8e5c5e4aa44a2ace55 Signed-off-by: Michael MacDonald <[email protected]>

daltonbohning reviewed May 2, 2024

View reviewed changes

tanabarr previously approved these changes May 2, 2024

View reviewed changes

kjacque reviewed May 3, 2024

View reviewed changes

mjmac added 2 commits May 4, 2024 17:51

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

0b63c75

Required-githooks: true Change-Id: Ib692576b6ce1c79c26e3e289fabe5dfb7e32ff21

Fix pool query test

140c2b2

Features: pool Required-githooks: true Change-Id: Ia552ad8d2c10005ad607c03bd785d5cae4176de2 Signed-off-by: Michael MacDonald <[email protected]>

mjmac dismissed tanabarr’s stale review via 140c2b2 May 4, 2024 19:50

mjmac marked this pull request as draft May 5, 2024 14:06

mjmac added 2 commits May 6, 2024 02:15

Rargh. Fix a Go unit test

b950a2c

Features: pool Required-githooks: true Change-Id: Ia76cdf30d8e1ad91677e0a00b0128e0e9648e969 Signed-off-by: Michael MacDonald <[email protected]>

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

18388a5

Required-githooks: true Change-Id: I1ea908bb41fe71ec082fbf614549e3158db772e4

mjmac added 2 commits May 6, 2024 14:20

forgot to update the tier_name field

fcd9fad

Required-githooks: true Change-Id: Icc08b67739586c328d944959991ddb564164a791 Signed-off-by: Michael MacDonald <[email protected]>

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

03c76ec

Change-Id: Id09db85bfe2f27ac91d2fdfd6f46935dc9011d47

mjmac marked this pull request as ready for review May 7, 2024 19:38

mjmac requested review from kjacque and tanabarr May 7, 2024 19:38

kjacque approved these changes May 7, 2024

View reviewed changes

jolivier23 approved these changes May 8, 2024

View reviewed changes

mjmac merged commit 2edbaca into master May 8, 2024
51 of 52 checks passed

mjmac deleted the mjmac/DAOS-10028-pool branch May 8, 2024 15:09

mjmac mentioned this pull request May 22, 2024

DAOS-15794 tools: Add --health-only flag to (dmg|daos) pool query #14414

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-10028 control: Move core types into daos package #14174

DAOS-10028 control: Move core types into daos package #14174

mjmac commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

daltonbohning left a comment

daosbuild1 commented Apr 16, 2024

kjacque left a comment

kjacque Apr 16, 2024

mjmac Apr 17, 2024

mjmac Apr 17, 2024

tanabarr left a comment

tanabarr Apr 17, 2024

mjmac Apr 17, 2024

tanabarr Apr 17, 2024

mjmac Apr 17, 2024

tanabarr Apr 17, 2024

daosbuild1 commented Apr 17, 2024

tanabarr Apr 18, 2024

mjmac Apr 19, 2024

daosbuild1 commented Apr 18, 2024

daosbuild1 commented Apr 18, 2024

kjacque left a comment

daosbuild1 commented Apr 20, 2024

daosbuild1 commented Apr 22, 2024

daltonbohning left a comment

mjmac commented May 2, 2024

daltonbohning left a comment

kjacque May 3, 2024

mjmac May 3, 2024

daosbuild1 commented May 3, 2024

daosbuild1 commented May 4, 2024

daosbuild1 commented May 5, 2024

daosbuild1 commented May 6, 2024

		piJSON[0] = ','
		return append(auxJSON[:len(auxJSON)-1], piJSON...), nil

DAOS-10028 control: Move core types into daos package #14174

DAOS-10028 control: Move core types into daos package #14174

Conversation

mjmac commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

daltonbohning left a comment

Choose a reason for hiding this comment

daosbuild1 commented Apr 16, 2024

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

tanabarr 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

daosbuild1 commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Apr 18, 2024

daosbuild1 commented Apr 18, 2024

kjacque left a comment

Choose a reason for hiding this comment

daosbuild1 commented Apr 20, 2024

daosbuild1 commented Apr 22, 2024

daltonbohning left a comment

Choose a reason for hiding this comment

mjmac commented May 2, 2024

daltonbohning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented May 3, 2024

daosbuild1 commented May 4, 2024

daosbuild1 commented May 5, 2024

daosbuild1 commented May 6, 2024