Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-13047 chk: properly handle start options #12242

Merged
merged 2 commits into from
Jun 12, 2023

Conversation

Nasf-Fan
Copy link
Contributor

Test-tag-hw-medium: Pass1Test

The start options for new instance should properly overwrite old ones. It also enhances CR pass 1 functionality tests:

  1. Verify DAOS checker under dryrun mode.

  2. Verify DAOS checker under auto mode.

  3. Verify that the DAOS checker can detect and repair the inconsistency under regular mode.

  4. Fix some typo for the test tags.

  5. More accurate comment for test flow.

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:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate watchers.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

Nasf-Fan added 2 commits May 30, 2023 15:53
Test-tag-hw-medium: Pass1Test

The start options for new instance should properly overwrite old ones.
It also enhances CR pass 1 functionality tests:

1. Verify DAOS checker under dryrun mode.

2. Verify DAOS checker under auto mode.

3. Verify that the DAOS checker can detect and repair the inconsistency
   under regular mode.

4. Fix some typo for the test tags.

5. More accurate comment for test flow.

Signed-off-by: Fan Yong <[email protected]>
Remove blank line.

Signed-off-by: Fan Yong <[email protected]>
@github-actions
Copy link

Bug-tracker data:
Ticket title is 'CR unit test for DAOS distributed check engine logic'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-13047

Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-13047_options branch from e0d5a68 to bc4c64a Compare May 31, 2023 04:00
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-13047_options branch from bc4c64a to da1e1e1 Compare May 31, 2023 04:07
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

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

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12242/3/execution/node/1162/log

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-13047_options branch from da1e1e1 to ce96c87 Compare May 31, 2023 13:08
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12242/4/execution/node/1212/log

@Nasf-Fan Nasf-Fan marked this pull request as ready for review June 4, 2023 10:07
@Nasf-Fan Nasf-Fan requested a review from a team as a code owner June 4, 2023 10:07
@Nasf-Fan Nasf-Fan requested review from mjmac and shimizukko June 5, 2023 06:49
src/tests/ftest/recovery/pass_1.py Outdated Show resolved Hide resolved
src/tests/ftest/recovery/pass_1.py Outdated Show resolved Hide resolved
src/tests/ftest/util/dmg_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@shimizukko shimizukko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should change the method name test_run_checker() to not start with test_ because it's the helper method.

break
time.sleep(5)

# 6.3 Verify that the checker detected the dangling pool.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# 6.3 Verify that the checker detected the dangling pool.
# 6.3 Verify that the checker detected the inconsistency.

dmg_command.check_set_policy(reset_defaults=True)

# 6.1 Start check with auto=off,
# that will find the dangling pool and remove it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# that will find the dangling pool and remove it.
# that will find the inconsistency and fix it.

src/tests/ftest/recovery/pass_1.py Outdated Show resolved Hide resolved
1. Enable check mode.
2. Run checker under dry-run mode.
3. Set repair policy as interaction.
4. Run checker under auto mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
4. Run checker under auto mode.
4. Run checker under auto mode and verify that it detects inconsistency.

@@ -187,32 +220,20 @@ def test_orphan_pool_trus_ps(self):
:avocado: tags=Pass1Test,test_orphan_pool_trust_ps
"""
errors = []
# Run step 1 to 6.
# 4. Run DAOS checker under kinds of mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's a good idea to remove steps from the method description. I know it's duplicate of what you wrote in run_checker_on_orphan_pool(), but it would make it harder to understand what this test does. If you really want to remove it, then you might want to improve the comment here. e.g.,

Suggested change
# 4. Run DAOS checker under kinds of mode.
# 1-4. Create a pool, remove the PS entry, list and verify that there's no pool, run and verify the checker behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The steps are moved to run_checker, not removed.

errors = self.run_checker_on_orphan_pool(
policies="POOL_NONEXIST_ON_MS:CIA_TRUST_MS")

# 7. Verify that the orphan pool was removed.
# Verify that the orphan pool was destroyed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's a good idea to remove the test steps from the method description.

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-13047_options branch from ce96c87 to 79b2322 Compare June 6, 2023 10:23
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-13047_options branch from 79b2322 to c19d9d9 Compare June 6, 2023 10:40
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-13047_options branch from c19d9d9 to 24a71b8 Compare June 6, 2023 11:03
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12242/7/execution/node/1237/log

@Nasf-Fan Nasf-Fan requested a review from daltonbohning June 7, 2023 01:26
@daltonbohning
Copy link
Contributor

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-13047_options branch from c19d9d9 to 24a71b8
yesterday

FYI - it's better to not force push after review so reviewers can see the diff

Copy link
Contributor

@daltonbohning daltonbohning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ftest LGTM

Comment on lines 1365 to 1366
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""
"""

@Nasf-Fan Nasf-Fan requested a review from daltonbohning June 8, 2023 00:21
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12242/8/execution/node/1267/log

@Nasf-Fan Nasf-Fan added the CR Catastrophic Recovery Feature label Jun 10, 2023
@Nasf-Fan
Copy link
Contributor Author

@mjmac @daltonbohning , would you please to help review the patch again? Thanks!

@mjmac mjmac merged commit 06ebead into feature/cat_recovery Jun 12, 2023
@mjmac mjmac deleted the Nasf-Fan/DAOS-13047_options branch June 12, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR Catastrophic Recovery Feature
Development

Successfully merging this pull request may close these issues.

5 participants