Skip to content

DAOS-19589 tests: Fix dmg_negative_test extend cases #19106

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

Open
liw wants to merge 1 commit into
base: master
Choose a base branch
from
+2 −2
Open

DAOS-19589 tests: Fix dmg_negative_test extend cases #19106

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/tests/ftest/osa/dmg_negative_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ def run_osa_dmg_test(self, num_pool, extend=False):
expected_result = str(sequence[2])

# Extend the pool
# There is no need to extend rank 0
# There is no need to extend rank 0-3

@shimizukko shimizukko Sep 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# There is no need to extend rank 0-3
# There is no need to extend rank 0-3 and 7

# Avoid DER_ALREADY
if extend is True and rank != "0":
if extend is True and rank in ["4", "5"]:

@daltonbohning daltonbohning Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is existing, so I don't expect this PR to fix it, but I think part of the problem with this logic is that the ranks are hardcoded. IMO it would be more robust to query the pool and extend only if the ranks are not already in the pool. I think reading such logic would make the behavior of the test more clear. But I will leave that for future enhancement.

@liw liw Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. This test has confused me quite a bit and taken several iterations to fix properly. On top of that the value seems moderate.

@rpadma2 rpadma2 Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am bit lost why this change is needed? The test is simple....

It takes the following test sequence and runs them in the loop based on whether extend needs to be done or not...

dmg_cmd_test:
# Rank list, target_list, Pass/Fail Condition
# Invalid Rank list
- ["7", "0,1", "Fail"]
# Invalid Target list
- ["1", "0,10", "Fail"]
- ["2", "10", "Fail"]
- ["3", "10", "Fail"]
# Invalid rank, target
- ["7", "11", "Fail"]
# Valid Rank and Target
- ["0", "1,2", "Pass"]
- ["4", "0,2", "Pass"]
- ["5", "1,2", "Pass"]

If some can explain me what is wrong with the original code, I can try to understand...

Basically it looks like the for loop takes each rows and uses the first value as a rank and try to run the dmg pool command, some ranks are invalid and some are valid...
For the following row is skipped ["0", "1,2", "Pass"] based on this check ---> if extend is True and rank != "0":

@rpadma2 rpadma2 Sep 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NOTE: This is a negative test... We are trying to pass invalid and some valid ranks... The only Valid rank test cases are related to ranks=0,4,5.

@rpadma2 rpadma2 Sep 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please provide me the Jenkins failure for the test which we are trying to address. To me, there is no need to change this test... I maybe wrong. But, I would like to review the failure before approving this PR.

@shimizukko shimizukko Sep 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this change is needed either. validate_results can handle failed dmg pool extend (non-zero exit status) if you pass in Fail to exp_result.

I'm guessing there's a reason that we don't want to extend rank 1, 2, 3, and 7, but we want to continue testing the rest of the commands with those ranks. If so, we should explain that somewhere.

self.log.info("=> Sequence %s: Extend rank %s onto pool %s", index, rank, pool)
if not self.validate_results(pool, expected_result, pool.extend(rank)):
self.fail("Error extending rank {} onto pool {}".format(rank, str(pool)))
Expand Down
Loading