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.

Sign up for GitHub

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

Jump to bottom

DAOS-16328 control: Update dmg pool list for MD-on-SSD #15490

Merged
merged 1 commit into from
Nov 15, 2024
Merged

DAOS-16328 control: Update dmg pool list for MD-on-SSD #15490

merged 1 commit into from
Nov 15, 2024
+196 −67

Conversation

tanabarr
Copy link
Contributor

@tanabarr tanabarr commented Nov 11, 2024 •
edited
Loading

Update dmg pool list -v tabular output for MD-on-SSD. Change column
titles to describe META and DATA tiers (rather than SCM and
NVME as for PMem mode).

Correct confusing behaviour by making Used and Imbalance table values
in non-verbose output refer to DATA tier always and don’t vary based
on value comparison. With previous implementation the-most-used and
the-most-imbalanced tier value would be shown and different tier
values could be shown in the same row (DAOS-12900).

Hide usage columns in table output if --no-query is set (DAOS-16701).

Hide upgrade and disabled columns if --no-query -v set (DAOS-16296).

Features: control
Skip-func-hw-test-medium-md-on-ssd: false
Skip-func-hw-test-large-md-on-ssd: false
Required-githooks: 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:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • 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.

@tanabarr tanabarr self-assigned this Nov 11, 2024
@github-actions GitHub Actions
Copy link

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

Ticket title is 'Update dmg pool list for MD-on-SSD P2'
Status is 'In Review'
Labels: 'md_on_ssd2'
https://daosio.atlassian.net/browse/DAOS-16328

@daosbuild1
Copy link
Collaborator

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

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15490/1/execution/node/1483/log

@tanabarr tanabarr force-pushed the tanabarr/control-listpools-mdonssd branch 2 times, most recently from 9fdaa23 to 76e993c Compare November 12, 2024 14:18
@tanabarr tanabarr marked this pull request as ready for review November 12, 2024 14:45
@tanabarr tanabarr requested review from a team as code owners November 12, 2024 14:45
@tanabarr tanabarr added control-plane work on the management infrastructure of the DAOS Control Plane meta-on-ssd Metadata on SSD Feature labels Nov 12, 2024
@daosbuild1
Copy link
Collaborator

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-15490/3/testReport/

@tanabarr tanabarr force-pushed the tanabarr/control-listpools-mdonssd branch from 76e993c to 4dc7047 Compare November 12, 2024 18:07
@daosbuild1
Copy link
Collaborator

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

@daosbuild1
Copy link
Collaborator

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

@tanabarr tanabarr force-pushed the tanabarr/control-listpools-mdonssd branch from 4dc7047 to 36ef9ba Compare November 13, 2024 13:21
@daosbuild1
Copy link
Collaborator

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-15490/5/testReport/

@tanabarr
DAOS-16328 control: Update dmg pool list for MD-on-SSD
c6ebd52 
Features: control
Skip-func-hw-test-medium-md-on-ssd: false
Skip-func-hw-test-large-md-on-ssd: false
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@tanabarr tanabarr force-pushed the tanabarr/control-listpools-mdonssd branch from 36ef9ba to c6ebd52 Compare November 13, 2024 14:12
@tanabarr tanabarr requested a review from a team as a code owner November 13, 2024 14:12
@daosbuild1
Copy link
Collaborator

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

knard-intel
knard-intel approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@knard-intel knard-intel left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor remark which could be fixed in a follow-up PR if needed.

docs/admin/pool_operations.md Show resolved Hide resolved
@tanabarr
Copy link
Contributor Author

Jenkins CI failing on unrelated NLT valgrind errors, otherwise passing

kjacque
kjacque approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@kjacque kjacque left a comment

Choose a reason for hiding this comment

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

Minor suggestions, nothing blocking.

src/control/lib/control/pool.go
@@ -533,7 +533,7 @@ func poolQueryInt(ctx context.Context, rpcClient UnaryInvoker, req *PoolQueryReq
return resp, err
}

// UpdateState update the pool state.
// UpdateState update the pool state based on response field values.
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling

Suggested change
// UpdateState update the pool state based on response field values.
// UpdateState updates the pool state based on response field values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"update" I think is valid as it describes the functionality from the perspective of the function itself e.g. as a function I "update blah blah blah"

docs/admin/pool_operations.md
Comment on lines +594 to +595
by some storage targets while space is still available on others. Again
for the NVMe or DATA tier.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wording nit.

Suggested change
by some storage targets while space is still available on others. Again
for the NVMe or DATA tier.
by some storage targets while space is still available on others. Applies only
for the NVMe or DATA tier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update in a follow-on

docs/admin/pool_operations.md
- the total pool size
- the percentage of used space (i.e., 100 * used space / total space)
- the imbalance percentage indicating whether data distribution across
- The total pool size (NVMe or DATA tier, not including Metadata tier).
Copy link
Contributor

Choose a reason for hiding this comment

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

In the SCM-only case (admittedly a corner case at this point), nothing has changed, has it? May be worth calling out here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can update in a follow-on, the behaviour for PMem has changed because only NVMe tier is displayed in the default mode, as you have said nothing changes for SCM-only (and there is no equivalent supported mode for MD-on-SSD)

@tanabarr tanabarr requested a review from a team November 15, 2024 10:05
@tanabarr tanabarr added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Nov 15, 2024
@daltonbohning daltonbohning merged commit 3c96375 into master Nov 15, 2024
55 of 58 checks passed
@daltonbohning daltonbohning deleted the tanabarr/control-listpools-mdonssd branch November 15, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knard-intel knard-intel knard-intel approved these changes

@kjacque kjacque kjacque approved these changes

@mjmac mjmac Awaiting requested review from mjmac

Assignees

@tanabarr tanabarr

Labels
control-plane work on the management infrastructure of the DAOS Control Plane forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. meta-on-ssd Metadata on SSD Feature
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tanabarr @daosbuild1 @kjacque @knard-intel @daltonbohning