DAOS-16329 chk: maintenance mode after checking pool with dryrun - b26 #14985

Nasf-Fan · 2024-08-22T09:29:37Z

Sometimes, after system shutdown unexpectedly, the users may expect to check their critical data under some kind of maintenance mode. Under such mode, no user data can be modified or moved or aggregated. That will guarantee no further potential (DAOS logic caused) damage can happen during the check.

For such purpose, we will enhance current DAOS CR logic with --dryrun option to allow the pool (after check) to be opened as immutable with disabling some mechanism that may potentially cause data modification or movement (such as rebuild or aggregation).

Under such mode, if client wants to connect to the pool, the read-only option must be specified. Similarly for opening container in such pool.

Test-tag: pr cat_recov
Allow-unstable-test: 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:

github-actions · 2024-08-22T09:29:55Z

Ticket title is 'Maintenance mode after CR checking the pool with dryrun option'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-16329

daosbuild1 · 2024-08-22T10:05:22Z

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-14985/1/testReport/

daosbuild1 · 2024-08-22T10:25:16Z

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

daosbuild1 · 2024-08-23T03:48:32Z

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-14985/2/testReport/

daosbuild1 · 2024-08-23T04:08:00Z

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

daltonbohning · 2024-08-26T15:38:53Z

src/tests/ftest/cart/util/cart_logtest.py

@@ -209,7 +209,7 @@ def subtract(self, val):
               "DER_NOTLEADER(-2008): 'Not service leader'")

 # Functions that are never reported as errors.
-IGNORED_FUNCTIONS = ('sched_watchdog_post', 'rdb_timerd')
+IGNORED_FUNCTIONS = ('sched_watchdog_post', 'rdb_timerd', 'cont_open')


Why are we ignoring cont_open here instead of fixing the errors? And what are the errors we're ignoring?

This is some temporary workaround for the following NLT warning:
https://build.hpdd.intel.com/job/daos-stack/job/daos/view/change-requests/job/PR-14912/10/VM_test/folder.-412761578/

cont_open() 07139793/8d9ab078/6908b833: No permission to open the container with flags 2, capas 7/0 warning in strict mode

I do not know how the new warning is valuable for us.

I do not know how to fix such NLT warning because even if I modified some existing logic line in cont_open, the CI test will also report similar NLT warning. @daltonbohning , any suggestion for that? Thanks!

We should not disable the test because something introduced a new warning.
This PR adds the line that it's complaining about:

daos/src/container/srv_container.c

Lines 2292 to 2301 in 055d8bd

if (((flags & DAOS_COO_EX) && !ds_sec_cont_can_open_ex(sec_capas)) ||

((flags & DAOS_COO_RW) && !ds_sec_cont_can_modify(sec_capas))) {

D_ERROR(DF_UUID "/" DF_UUID "/" DF_UUID ": permission denied opening the "

"container with flags " DF_X64 ", capas " DF_X64 "/" DF_X64 "\n",

DP_UUID(cont->c_svc->cs_pool_uuid), DP_UUID(pool_hdl->sph_uuid),

DP_UUID(cont->c_uuid), flags, pool_hdl->sph_sec_capas, sec_capas);

daos_prop_free(prop);

rc = -DER_NO_PERM;

goto out;

}

I think the problem is it doesn't follow the standard format with the rc in the error. The same would apply for other error messages as well

Sorry, that was the wrong code snippet. But I think the problem is the same

According to the commit edeafc1 that introduced the IGNORED_FUNCTIONS list, I did not find any special reason for adding "rdb_timerd" into such list except reducing unnecessary "fail" report.

In fact, such NLT warning for cont_open log exists on current master, it is not reported as CI failure if no patch touch related code. But my patch touches it because I need more information in the log, then it failed the CI test.

From the test logic perspective (although I do not agree), it seems that such log line should not be there.
@daltonbohning , do you still think it is not an acceptable solution to "skip" the existing cont_open NLT warning?

Here is an example for the NLT warning existence:
https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15055/1/VM_test/

