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.

Sign up for GitHub

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

Remove hardcoded nova-compute app name and add ceph keys test #1141

Closed
wants to merge 3 commits into from
Closed

Remove hardcoded nova-compute app name and add ceph keys test #1141

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d7d884
[WIP] Remove hardcoded nova-compute app name and add ceph keys test
rodrigogansobarbieri Sep 28, 2023
a155736
review feedback
rodrigogansobarbieri Oct 3, 2023
76a2b81
added unit test
rodrigogansobarbieri Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion zaza/openstack/charm_tests/ceph/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,7 +1285,8 @@ def test_check_pool_types(self):
"""Check type of pools created for clients."""
app_pools = [
('glance', 'glance'),
('nova-compute', 'nova'),
(zaza_openstack.get_app_names_for_charm('nova-compute')[0],
'nova'),
('cinder-ceph', 'cinder-ceph')]
runtime_pool_details = zaza_ceph.get_ceph_pool_details()
for app, pool_name in app_pools:
Expand Down
119 changes: 88 additions & 31 deletions zaza/openstack/charm_tests/nova/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import json
import logging
import os
import re
import tempfile
import tenacity
import unittest
Expand Down Expand Up @@ -127,7 +128,9 @@ def check_tpm(stdin, stdout, stderr):
password=password, privkey=privkey,
verify=check_tpm)

@test_utils.skipUntilVersion('nova-compute', 'nova-common', '3:23.0.0')
@test_utils.skipUntilVersion(
openstack_utils.get_app_names_for_charm('nova-compute')[0],
'nova-common', '3:23.0.0')
def test_launch_vtpm_1_2_instance(self):
"""Launch an instance using TPM 1.2."""
self.RESOURCE_PREFIX = 'zaza-nova'
Expand All @@ -138,7 +141,9 @@ def test_launch_vtpm_1_2_instance(self):
# Note: TPM 1.2 presents tpm0 as a device
self._check_tpm_device(instance, '/dev/tpm0')

@test_utils.skipUntilVersion('nova-compute', 'nova-common', '3:23.0.0')
@test_utils.skipUntilVersion(
openstack_utils.get_app_names_for_charm('nova-compute')[0],
'nova-common', '3:23.0.0')
def test_launch_vtpm_2_instance(self):
"""Launch an instance using TPM 2.0."""
self.RESOURCE_PREFIX = 'zaza-nova'
Expand Down Expand Up @@ -265,8 +270,9 @@ def fetch_nova_service_hostname(self, unit_name):

def test_940_enable_disable_actions(self):
"""Test disable/enable actions on nova-compute units."""
nova_units = zaza.model.get_units('nova-compute',
model_name=self.model_name)
nova_units = zaza.model.get_units(
self.application_name,
model_name=self.model_name)

# Check that nova-compute services are enabled before testing
for service in self.nova_client.services.list(binary='nova-compute'):
Expand Down Expand Up @@ -307,8 +313,9 @@ def check_instance_count(expect_count, unit_name):
instances = result.data.get('results', {}).get('instance-count')
self.assertEqual(instances, str(expect_count))

nova_unit = zaza.model.get_units('nova-compute',
model_name=self.model_name)[0]
nova_unit = zaza.model.get_units(
self.application_name,
model_name=self.model_name)[0]

check_instance_count(0, nova_unit.entity_id)

Expand Down Expand Up @@ -345,8 +352,9 @@ def wait_for_nova_compute_count(expected_count):
sleep_timeout = 10
return False

all_units = zaza.model.get_units('nova-compute',
model_name=self.model_name)
all_units = zaza.model.get_units(
self.application_name,
model_name=self.model_name)

unit_to_remove = all_units[0]

Expand All @@ -372,7 +380,7 @@ def wait_for_nova_compute_count(expected_count):

