[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
2 times, most recently
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...
There was a problem hiding this comment.
I think this is how we are doing other tests with masks as well. We may need to have an overhaul of the tests to do it key based instead of value based. It easier to search for the value instead of the key.
| 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.
There was a problem hiding this comment.
This is an initial start to get the tests in. I am intending to evolve this moving forward. Having these is better than not having any tests at all. So, for now I am only doing cinder.conf and the primary configs at first, and will evolve these over time.
| 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. |
Initial tests created as a first phase, with more to add in the future Signed-off-by: Arif Ali <arif.ali@canonical.com>
arif-ali
force-pushed
the
sos-arif-openstack-tests
branch
from
7ac3312
to
05be0db
Compare
|
@MrStupnikov I've made some modifications to the tests, and actually found one key that wasn't already being obfuscated in |
|
@TurboTurtle @pmoravec @MrStupnikov any further feedback. I know all the keys are not there, or not all files, but that would be a gradual thing that I will do eventually, getting the first bits to help move this forward |
| self.teardown_mocked_packages() | ||
| self.teardown_mocked_files() | ||
| self.teardown_mocked_packages() |
There was a problem hiding this comment.
Just curious: why this change?
There was a problem hiding this comment.
When you mock both files and packages, and the packages have the same files; the tear_down_packages() will delete the file that was mocked, and hence the teardown_mocked_files() couldn't tear down and wouldn't be able to find the file. It was one of the issues I had faced while I was working on this particular PR
There was a problem hiding this comment.
Makes sense; thanks for explanation!
There was a problem hiding this comment.
The code LGTM, but I'm still not clear on why we're not doing an actual install in our CI like we do for Foreman. We're going through the effort of installing some release package, but still dropping manual files on the test host.
This seems a little bit of a conflict here - if we're installing a release package, why don't we execute a "normal" setup so that we don't have to drop the manual files, and are instead testing against what a real, actual install would look like after an installation (so that our test system would automatically reflect changes that go into the project)?
Are you suggesting that we do an all-in-one openstack deployment on a heavy machine/VM in GCE, and that has all the components and then do the tests against that? |
I guess? We do a similar thing with Foreman, where we take a larger GCE instance and do a scripted setup. Granted, OSP has a heavier footprint than Foreman so I'm not sure if that's feasible as a test bed. |
OK, makes sense, I have an idea to potentially use |
Good point. I am not against having OSP instances, but as a kind of assesment, we should answer below questions: How big such instance can be (compared to Foreman one)? How much it would increase cost of testing / payment for GCE (just rough estimate, if possible)? How much it would slow down the whole testing (probably not much) - esp. relevant to "what all changes in PR can trigger testing on the OSP instance" question (cf. https://github.com/sosreport/sos/blob/main/.cirrus.yml#L317 as an example)? |
I've closed this particular issue, as the new method will look completely different. I tried I've tried running sosreport on that, as is, it won't trigger as the packages wouldn't have installed. The config files are in the right place, so we can assert files collected and check the obfuscation. Alternatively, I can try Ubuntu doesn't have an all-in-one package per say; and even then it would be sunbeam; and that would require a high mem VM in GCE and would take some time to automate; but that would not replicate the same thing based on Canonical OpenStack right now. The only other way to do this is, is to manually deploy everything with all the configs using a script; and that would require a significant development time. If this is the preferred route, then we keep this PR closed; and I will work on an alternative, which may take a few months to get done |
|
Afaik we mostly use 4GB machines ( Manual deply would require a lot of.. well, manual work, better to use either approach from this PR or an image one. Either that sounds good to me. |
Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here * Start testing kubernetes as well, initial tests for microk8s, more to follow Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
* Move the juju tests now to sunbeam, now that we deploy using juju here, no need to mock the configuration files. * Include plugins that are associated here - kubernetes - microk8s - ovn_host - juju Related: sosreport#3622 Signed-off-by: Arif Ali <arif.ali@canonical.com>
Inititally doing for the most common openstack services
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines