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

Implement the tests for the cinder-lvm charm #626

Merged
merged 5 commits into from Sep 29, 2021
Merged

Conversation

lmlg
Copy link
Contributor

@lmlg lmlg commented Aug 25, 2021

The cinder-lvm charm is a subordinate of the cinder charm. The spec
can be found here:
https://github.com/openstack/charm-specs/blob/master/specs/xena/backlog/cinder-lvm.rst

This brings the changes from the 'cinder-lvm' repo itself, from where
this class will be removed.
zaza/openstack/charm_tests/cinder_lvm/tests.py Outdated Show resolved Hide resolved
zaza/openstack/charm_tests/cinder_lvm/tests.py Outdated Show resolved Hide resolved
"""Test creating a volume after remove missing ones in a group."""
self._create_volume()

@with_conf('cinder-lvm', {'remove-missing-force': 'true'})
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this under the hood should end up calling vgreduce --removemissing ..., I think you may need to setup a scenario where there are VGs referencing missing PVs so we are certain that under those circumstances it's possible to create volumes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@freyes as discussed a couple days ago, cinder makes it hard to actually test this in a clean manner, since only a single volume name can be configured as opposed to a comma-separated list. We could, of course, manually create them in the tests, but it will end up polluting the test environment, since all the LVM stuff are global settings and can't be isolated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good test to have. Yes, it will manipulate the test environment - but that can be done as the last test to run so it doesn't inadvertently pollute other tests that are exercised. However, I think I'm comfortable with this being a subsequent improvement to the cinder-lvm tests as long as @lmlg is willing to commit to do those in a follow-on patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can absolutely commit them in a follow up PR.

Instead of introducing a new decorator to temporarily change the
configuration, use the existing infrastructure. Furthermore, avoid
passing the model name in some calls since it's implied already.
Instead of using '/dev/vdc' for a test, use a loop device, since
it should always be available and it allows us to set a specific
size for it.
@wolsen wolsen merged commit a9d233d into master Sep 29, 2021
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.

None yet

3 participants