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 ignore unreachable option to the serial variable Feature #37309 #37587

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@zopar

zopar commented Mar 19, 2018

SUMMARY

This change permit to pass an ignore unreachable option to serial variable.
Modified also documentation and tests.
It is also backward compatible. You can pass serial option for example as:
[ [2,1], 2]
So, you divide hosts in groups of 2 and with 1 (True) you specific to not count unreachable.
In this case for example first group does not stop the play in case of 2 unreachable.
It is backward compatible because you can also continue to use old syntax and the play works
as expected by old syntax

ISSUE TYPE
COMPONENT NAME
ANSIBLE VERSION
Used ansible 2.4.3.0 for develop it, but basically works with all versions
it is also backward compatible
ADDITIONAL INFORMATION

_get_serialized_batches now return 2 lists
serialized_batches, ignore_unreachable
ignore_unreachable contains a list of 0,1 (or False, True) that permit to skip the count of unreachable hosts.
Run function now does not stops in case of unreachable in a group.
We could use also only one list, including the ignore value in the serialized_batches, but I preferred to
keep it clean to have more readable and elegant code.
I also updated the related test cases and documentation


@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 19, 2018

@bcoca

This comment has been minimized.

Member

bcoca commented Mar 19, 2018

So i like the feature, i just don't think that the implementation should be positional info inside the existing serial values.

@bcoca bcoca removed the needs_triage label Mar 19, 2018

@ansibot ansibot removed the needs_info label Mar 19, 2018

@zopar

This comment has been minimized.

zopar commented Mar 19, 2018

Hi bcoca,
If we do not want to use it, we need in any case to find a method to pass the option.
For example we could add a new variable like:
serial: [2, 2]
ignore_unreachable: [True, False]

But this create a validation problem, we need to be sure that ignore length is the same of serial and raise an exception.

As we know, the problem is that the executor consider unreachable as failed, if we want to not consider this behavior, we need to pass in someway an option for the executor.

The useful thing of this implementation is that is fully compatible with old implementations and we do not need to modify anything else, you can use:
[2, 2] or [[2, 0], [2,0]] without problems, so only who want to bypass the unreachable will use the list of list expression. Also you can mix safely list and integers.

@dharmabumstead

This comment has been minimized.

Contributor

dharmabumstead commented Mar 20, 2018

Thanks for the PR, @zopar. If this goes forward the documentation updates will need a thorough edit/rewrite. Please don't merge without that happening first.

@dharmabumstead

Documentation updates need an edit/rewrite before this gets merged.

@zopar

This comment has been minimized.

zopar commented Mar 20, 2018

I already added modification to the documentation, it need a review and possibly some improvement on it.
I am Sorry but buildbot has a problem on pass test on centos 6, I already opened a bug report with a possible solution for one related to pyopenssl. For the other maybe (I hope) it was a temporary problem or directly correlated to shippable.sh because I am unable to reproduce it on my box using docker.
Happy to contribute :)

@jimi-c

I'm not sure I like this syntax. Having to use a positional type argument for this will cause problems. I'd prefer that instead of a list a dict were used so the fields would be labeled.

@@ -216,6 +216,23 @@ the pool.
In the event of a problem, fix the few servers that fail using Ansible's automatically generated
retry file to repeat the deploy on just those servers.
You can also ignore unreachable node to go ahead with your job, suppose for example you have 6 nodes and you
want to dived it in groups of 2, an example of serialization option should be::

This comment has been minimized.

@jimi-c

jimi-c Mar 20, 2018

Member

Grammar, typos here. Suggested fix:

You can also ignore unreachable nodes to go ahead with your job. For example, if you have six (6) nodes and you
want to divide it into groups of two (2), an example of the serialization option should be::

This comment has been minimized.

@zopar

zopar Apr 13, 2018

Hi @jimi-c , what do you think about this modification?
If this is ok for you, I will do same modification in the other files. Eventually I could open a new clean merge request and we will close this one.

You can also ignore unreachable nodes to go ahead with your job. For example, if you have six (6) nodes and you want to divide it into groups of two (2), an example of the serialization option should be::

---

- hosts: webservers
  serial: [[2, 1], 2, 2]

This is also equivalent to::

---

- hosts: webservers
  serial: [[2, 1], [2,0], [2,0]]

