From 1daa2705a9f4d64d7b14d3d73473cfa2b5f87944 Mon Sep 17 00:00:00 2001 From: Frank Brehm Date: Mon, 23 Oct 2023 18:31:54 +0200 Subject: [PATCH] Refactoring for better disk handling --- lib/cr_tf/errors.py | 22 ++++- lib/cr_tf/handler/files.py | 49 ++++++++--- lib/cr_tf/handler/vmware.py | 5 +- lib/cr_tf/terraform/disk.py | 159 +++++++++++++++++++++++++----------- lib/cr_tf/terraform/vm.py | 67 +++++++++++---- 5 files changed, 227 insertions(+), 75 deletions(-) diff --git a/lib/cr_tf/errors.py b/lib/cr_tf/errors.py index b4f6fa3..bd0034d 100644 --- a/lib/cr_tf/errors.py +++ b/lib/cr_tf/errors.py @@ -16,7 +16,7 @@ from fb_tools.config import ConfigError from .xlate import XLATOR -__version__ = '1.2.1' +__version__ = '1.3.0' _ = XLATOR.gettext ngettext = XLATOR.ngettext @@ -57,6 +57,26 @@ class TerraformVmDefinitionError(TerraformVmError): pass +# ============================================================================= +class TerraformVmTooManyDisksError(TerraformVmDefinitionError): + """Exception class for the case, that too many disks should be connected to a VM.""" + + # ------------------------------------------------------------------------- + def __init__(self, given_disks, max_disks=60): + """Initiate this exception class.""" + self.given_disks = int(given_disks) + self.max_disks = int(max_disks) + + # ------------------------------------------------------------------------- + def __str__(self): + """Typecast into a string.""" + msg = _( + "There should be too many disks ({gd}) assigned to a VM. " + "There are max. {maxd} disks allowed to assign to a VM.").format( + gd=self.given_disks, maxd=self.max_disks) + return msg + + # ============================================================================= class NetworkNotExistingError(ExpectedHandlerError): """Special error class for the case, if the expected network is not existing.""" diff --git a/lib/cr_tf/handler/files.py b/lib/cr_tf/handler/files.py index 10057ea..fb11b71 100644 --- a/lib/cr_tf/handler/files.py +++ b/lib/cr_tf/handler/files.py @@ -27,7 +27,7 @@ from ..errors import AbortExecution from ..xlate import XLATOR -__version__ = '0.1.0' +__version__ = '0.2.0' LOG = logging.getLogger(__name__) _ = XLATOR.gettext @@ -257,6 +257,9 @@ class CrTfHandlerFilesMixin(): vs_pwd = self.config.vsphere[vs_name].password vs_dc = self.config.vsphere[vs_name].dc + rhsm_user = self.config.rhsm_user + rhsm_password = self.config.rhsm_password + content = textwrap.dedent('''\ ## filename: terraform.tfvars ## This file declares the values for the variables to be used in the instance.tf playbook @@ -270,6 +273,7 @@ class CrTfHandlerFilesMixin(): # # vsphere_username = "" # vsphere_userpassword = "" + # rhsm_user_password = "" # # with the correct values. This file will not be under GIT control # @@ -291,6 +295,10 @@ class CrTfHandlerFilesMixin(): content += 'vsphere_username = "{}"\n'.format(vs_user) if vs_pwd: content += 'vsphere_userpassword = "{}"\n'.format(vs_pwd) + if rhsm_user: + content += 'rhsm_user_name = "{}"\n'.format(rhsm_user) + if rhsm_password: + content += 'rhsm_user_password = "{}"\n'.format(rhsm_password) content += '\n' LOG.debug(_("Creating {!r} ...").format('private.auto.tfvars')) @@ -344,6 +352,21 @@ class CrTfHandlerFilesMixin(): ''') content += tpl.format(dc=vs_dc) + tpl = textwrap.dedent('''\ + variable "rhsm_user_name" {{ + default = "{rhsm_user}" + description = "Username of the RedHat subscription management user." + type = string + }} + + variable "rhsm_user_password" {{ + description = "Password of the RedHat subscription management user." + type = string + }} + + ''') + content += tpl.format(rhsm_user=self.config.default_rhsm_user) + tpl = textwrap.dedent('''\ variable "timezone" {{ default = "{tz}" @@ -541,8 +564,8 @@ class CrTfHandlerFilesMixin(): content += self._create_instfile_if(vm, iface, i, tpl_vm) i += 1 - for unit_id in sorted(vm.disks.keys()): - content += self._create_instfile_disk(vm, unit_id) + for disk_name in sorted(vm.disks.keys()): + content += self._create_instfile_disk(vm, disk_name) content += textwrap.indent(textwrap.dedent('''\ cdrom { @@ -610,12 +633,14 @@ class CrTfHandlerFilesMixin(): memory_hot_add_enabled = "true" boot_delay = "{b}" guest_id = {g} + scsi_controller_count = "{c_count}" '''), ' ').format( - g=guest_id, cpu=vm.num_cpus, f=vm.folder, m=vm.memory, b=int(vm.boot_delay * 1000)) + g=guest_id, cpu=vm.num_cpus, f=vm.folder, m=vm.memory, b=int(vm.boot_delay * 1000), + c_count=vm.disks.get_ctrlr_count()) if vm.vm_template: tpl = ' scsi_type = data.vsphere_virtual_machine.{}.scsi_type\n' content += tpl.format(tpl_vm.tf_name) - content += '\n' + content += ' enable_disk_uuid = "true"\n\n' content += textwrap.indent(textwrap.dedent('''\ lifecycle { @@ -651,21 +676,21 @@ class CrTfHandlerFilesMixin(): return content # -------------------------------------------------------------------------- - def _create_instfile_disk(self, vm, unit_id): + def _create_instfile_disk(self, vm, disk_name): # ## Disk definitions if self.verbose > 1: - LOG.debug(_("Generating disk definition {i} of {v!r}.").format(i=unit_id, v=vm.name)) - disk = vm.disks[unit_id] + LOG.debug(_("Generating disk definition {n} of {v!r}.").format(n=disk_name, v=vm.name)) + disk = vm.disks[disk_name] content = textwrap.indent(textwrap.dedent('''\ disk {{ - label = "disk{i}" + label = "{n}" size = "{s}" eagerly_scrub = "false" thin_provisioned = "false" - '''), ' ').format(i=unit_id, s=int(disk.size_gb)) - if unit_id > 0: - content += ' unit_number = {}\n'.format(unit_id) + unit_number = {i} + '''), ' ').format(n=disk_name, i=disk.unit_number, s=int(disk.size_gb)) + content += ' }\n\n' return content diff --git a/lib/cr_tf/handler/vmware.py b/lib/cr_tf/handler/vmware.py index aaad324..d7e0593 100644 --- a/lib/cr_tf/handler/vmware.py +++ b/lib/cr_tf/handler/vmware.py @@ -31,7 +31,7 @@ from ..errors import AbortExecution from ..xlate import XLATOR -__version__ = '0.1.1' +__version__ = '0.1.2' LOG = logging.getLogger(__name__) _ = XLATOR.gettext @@ -621,6 +621,9 @@ class CrTfHandlerVmwMixin(): # -------------------------------------------------------------------------· def _validate_dscluster_vm(self, vm): + if self.verbose > 2: + LOG.debug('Disk mappings:' + '\n' + pp(vm.disks._map)) + needed_gb = 0.0 if not vm.already_existing: for unit_number in vm.disks.keys(): diff --git a/lib/cr_tf/terraform/disk.py b/lib/cr_tf/terraform/disk.py index 330836f..863afee 100644 --- a/lib/cr_tf/terraform/disk.py +++ b/lib/cr_tf/terraform/disk.py @@ -17,6 +17,8 @@ try: except ImportError: from collections import MutableMapping +from numbers import Number + # Third party modules # Own modules @@ -24,9 +26,12 @@ from fb_tools.obj import FbBaseObject from ..config import CrTfConfiguration +from ..errors import TerraformVmDefinitionError +from ..errors import TerraformVmTooManyDisksError + from ..xlate import XLATOR -__version__ = '1.2.2' +__version__ = '1.3.0' LOG = logging.getLogger(__name__) @@ -43,28 +48,54 @@ class TerraformDisk(FbBaseObject): min_size_gb = CrTfConfiguration.default_disk_min_size max_size_gb = CrTfConfiguration.default_disk_max_size + disks_per_scsi_ctrlr = 15 + max_scsi_ctrlrs = 4 + max_scsi_disks = disks_per_scsi_ctrlr * max_scsi_ctrlrs + msg_no_disk_dict = _("Object {o!r} is not a {e} object.") # ------------------------------------------------------------------------- def __init__( - self, appname=None, verbose=0, version=__version__, base_dir=None, initialized=False, - root_disk=False, unit_number=0, size_gb=None): + self, name=None, root_disk=False, unit_number=0, size_gb=None, + version=__version__, initialized=False, *args, **kwargs): + self._name = 'disk' self._root_disk = bool(root_disk) self._unit_number = 0 self._size_gb = self.default_size super(TerraformDisk, self).__init__( - appname=appname, verbose=verbose, version=version, base_dir=base_dir, + version=version, initialized=False, + *args, **kwargs, ) + if name: + self.name = name + self._set_unit_number(unit_number) if size_gb is not None: self.size_gb = size_gb self.initialized = initialized + # ----------------------------------------------------------- + @property + def name(self): + """The name of the disk.""" + return self._name + + @name.setter + def name(self, value): + if value is None: + msg = _("The name of a disk don't may be None.") + raise TerraformVmDefinitionError(msg) + v = str(value).strip() + if v == '': + msg = _("The name of a disk don't may be empty.") + raise TerraformVmDefinitionError(msg) + self._name = v + # ----------------------------------------------------------- @property def root_disk(self): @@ -109,6 +140,7 @@ class TerraformDisk(FbBaseObject): """ res = super(TerraformDisk, self).as_dict(short=short) + res['name'] = self.name res['default_size'] = self.default_size res['max_size_gb'] = self.max_size_gb res['min_size_gb'] = self.min_size_gb @@ -118,6 +150,24 @@ class TerraformDisk(FbBaseObject): return res + # ------------------------------------------------------------------------- + def __repr__(self): + """Typecast into a string for reproduction.""" + out = '<%s(' % (self.__class__.__name__) + + fields = [] + fields.append('name={!r}'.format(self.name)) + fields.append('root_disk={!r}'.format(self.root_disk)) + fields.append('unit_number={!r}'.format(self.unit_number)) + fields.append('size_gb={!r}'.format(self.size_gb)) + fields.append('appname={!r}'.format(self.appname)) + fields.append('verbose={!r}'.format(self.verbose)) + fields.append('base_dir={!r}'.format(self.base_dir)) + fields.append('initialized={!r}'.format(self.initialized)) + + out += ', '.join(fields) + ')>' + return out + # ------------------------------------------------------------------------- def _set_unit_number(self, value): val = int(value) @@ -142,7 +192,7 @@ class TerraformDisk(FbBaseObject): LOG.debug(_("Copying Terraform disk object with unit ID {}.").format(self.unit_number)) disk = self.__class__( - appname=self.appname, verbose=self.verbose, base_dir=self.base_dir, + appname=self.appname, verbose=self.verbose, base_dir=self.base_dir, name=self.name, initialized=self.initialized, root_disk=self.root_disk, unit_number=self.unit_number, size_gb=self.size_gb) @@ -154,6 +204,8 @@ class TerraformDisk(FbBaseObject): if not isinstance(other, TerraformDisk): raise TypeError(self.msg_no_disk_dict.format(o=other, e='TerraformDisk')) + if self.name != other.name: + return False if self.unit_number != other.unit_number: return False if self.root_disk != other.root_disk: @@ -170,14 +222,14 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): A dictionary containing TerraformDisk objects. It works like a dict. i.e.: - disks = TerraformDiskDict(TerraformDisk(unit_number=0, root=True, size_gb=48, ...)) - and - disks[0] returns a TerraformDisk object for the unit_id = 0 + * disks = TerraformDiskDict(TerraformDisk(name='disk0',unit_number=0, root=True, size_gb=48, ...)) + * disks[0] returns the first TerraformDisk object in the list of sorted disk names + * disks['disk0'] returns the TerraformDisk object with the name 'disk0'. """ msg_invalid_disk_type = _("Invalid disk type {{!r}} to set, only {} allowed.").format( 'TerraformDisk') - msg_key_not_unit_number = _("The key {k!r} must be equal to the unit_number of the disk {u}.") + msg_key_not_name = _("The key {k!r} must be equal to the name of the disk {n!r}.") msg_none_type_error = _("None type as key is not allowed.") msg_empty_key_error = _("Empty key {!r} is not allowed.") msg_no_disk_dict = _("Object {o!r} is not a {e} object.") @@ -204,10 +256,10 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if not isinstance(disk, TerraformDisk): raise TypeError(self.msg_invalid_disk_type.format(disk.__class__.__name__)) - if disk.unit_number != key: - raise KeyError(self.msg_key_not_unit_number.format(k=key, u=disk.unit_number)) + if disk.name != key: + msg = self.msg_key_not_name.format(k=key, n=disk.name) - self._map[disk.unit_number] = disk + self._map[key] = disk # ------------------------------------------------------------------------- def append(self, disk): @@ -215,7 +267,7 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if not isinstance(disk, TerraformDisk): raise TypeError(self.msg_invalid_disk_type.format(disk.__class__.__name__)) - self._set_item(disk.unit_number, disk) + self._set_item(disk.name, disk) # ------------------------------------------------------------------------- def _get_item(self, key): @@ -223,8 +275,13 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if key is None: raise TypeError(self.msg_none_type_error) - unit_number = int(key) - return self._map[unit_number] + if isinstance(key, Number): + num = int(key) + keys = self.keys() + name = keys[num] + return self._map[name] + + return self._map[key] # ------------------------------------------------------------------------- def get(self, key): @@ -236,12 +293,16 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if key is None: raise TypeError(self.msg_none_type_error) - unit_number = int(key) + name = str(key) + if isinstance(key, Number): + num = int(key) + keys = self.keys() + name = keys[num] - if not strict and unit_number not in self._map: + if not strict and name not in self._map: return - del self._map[unit_number] + del self._map[name] # ------------------------------------------------------------------------- # The next five methods are requirements of the ABC. @@ -259,8 +320,8 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): # ------------------------------------------------------------------------- def __iter__(self): - for unit_number in self.keys(): - yield unit_number + for name in self.keys(): + yield name # ------------------------------------------------------------------------- def __len__(self): @@ -269,7 +330,7 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): # ------------------------------------------------------------------------- # The next methods aren't required, but nice for different purposes: def __str__(self): - '''returns simple dict representation of the mapping''' + """returns simple dict representation of the mapping""" return str(self._map) # ------------------------------------------------------------------------- @@ -286,21 +347,20 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if key is None: raise TypeError(self.msg_none_type_error) - unit_number = int(key) - return unit_number in self._map + return key in self._map # ------------------------------------------------------------------------- def keys(self): - return sorted(self._map.keys()) + return sorted(self._map.keys(), key=str.lower) # ------------------------------------------------------------------------- def items(self): item_list = [] - for unit_number in self.keys(): - item_list.append((unit_number, self._map[unit_number])) + for name in self.keys(): + item_list.append((name, self._map[name])) return item_list @@ -308,8 +368,8 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): def values(self): value_list = [] - for unit_number in self.keys(): - value_list.append(self._map[unit_number]) + for name in self.keys(): + value_list.append(self._map[name]) return value_list # ------------------------------------------------------------------------- @@ -334,8 +394,7 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if key is None: raise TypeError(self.msg_none_type_error) - unit_number = int(key) - return self._map.pop(unit_number, *args) + return self._map.pop(key, *args) # ------------------------------------------------------------------------- def popitem(self): @@ -343,10 +402,10 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if not len(self._map): return None - unit_number = self.keys()[0] - disk = self._map[unit_number] - del self._map[unit_number] - return (unit_number, disk) + name = self.keys()[0] + disk = self._map[name] + del self._map[name] + return (name, disk) # ------------------------------------------------------------------------- def clear(self): @@ -358,24 +417,21 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): if key is None: raise TypeError(self.msg_none_type_error) - unit_number = int(key) - if not isinstance(default, TerraformDisk): raise TypeError(self.msg_invalid_disk_type.format(default.__class__.__name__)) - if unit_number in self._map: + if key in self._map: return self._map[unit_number] - self._set_item(unit_number, default) + self._set_item(key, default) return default # ------------------------------------------------------------------------- def update(self, other): if isinstance(other, TerraformDiskDict) or isinstance(other, dict): - for key in other.keys(): - unit_number = int(key) - self._set_item(unit_number, other[key]) + for name in other.keys(): + self._set_item(name, other[name]) return for tokens in other: @@ -390,8 +446,8 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): res = super(TerraformDiskDict, self).as_dict(short=short) res['map'] = {} - for unit_number in self._map: - res['map'][unit_number] = self._map[unit_number].as_dict(short) + for name in self._map: + res['map'][name] = self._map[name].as_dict(short) return res @@ -399,8 +455,8 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): def as_list(self, short=True): res = [] - for unit_number in self.keys(): - res.append(self._map[unit_number].as_dict(short)) + for name in self.keys(): + res.append(self._map[name].as_dict(short)) return res # ------------------------------------------------------------------------- @@ -413,14 +469,23 @@ class TerraformDiskDict(MutableMapping, FbBaseObject): appname=self.appname, verbose=self.verbose, base_dir=self.base_dir, initialized=False) - for unit_number in self._map: - new.append(copy.copy(self._map[unit_number])) + for name in self._map: + new.append(copy.copy(self._map[name])) if self.initialized: new.initialized = True return new + # ------------------------------------------------------------------------- + def get_ctrlr_count(self): + + if len(self) <= 1: + return 1 + if len(self) >= TerraformDisk.max_scsi_ctrlrs: + return TerraformDisk.max_scsi_ctrlrs + return len(self) + # ============================================================================= if __name__ == "__main__": diff --git a/lib/cr_tf/terraform/vm.py b/lib/cr_tf/terraform/vm.py index 51dadbf..55d6aa2 100644 --- a/lib/cr_tf/terraform/vm.py +++ b/lib/cr_tf/terraform/vm.py @@ -28,6 +28,7 @@ from fb_tools.common import human2mbytes, is_sequence from fb_tools.handling_obj import HandlingObject from ..errors import TerraformVmDefinitionError +from ..errors import TerraformVmTooManyDisksError from ..config import CrTfConfiguration @@ -37,7 +38,7 @@ from .disk import TerraformDisk, TerraformDiskDict from .interface import TerraformInterface -__version__ = '1.6.3' +__version__ = '1.7.0' LOG = logging.getLogger(__name__) @@ -443,6 +444,8 @@ class TerraformVm(HandlingObject): vm.rootdisk_size = value return True + max_disks = TerraformDisk.max_scsi_disks + if cls.re_key_root_disk.search(key): if isinstance(value, Mapping): for (p_key, p_val) in value.items(): @@ -455,23 +458,35 @@ class TerraformVm(HandlingObject): return True if cls.re_key_data_disk.search(key): + unit_number = cls._get_disk_unit(1) if isinstance(value, Mapping): - vm._add_data_disk(value) + vm._add_data_disk(value, 'disk1', unit_number) elif value is None: - if 1 in vm.disks: - del vm.disks[1] + if unit_number in vm.disks: + del vm.disks[unit_number] else: LOG.error(_("Could not evaluate data disk from {!r}.").format(value)) return True if cls.re_key_data_disks.search(key): if is_sequence(value): - unit_number = 1 - if 1 in vm.disks: - unit_number = 2 + current_disk = 1 + if len(vm.disks) == 2: + current_disk = 2 + total_disks = 2 + len(value) + else: + total_disks = 1 + len(value) + + if total_disks > max_disks: + raise TerraformVmTooManyDisksError(total_disks, max_disks) + + # unit_number = cls._get_disk_unit(current_disk) + + name = "disk{}".format(current_disk) for disk_data in value: - vm._add_data_disk(disk_data, unit_number) - unit_number += 1 + unit_number = cls._get_disk_unit(current_disk) + vm._add_data_disk(disk_data, name, unit_number) + current_disk += 1 elif value is None: if verbose > 1: LOG.debug(_("Data disks for VM {!r} were set to None.").format(vm.name)) @@ -1344,17 +1359,23 @@ class TerraformVm(HandlingObject): if self.verbose > 2: LOG.debug(_("Resetting root disk.")) + disk_name = 'disk0' + disk = TerraformDisk( + name=disk_name, root_disk=True, unit_number=0, size_gb=self.rootdisk_size, appname=self.appname, verbose=self.verbose, base_dir=self.base_dir, - root_disk=True, unit_number=0, size_gb=self.rootdisk_size, initialized=True) - self.disks[0] = disk + self.disks[disk_name] = disk # ------------------------------------------------------------------------- - def _add_data_disk(self, disk_def, unit_number=1): + def _add_data_disk(self, disk_def, name, unit_number=1): + + params = { + 'name': name, + 'unit_number': unit_number, + } - params = {'unit_number': unit_number} for key in disk_def.keys(): val = disk_def[key] if self.re_disk_size.search(key) and val: @@ -1376,9 +1397,27 @@ class TerraformVm(HandlingObject): params['base_dir'] = self.base_dir disk = TerraformDisk(**params) + disk.initialized = True if self.verbose > 2: LOG.debug(_("Got data disk:") + "\n" + pp(disk.as_dict())) - self.disks[unit_number] = disk + self.disks[name] = disk + + # ------------------------------------------------------------------------- + @classmethod + def _get_disk_unit(cls, current_disk=0): + """Tries to evaluate the disk_unit my the current number in the list.""" + disks_per_ctrlr = TerraformDisk.disks_per_scsi_ctrlr + max_scsi_ctrlrs = TerraformDisk.max_scsi_ctrlrs + max_disks = TerraformDisk.max_scsi_disks + + if current_disk >= max_disks: + raise TerraformVmTooManyDisksError(current_disk + 1, max_disks) + + ctrlr_id = current_disk % max_scsi_ctrlrs + id_offset = current_disk // max_scsi_ctrlrs + unit_id = ctrlr_id * disks_per_ctrlr + id_offset + + return unit_id # ============================================================================= -- 2.39.5