Skip to content

HDDS-9450. Enhance om-leader-transfer.robot#5439

Merged
adoroszlai merged 5 commits intoapache:masterfrom
LZD-PratyushBhatt:HDDS-9450
Oct 17, 2023
Merged

HDDS-9450. Enhance om-leader-transfer.robot#5439
adoroszlai merged 5 commits intoapache:masterfrom
LZD-PratyushBhatt:HDDS-9450

Conversation

@LZD-PratyushBhatt
Copy link
Contributor

@LZD-PratyushBhatt LZD-PratyushBhatt commented Oct 13, 2023

What changes were proposed in this pull request?

Enhancing om-leader-transfer.robot by testing more cli options

What is the link to the Apache JIRA

HDDS-9450

How was this patch tested?

Modified the robot tests.

Passed CI: https://github.com/LZD-PratyushBhatt/ozone/actions/runs/6506536393/job/17673227311?pr=9

@LZD-PratyushBhatt
Copy link
Contributor Author

Passing in local.

❯ ../test-single.sh s3g omha/om-leader-transfer.robot
==============================================================================
Om-Leader-Transfer :: Smoketest for OM leader transfer                        
==============================================================================
Transfer Leadership for OM with Valid ServiceID Specified             | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, Valid Service... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, Unconfigured ... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, Invalid Servi... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM without ServiceID specified                | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, No ServiceID ... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Valid ServiceID Specified    | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, Vali... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, Unco... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, Inva... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly without ServiceID specified       | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, No S... | PASS |
------------------------------------------------------------------------------
Om-Leader-Transfer :: Smoketest for OM leader transfer                | PASS |
12 tests, 12 passed, 0 failed

@LZD-PratyushBhatt
Copy link
Contributor Author

Hi @adoroszlai can you please review this?
Thanks!

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @LZD-PratyushBhatt for working on this. Mostly looks good, some code improvements suggested.

@kerneltime
Copy link
Contributor

cc @tanvipenumudy

@kerneltime
Copy link
Contributor

cc @swamirishi

@LZD-PratyushBhatt
Copy link
Contributor Author

Pass results after the change:

Om-Leader-Transfer :: Smoketest for OM leader transfer                        
==============================================================================
Transfer Leadership for OM with Valid ServiceID Specified             | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, Valid Service... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, Unconfigured ... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, Invalid Servi... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM without ServiceID specified                | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM with Multiple ServiceIDs, No ServiceID ... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Valid ServiceID Specified    | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, Vali... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, Unco... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, Inva... | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly without ServiceID specified       | PASS |
------------------------------------------------------------------------------
Transfer Leadership for OM randomly with Multiple ServiceIDs, No S... | PASS |
------------------------------------------------------------------------------
Om-Leader-Transfer :: Smoketest for OM leader transfer                | PASS |
12 tests, 12 passed, 0 failed

Copy link
Contributor

@tanvipenumudy tanvipenumudy left a comment

Choose a reason for hiding this comment

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

Thank you @LZD-PratyushBhatt for the patch, the changes look good to me.

Previously, we encountered failures in the om-leader-transfer tests when writes preceded them, as discussed in PR #4844 comment.

NIT: Testing leader transfer commands with writes executed prior might be worthwhile (if possible).

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @LZD-PratyushBhatt for updating the patch. I have some ideas for future improvement ;), but we can merge this as is.

@adoroszlai adoroszlai merged commit f2584fb into apache:master Oct 17, 2023
@adoroszlai
Copy link
Contributor

Thanks @LZD-PratyushBhatt for the patch, @tanvipenumudy for the review.

ibrusentsev pushed a commit to ibrusentsev/ozone that referenced this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants