Reorder root disk as first disk in OSMorphing stage #202
Conversation
Dany9966
force-pushed
the
get-root-device
branch
from
421081d
to
6244534
Compare
Dany9966
force-pushed
the
get-root-device
branch
from
6244534
to
7adcf74
Compare
There was a problem hiding this comment.
From my current PoV the proposed patch's methodology is too unreliable to include in its current state.
The correct solution would be to have the providers keep track of the device paths of each disk device within the OSMorphing worker (ideally using the Replicator for Linuxes and some basic heuristic for Windowses)
One way to achieve it with no modifications to the existing provider interfaces would be to:
- have the
deploy_os_morphing_resourcesmethod of providers also add something like anosmorphing_device_mappingfield into theosmorphing_info - have the core tools crosscheck the supposed root device returned by
osmorphing_manager.morph_imageand reorder the actual 'volumes_info' (not! the 'volumes_info' copy which may or may not be present ininstance_deployment_info) - the latter may require adding the 'volumes_info' param to the 'FINALIZE_REPLICA_INSTANCE_DEPLOYMENT' task's provider interface call so that the providers can decide for themselves what to do with the
volumes_info's ordering when the time comes.
| LOG.debug('Volumes info after root disk reordering: %s', volumes_info) | ||
|
|
||
| return { | ||
| 'instance_deployment_info': instance_deployment_info} |
There was a problem hiding this comment.
Simply putting the volumes_info back into the instance_deployment_info and hoping that the plugins will handle it isn't a safe way to go about things...
Please make the OSMorphingTask both require and return the volumes_info and manipulate it directly instead.
Out goal should be to not have to care/check on any of the plugins following this patch.
| return {} | ||
| volumes_info = instance_deployment_info.get('volumes_info', []) | ||
| LOG.debug('Volumes info before root disk reordering: %s', volumes_info) | ||
| _reorder_root_disk(volumes_info, osmorphing_info.get('os_root_dev'), |
There was a problem hiding this comment.
Having us randomly depend on osmorphing_manager.morph_image setting the os_root_dev within the mutable osmorphing_info is not exactly safe.
Please make osmorphing_manager.morph_image explicitly return the root device.
Additionally, please ensure within osmorphing_manager.morph_image that a copy of the osmorphing_info is made/mutated before being passed to the actual OSMount/OSMorphing tools.
| if os_type == constants.OS_TYPE_LINUX: | ||
| pattern = r'[0-9]' | ||
| root_disk = re.sub(pattern, '', root_device) | ||
| disk_index = ord(root_disk[-1]) - ord(linux_root_default) |
There was a problem hiding this comment.
Please declare a default value for disk_index before these if statements.
| if os_type == constants.OS_TYPE_LINUX: | ||
| pattern = r'[0-9]' | ||
| root_disk = re.sub(pattern, '', root_device) | ||
| disk_index = ord(root_disk[-1]) - ord(linux_root_default) |
There was a problem hiding this comment.
This may seem like a stretch but this code will completely miss its mark in case of clouds where separate device models would dictate a separate device name prefix (e.g. a setup where 2-3 disks are "/dev/vdN" and 2-3 are "/dev/sdn".
From my PoV there is no sane way of handling this without having the providers knowing and passing the disk device mapping (i.e. "Disk ID %s is /dev/sdc") from the DEPLOY_OS_MORPHING_RESOURCES task directly.
| if os_type == constants.OS_TYPE_LINUX: | ||
| pattern = r'[0-9]' | ||
| root_disk = re.sub(pattern, '', root_device) | ||
| disk_index = ord(root_disk[-1]) - ord(linux_root_default) |
There was a problem hiding this comment.
This code will also completely miss the mark if the worker image itself has multiple disks, so a better solution is clearly required.
No description provided.