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

Fix: 002-test-reject-disk-with-lvm-signature.sh #240

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix: 002-test-reject-disk-with-lvm-signature.sh #240

wants to merge 1 commit into from

Conversation

shishir-a412ed
Copy link

Signed-off-by: Shishir Mahajan shishir.mahajan@redhat.com

@shishir-a412ed
Copy link
Author

Right now when I run test 002-test-reject-disk-with-lvm-signature.sh on upstream/master branch I see the following errors in the logs.

Running test ./002-test-reject-disk-with-lvm-signature.sh
  Physical volume "/dev/vdb" successfully created.
ERROR: Found LVM2_member signature on device /dev/vdb at offset 0x218. Wipe signatures using wipefs or use WIPE_SIGNATURES=true and retry.
  Volume group "css-test-foo" not found
  Cannot process volume group css-test-foo
  Please enter a physical volume path
  Run `pvremove --help' for more information.
Error: /dev/vdb: unrecognised disk label
/dev/vdb: 8 bytes were erased at offset 0x00000218 (LVM2_member): 4c 56 4d 32 20 30 30 31

This PR fixes that problem.

ping @rhvgoyal
PS: I will open a separate PR for test 102-test-reject-disk-with-lvm-signature.sh which has same issue.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 32ec977) made this pull request unmergeable. Please resolve the merge conflicts.

@rhvgoyal
Copy link
Collaborator

@shishir-a412ed can you rebase this PR on top of latest tree.

@shishir-a412ed
Copy link
Author

@rhvgoyal PTAL.

rhvgoyal
rhvgoyal reviewed Apr 12, 2017
View reviewed changes
tests/002-test-reject-disk-with-lvm-signature.sh Outdated

cleanup $vg_name "$devs"
cleanup_pvs $devs
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should continue to call cleanup(), right? In case there is a bug in tool and it has done something with device and added it to volume group etc, we need to clean all that state.

So, IMO, we need to call both cleanup() and cleanup_pvs().

Copy link
Author

Choose a reason for hiding this comment

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

For this particular test, we fail right away since there are signatures on the block device.
So ideally no volume group (VG) should exist. If I call both cleanup() and cleanup_pvs().

  1. I will see same errors in the logs like Cannot process volume group css-test-foo since volume group is not present.
  2. There will be duplication of code in cleanup() and cleanup_pvs() e.g both are calling wipe_signatures.

How I implemented was: cleanup_pvs() is a condensed version of cleanup() which is sufficient just for this test.

Shishir

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shishir-a412ed What if code is buggy and it does not recognize the signature on disk and continues to create partition and add it to volume group?

Copy link
Author

Choose a reason for hiding this comment

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

@rhvgoyal Then we have a much bigger problem than the test :)
Let me come to your desk.

rhvgoyal
rhvgoyal reviewed Apr 12, 2017
View reviewed changes
tests/002-test-reject-disk-with-lvm-signature.sh Outdated
return $test_status
}

cleanup_pvs() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

cleanup_pvs_sig()

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@rhvgoyal
Copy link
Collaborator

@shishir-a412ed Can you please take care of minor comments.

@shishir-a412ed
Copy link
Author

@rhvgoyal PTAL.

@rhvgoyal
Copy link
Collaborator

I think we can modify remove_pvs() to remove pvs both from partition and disk if pv exists. So we can implement a function pv_exists() and if pv exists, remove it. And then do this check both on partition as well device.

Same thing we can do for volume group. check for vg_exists() and if volume group exists, remove it. That will get rid of error.

@shishir-a412ed shishir-a412ed force-pushed the fix_test_002 branch 2 times, most recently from 0f306f0 to 206504a Compare April 19, 2017 21:01
@shishir-a412ed
Copy link
Author

@rhvgoyal PTAL.

rhvgoyal
rhvgoyal reviewed Apr 20, 2017
View reviewed changes
tests/libtest.sh Outdated
@@ -50,6 +50,19 @@ vg_exists() {
return 1
}

# Tests if the physical volume pv_name exists
pv_exists() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@shishir-a412ed I just implemented pv_exists() in libcss.sh. How about that implementation?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Fix: 002-test-reject-disk-with-lvm-signature.sh
919d4da 
Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
@shishir-a412ed
Copy link
Author

@rhvgoyal PTAL.

@rhatdan
Copy link
Member

rhatdan commented Jun 30, 2017

@rhvgoyal We should either close these pull requests or rework them or merge them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhvgoyal rhvgoyal rhvgoyal left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shishir-a412ed @rh-atomic-bot @rhvgoyal @rhatdan