We have three (3) groups of nodes and every group contains two (2) nodes:
The first group is represented by the list [2, 1] and has a value of 1 (True).
This boolean value represents the answer to the question "Do you want to ignore unreachable machines?".
The second and third group are both represented by [2, 0] and the implicit answer to the previous question is 0 (False).
If you do not explicitly set 1 (True) the default value will be 0 (False).
At this point, the ansible run will have this behavior:
If in the first group one or both nodes are unreachable, ansible will not stop and continue with the
second group.
If in the second group one node is unreachable, ansible will not stop and continue with the third group;
this is the default behavior, also in old versions of ansible.
If in the second group both nodes are unreachable, ansible will stop the job.

In this case, the first group represented by the list [2, 1] has a value of 1 (True) regarding the question
"Do you want to ignroe unreachable?". For others groups the implicit answer to the question is 0 (False).

This comment has been minimized.

@jimi-c

jimi-c Mar 20, 2018

Member

More typos here.

This comment has been minimized.

@jimi-c

jimi-c Mar 20, 2018

Member

@dharmabumstead review more wording/style here.

This comment has been minimized.

@zopar

zopar Mar 24, 2018

@jimi-c Thank you, let me know what do you like to modify, if necessary I will do a new push request for the merge.

@zopar

This comment has been minimized.

zopar commented Mar 20, 2018

I think the positional is the most simple way to implement the future. The code use always the first position for the group and second for the True, False (1,0)
If you do not have a list but an integer, the second position will be 0 as default
If you have an invalid value in the second position, it will be automatically replaced with 0
If you have more than 2 positional only the first 2 are considered

If you use a dictionary you need an ordered dict, otherwise you can't loop over the inserted values in sequence.
At most we could use a dict for the second value, like [[2, {ignore: True}], 2] but this only make things more complex.
Using a dict also could need more effort to make the new feature backward compatible.

I do not think we could have confusion with positional, we are responsible users and the documentation can explain the context. Also, the default context is always to use 0 if 1 is not explicit, so we will have always the standard behaviour.

P.S. Sorry for some typos, English is not my first language.

@zopar

This comment has been minimized.

zopar commented Mar 20, 2018

Anyway this is the eventual code for a dict
I hope you means this type of input as dict and I understood correctly your proposal

If we have
serial: [{2: 0}, 1, {2: 33}, {2: }, {4: True}, {2: False}, {3: "False"}, {2: 1}]

This code

         for i in play.serial:
            if isinstance(i, dict):
                k, v = i.items()[0]
                serial_batch_list.append(k)
                if v == 1:
                    ignore_unreachable_list.append(v)
                else:
                    ignore_unreachable_list.append(0)
            else:
                serial_batch_list.append(i)
                ignore_unreachable_list.append(0)

And result will be 2 lists:
[2, 1, 2, 2, 4, 2, 3, 2]
[0, 0, 0, 0, True, 0, 0, 1]

It is possible also to permit the use of list with positional on serial variable if we like.

This piece of code need also to be improved with a check of a wrong value like this
For example this:
serial: [{4, True}, {3: 1}]
will generate these 2 lists, that are wrong
[True, 3]
[0, 1]

At this point should be better to write a function for this process with related unit tests.
More in general I think the dict system could increase the risk of a typo when writing serial and eventual bugs inside the executor.
Let me know what do you think about it, I am happy to help.

@ansibot ansibot added the stale_ci label Apr 1, 2018

Last value will result as [2, 0] because "False" in this case is a string and not a boolean.

This comment has been minimized.

@zopar

zopar Apr 13, 2018

I would like to modify the note to

.. note::
Valid values are only 0 (False) and 1 (True), all other values will be ignored and 0 will be used. Example::


- hosts: webservers
  serial: [[2, 1], [2, 2], [2, "one"], [2,0], 2, [4, False], [3, True], [2, "False"]]

The second list [2, 2] is not valid and ansible will replace it with [2, 0]
The third list [2, "one"] is not valid and ansible will replace it with [2, 0]
In the fifth position we have a single value (2) that implicitly represents [2, 0]
The last list, [2, "False"] is not valid, because "False" in this case is a string and not a boolean and ansible will replace it with [2, 0]

@ansibot ansibot added the affects_2.6 label May 23, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Aug 3, 2018

@ansibot ansibot added the docsite label Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment