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-13510 chk: Query output should include dry-run status #12294

Closed
wants to merge 2 commits into from

Conversation

mjmac
Copy link
Contributor

@mjmac mjmac commented Jun 7, 2023

If the checker is running in dry-run mode, notify the user.

Required-githooks: true

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

@mjmac mjmac requested a review from a team as a code owner June 7, 2023 17:55
@github-actions
Copy link

github-actions bot commented Jun 7, 2023

Bug-tracker data:
Ticket title is 'Enhance check report message to describe the inconsistency under dryrun mode'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-13510

@mjmac mjmac requested a review from Nasf-Fan June 7, 2023 17:56
@mjmac
Copy link
Contributor Author

mjmac commented Jun 7, 2023

@Nasf-Fan: Please take a look to see if this is what you had in mind. The only issue that I can see is that there does not appear to be a way to get out of dry-run mode. If I run dmg check start --dry-run, then dmg check start --reset, the checker seems to stay in dry-run mode. I have also tried running dmg check stop && dmg check start, but the flag is persistent in the checker properties, it seems.

I think probably we need to clear the property flag if the user did not specify it with the start command. I tried to implement that, but ran into some trouble. Perhaps you can suggest a solution that I can include with this patch, thanks.

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.

If the checker is running in dry-run mode, notify the user.

Required-githooks: true

Signed-off-by: Michael MacDonald <[email protected]>
@mjmac mjmac force-pushed the mjmac/DAOS-13510 branch from a1738f6 to 033aaf4 Compare June 7, 2023 18:46
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.

cru->cru_detail_nr, cru->cru_details);
dryrun = ins->ci_prop.cp_flags & CHK__CHECK_FLAG__CF_DRYRUN;
rc = chk_report_upcall(cru->cru_gen, *seq, cru->cru_cla, cru->cru_act, cru->cru_result,
cru->cru_rank, cru->cru_target, cru->cru_pool, cru->cru_pool_label,
Copy link
Collaborator

Choose a reason for hiding this comment

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

(style) line over 100 characters

rc = chk_report_upcall(cru->cru_gen, *seq, cru->cru_cla, cru->cru_act, cru->cru_result,
cru->cru_rank, cru->cru_target, cru->cru_pool, cru->cru_pool_label,
cru->cru_cont, cru->cru_cont_label, cru->cru_obj, cru->cru_dkey,
cru->cru_akey, cru->cru_msg, cru->cru_option_nr, cru->cru_options,
Copy link
Collaborator

Choose a reason for hiding this comment

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

(style) line over 100 characters

@daosbuild1
Copy link
Collaborator

@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-12294/2/execution/node/1237/log

@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-12294/2/execution/node/1284/log

@Nasf-Fan
Copy link
Contributor

Nasf-Fan commented Jun 9, 2023

@Nasf-Fan: Please take a look to see if this is what you had in mind. The only issue that I can see is that there does not appear to be a way to get out of dry-run mode. If I run dmg check start --dry-run, then dmg check start --reset, the checker seems to stay in dry-run mode. I have also tried running dmg check stop && dmg check start, but the flag is persistent in the checker properties, it seems.

I think probably we need to clear the property flag if the user did not specify it with the start command. I tried to implement that, but ran into some trouble. Perhaps you can suggest a solution that I can include with this patch, thanks.

Does MS store the "dryrun" flag somewhere? If not, then when you start checker next time without "--dry-run", then the former "dryrun" will be automatically removed. If it does not work, then it is daos engine bug, and I need to fix it. Would you please to verify the "dryrun" behavior against the patch #12242 ? In such patch, we start checker with "dryrun" firstly, and then without "dryrun" in subsequent "check start". According to the CI test result, it works.

@Nasf-Fan
Copy link
Contributor

Nasf-Fan commented Jun 9, 2023

As my understand, MS can have more simple way to know whether current check instance is running under "dryrun" mode or not. For example, when user input "dmg check query", then MS can handle that in two steps:

  1. Firstly, trigger DRPC_METHOD_MGMT_CHK_PROP to DAOS engine, then it will get current checker start options, that includes the "dryrun" if have.
  2. Then, trigger DRPC_METHOD_MGMT_CHK_QUERY to DAOS engine as you orignal "dmg check query" does.

With the firstly step checker property information, you can know whether current checker is running under dryrun mode or not. That will much simplify the patch. How do you think?

Features: recovery control

Required-githooks: true
@daosbuild1 daosbuild1 dismissed their stale review October 2, 2023 16:01

Updated patch

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.

@kjacque kjacque self-assigned this Oct 2, 2023
Copy link
Contributor

@Nasf-Fan Nasf-Fan left a comment

Choose a reason for hiding this comment

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

@kjacque, as my understand, MS can have more simple way to know whether current check instance is running under "dryrun" mode instead of changing the API just for the dryrun flag. For example, when user input "dmg check query", then MS can handle that in two steps:

Firstly, trigger DRPC_METHOD_MGMT_CHK_PROP to DAOS engine, then it will get current checker start options, that includes the "dryrun" flag if have.

Then, trigger DRPC_METHOD_MGMT_CHK_QUERY to DAOS engine as you orignal "dmg check query" does.

With the firstly step checker property information, you can know whether current checker is running under dryrun mode or not. That will much simplify current patch.

@mjmac mjmac closed this May 1, 2024
@mjmac mjmac deleted the mjmac/DAOS-13510 branch May 1, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants