feat: New xen orchestra module #8470

shinuza · 2024-06-06T13:36:35Z

SUMMARY

Added a xen_orchestra module to create/start/restart/stop/delete virtual machines

ISSUE TYPE

New Module/Plugin Pull Request

COMPONENT NAME

xen_orchestra

felixfontein

Thanks for your contribution! I've added some first comments.

Please note https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins, which has some more information on what you need to add/to for new modules.

plugins/modules/xen_orchestra.py

shinuza · 2024-06-11T15:12:22Z

I've updated the code based on your comments, please note:

Some of the code comes from the inventory plugin for Xen Orchestra, meaning that some of your suggestions should probably be backported there.
This includes the minimum websocket-client version, which as you mentioned is 1.0.1 in the inventory code
Among these changes the use_ssl -> use_tls change would be breaking
I don't know at which point the code should be mutualized (e.g for future developments), or even if there are best-practices to mutualize code between the inventory plugins and modules.

felixfontein · 2024-06-14T19:26:37Z

I've updated the code based on your comments, please note:

* Some of the code comes from the inventory plugin for Xen Orchestra, meaning that some of your suggestions should probably be backported there.

👍

* Among these changes the `use_ssl` -> `use_tls` change would be breaking

If applied to the inventory plugin, yes, but there an alias can be added for the old name to keep compatibility.

* I don't know at which point the code should be mutualized (e.g for future developments), or even if there are best-practices to mutualize code between the inventory plugins and modules.

There's plugins/docs_fragments/ for common documentation, and plugins/module_utils/ for shared code.

Generally sharing similar/equal docs and code is a good idea, instead of simply copying.

plugins/modules/xen_orchestra.py

Co-authored-by: Felix Fontein <[email protected]>

felixfontein · 2024-06-27T12:08:20Z

plugins/modules/xen_orchestra_instance.py

+    vm_uid = module.params['vm_uid']
+
+    if state == 'stopped':
+        result = xen_orchestra.stop_vm(vm_uid)


Where are idempotency checks (is the VM already stopped)?

See line 230, we set the task as changed=false if the api returns an error saying the VM is not in a paused, suspended or running state (meaning it's already stopped)

plugins/modules/xen_orchestra_instance.py

Update version from 9.1.0 to 9.2.0 Co-authored-by: Felix Fontein <[email protected]>

shinuza · 2024-07-08T13:35:37Z

Hello,

Any more updates required? @felixfontein

russoz

hi @shinuza
Thanks for your contribution. There are few things that need tending to, see comments below. Additionally, per the collection guidelines, you must also include tests (preferably integration tests, but if not, at least unit tests).

russoz · 2024-07-10T08:08:35Z

plugins/modules/xen_orchestra_instance.py

+  template:
+    description:
+      - UID of a template to create Virtual Machine from.
+      - Muse be provided when O(state=present)


Suggested change

- Muse be provided when O(state=present)

- Must be provided when O(state=present).

russoz · 2024-07-10T08:11:34Z

plugins/modules/xen_orchestra_instance.py

+
+    if state == 'present':
+        result = xen_orchestra.create_vm()
+        module.exit_json(changed=True, vm_uid=result)


If the VM is already present and no configuration changed, this should be False for idempotency.

russoz · 2024-07-10T08:14:06Z

plugins/modules/xen_orchestra_instance.py

+            raise self.module.fail_json(
+                'Could not delete vm: {0}'.format(answer['error']))
+
+        return answer['result']


Is that a boolean value? Because it is being passed back to the user as changed.

russoz · 2024-07-10T08:16:01Z

plugins/modules/xen_orchestra_instance.py

+        waited = 0
+        while waited < self.CALL_TIMEOUT:
+            response = json.loads(self.conn.recv())
+            if 'id' in response and response['id'] == id:


This can be simplified to:

Suggested change

if 'id' in response and response['id'] == id:

if response.get('id') == id:

felixfontein · 2024-07-12T20:57:23Z

plugins/modules/xen_orchestra_instance.py

+  vm_uid:
+    description:
+      - UID of the target Virtual Machine. Required when O(state=absent), O(state=started), O(state=stopped) or
+        O(state=restarted)


Suggested change

O(state=restarted)

O(state=restarted).

felixfontein · 2024-08-09T14:03:07Z

Ping @shinuza

needs_info

ansibullbot · 2024-09-10T15:06:27Z

@shinuza This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

shinuza added 2 commits June 6, 2024 15:34

wip

ca23b67

Added xen_orchestra module

0ff6757

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) labels Jun 6, 2024

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch labels Jun 7, 2024

felixfontein reviewed Jun 8, 2024

View reviewed changes

shinuza added 2 commits June 11, 2024 15:53

fix: pep8

5f363c2

fix: Documentation / conformance updates

77855da

shinuza added 2 commits June 11, 2024 17:12

fix: Trailing space

9e01076

fix: Updated maintainers meta file

764d558

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 11, 2024

felixfontein reviewed Jun 14, 2024

View reviewed changes

plugins/modules/xen_orchestra.py Outdated Show resolved Hide resolved

plugins/modules/xen_orchestra.py Outdated Show resolved Hide resolved

shinuza and others added 3 commits June 22, 2024 11:33

Update plugins/modules/xen_orchestra.py

48f97e1

Co-authored-by: Felix Fontein <[email protected]>

feat: Renamed xen_orchestra -> xen_orchestra_instance

0f8ebf8

chore: Update BOTMETA file

fbdae4d

felixfontein reviewed Jun 27, 2024

View reviewed changes

Update plugins/modules/xen_orchestra_instance.py

b5269b3

Update version from 9.1.0 to 9.2.0 Co-authored-by: Felix Fontein <[email protected]>

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jul 7, 2024

russoz reviewed Jul 10, 2024

View reviewed changes

felixfontein reviewed Jul 12, 2024

View reviewed changes

ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New xen orchestra module #8470

feat: New xen orchestra module #8470

shinuza commented Jun 6, 2024 •

edited

Loading

This comment was marked as outdated.

felixfontein left a comment

shinuza commented Jun 11, 2024

felixfontein commented Jun 14, 2024

felixfontein Jun 27, 2024

shinuza Jun 27, 2024 •

edited

Loading

shinuza commented Jul 8, 2024

russoz left a comment

russoz Jul 10, 2024

russoz Jul 10, 2024

russoz Jul 10, 2024

russoz Jul 10, 2024

felixfontein Jul 12, 2024

felixfontein commented Aug 9, 2024

ansibullbot commented Sep 10, 2024

	- Muse be provided when O(state=present)
	- Must be provided when O(state=present).

	if 'id' in response and response['id'] == id:
	if response.get('id') == id:

feat: New xen orchestra module #8470

Are you sure you want to change the base?

feat: New xen orchestra module #8470

Conversation

shinuza commented Jun 6, 2024 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

shinuza commented Jun 11, 2024

felixfontein commented Jun 14, 2024

felixfontein Jun 27, 2024

Choose a reason for hiding this comment

shinuza Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

shinuza commented Jul 8, 2024

russoz left a comment

Choose a reason for hiding this comment

russoz Jul 10, 2024

Choose a reason for hiding this comment

russoz Jul 10, 2024

Choose a reason for hiding this comment

russoz Jul 10, 2024

Choose a reason for hiding this comment

russoz Jul 10, 2024

Choose a reason for hiding this comment

felixfontein Jul 12, 2024

Choose a reason for hiding this comment

felixfontein commented Aug 9, 2024

ansibullbot commented Sep 10, 2024

shinuza commented Jun 6, 2024 •

edited

Loading

shinuza Jun 27, 2024 •

edited

Loading