I'm not very familiar with NLT so I don't know what is best here. It just seems bad to disable a subset of testing because it's failing. @ashleypittman is the best source here but is on sabbatical.
If it's pre-existing, maybe this applies? Copied from our dev channel

Tips from Ashley:
the way Jenkins works is it checks if messages are the same, and obviously it can't just check the line number for this as that changes. It takes a snapshot of the 5 lines before and after the line where the error is reported, and if these are the same then the error is treated as the same. If these lines are modified then the error does not match and therefore you get "one removed error", "one error added"

in which case the procedure is the same as for adding a new error, complete your PR as you want it, run with Allow-unstable-test: true commit tags, the PR will need force-landing but once it's landed master will run and create new reference builds so other PRs should not be affected.

I.e. add Allow-unstable-test: true to your commit pragmas and request force landing, since it's a pre-existing error

let me try.

Sometimes, after system shutdown unexpectedly, the users may expect to check their critical data under some kind of maintenance mode. Under such mode, no user data can be modified or moved or aggregated. That will guarantee no further potential (DAOS logic caused) damage can happen during the check. For such purpose, we will enhance current DAOS CR logic with --dryrun option to allow the pool (after check) to be opened as immutable with disabling some mechanism that may potentially cause data modification or movement (such as rebuild or aggregation). Under such mode, if client wants to connect to the pool, the read-only option must be specified. Similarly for opening container in such pool. Test-tag: pr cat_recov Allow-unstable-test: true Signed-off-by: Fan Yong <[email protected]>

daltonbohning

Will reserve approval for those familiar with core code, I think we will need to force land for the existing NLT failure

addressed

Nasf-Fan · 2024-09-27T15:10:29Z

Ping reviewers for the backport patch. Thanks!

daltonbohning · 2024-10-01T15:32:18Z

FYI - will need to wait for 2.6.1 GA, then merge approval for this ticket, then we can merge.

daltonbohning · 2024-10-08T15:17:22Z

Ticket does not have approval to merge to 2.6.2

Nasf-Fan · 2024-10-09T02:22:16Z

Ticket does not have approval to merge to 2.6.2

So we will not land this patch to any 2.6 based branch any longer, right?

daltonbohning · 2024-10-09T14:17:14Z

Ticket does not have approval to merge to 2.6.2

So we will not land this patch to any 2.6 based branch any longer, right?

It means we need to go through the merge approval process for this ticket/PR

Nasf-Fan · 2024-10-14T05:06:35Z

Ticket does not have approval to merge to 2.6.2

It is shown on the Jira ticket DAOS-16329 that Approved to Merge for 2.6.2 Community Release

daltonbohning · 2024-10-14T15:40:14Z

Ticket does not have approval to merge to 2.6.2

It is shown on the Jira ticket DAOS-16329 that Approved to Merge for 2.6.2 Community Release

To be clear, I posted this on Oct 8 and Approved to Merge for 2.6.2 Community Release was not set until Oct 9. We need approval to merge before requesting gatekeeper

Nasf-Fan force-pushed the Nasf-Fan/DAOS-16329_1_b26 branch from f7a7ee1 to 792a624 Compare August 23, 2024 03:17

Nasf-Fan force-pushed the Nasf-Fan/DAOS-16329_1_b26 branch from 792a624 to 055d8bd Compare August 23, 2024 05:58

Nasf-Fan marked this pull request as ready for review August 25, 2024 15:32

Nasf-Fan requested review from a team as code owners August 25, 2024 15:32

daltonbohning previously requested changes Aug 26, 2024

View reviewed changes

Nasf-Fan force-pushed the Nasf-Fan/DAOS-16329_1_b26 branch from 055d8bd to d10aad6 Compare September 5, 2024 02:01

Nasf-Fan requested a review from daltonbohning September 6, 2024 03:22

daltonbohning reviewed Sep 9, 2024

View reviewed changes

Nasf-Fan requested review from wangshilong and kjacque September 26, 2024 00:02

Nasf-Fan added clean-cherry-pick Cherry-pick from another branch that did not require additional edits release-2.6.2 Targeted for release 2.6.2 labels Sep 26, 2024

