DAOS-14698 crtl: Create container with attributes #15300

knard-intel · 2024-10-11T21:23:43Z

Description

Add new option attr to daos cont create allowing to define container user attributes.

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:

github-actions · 2024-10-11T21:24:02Z

Ticket title is 'Creating daos container with dfuse cache settings.'
Status is 'In Review'
Labels: 'triaged,usability'
Errors are Unknown component
https://daosio.atlassian.net/browse/DAOS-14698

Add new option set-attr to daos cont create allowing to define container user attributes. Features: control dfuse Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

daosbuild1 · 2024-10-18T16:00:49Z

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-15300/4/testReport/

Update and add new functional tests. Features: control dfuse container Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

ashleypittman · 2024-10-21T10:33:43Z

src/tests/ftest/dfuse/container_attrs.py

+        # Start the servers and agents
+        super().setUp()
+
+    def check_attrs(self, dfuse, container_name):


This is a helper function so should be "private" to the class.

Suggested change

def check_attrs(self, dfuse, container_name):

def _check_attrs(self, dfuse, container_name):

Move check_attrs() to private.

Move check_attrs() to private.

Fixed with commit 865f2e5

src/tests/ftest/dfuse/container_attrs.py

daosbuild1 · 2024-10-21T10:49:27Z

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-15300/6/testReport/

Integrate reviewers comments: - Move check_attrs() to private Features: control dfuse container Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

daosbuild1 · 2024-10-21T12:54:10Z

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-15300/7/testReport/

mchaarawi · 2024-10-21T14:12:57Z

src/include/daos_uns.h

+duns_create_path_attr(daos_handle_t poh, const char *path, int count, char const *const names[],
+		      void const *const values[], size_t const sizes[], struct duns_attr_t *attrp);


im not a fan of this new API. it might be useful to have something for the command line.
but for the API, it's not a big deal to do
daos_cont_create()
daos_cont_set_attr()
I would suggest to remove it.

Not sure to fully understand your comments.
Are you suggesting to remove duns_create_path_attr() and duns_create_path() functions or just the first one.
If we are just removing duns_create_path_attr(), then we will have two solutions from my understanding:

Change the dfuse code to poll the attributes of its mounted containers.

split the function duns_create_path_attr() to not create and bind the container in the same function. By this way, we will be able to use the function daos_cont_set_attr() between the two function calls.

no you cannot remove duns_create_path()!
my question is why you are adding this new API (duns_crreate_path_attr()) ?
what is the challenge of doing:
duns_create_path()
daos_cont_set_attr()
?

From my understanding, doing this will not work as the attributes are cached inside DFuse after the binding.
Thus, when we will get back from duns_create_path() , the call to daos_cont_set_attr() will not be taken into account by DFuse.
@ashleypittman could you confirm this.

I've taken a closer look and think you're both right, having a new API doesn't seem like the right solution here, however I hadn't realised it was the same C function that both created the container and inserted it into the POSIX namespace.

The best path might be:
daos_cont_create()
daos_cont_set_attr()
duns_link_cont()

I was not aware of the duns_link_cont() function, which for sure is a better solution.
Thanks @ashleypittman for the hints and @mchaarawi for the relevant remark :-)

Remove useless new function duns_create_path_attr()

Split sub container creation

I was not aware of the duns_link_cont() function, which for sure is a better solution. Thanks @ashleypittman for the hints and @mchaarawi for the relevant remark :-)

Remove useless new function duns_create_path_attr()

Split sub container creation

Fixed with commit 819b856

kjacque

From the Go side, the approach looks fine to me. Will wait for the other requested changes to come through before approving.

src/control/cmd/daos/container.go

daosbuild1 · 2024-10-21T22:41:57Z

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-15300/7/execution/node/1217/log

…/daos-14698 Required-githooks: true

Integrate reviewers comments: - Change tags from pr to daily_regression - Add container type in the log message - Add daos_cmd feature tag Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

…/daos-14698 Skip-func-hw-test-medium-md-on-ssd: false Skip-nlt: true Test-tag: DfuseContainerAttrs Required-githooks: true

daosbuild1 · 2024-10-25T07:33:19Z

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-15300/15/execution/node/1120/log

…/daos-14698

daosbuild1 · 2024-10-25T13:01:34Z

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-15300/16/execution/node/491/log