# Wait for nova-compute service to be removed from the
# nova-cloud-controller
if not wait_for_nova_compute_count(0):
if not wait_for_nova_compute_count(service_count-1):
Copy link

Choose a reason for hiding this comment

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

Based on the (hidden) code above this, service_count must either be 0 or 1, so this change and below aren't actually necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

also need whitespace around the '-'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is "future code", removing hardcoded values without changing the behavior, and keeping it ready to support more than 1 unit

self.fail("nova-compute service was not unregistered from the "
"nova-cloud-controller as expected.")

Expand All @@ -382,7 +390,7 @@ def wait_for_nova_compute_count(expected_count):
'register-to-cloud',
raise_on_failure=True)

if not wait_for_nova_compute_count(1):
if not wait_for_nova_compute_count(service_count):
self.fail("nova-compute service was not re-registered to the "
"nova-cloud-controller as expected.")

Expand All @@ -398,12 +406,14 @@ def test_311_pci_alias_config_compute(self):
"""
# We are not touching the behavior of anything older than QUEENS
if self.current_release >= self.XENIAL_QUEENS:
self._test_pci_alias_config("nova-compute", ['nova-compute'])
self._test_pci_alias_config(
self.application_name, ['nova-compute'])

def test_500_hugepagereport_action(self):
"""Test hugepagereport action."""
for unit in zaza.model.get_units('nova-compute',
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
model_name=self.model_name):
logging.info('Running `hugepagereport` action'
' on unit {}'.format(unit.entity_id))
action = zaza.model.run_action(
Expand Down Expand Up @@ -446,12 +456,13 @@ def test_920_change_aa_profile(self):
logging.info(
'Waiting for services ({}) to be restarted'.format(services))
zaza.model.block_until_services_restarted(
'nova-compute',
self.application_name,
mtime,
services,
model_name=self.model_name)
for unit in zaza.model.get_units('nova-compute',
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
model_name=self.model_name):
logging.info('Checking number of profiles in complain '
'mode in {}'.format(unit.entity_id))
run = zaza.model.run_on_unit(
Expand All @@ -470,10 +481,50 @@ def test_901_pause_resume(self):
with self.pause_resume(['nova-compute']):
logging.info("Testing pause resume")

def test_904_test_ceph_keys(self):
"""Run pause and resume tests.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

docstring needs to be a proper one instead of this copy-pasted crap

