[tests] Add avocado tests for openstack plugins #3622
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
mock teardown is potentially removing |
arif-ali
force-pushed
the
sos-arif-openstack-tests
branch
from
967f9f1
to
76d140e
Compare
ordering of the files. if |
| class OpenstackPluginTest(StageOneReportTest): | ||
| """Basic sanity check to make sure common config files are collected | ||
|
|
||
| :avocado: tags=stageone | ||
| """ | ||
|
|
||
| sos_cmd = ('-o openstack_cinder,openstack_keystone,openstack_glance,' | ||
| 'openstack_neutron,openstack_nova') |
There was a problem hiding this comment.
Would it be better to mark this as a StageTwo test and have it temporarily install a set of openstack packages? When I was first looking at OSP testing at RH, I was looking at doing a base install like we do for Foreman (i.e. the openstack tests would become their own entity under product_tests and report a new CI job) but there were some hurdles to that on single node that I'm not fully recallling at the moment.
In lieu of that, can we temporarily install some openstack packages to get some more confidence in the test? As it is, won't all of these tests pass simply due to them being missing on the test VMs?
There was a problem hiding this comment.
I hear you, and was thinking the same, at one point.
One of the main purposes to add these as is, is to automate some the testing we at Canonical were doing at deb release time, so that we can assert them and ensure we are collecting them all automatically and spend less time collecting the data.
Happy to amend them to StageTwo, if it helps to do it here for RH OSP and Canonical OS
There was a problem hiding this comment.
I this kind of points out the discussion point from #3611
If we are doing this in stagetwo, the file will exist, hence we should have a hard assert function. This way we know 100% that we are collecting it, wdyt?
There was a problem hiding this comment.
Yeah, that's why I said I'd be onboard with a hard assert for assertFileCollected() and a renaming of the current method so that we have both "we absolutely 100% have this file" and "we are validating the behavior of add_copy_spec()".
I'd say let's add the temporary package install for these openstack packages (I'm assuming here that the initial files get laid down by the packages, and not by some user-executed configuration script) and mark this as StageTwo.
There was a problem hiding this comment.
Cool, no worries, I'll get this re factored and tested
arif-ali
force-pushed
the
sos-arif-openstack-tests
branch
from
76d140e
to
95cb63e
Compare
|
@TurboTurtle the ubuntu bits was actually quite easy, but centos was a real pain, as you have to install extra package for the openstack repo, and then enable crb repo for glance I've put my ideas (it has issues), but wanted to see if everyone is on-board with the way the stagetwo is being done here |
arif-ali
force-pushed
the
sos-arif-openstack-tests
branch
4 times, most recently
from
ae5cce6
to
e0cfc73
Compare
|
we should teardown the files before packages. If we tear down package before files, then the file will not exist, and fail. Also ensure we collect This should now work for ubuntu distros, will need extra expertise for the RH bits |
|
While testing on centos9, I was able to overcome the issues represented here, and the package |
|
Using the same image in Cirrus, did a quick deploy in my GCE account, and I don't have the same issue as the CI |
arif-ali
force-pushed
the
sos-arif-openstack-tests
branch
from
e0cfc73
to
23552b0
Compare
|
After some debugging last night, I was able to get centos stream 9 to work. Centos Stream 8 has python 3.6 and avocado-framework 103.0 isn't supported and hence the Need to work out a different way to test centos stream 8 for this stagetwo test |
| '/etc/keystone/keystone.conf', '2zx9jZZtxdn4grG3xcMV4PwgGwY7X7fP') | ||
|
|
||
| def test_keystone_ldap_conf_scrubbed(self): | ||
|
|
There was a problem hiding this comment.
Nitpick: redundant empty line?
There was a problem hiding this comment.
Yup, there's probably a few of these, will clean up once I have a full set of tests passing
| if this_distro.version == "9": | ||
| self.osp_release = "yoga" | ||
| if this_distro.version == "8": | ||
| self.osp_release = "victoria" |
There was a problem hiding this comment.
Knowing OSP a little only, but why do we test on unmaintained releases? (https://releases.openstack.org/)
EDIT, per https://docs.openstack.org/install-guide/environment-packages-rdo.html#operating-system , Ussuri to Yoga are compatible with CentOS8 and Xena and newer with CentOS9.
There was a problem hiding this comment.
Thanks for the link, just made it consistent to be yoga, and enable the right repos for 8/9 and got it through, I think
I wanted to comment out on this topic (why the centos-8 fails to install OSP packages) but I see you are ahead of me here :). |
arif-ali
force-pushed
the
sos-arif-openstack-tests
branch
from
23552b0
to
d683bd2
Compare
Inititally doing for the most common ones Signed-off-by: Arif Ali <arif.ali@canonical.com>
arif-ali
force-pushed
the
sos-arif-openstack-tests
branch
from
d683bd2
to
7ac3312
Compare
| def test_cinder_conf_collected_and_scrubbed(self): | ||
| self.assertFileCollected('/etc/cinder/cinder.conf') | ||
|
|
||
| keys_to_mask = [ |
There was a problem hiding this comment.
We have a list of protect_keys defined for openstack_cinder plugin. I don't get the idea of testing only subset of them.
| self.assertFileCollected('/etc/cinder/cinder.conf') | ||
|
|
||
| keys_to_mask = [ | ||
| 'cmB4zBYq3VWFMNqNKFLcqS5Zq8ystLLsTd5BFLbCtX67qShnhgHFxxRFjkhbY54x', |
There was a problem hiding this comment.
Let's imaging a situation when one of specified keys will not be masked and error will be raised. How an engineer should troubleshoot this? Copy key from output, open test configuration file, fine hash, copy parameter and start looking why it is no longer masked? IMO this looks a bit suboptimal.
I also don't like the idea of using static list of parameters to mask in tests separately from list of parameters to mask in sosreport itself...
| self.assertFileNotHasContent( | ||
| '/etc/cinder/cinder.conf', key) | ||
|
|
||
| def test_glance_conf_collected_and_scrubbed(self): |
There was a problem hiding this comment.
Same comment as for cinder: we only test subset if parameters and are not doing this optimally.
| def test_cinder_conf_collected_and_scrubbed(self): | ||
| self.assertFileCollected('/etc/cinder/cinder.conf') | ||
|
|
||
| keys_to_mask = [ |
There was a problem hiding this comment.
BTW, technically we are checking if values were masked, not keys.
| self.assertFileNotHasContent('/etc/glance/glance-api.conf', key) | ||
|
|
||
| def test_heat_conf_collected(self): | ||
| self.assertFileCollected('/etc/glance/heat.conf') |
There was a problem hiding this comment.
Config file path looks odd to me. Why heat.conf is in /etc/glance folder?
| self.assertFileCollected('/etc/keystone/keystone.conf') | ||
| self.assertFileCollected('/etc/keystone/keystone.policy.yaml') | ||
|
|
||
| self.assertFileNotHasContent( |
There was a problem hiding this comment.
Same as with cinder, there are many more protected keys in keystone.conf
| for key in keys_to_mask: | ||
| self.assertFileNotHasContent('/etc/glance/glance-api.conf', key) | ||
|
|
||
| def test_heat_conf_collected(self): |
There was a problem hiding this comment.
Why we are not checking if data was sanitized for Heat?
| self.assertFileNotCollected( | ||
| '/etc/neutron/plugins/ml2/neutron-api-plugin-ovn.crt') | ||
|
|
||
| def test_neutron_conf_collected_and_scrubbed(self): |
| for key in keys_to_mask: | ||
| self.assertFileNotHasContent('/etc/neutron/neutron.conf', key) | ||
|
|
||
| def test_nova_conf_collected_and_scrubbed(self): |
| ] | ||
| for key in keys_to_mask: | ||
| self.assertFileNotHasContent( | ||
| '/etc/cinder/cinder.conf', key) |
There was a problem hiding this comment.
The logic of sos is different here: we sanitize all collected configuration files, not only cinder.conf. This is not changing anything for this specific implementation, but fundamentally implemented tests are used to check specific file, while sos sanitize all collected config.
|
@MrStupnikov thanks for your constructive and detailed feedback, it's very much appreciative
Now that I have overcome the challenges of getting both centos and ubuntu in the same set of tests, I can start adding all the items that are brought forward to me, as well as some that I will be going through personally, and will get these through. Your feedback will be extremely valuable as I go through this process. |
Inititally doing for the most common openstack services
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines