Skip to content

Commit

Permalink
Improving "ovirt_disk" (mostly documentation) (#562)
Browse files Browse the repository at this point in the history 
* Improve "disaster_recovery" documentation "README.md" file.

Improving "disaster_recovery" role variables documentation table:
1. Minor (IDE-automatic) table reformatting to eliminate
a few IDE warnings.
2. Adding backquotes around values to denote them "as code".

Signed-off-by: Pavel Bar <pbar@redhat.com>

* Improving "ovirt_disk" phrasings

Improving "ovirt_disk" - documentation, code comments & messages:
1. Minor rephrasing.
2. Fixing typos.
3. Consistent upper-case usage.

Signed-off-by: Pavel Bar <pbar@redhat.com>

* Small "ovirt_disk" fixes.

1. Improve "ovirt_disk::id" description to specify that "alias"
is also an option. This also makes the description to be consistent
with the "ovirt_disk::name" description.
2. Fixing warnings:
  a. Redundant "ssl" import causing "Unused import statement 'import ssl'".
  b. Several "Local variable 'XYZ' value is not used".
  c. Several "Shadows name 'disk' from outer scope".

Signed-off-by: Pavel Bar <pbar@redhat.com>

* Adding changelog.

Signed-off-by: Pavel Bar <pbar@redhat.com>

Signed-off-by: Pavel Bar <pbar@redhat.com>
  • Loading branch information
@barpavel
barpavel authored Nov 23, 2022
1 parent d8f0848 commit ea2f6b5
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 37 deletions.
3 changes: 3 additions & 0 deletions changelogs/fragments/562-improve-documentation-ovirt_disk-and-disaster_recovery.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
minor_changes:
- Improving "ovirt_disk" and "disaster_recovery" documentation (https://github.com/oVirt/ovirt-ansible-collection/pull/562).
40 changes: 17 additions & 23 deletions plugins/modules/ovirt_disk.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
options:
id:
description:
- "ID of the disk to manage. Either C(id) or C(name) is required."
- "ID of the disk to manage. Either C(id) or C(name)/C(alias) is required."
type: str
name:
description:
Expand Down Expand Up @@ -65,7 +65,7 @@
if you want to upload the disk even if the disk with C(id) or C(name) exists,
then please use C(force) I(true). If you will use C(force) I(false), which
is default, then the disk image won't be uploaded."
- "Note that to upload iso the C(format) should be 'raw'"
- "Note that in order to upload iso the C(format) should be 'raw'."
type: str
aliases: ['image_path']
size:
Expand Down Expand Up @@ -211,7 +211,7 @@
activate:
description:
- I(True) if the disk should be activated.
- When creating disk of virtual machine it is set to I(True).
- When creating disk of Virtual Machine it is set to I(True).
type: bool
backup:
description:
Expand All @@ -229,13 +229,13 @@
version_added: 1.2.0
propagate_errors:
description:
- Indicates if disk errors should cause virtual machine to be paused or if disk errors should be
- Indicates if disk errors should cause Virtual Machine to be paused or if disk errors should be
- propagated to the the guest operating system instead.
type: bool
version_added: 1.2.0
pass_discard:
description:
- Defines whether the virtual machine passes discard commands to the storage.
- Defines whether the Virtual Machine passes discard commands to the storage.
type: bool
version_added: 1.2.0
uses_scsi_reservation:
Expand Down Expand Up @@ -373,7 +373,6 @@

import json
import os
import ssl
import subprocess
import time
import traceback
Expand Down Expand Up @@ -423,7 +422,7 @@ def create_transfer_connection(module, transfer, context, connect_timeout=10, re
try:
connection.connect()
except Exception as e:
# Typically ConnectionRefusedError or socket.gaierror.
# Typically, "ConnectionRefusedError" or "socket.gaierror".
module.warn("Cannot connect to %s, trying %s: %s" % (transfer.transfer_url, transfer.proxy_url, e))

url = urlparse(transfer.proxy_url)
Expand Down Expand Up @@ -552,8 +551,6 @@ def finalize_transfer(connection, module, transfer_id):


def download_disk_image(connection, module):
transfers_service = connection.system_service().image_transfers_service()
hosts_service = connection.system_service().hosts_service()
transfer = start_transfer(connection, module, otypes.ImageTransferDirection.DOWNLOAD)
try:
extra_args = {}
Expand All @@ -579,8 +576,6 @@ def download_disk_image(connection, module):


def upload_disk_image(connection, module):
transfers_service = connection.system_service().image_transfers_service()
hosts_service = connection.system_service().hosts_service()
transfer = start_transfer(connection, module, otypes.ImageTransferDirection.UPLOAD)
try:
extra_args = {}
Expand Down Expand Up @@ -715,9 +710,9 @@ def update_storage_domains(self, disk_id):
action='copy',
entity=disk,
action_condition=(
lambda disk: new_disk_storage.id not in [sd.id for sd in disk.storage_domains]
lambda d: new_disk_storage.id not in [sd.id for sd in d.storage_domains]
),
wait_condition=lambda disk: disk.status == otypes.DiskStatus.OK,
wait_condition=lambda d: d.status == otypes.DiskStatus.OK,
storage_domain=otypes.StorageDomain(
id=new_disk_storage.id,
),
Expand Down Expand Up @@ -789,7 +784,7 @@ def get_vm_service(connection, module):

if vm_id is None:
module.fail_json(
msg="VM don't exists, please create it first."
msg="VM doesn't exist, please create it first."
)

return vms_service.vm_service(vm_id)
Expand Down Expand Up @@ -845,8 +840,8 @@ def main():

lun = module.params.get('logical_unit')
host = module.params['host']
# Fail when host is specified with the LUN id. Lun id is needed to identify
# an existing disk if already available inthe environment.
# Fail when host is specified with the LUN id. LUN id is needed to identify
# an existing disk if already available in the environment.
if (host and lun is None) or (host and lun.get("id") is None):
module.fail_json(
msg="Can not use parameter host ({0!s}) without "
Expand All @@ -857,7 +852,6 @@ def main():
check_params(module)

try:
disk = None
state = module.params['state']
auth = module.params.get('auth')
connection = create_connection(auth)
Expand All @@ -875,14 +869,14 @@ def main():
else:
disk = disks_module.search_entity(search_params=searchable_attributes(module))
if vm_service and disk and state != 'attached':
# If the VM don't exist in VMs disks, but still it's found it means it was found
# If the VM doesn't exist in VMs disks, but still it's found it means it was found
# for template with same name as VM, so we should force create the VM disk.
force_create = disk.id not in [a.disk.id for a in vm_service.disk_attachments_service().list() if a.disk]

ret = None
# First take care of creating the VM, if needed:
if state in ('present', 'detached', 'attached'):
# Always activate disk when its being created
# Always activate disk when it is created.
if vm_service is not None and disk is None:
module.params['activate'] = module.params['activate'] is None or module.params['activate']
ret = disks_module.create(
Expand All @@ -895,17 +889,17 @@ def main():
)
is_new_disk = ret['changed']
ret['changed'] = ret['changed'] or disks_module.update_storage_domains(ret['id'])
# We need to pass ID to the module, so in case we want detach/attach disk
# We need to pass ID to the module, so in case we want to detach/attach disk
# we have this ID specified to attach/detach method:
module.params['id'] = ret['id']

# Upload disk image in case it's new disk or force parameter is passed:
# Upload disk image in case it is a new disk or force parameter is passed:
if module.params['upload_image_path'] and (is_new_disk or module.params['force']):
if module.params['format'] == 'cow' and module.params['content_type'] == 'iso':
module.warn("To upload an ISO image 'format' parameter needs to be set to 'raw'.")
uploaded = upload_disk_image(connection, module)
ret['changed'] = ret['changed'] or uploaded
# Download disk image in case it's file don't exist or force parameter is passed:
# Download disk image in case the file doesn't exist or force parameter is passed:
if (
module.params['download_image_path'] and (not os.path.isfile(module.params['download_image_path']) or module.params['force'])
):
Expand Down Expand Up @@ -986,7 +980,7 @@ def main():
module.params.get('bootable'),
module.params.get('uses_scsi_reservation'),
module.params.get('pass_discard'), ]):
module.warn("Cannot use 'interface', 'activate', 'bootable', 'uses_scsi_reservation' or 'pass_discard' without specifing VM.")
module.warn("Cannot use 'interface', 'activate', 'bootable', 'uses_scsi_reservation' or 'pass_discard' without specifying VM.")

# When the host parameter is specified and the disk is not being
# removed, refresh the information about the LUN.
Expand Down
27 changes: 13 additions & 14 deletions roles/disaster_recovery/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,19 @@ The `disaster_recovery` role responsible to manage the disaster recovery scenari
Role Variables
--------------

| Name | Default value | |
|-------------------------|-----------------------|-----------------------------------------------------|
| dr_ignore_error_clean | False | Specify whether to ignore errors on clean engine setup.<br/>This is mainly being used to avoid failures when trying to move a storage domain to maintenance/detach it. |
| dr_ignore_error_recover | True | Specify whether to ignore errors on recover. |
| dr_partial_import | True | Specify whether to use the partial import flag on VM/Template register.<br/>If True, VMs and Templates will be registered without any missing disks, if false VMs/Templates will fail to be registered in case some of their disks will be missing from any of the storage domains. |
| dr_target_host | secondary | Specify the default target host to be used in the ansible play.<br/> This host indicates the target site which the recover process will be done. |
| dr_source_map | primary | Specify the default source map to be used in the play.<br/> The source map indicates the key which is used to get the target value for each attribute which we want to register with the VM/Template. |
| dr_reset_mac_pool | True | If True, then once a VM will be registered, it will automatically reset the mac pool, if configured in the VM. |
| dr_cleanup_retries_maintenance | 3 | Specify the number of retries of moving a storage domain to maintenance VM as part of a fail back scenario. |
| dr_cleanup_delay_maintenance | 120 | Specify the number of seconds between each retry as part of a fail back scenario. |
| dr_clean_orphaned_vms | True | Specify whether to remove any VMs which have no disks from the setup as part of cleanup. |
| dr_clean_orphaned_disks | True | Specify whether to remove lun disks from the setup as part of engine setup. |
| dr_running_vms | /tmp/ovirt_dr_running_vm_list | Specify the file path which is used to contain the data of the running VMs in the secondary setup before the failback process run on the primary setup after the secondary site cleanup was finished. Note that the /tmp folder is being used as default so the file will not be available after system reboot.

| Name | Default value | |
|--------------------------------|----------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| dr_ignore_error_clean | `False` | Specify whether to ignore errors on clean engine setup.<br/>This is mainly being used to avoid failures when trying to move a storage domain to maintenance/detach it. |
| dr_ignore_error_recover | `True` | Specify whether to ignore errors on recover. |
| dr_partial_import | `True` | Specify whether to use the partial import flag on VM/Template register.<br/>If `True`, VMs and Templates will be registered without any missing disks, if `False` VMs/Templates will fail to be registered in case some of their disks will be missing from any of the storage domains. |
| dr_target_host | `secondary` | Specify the default target host to be used in the ansible play.<br/> This host indicates the target site which the recover process will be done. |
| dr_source_map | `primary` | Specify the default source map to be used in the play.<br/> The source map indicates the key which is used to get the target value for each attribute which we want to register with the VM/Template. |
| dr_reset_mac_pool | `True` | If `True`, then once a VM will be registered, it will automatically reset the mac pool, if configured in the VM. |
| dr_cleanup_retries_maintenance | `3` | Specify the number of retries of moving a storage domain to maintenance VM as part of a fail back scenario. |
| dr_cleanup_delay_maintenance | `120` | Specify the number of seconds between each retry as part of a fail back scenario. |
| dr_clean_orphaned_vms | `True` | Specify whether to remove any VMs which have no disks from the setup as part of cleanup. |
| dr_clean_orphaned_disks | `True` | Specify whether to remove lun disks from the setup as part of engine setup. |
| dr_running_vms | `/tmp/ovirt_dr_running_vm_list` | Specify the file path which is used to contain the data of the running VMs in the secondary setup before the failback process run on the primary setup after the secondary site cleanup was finished. Note that the `/tmp` folder is being used as default so the file will not be available after system reboot. |

Example Playbook
----------------
Expand Down

0 comments on commit ea2f6b5

Please sign in to comment.