Pause service and check services are stopped then resume and check
they are started
"""
# Expected default and alternate values
if zaza.model.get_application_config(
self.application_name)['libvirt-image-backend'].get(
'value') != 'rbd':
return

regex = re.compile(r"^\[client.(.+)\]\n\tkey = (.+)$")
key_dict = {}

def check_keyring(app_name_dict_list):
for app_name_dict in app_name_dict_list:
for app_name, key_list in app_name_dict.items():
for unit in zaza.model.get_units(
app_name, model_name=self.model_name):
for key_app_name in key_list:
keyring_file = (
'/etc/ceph/ceph.client.{}.keyring'.format(
key_app_name))
data = str(generic_utils.get_file_contents(
unit, keyring_file))

result = regex.findall(data)
self.assertEqual(2, len(result))
Copy link

Choose a reason for hiding this comment

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

nit: might be more obvious if you pull the key/value out here:

app_name, app_value = result

But just a nit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would raise value error if the length is different. The assert validation is less imprecise as an error message.

self.assertEqual(result[0], key_app_name)
for k, v in key_dict.items():
if k == result[0]:
self.assertEqual(v, result[1])
else:
self.assertNotEqual(v, result[1])
key_dict[result[0]] = result[1]

check_keyring([{self.application_name: [self.application_name]}])
Copy link
Contributor Author

@rodrigogansobarbieri rodrigogansobarbieri Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this was coded for stable/yoga where it does not have the partial fix that is on master. This needs to be adjusted to be able to run against the code on master.

Copy link

Choose a reason for hiding this comment

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

So I was going to ask about this, is the typical argument just a list of dictionaries like:

[{'nova-compute': ['nova-compute']}]

And then L500 above is just iterating through that? Seems it could be simplified if so.

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 rewrote that logic in the new version I am going to push soon


def test_930_check_virsh_default_network(self):
"""Test default virt network is not present."""
for unit in zaza.model.get_units('nova-compute',
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
model_name=self.model_name):
logging.info('Checking default network is absent on '
'unit {}'.format(unit.entity_id))
run = zaza.model.run_on_unit(
Expand All @@ -492,8 +543,9 @@ class NovaComputeActionTest(test_utils.OpenStackBaseTest):

def test_virsh_audit_action(self):
"""Test virsh-audit action."""
for unit in zaza.model.get_units('nova-compute',
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure that self.application_name is being set to the name of the application in the same way as get_app_names_for_charm()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is somewhat similar. The base class code that assigns self.application_name checks if each app starts with the charm_name declared in tests.yaml, and it breaks when it finds the first one. We could remove the break and go on creating a "self.application_nameS" variable and we ultimately have the same as get_app_names_for_charm. Ultimately I'd prefer if it uses "in (contains)" instead of startswith though.

model_name=self.model_name):
logging.info('Running `virsh-audit` action'
' on unit {}'.format(unit.entity_id))
action = zaza.model.run_action(
Expand All @@ -520,8 +572,9 @@ def test_vgpu_in_nova_conf(self):
vgpu-device-mappings has been set to something not empty like
"{'nvidia-108': ['0000:c1:00.0']}".
"""
for unit in zaza.model.get_units('nova-compute',
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
model_name=self.model_name):
nova_conf_file = '/etc/nova/nova.conf'
nova_conf = str(generic_utils.get_file_contents(unit,
nova_conf_file))
Expand Down Expand Up @@ -775,12 +828,14 @@ class NovaCloudControllerActionTest(test_utils.OpenStackBaseTest):
def test_sync_compute_az_action(self):
"""Test sync-compute-availability-zones action."""
juju_units_az_map = {}
compute_config = zaza.model.get_application_config('nova-compute')
compute_config = zaza.model.get_application_config(
self.application_name)
default_az = compute_config['default-availability-zone']['value']
use_juju_az = compute_config['customize-failure-domain']['value']

