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

[WIP] ovirtlago: add generic waiters class #483

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nvgoldin
Copy link
Contributor

@nvgoldin nvgoldin commented Mar 20, 2017

The first commit adds a waiters.py class. The class goal is to set common infrastructure for tasks that require to poll the SDK periodically for a status of a request. It generates the 'waiters' methods from the data/waiters.yaml file. This can minimize the repetitive task of (here and in OST):

def some_test(...):
    ...
    def _is_host_up():
      .....
    testlib.assert_true_within(_is_host_up, timeout=...)

Down to:

   def some_test(...):
       ...
       waiters.host_up(id=id)

And additionally, as these tasks are quite repetitive and similar: allow adding new 'waiters' by patching the YAML instead of the code.

@nvgoldin nvgoldin added this to the Demo Tool milestone Mar 20, 2017
@nvgoldin nvgoldin self-assigned this Mar 20, 2017
@nvgoldin nvgoldin requested review from eedri, mykaul and gbenhaim March 20, 2017 00:27
@mykaul
Copy link

mykaul commented Mar 20, 2017

Can you check how our Ansible module is doing it? Let's try to share code. See https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/ovirt.py for example.

@nvgoldin
Copy link
Contributor Author

@mykaul - Sure.

I think the equivalent method is here:
https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/ovirt.py#L298-L330
few differences I see:
In the Ansible module the wait method is invoked directly with the target parameters, as done in:
https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/ovirt/ovirt_hosts.py#L299-L303

...
        wait(
            service=host_service,
            condition=lambda host: host.status == hoststate.UP,
            fail_condition=failed_state,
        )
...

Here, the waiters class is a little more 'high level', as the methods are auto-generated with the parameters, which is less flexible when invoking, but simpler:

   waiters.host_up(id)

The Ansible code is written to integrate with the ansible tasks, so eventually at the higher level adding a host and pooling bundled together looks like this(taken from OST):
https://github.com/oVirt/ovirt-system-tests/blob/master/ansible-suite-master/ovirt-deploy/tasks/bootstrap.yml#L48-L60

- name: Add hosts to oVirt engine
  ovirt_hosts:
    auth: "{{ ovirt_auth }}"
    name: "{{ item.0 }}"
    address: "{{ item.1 }}"
    password: "{{ host_password }}"
    cluster: "{{ cluster_name }}"
    override_iptables: true
    timeout: 300
    poll_interval: 10
  with_together:
    - "{{ host_names}}"
    - "{{ host_ips }}"

Which is quite nice and explains why the wait method was built like that.

What I'm not sure at the moment is how we can share the code between the modules, without using Ansible directly. Maybe we should just go there?

If using Ansible directly, the difficulty might be in OST.. As new tests could require more fine-grained control than what the Ansible module provides.

I should say that the inspiration for 'waiters' I took from 'boto'(AWS official SDK), for example:
http://boto3.readthedocs.io/en/latest/reference/services/ec2.html?highlight=waiters#EC2.Waiter.InstanceRunning

In parallel to each 'Action', like we have in the SDK, they provide out-of-the-box 'waiters' methods, which are implemented in the client side.

Copy link
Member

@gbenhaim gbenhaim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please split this pull request into two parts ?

  1. Adding "lago ovirt stop/start"
  2. Adding waiters.

[1] is more urgent for us, and I think that we need to discuss about [2], and check how we can utilize ovirt ansible module.

@nvgoldin
Copy link
Contributor Author

@gbenhaim - sure will do.

@nvgoldin nvgoldin changed the title [WIP] Add 'lago ovirt stop/start' and waiters class [WIP] ovirtlago: add generic waiters class Mar 20, 2017
@nvgoldin nvgoldin removed this from the Demo Tool milestone Mar 20, 2017
@nvgoldin nvgoldin force-pushed the add_waiters branch 3 times, most recently from 9ad23fd to 22b1a07 Compare March 20, 2017 12:03
This new class creates a common infrastructure for tasks that require
polling results periodically from the SDK.
The methods in the 'waiters' class are generated from the data/waiters.yaml
file. Each method defined in the YAML is in charge of polling the Engine
for a status of a request. In this commit only 3 methods were
configured in the YAML:

waiters.vm_up(api, id)
waiters.vm_down(api, id)
waiters.host_up(api, id)

Invoking each of these methods will check if the VM with the given id
reached an 'accepting' state as defined in the YAML 'acceptors' section.
It will run for 'max_attempts', with a 'delay' between and block until.

The polling might end earlier, if the VM reached any of the 'rejectors'
states as defined in the YAML, and then a 'WaiterError' exception will
be thrown.

Adding more 'waiters' requires only updating the YAML.

Signed-off-by: Nadav Goldin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants