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.

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

Added ability to take snapshots on XenServer #55247

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@freaker2k7
Copy link

freaker2k7 commented Apr 13, 2019

SUMMARY

Added a new parameter is_snapshot which is mutually exclusive with the existing is_template parameter which adds the ability to take snapshots.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

xenserver_guest module

ADDITIONAL INFORMATION

Sometimes it's nice to just take a snapshot without turning the VM into a template.
For example a daily backup, or save a snapshot of a test for a few hours, etc.

No additional info

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 13, 2019

@bvitnik

This comment has been minimized.

Copy link
Contributor

bvitnik commented Apr 13, 2019

@freaker2k7 Hi. I very much appreciate the effort, but unfortunately, supporting snapshots will not be this easy. Let me ask you right away. Did you test your code before making this pull request?

Here is why. You are calling XenAPI method VM.set_is_a_snapshot which does not exist (according to documentation). In fact, is_a_snapshot parameter is read only parameter and is only possible to set by making a snapshot using VM.snapshot XenAPI method. That method requires snapshot name as parameter so the module will also need additional parameter for snapshot name. Please, consult XenAPI documentation:

http://xapi-project.github.io/xen-api/classes/vm.html

To cut story short, snapshot management would be hard to incorporate in xenserver_guest module and for many reasons is best to be implemented in a separate module. Look at vmware_guest_snapshot and vmware_guest_snapshot_facts modules for example. I have plans to make such a module but it will not be soon. If you are willing to step in and make such a module, I'll be glad to help.

@ansibot ansibot removed the needs_triage label Apr 13, 2019

@freaker2k7

This comment has been minimized.

Copy link
Author

freaker2k7 commented Apr 13, 2019

I'll be honest - No I didn't test it :(

But... I didn't invent this method I'm calling - https://developer-docs.citrix.com/projects/xenserver-management-api/en/7.1-ltsr/api-ref-autogen/#rpc-name-setisasnapshot

I'll be able to test it through on Sunday-Monday, currently I'm commiting from my PC.

@bvitnik

This comment has been minimized.

Copy link
Contributor

bvitnik commented Apr 13, 2019

The set_is_a_snapshot method you are referring to is for VDI class, not for VM class. I believe that even for VDI it is an error in Citrix documentation. It does not exist in XenAPI project documentation, along with other set_ methods for read only fields. Compare this:

https://developer-docs.citrix.com/projects/xenserver-management-api/en/7.1-ltsr/api-ref-autogen/#class-vdi

to this:

http://xapi-project.github.io/xen-api/classes/vdi.html

Just in case, I've tested the method on both VM and VDI. On XenServer 7.2 for VM I get:

XenAPI.Failure: ['MESSAGE_METHOD_UNKNOWN', 'VM.set_is_a_snapshot']

for VDI, oddly enough, I get:

XenAPI.Failure: ['PERMISSION_DENIED', '']

Permission denied error should be accompanied by a message explaining why it was denied. I got an empty string here. This implies that there is some bug in XenAPI implementation itself on at least XenServer 7.2. Citrix XenAPI docs for latest version (7.6) do not have set_is_a_snapshot method under VDI class:

https://developer-docs.citrix.com/projects/xenserver-management-api/en/latest/api-ref-autogen/#class-vdi

I believe you have been misled :)

Anyway, VM.snapshot and VDI.snapshot should be used to make snapshots.

@freaker2k7

This comment has been minimized.

Copy link
Author

freaker2k7 commented Apr 13, 2019

Thank you so much for the help!!

I'll look into the documentation tomorrow, If there's nothing I can do, I'll close this pull request,
Otherwise, you'll see an update :)

@bvitnik

This comment has been minimized.

Copy link
Contributor

bvitnik commented Apr 13, 2019

No problem.

In any case, snapshot management should be implemented in separate module. It's more of a requirement than a suggestion. If you are willing to commit yourself to making the module, please don't use this pull request for it. Make a new one.

@freaker2k7 freaker2k7 closed this Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.