for unit in zaza.model.get_units('nova-compute',
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
model_name=self.model_name):
zone = default_az
if use_juju_az:
result = zaza.model.run_on_unit(unit.name,
Expand Down Expand Up @@ -943,12 +998,12 @@ def test_230_resize_to_the_same_host(self):
# Make config change, wait for the services restart on
# nova-cloud-controller
with self.config_change(set_default, set_alternate):
# The config change should propagate to nova-cmopute units
# The config change should propagate to nova-compute units
# Wait for them to get settled after relation data has changed
logging.info(
'Waiting for services ({}) to be restarted'.format(services))
zaza.model.block_until_services_restarted(
'nova-compute',
self.application_name,
mtime,
services,
model_name=self.model_name)
Expand All @@ -957,8 +1012,9 @@ def test_230_resize_to_the_same_host(self):
# it's changed
nova_cfg = ConfigParser()

for unit in zaza.model.get_units('nova-compute',
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
model_name=self.model_name):
logging.info('Checking value of {} in {}'.format(
nova_config_key, unit.entity_id))
result = zaza.model.run_on_unit(
Expand Down Expand Up @@ -1156,8 +1212,9 @@ def test_security_checklist(self):
else:
expected_failures.extend(tls_checks)

for unit in zaza.model.get_units(self.application_name,
model_name=self.model_name):
for unit in zaza.model.get_units(
self.application_name,
model_name=self.model_name):
logging.info('Running `security-checklist` action'
' on unit {}'.format(unit.entity_id))
test_utils.audit_assertions(
Expand Down
2 changes: 1 addition & 1 deletion zaza/openstack/charm_tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ def restart_on_changed(self, config_file, default_config, alternate_config,
a service.
:type pgrep_full: bool
"""
# lead_unit is only useed to grab a timestamp, the assumption being
# lead_unit is only used to grab a timestamp, the assumption being
# that all the units times are in sync.

mtime = model.get_unit_time(
Expand Down
6 changes: 4 additions & 2 deletions zaza/openstack/charm_tests/vault/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import zaza.openstack.utilities.cert
import zaza.openstack.utilities.generic
import zaza.openstack.utilities.exceptions as zaza_exceptions
from zaza.openstack.utilities.openstack import get_app_names_for_charm
import zaza.utilities.juju as juju_utils


Expand Down Expand Up @@ -187,12 +188,13 @@ def auto_initialize(cacert=None, validation_application='keystone', wait=True,
validate_ca(cacertificate, application=validation_application)
# Once validation has completed restart nova-compute to work around
# bug #1826382
nova_compute_app_name = get_app_names_for_charm('nova-compute')[0]
cmd_map = {
'nova-cloud-controller': ('systemctl restart '
'nova-scheduler nova-conductor'),
'nova-compute': 'systemctl restart nova-compute',
nova_compute_app_name: 'systemctl restart nova-compute',
}
for app in ('nova-compute', 'nova-cloud-controller',):
for app in (nova_compute_app_name, 'nova-cloud-controller',):
Copy link

Choose a reason for hiding this comment

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

I might be misunderstanding the intent here, but if you anticipate that there
might be several apps that use the nova-compute charm, wouldn't you want to
restart services on all of those? However, you only store the first application
in line 191.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that was partial future-proofing code, it is easy to make it fully future proof, making those changes now. I was initially just fixing the error where it was hardcoded.

try:
for unit in zaza.model.get_units(app):
result = zaza.model.run_on_unit(
Expand Down
25 changes: 24 additions & 1 deletion zaza/openstack/utilities/openstack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,8 @@ def add_interface_to_netplan(server_name, mac_address):
application_names = ('neutron-openvswitch',)
elif ovn_present():
# OVN chassis is a subordinate to nova-compute
application_names = ('nova-compute', 'ovn-dedicated-chassis')
application_names = (get_app_names_for_charm('nova-compute')[0],
'ovn-dedicated-chassis')
else:
application_names = ('neutron-gateway',)

Expand Down Expand Up @@ -2693,6 +2694,28 @@ def upload_image_to_glance(glance, local_path, image_name, disk_format='qcow2',
return image


def get_app_names_for_charm(charm_name, model_name=None):
"""Get all application names for a given charm in the model.
:returns: List of application names
:rtype: List[str]
"""
status = juju_utils.get_full_juju_status(model_name=model_name)
result = []
try:
for app_name, app_data in status['applications'].items():
try:
if charm_name == app_data['charm-name']:
result.append(app_name)
except KeyError:
# Older juju versions may not have the field "charm-name"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see references to "charm-name" as far back as juju 1.25 and it is definitely in 2.3, maybe this is overkill?

Copy link
Contributor Author

@rodrigogansobarbieri rodrigogansobarbieri Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have seen juju status json without this field while coding probelius, then a later juju update introduced this field consistently. A bit overkill probably, but harmless I suppose. Given this is a CI code then I guess it is ok to remove and keep only what we expect to be the latest.

if charm_name in app_data['charm']:
Copy link

Choose a reason for hiding this comment

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

Do older versions return a list here for app_data['charm']? If not then a
simple string comparison such as you used a few lines above would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the value here could be 'nova-compute-kvm' for example, so we are checking if the 'nova-compute' string is contained in app_data['charm'] string

result.append(app_name)
except KeyError:
pass
return result


def is_ceph_image_backend(model_name=None):
"""Check if glance is related to ceph, therefore using ceph as backend.
Expand Down
Loading