…/daos-14698 Required-githooks: true

Fix NLT uns_link test Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

…/daos-14698 Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

kjacque

Go changes LGTM.

Fix NLT fault injection issue Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

daosbuild1 · 2024-11-01T04:40:17Z

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-15300/21/execution/node/1210/log

ashleypittman

It would be good to add some attributes to the new container in the test_alloc_fail_cont_create function in NLT, this would check the unwinding code is correct in case of failure partway through.

ashleypittman · 2024-11-04T08:24:41Z

src/client/dfs/duns.c

+		rc = stat(path, &finfo);
+		if (rc) {
+			rc = errno;
+			D_ERROR("Failed to access new container: %d (%s)\n", rc, strerror(rc));


I know this is moved, not new code but it would be good to use the correct macros here. The S here stands for system, that is it's a errno.

Suggested change

D_ERROR("Failed to access new container: %d (%s)\n", rc, strerror(rc));

DS_ERROR(rc, "Failed to access new container");

Use up to date logging macros

Use up to date logging macros

Fixed with commit 0561a5b

ashleypittman · 2024-11-04T08:26:02Z

src/client/dfs/duns.c

+	if (rc2 != -DER_SUCCESS) {
+		DL_ERROR(rc2, "failed to close container");
+		if (rc2 == -DER_NOMEM)
+			// Second close to properly handle fault injection
+			daos_cont_close(coh, NULL);
+		else if (rc == -DER_SUCCESS)
+			rc = daos_der2errno(rc2);
+	}


I thought we'd designed out these hacks by now but perhaps there's still a few there. Container close would be a tricky one to re-try at a higher level.

…/daos-14698 Required-githooks: true

Fix functional tests: ACL not properly set with dfs_link_cont() function. Integrate reviewers comments: - Use up to date logging macros Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

Improve deployment setting of the DfuseContainerAttrs functional test. Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

daosbuild1 · 2024-11-05T00:30:30Z

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-15300/24/execution/node/1211/log

…/daos-14698 Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

daosbuild1 · 2024-11-05T22:13:23Z

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

…/daos-14698

Fix functional test test_dfuse_subcontainer_attrs. Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

Fix pylint errors. Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

knard-intel force-pushed the ckochhof/dev/master/daos-14698 branch 2 times, most recently from 3a1167a to 7d8cdd0 Compare October 16, 2024 09:58

ashleypittman mentioned this pull request Oct 17, 2024

DAOS-16568 tests: Better cmdline control of NLT tests #15141

Merged

18 tasks

DAOS-14698 crtl: Create container with attributes

5fcd159

Add new option set-attr to daos cont create allowing to define container user attributes. Features: control dfuse Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

knard-intel force-pushed the ckochhof/dev/master/daos-14698 branch from 7d8cdd0 to 77f33b2 Compare October 18, 2024 15:13

knard-intel force-pushed the ckochhof/dev/master/daos-14698 branch from 77f33b2 to 8421816 Compare October 21, 2024 10:01

DAOS-14698 crtl: Create container with attributes

315f440

Update and add new functional tests. Features: control dfuse container Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

knard-intel force-pushed the ckochhof/dev/master/daos-14698 branch from 8421816 to 315f440 Compare October 21, 2024 10:05

knard-intel marked this pull request as ready for review October 21, 2024 10:27

knard-intel requested review from a team as code owners October 21, 2024 10:27

knard-intel requested review from ashleypittman, daltonbohning, mjmac and mchaarawi October 21, 2024 10:28

ashleypittman reviewed Oct 21, 2024

View reviewed changes

knard-intel requested a review from ashleypittman October 21, 2024 12:10

DAOS-14698 crtl: Create container with attributes

865f2e5

Integrate reviewers comments: - Move check_attrs() to private Features: control dfuse container Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

mchaarawi reviewed Oct 21, 2024

View reviewed changes

kjacque reviewed Oct 21, 2024

View reviewed changes

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

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

4dd51bd

…/daos-14698 Required-githooks: true

phender previously approved these changes Oct 23, 2024

View reviewed changes

kanard38 added 2 commits October 24, 2024 12:22

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

a3d0a02

…/daos-14698 Required-githooks: true

knard-intel dismissed phender’s stale review via 7f26733 October 24, 2024 12:30

knard-intel requested review from daltonbohning and phender October 24, 2024 12:31

phender previously approved these changes Oct 24, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

5e72550

…/daos-14698 Skip-func-hw-test-medium-md-on-ssd: false Skip-nlt: true Test-tag: DfuseContainerAttrs Required-githooks: true

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

5dca3b4

…/daos-14698

kanard38 added 2 commits October 28, 2024 08:00

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

98bd3c2

…/daos-14698 Required-githooks: true

DAOS-14698 crtl: Create container with attributes

a390e8a

Fix NLT uns_link test Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

knard-intel dismissed phender’s stale review via a390e8a October 28, 2024 09:45

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

9af9320

…/daos-14698 Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

kjacque reviewed Oct 30, 2024

View reviewed changes

DAOS-14698 crtl: Create container with attributes

d2ee639

Fix NLT fault injection issue Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

ashleypittman reviewed Nov 4, 2024

View reviewed changes

kanard38 added 4 commits November 4, 2024 10:41

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

90186bc

…/daos-14698 Required-githooks: true

DAOS-14698 crtl: Create container with attributes

263168f

Improve deployment setting of the DfuseContainerAttrs functional test. Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

DAOS-14698 crtl: Create container with attributes

436436f

Improve deployment setting of the DfuseContainerAttrs functional test. Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

knard-intel self-assigned this Nov 4, 2024

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

f30184d

…/daos-14698 Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

kanard38 added 3 commits November 15, 2024 10:01

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

b1aaa8f

…/daos-14698

DAOS-14698 crtl: Create container with attributes

af54e76

Fix functional test test_dfuse_subcontainer_attrs. Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

DAOS-14698 crtl: Create container with attributes

d230c87

Fix pylint errors. Features: control dfuse container daos_cmd Required-githooks: true Signed-off-by: Cedric Koch-Hofer <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-14698 crtl: Create container with attributes #15300

DAOS-14698 crtl: Create container with attributes #15300

knard-intel commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

daosbuild1 commented Oct 18, 2024

ashleypittman Oct 21, 2024

knard-intel Oct 21, 2024 •

edited

Loading

knard-intel Oct 21, 2024

daosbuild1 commented Oct 21, 2024

daosbuild1 commented Oct 21, 2024

mchaarawi Oct 21, 2024

knard-intel Oct 21, 2024

mchaarawi Oct 21, 2024

knard-intel Oct 21, 2024

ashleypittman Oct 21, 2024

knard-intel Oct 21, 2024 •

edited

Loading

knard-intel Oct 22, 2024

kjacque left a comment

daosbuild1 commented Oct 21, 2024

daosbuild1 commented Oct 25, 2024

daosbuild1 commented Oct 25, 2024

kjacque left a comment

daosbuild1 commented Nov 1, 2024

ashleypittman left a comment

ashleypittman Nov 4, 2024

knard-intel Nov 4, 2024 •

edited

Loading

knard-intel Nov 4, 2024

ashleypittman Nov 4, 2024

daosbuild1 commented Nov 5, 2024

daosbuild1 commented Nov 5, 2024

	def check_attrs(self, dfuse, container_name):
	def _check_attrs(self, dfuse, container_name):

		duns_create_path_attr(daos_handle_t poh, const char path, int count, char const const names[],
		void const const values[], size_t const sizes[], struct duns_attr_t attrp);

	D_ERROR("Failed to access new container: %d (%s)\n", rc, strerror(rc));
	DS_ERROR(rc, "Failed to access new container");

DAOS-14698 crtl: Create container with attributes #15300

Are you sure you want to change the base?

DAOS-14698 crtl: Create container with attributes #15300

Conversation

knard-intel commented Oct 11, 2024 • edited Loading

Description

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Oct 11, 2024 • edited Loading

daosbuild1 commented Oct 18, 2024

Choose a reason for hiding this comment

knard-intel Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 21, 2024

daosbuild1 commented Oct 21, 2024

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

knard-intel Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjacque left a comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 21, 2024

daosbuild1 commented Oct 25, 2024

daosbuild1 commented Oct 25, 2024

kjacque left a comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 1, 2024

ashleypittman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knard-intel Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 5, 2024

daosbuild1 commented Nov 5, 2024

knard-intel commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

knard-intel Oct 21, 2024 •

edited

Loading

knard-intel Oct 21, 2024 •

edited

Loading

knard-intel Nov 4, 2024 •

edited

Loading