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

Add new ceph-fs configuration test #774

Merged
merged 4 commits into from Jun 3, 2022

Conversation

ethanmye-rs
Copy link
Contributor

This PR adds functional tests for the charm-ceph-fs config options added in the following review:
https://review.opendev.org/c/openstack/charm-ceph-fs/+/842555

@ChrisMacNaughton
Copy link
Collaborator

This isn't really a functional test; it's purely a test that config was written to a file, without any verification that Ceph even cares about that config line. Broadly, the desired approach for functional testing is to verify that the things you're trying to do actually causes the desired effect; as a basic example: testing that a CephFS share can be created, written to, and read from is infinitely more valuable than testing that we wrote the config we think we did.

It's possible to get the same confidence int his change via unit-testing in the charm itself, rather than cating the ceph.conf to ensure that the expected changes appeared.

@ethanmye-rs
Copy link
Contributor Author

This isn't really a functional test; it's purely a test that config was written to a file, without any verification that Ceph even cares about that config line.

In my experience, Ceph ignores invalid config options in the .conf file, but that is anecdotal. From our earlier discussion, I agree querying the Ceph daemon directly is a better test to ensure Ceph actually understood and accepted the config values passed.

It's possible to get the same confidence int his change via unit-testing in the charm itself, rather than cating the ceph.conf to ensure that the expected changes appeared.

I've left the unit tests as they are in the opendev repo and updated this repo with the 'functional' tests that ask Ceph for the configuration directly. Thanks for the help!

Copy link
Collaborator

@ChrisMacNaughton ChrisMacNaughton left a comment

Choose a reason for hiding this comment

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

I just have one issue with the tests now, it looks like a debug print was left.

zaza/openstack/charm_tests/ceph/fs/tests.py Outdated Show resolved Hide resolved
openstack-mirroring pushed a commit to openstack/charm-ceph-fs that referenced this pull request Jun 3, 2022
Closes-Bug: #1891409
Func-Test-PR: openstack-charmers/zaza-openstack-tests#774
Change-Id: If2bdd5c0f2afa1843e686cf69570a50901c85875
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Jun 3, 2022
* Update charm-ceph-fs from branch 'master'
  to 8f31a33a0fd032b079f76abb20f68c4e71e3f50c
  - Enable charm to configure mds cache options.
    
    Closes-Bug: #1891409
    Func-Test-PR: openstack-charmers/zaza-openstack-tests#774
    Change-Id: If2bdd5c0f2afa1843e686cf69570a50901c85875
@ChrisMacNaughton ChrisMacNaughton merged commit 5ad8ba1 into openstack-charmers:master Jun 3, 2022
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