Conversation
29a4b38 to
aff0305
Compare
|
Ticket title is 'osa/dmg_negative_test.py:OSADmgNegativeTest.test_osa_dmg_cmd_without_extend- pool reintegration rank is stating OK without pool exclude the rank ' |
|
Test stage Functional Cluster Box Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19106/2/testReport/ |
aff0305 to
4f899b4
Compare
liw
changed the title
|
Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-19106/2/execution/node/1837/log |
|
Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-19106/3/execution/node/1059/log |
The logic of run_osa_dmg_test when extend is True seems strange:
- It avoids extending the pool to rank 0, but treats the other ranks
in the pool differently.
- It uses the expected results which sometimes are based on invalid
targets, even though the extend command does not take targets
parameter.
This might have led to the extend command returning an error when given
a rank that is already in the pool, while the exclude, drain, and
reintegrate commands all succeed when given ranks that is already
excluded, drained, and reintegrated, respectively. #18651 and #19026
have changed the extend case to succeed. This patch fixes
dmg_negative_test to match the new behavior.
Test-tag: OSADmgNegativeTest
Signed-off-by: Li Wei <liwei@hpe.com>
4f899b4 to
0504206
Compare
| # There is no need to extend rank 0-3 | ||
| # Avoid DER_ALREADY | ||
| if extend is True and rank != "0": | ||
| if extend is True and rank in ["4", "5"]: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. This test has confused me quite a bit and taken several iterations to fix properly. On top of that the value seems moderate.
There was a problem hiding this comment.
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":
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I consider this ready to land as soon as there's a second approval. |
| # There is no need to extend rank 0-3 | ||
| # Avoid DER_ALREADY | ||
| if extend is True and rank != "0": | ||
| if extend is True and rank in ["4", "5"]: |
There was a problem hiding this comment.
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.
| # There is no need to extend rank 0-3 | ||
| # Avoid DER_ALREADY | ||
| if extend is True and rank != "0": | ||
| if extend is True and rank in ["4", "5"]: |
There was a problem hiding this comment.
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.
|
|
||
| # Extend the pool | ||
| # There is no need to extend rank 0 | ||
| # There is no need to extend rank 0-3 |
There was a problem hiding this comment.
| # There is no need to extend rank 0-3 | |
| # There is no need to extend rank 0-3 and 7 |
5 participants
The logic of run_osa_dmg_test when extend is True seems strange:
It avoids extending the pool to rank 0, but treats the other ranks
in the pool differently.
It uses the expected results which sometimes are based on invalid
targets, even though the extend command does not take targets
parameter.
This might have led to the extend command returning an error when given
a rank that is already in the pool, while the exclude, drain, and
reintegrate commands all succeed when given ranks that is already
excluded, drained, and reintegrated, respectively. #18651 and #19026
have changed the extend case to succeed. This patch fixes
dmg_negative_test to match the new behavior.
Test-tag: OSADmgNegativeTest
Steps for the author:
After all prior steps are complete: