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

Make Ceph tests self-contained. #1177

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

lmlg
Copy link
Contributor

@lmlg lmlg commented Jan 2, 2024

This PR implements the needed changes so that the Ceph charms can be run without a full Openstack environment. In particular, the RBD mirror charm needed tweaking so as to not depend on Cinder, using pools of its own creating, and skipping some tests when needed.

lmlg added 2 commits January 2, 2024 19:26
This PR implements the needed changes so that the Ceph charms
can be run without a full Openstack environment. In particular,
the RBD mirror charm needed tweaking so as to not depend on
Cinder, using pools of its own creating.
Copy link
Collaborator

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

Please see my inline comments.

zaza/openstack/charm_tests/ceph/rbd_mirror/tests.py Outdated Show resolved Hide resolved
zaza/openstack/charm_tests/ceph/rbd_mirror/tests.py Outdated Show resolved Hide resolved
zaza/openstack/charm_tests/ceph/rbd_mirror/tests.py Outdated Show resolved Hide resolved
Comment on lines 189 to 190
zaza.model.get_application(cls.cinder_ceph_app_name)
cls.with_cinder = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is caching whether the cinder-ceph application is deployed in the model at the beginning of the tests. As caching is one of the hard problems, perhaps what's really needed is a predicate function that can be called at each test.

e.g. if some future test decided to test what happens when cinder-ceph is removed from the model (I don't why!) then this caching would break. I think we should all develop more a mindset around avoiding caching where possible unless it really is a performance or memory issue.

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 understand where you're coming from. In the context of this PR, this caching is only looked at the beginning of a couple of tests to skip them in an application is not present. It's pretty harmless in my opinion, but I can accommodate further changes. For example, I can document how it's supposed to be used or replace the caching for a call that looks for the application on the model on each run. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think documenting it in the code would provide enough sign-posting if you want to keep the cached value; however, if you can avoid caching altogether then it would make the code more robust from a maintenance perspective. If there are performance issues, or other undesirable side-effects, then caching is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, there's no longer caching.

Copy link
Collaborator

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

LGTM; thanks for the changes and simplifying the code.

@ajkavanagh ajkavanagh merged commit a276331 into openstack-charmers:master Jan 8, 2024
3 checks passed
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.

2 participants