kjacque approved these changes Sep 27, 2024

View reviewed changes

wangshilong approved these changes Sep 29, 2024

View reviewed changes

Nasf-Fan requested a review from a team September 29, 2024 01:33

daltonbohning removed the request for review from a team October 8, 2024 15:17

Nasf-Fan requested a review from a team October 21, 2024 02:30

daltonbohning merged commit c821379 into release/2.6 Oct 21, 2024
50 of 52 checks passed

daltonbohning deleted the Nasf-Fan/DAOS-16329_1_b26 branch October 21, 2024 18:14

daltonbohning added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Oct 21, 2024

This was referenced Oct 28, 2024

Merge upstream/release/2.6 into upstream/google/2.6 #15412

Closed

Merge upstream/release/2.6 into upstream/google/2.6 #15414

Closed

Merge upstream/release/2.6 into upstream/google/2.6 #15415

Closed

mjmac mentioned this pull request Nov 6, 2024

Merge upstream/release/2.6 into upstream/google/2.6 #15460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-16329 chk: maintenance mode after checking pool with dryrun - b26 #14985

DAOS-16329 chk: maintenance mode after checking pool with dryrun - b26 #14985

Nasf-Fan commented Aug 22, 2024

github-actions bot commented Aug 22, 2024 •

edited

Loading

daosbuild1 commented Aug 22, 2024

daosbuild1 commented Aug 22, 2024

daosbuild1 commented Aug 23, 2024

daosbuild1 commented Aug 23, 2024

daltonbohning Aug 26, 2024

Nasf-Fan Aug 27, 2024

Nasf-Fan Aug 28, 2024

daltonbohning Aug 28, 2024

daltonbohning Aug 28, 2024

Nasf-Fan Aug 31, 2024

Nasf-Fan Sep 2, 2024

Nasf-Fan Sep 2, 2024

daltonbohning Sep 3, 2024

Nasf-Fan Sep 5, 2024

daltonbohning left a comment

Nasf-Fan commented Sep 27, 2024

daltonbohning commented Oct 1, 2024

daltonbohning commented Oct 8, 2024

Nasf-Fan commented Oct 9, 2024

daltonbohning commented Oct 9, 2024

Nasf-Fan commented Oct 14, 2024

daltonbohning commented Oct 14, 2024

	if (((flags & DAOS_COO_EX) && !ds_sec_cont_can_open_ex(sec_capas)) \|\|
	((flags & DAOS_COO_RW) && !ds_sec_cont_can_modify(sec_capas))) {
	D_ERROR(DF_UUID "/" DF_UUID "/" DF_UUID ": permission denied opening the "
	"container with flags " DF_X64 ", capas " DF_X64 "/" DF_X64 "\n",
	DP_UUID(cont->c_svc->cs_pool_uuid), DP_UUID(pool_hdl->sph_uuid),
	DP_UUID(cont->c_uuid), flags, pool_hdl->sph_sec_capas, sec_capas);
	daos_prop_free(prop);
	rc = -DER_NO_PERM;
	goto out;
	}

DAOS-16329 chk: maintenance mode after checking pool with dryrun - b26 #14985

DAOS-16329 chk: maintenance mode after checking pool with dryrun - b26 #14985

Conversation

Nasf-Fan commented Aug 22, 2024

Before requesting gatekeeper:

Gatekeeper:

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

daosbuild1 commented Aug 22, 2024

daosbuild1 commented Aug 22, 2024

daosbuild1 commented Aug 23, 2024

daosbuild1 commented Aug 23, 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

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

daltonbohning left a comment

Choose a reason for hiding this comment

Nasf-Fan commented Sep 27, 2024

daltonbohning commented Oct 1, 2024

daltonbohning commented Oct 8, 2024

Nasf-Fan commented Oct 9, 2024

daltonbohning commented Oct 9, 2024

Nasf-Fan commented Oct 14, 2024

daltonbohning commented Oct 14, 2024

github-actions bot commented Aug 22, 2024 •

edited

Loading