oVirt / python-ovirt-engine-sdk4

Python SDK for version 4 of the oVirt Engine API
Apache License 2.0
15 stars 25 forks source link

Added example to recreate VM #18

Closed saifabusaleh closed 3 years ago

saifabusaleh commented 3 years ago

Added example that shows how to recreate VM while maintaining VM permissions

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2012776

ahadas commented 3 years ago

My main question is, at what point is the new VM created? I cant seem to find it.

Yeah, I hesitated on whether to comment on the 'creation' part before I think it would be better not to say the VM is recreated but its disks are re-provisioned from the template - the fact that UpdateVmVersion recreates the VM is an implementation details. Saif, what do you think?

saifabusaleh commented 3 years ago

My main question is, at what point is the new VM created? I cant seem to find it.

Yeah, I hesitated on whether to comment on the 'creation' part before I think it would be better not to say the VM is recreated but its disks are re-provisioned from the template - the fact that UpdateVmVersion recreates the VM is an implementation details. Saif, what do you think?

I think it is depending on the purpose of python sdk examples

back to point 2, we are not showing official usage of python sdk here (e.g we are not calling vm_service.stop and expect VM to be stopped), so it might not be the best learning script here

I think I will leave it to maintainers of the project to decide :)

ahadas commented 3 years ago

My main question is, at what point is the new VM created? I cant seem to find it.

Yeah, I hesitated on whether to comment on the 'creation' part before I think it would be better not to say the VM is recreated but its disks are re-provisioned from the template - the fact that UpdateVmVersion recreates the VM is an implementation details. Saif, what do you think?

I think it is depending on the purpose of python sdk examples

* If the examples here to be used by users as "black-box", they should not care how the VM is really recreated

* if the examples is meant to be learning material, I think it worth mentioning that the VM is recreated by changing template version while writing note, that this is implementation detail and might change in the future

It's kind of both - it's rather a reference rather than a supported artifact we provide but they also demonstrate particular functionality the user can get by executing a script as-is.

back to point 2, we are not showing official usage of python sdk here (e.g we are not calling vm_service.stop and expect VM to be stopped), so it might not be the best learning script here

And why not stopping the VM?

I think I will leave it to maintainers of the project to decide :)

The question is what you aimed to provide here - had it been a script for recreating a VM, I would expect it to remove the VM and re-provision it or remove the disk(s) of the VM, re-provision the disk(s) and attach the new disk(s) to the VM But that's not the case here, you provide this as an example for how to revert the state of a VM that is part of a VM pool to its original state, right? It's ok to explain that the VM is recreated as part of this script but that should appear inside the script imho, the title of the script should rather be its main purpose

saifabusaleh commented 3 years ago

recreated

My main question is, at what point is the new VM created? I cant seem to find it.

Yeah, I hesitated on whether to comment on the 'creation' part before I think it would be better not to say the VM is recreated but its disks are re-provisioned from the template - the fact that UpdateVmVersion recreates the VM is an implementation details. Saif, what do you think?

I think it is depending on the purpose of python sdk examples

* If the examples here to be used by users as "black-box", they should not care how the VM is really recreated

* if the examples is meant to be learning material, I think it worth mentioning that the VM is recreated by changing template version while writing note, that this is implementation detail and might change in the future

It's kind of both - it's rather a reference rather than a supported artifact we provide but they also demonstrate particular functionality the user can get by executing a script as-is.

back to point 2, we are not showing official usage of python sdk here (e.g we are not calling vm_service.stop and expect VM to be stopped), so it might not be the best learning script here

And why not stopping the VM?

I think I will leave it to maintainers of the project to decide :)

The question is what you aimed to provide here - had it been a script for recreating a VM, I would expect it to remove the VM and re-provision it or remove the disk(s) of the VM, re-provision the disk(s) and attach the new disk(s) to the VM But that's not the case here, you provide this as an example for how to revert the state of a VM that is part of a VM pool to its original state, right? It's ok to explain that the VM is recreated as part of this script but that should appear inside the script imho, the title of the script should rather be its main purpose

I agree

the reason that the script name is like this is that I thought it can be useful also for VM not in pool (like previous comment we had)

but if we want we can make it specific to pool and I can change the script name and description

What do you think?

mz-pdm commented 3 years ago

Good for me now, thanks.

oliel commented 3 years ago

ok it looks like the script name is a matter of interpretation, and the rest is fine, good enough for me, approved

ahadas commented 3 years ago

@saifabusaleh please change the title of the script (Show how to recreate VM while maintaining VM permissions) to reflect that it's about re-provisioning a VM in a VM pool rather than recreating a VM while preserving its permissions and add stop-vm at the beginning, separately