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

Skip ha/drbd_passive test when not in TWO_NODES scenario #15324

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

alvarocarvajald
Copy link
Contributor

@alvarocarvajald alvarocarvajald commented Aug 4, 2022

Currently the 3 cluster node scenario tested for Maintenance jobs is always scheduling the ha/drbd_passive test module, even when this particular test module is not designed to work in cluster scenarios with more than 2 nodes.

As a result, whenever a MU job is triggered with a package that requires a drbd test, the 3 node scenario job will fail as it is skipped in the third node before any of the barrier_wait() calls. This means the other nodes, remain blocked in a barrier_wait() call until they reach MAX_JOB_TIME and fail the whole scenario.

One possible solution would be to create a different schedule for the 3 cluster node scenario, but this implies a PR to this repo, as well as a change to the job settings.

This PR instead modifies ha/drbd_passive so it is skipped when running in scenarios with the setting TWO_NODES=no. It will also skip the test module ha/filesystem that is usually scheduled right after ha/drbd_passive, as there would be no block device on which to test the filesystem creation.

P.S.: I also include here a small optimization in ha/vg test module to avoid having multiple calls to hacluster::read_tag which sends commands to SUT.

@emiura
Copy link
Contributor

emiura commented Aug 5, 2022

One possible solution would be to create a different schedule for the 3 cluster node scenario, but this implies a PR to this repo, as well as a change to the job settings. - don't we need a PR to add the "TWO_NODES=no" anyway? Anyway, if it passes your vr, lgtm.

@alvarocarvajald
Copy link
Contributor Author

One possible solution would be to create a different schedule for the 3 cluster node scenario, but this implies a PR to this repo, as well as a change to the job settings. - don't we need a PR to add the "TWO_NODES=no" anyway? Anyway, if it passes your vr, lgtm.

It is already there:

If memory serves, TWO_NODE=no is used by other test modules and added quite a while ago. I am just simply re-using the same setting for this particular test module.

In fact, I think L47 on tests/ha/drbd_passive.pm which reads:

    if ((!is_node(1) && !is_node(2)) || check_var('TWO_NODES', 'no')) {

Could be re-written safely as only:

    if (check_var('TWO_NODES', 'no')) {

But I am leaving the node checks just in case there is already a working test using ha/drbd_passive with more than 2 nodes and that has not set TWO_NODES=no.

$resource = 'drbd_active';
}
elsif ($tag eq 'skip_fs_test') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this duplicate of line 21 ?
And if it should be at begging because tag drbd_passive would be evaluated before this skip thus not skip.

Copy link
Contributor Author

@alvarocarvajald alvarocarvajald Aug 9, 2022

Choose a reason for hiding this comment

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

Isn't this duplicate of line 21 ?

Not exactly the same. L21 would skip the test when not on drbd Maintenance Updates, but continue it if testing a drbd MU, such as in https://openqa.suse.de/tests/9247409#step/drbd_passive/5

L44 on the other hand is skipping the test on scenarios where the test would not work (such as in 3 node) per L47-L51 in ha/drbd_passive.

And if it should be at begging because tag drbd_passive would be evaluated before this skip thus not skip.

Not a problem. $tag would be one or the other, and never both: https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/lib/hacluster.pm#L458-L480

(This probably could've been implemented with get_var() and set_var() instead, but no idea why Loic went with a local file in the cluster nodes for these tags. Could be a candidate for a re-write in the future)

Copy link
Contributor

Choose a reason for hiding this comment

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

But instead of two places where the test will return there could be just one at the beginning of the if statement. Otherwise fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it. Let me move both exit conditions to the same line and test.

Copy link
Contributor Author

@alvarocarvajald alvarocarvajald Aug 9, 2022

Choose a reason for hiding this comment

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

New verification runs:

Cannot run VR with the drbd MU as the incident repository is gone: http://mango.qa.suse.de/tests/4893#step/iscsi_client/12

Should we merge with the new change or do I rollback 7997ebe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Merge this PR, you are writing skip_fs_test tag in first commit

@alvarocarvajald alvarocarvajald changed the title Skip ha/drbd_passive test when not in TWO_NODE scenario Skip ha/drbd_passive test when not in TWO_NODES scenario Aug 9, 2022
Also skip related ha/filesystem test as if ha/drbd_passive test module
is skipped, there would be no block device on which to create the FS.
@alvarocarvajald alvarocarvajald merged commit 6663dfc into os-autoinst:master Aug 10, 2022
@alvarocarvajald alvarocarvajald deleted the drbd-on-3nodes branch August 10, 2022 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants