Repository: ansible/ansible
Pull request: Change net_* default behaviour on missing module
Signed-off-by: NilashishC <nilashishchakraborty8@gmail.com>

##### SUMMARY
- Fixes #35869 
- This changes the behaviour of net_* modules.
- Instead of depending on `fail_on_missing_module`, this fix will cause the task to fail by default if the respective implementation module is not present. 

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
net_base.py

##### ANSIBLE VERSION
```
devel
```



Problem statement:
net_ping on vyos does not work
##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
net_ping

##### ANSIBLE VERSION
```
ansible 2.5.0
  config file = /home/vagrant/.ansible.cfg
  configured module search path = [u'/home/vagrant/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Aug  4 2017, 00:39:18) [GCC 4.8.5 20150623 (Red Hat 4.8.5-16)]
```

##### CONFIGURATION
<!---
[vagrant@ansible test]$ ansible-config dump --only-changed
DEFAULT_GATHERING(/home/vagrant/.ansible.cfg) = explicit
DEFAULT_HOST_LIST(/home/vagrant/.ansible.cfg) = [u'/home/vagrant/ansible-networking-vagrant-demo/training-course/hosts']
DEPRECATION_WARNINGS(/home/vagrant/.ansible.cfg) = False
HOST_KEY_CHECKING(/home/vagrant/.ansible.cfg) = False
PARAMIKO_HOST_KEY_AUTO_ADD(/home/vagrant/.ansible.cfg) = True
-->

##### OS / ENVIRONMENT
CentOS Linux release 7.4.1708 (Core)

##### SUMMARY
there is no vyos_ping, but the net_ping will return with success, you have to use `-vvvv` to actually see that its not doing anything

##### STEPS TO REPRODUCE
just use net_ping on vyos platform

```yaml
ok: [spine01] => (item=192.168.22.2) => {
    "changed": false,
    "failed": false,
    "item": "192.168.22.2",
    "msg": "Could not find implementation module net_ping for vyos"
}
```


##### EXPECTED RESULTS
```yml
ok: [spine01] => (item=192.168.22.2) => {
    "changed": false,
    "failed": true,
    "item": "192.168.22.2",
    "msg": "Could not find implementation module net_ping for vyos"
}
```
##### ACTUAL RESULTS
see above

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/network/system/net_ping.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/system/net_ping.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @Qalthos @ganeshrn @gundalow @jmcgill298 @kedarX @privateip @rcarrillocruz @trishnaguha
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
from chat-> 

>@rcarrillocruz Ricardo Carrillo Cruz [6:24 AM]
>sean: from memory, that was a requirement from @privateip, to not fail if underlying platform is not present. You can circumvent that by setting in the task 'fail_on_missing_module: yes'

seems like this would cause problems... I ran a few tests, a few should fail on purpose... "this shouldn't be passing".  There is no indication anything is wrong unless you turn up verbosity.  We do this on purpose?
@IPvSean not sure I follow. Do you mean that if you don't set 'fail_on_missing_module: yes' at task level we should display there's no underlying module without turning up verbosity?
Or do you mean when we do set it?
If we set that task var to yes, the play fails and the error message is shown:

```
(ansible) [ricky@ricky-laptop playbooks]$ cat vyos/test_net_ping.yaml 
- hosts: vyos
  gather_facts: no
  connection: network_cli

  tasks:
  - name: Ping Google
    net_ping:
      dest: 8.8.8.8
      fail_on_missing_module: yes
(ansible) [ricky@ricky-laptop playbooks]$ ansible-playbook -i inventory.network vyos/test_net_ping.yaml

PLAY [vyos] **********************************************************************************************************************************************************************************

TASK [Ping Google] ***************************************************************************************************************************************************************************
fatal: [an-vyos-02.ansible.eng.rdu2.redhat.com]: FAILED! => {"changed": false, "failed": true, "msg": "Could not find implementation module net_ping for vyos"}
        to retry, use: --limit @/home/ricky/devel/playbooks/vyos/test_net_ping.retry

PLAY RECAP ***********************************************************************************************************************************************************************************
an-vyos-02.ansible.eng.rdu2.redhat.com : ok=0    changed=0    unreachable=0    failed=1   
```
@rcarrillocruz 

>@IPvSean not sure I follow. Do you mean that if you don't set 'fail_on_missing_module: yes' at task level we should display there's no underlying module without turning up verbosity?

yes... and even go further.  I will like the default behavior should be failure.  The task is reporting that it ran successfully, when nothing actually happened.  I feel like this is a really bad idea.  The only way I actually uncovered this was I had a test that was supposed to fail and it passed, then I dug deeper.
cc @gdpak
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
I replicated this issue on ansible 2.5 environment and there are same issues on vyos, eos and iosxr.
It looks net_ping module is failed to call device-specific ping modules but not failed on result.

Here is the result of net_ping with -vvvvv option.
```
ansible-playbook -i hosts netping_capture.yml -vvvvv
ansible-playbook 2.5.0
  config file = /home/sysadmin/ansible/ansible.cfg
  configured module search path = ['/home/sysadmin/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.5/dist-packages/ansible
  executable location = /usr/local/bin/ansible-playbook
  python version = 3.5.2 (default, Nov 23 2017, 16:37:01) [GCC 5.4.0 20160609]
Using /home/sysadmin/ansible/ansible.cfg as config file
setting up inventory plugins
Parsed /home/sysadmin/ansible/hosts inventory source with ini plugin
Loading callback plugin default of type stdout, v2.0 from /usr/local/lib/python3.5/dist-packages/ansible/plugins/callback/default.py

PLAYBOOK: netping_capture.yml **************************************************
1 plays in netping_capture.yml

PLAY [all] *********************************************************************
META: ran handlers

TASK [net_ping to server(192.168.1.20)] ****************************************
task path: /home/sysadmin/ansible/netping_capture.yml:5
<192.168.1.22> attempting to start connection
<192.168.1.22> using connection plugin network_cli
<192.168.1.24> attempting to start connection
<192.168.1.24> using connection plugin network_cli
<192.168.1.21> attempting to start connection
<192.168.1.21> using connection plugin network_cli
<192.168.1.21> local domain socket does not exist, starting it
<192.168.1.21> control socket path is /home/sysadmin/.ansible/pc/b22b840fbf
<192.168.1.21> <192.168.1.21> ESTABLISH CONNECTION FOR USER: sysadmin on PORT 22 TO 192.168.1.21
<192.168.1.21> <192.168.1.21> ssh connection done, setting terminal
<192.168.1.21> <192.168.1.21> loaded terminal plugin for network_os eos
<192.168.1.21> <192.168.1.21> loaded cliconf plugin for network_os eos
<192.168.1.21> <192.168.1.21> firing event: on_open_shell()
<192.168.1.21> <192.168.1.21> firing event: on_become
<192.168.1.21> <192.168.1.21> ssh connection has completed successfully
<192.168.1.21> connection to remote device started successfully
<192.168.1.21> local domain socket listeners started successfully
<192.168.1.21> 
<192.168.1.21> local domain socket path is /home/sysadmin/.ansible/pc/b22b840fbf
Getting network OS from inventory
ok: [192.168.1.21] => {
    "changed": false,
    "msg": "Could not find implementation module net_ping for eos"
}
<192.168.1.22> local domain socket does not exist, starting it
<192.168.1.22> control socket path is /home/sysadmin/.ansible/pc/3fe6c367f4
<192.168.1.22> <192.168.1.22> ESTABLISH CONNECTION FOR USER: sysadmin on PORT 22 TO 192.168.1.22
<192.168.1.22> <192.168.1.22> ssh connection done, setting terminal
<192.168.1.22> <192.168.1.22> loaded terminal plugin for network_os vyos
<192.168.1.22> <192.168.1.22> loaded cliconf plugin for network_os vyos
<192.168.1.22> <192.168.1.22> firing event: on_open_shell()
<192.168.1.22> <192.168.1.22> ssh connection has completed successfully
<192.168.1.22> connection to remote device started successfully
<192.168.1.22> local domain socket listeners started successfully
<192.168.1.22> 
<192.168.1.22> local domain socket path is /home/sysadmin/.ansible/pc/3fe6c367f4
Getting network OS from inventory
ok: [192.168.1.22] => {
    "changed": false,
    "msg": "Could not find implementation module net_ping for vyos"
}
<192.168.1.24> local domain socket does not exist, starting it
<192.168.1.24> control socket path is /home/sysadmin/.ansible/pc/bf4d5da4e1
<192.168.1.24> <192.168.1.24> ESTABLISH CONNECTION FOR USER: sysadmin on PORT 22 TO 192.168.1.24
<192.168.1.24> <192.168.1.24> ssh connection done, setting terminal
<192.168.1.24> <192.168.1.24> loaded terminal plugin for network_os iosxr
<192.168.1.24> <192.168.1.24> loaded cliconf plugin for network_os iosxr
<192.168.1.24> <192.168.1.24> firing event: on_open_shell()
<192.168.1.24> <192.168.1.24> firing event: on_become
<192.168.1.24> <192.168.1.24> ssh connection has completed successfully
<192.168.1.24> connection to remote device started successfully
<192.168.1.24> local domain socket listeners started successfully
<192.168.1.24> 
<192.168.1.24> local domain socket path is /home/sysadmin/.ansible/pc/bf4d5da4e1
Getting network OS from inventory
ok: [192.168.1.24] => {
    "changed": false,
    "msg": "Could not find implementation module net_ping for iosxr"
}

TASK [debug] *******************************************************************
task path: /home/sysadmin/ansible/netping_capture.yml:11
<192.168.1.21> attempting to start connection
<192.168.1.21> using connection plugin network_cli
<192.168.1.22> attempting to start connection
<192.168.1.22> using connection plugin network_cli
<192.168.1.24> attempting to start connection
<192.168.1.24> using connection plugin network_cli
<192.168.1.21> found existing local domain socket, using it!
<192.168.1.21> updating play_context for connection
<192.168.1.21> 
<192.168.1.21> local domain socket path is /home/sysadmin/.ansible/pc/b22b840fbf
ok: [192.168.1.21] => {
    "msg": {
        "changed": false,
        "failed": false,
        "msg": "Could not find implementation module net_ping for eos"
    }
}
<192.168.1.22> found existing local domain socket, using it!
<192.168.1.22> updating play_context for connection
<192.168.1.22> 
<192.168.1.22> local domain socket path is /home/sysadmin/.ansible/pc/3fe6c367f4
ok: [192.168.1.22] => {
    "msg": {
        "changed": false,
        "failed": false,
        "msg": "Could not find implementation module net_ping for vyos"
    }
}
<192.168.1.24> found existing local domain socket, using it!
<192.168.1.24> updating play_context for connection
<192.168.1.24> 
<192.168.1.24> local domain socket path is /home/sysadmin/.ansible/pc/bf4d5da4e1
ok: [192.168.1.24] => {
    "msg": {
        "changed": false,
        "failed": false,
        "msg": "Could not find implementation module net_ping for iosxr"
    }
}
META: ran handlers
META: ran handlers

PLAY RECAP *********************************************************************
192.168.1.21               : ok=2    changed=0    unreachable=0    failed=0   
192.168.1.22               : ok=2    changed=0    unreachable=0    failed=0   
192.168.1.24               : ok=2    changed=0    unreachable=0    failed=0   
```
I also do tcpdump and comfirmed there is no icmp packet received on destination server.

@privateip @IPvSean As Ansible moves to more role-driven development, would this still be required? I would assume ping actions would be covered under future network operations roles.
imho its a bug, b/c its not really apparent that this does nothing, it just
reports OK unless you up verbosity.  I think I wouldn't waste time
implementing vyos_ping if that will be implemented in a role, which is
where I think your head is at.

On Tue, Apr 17, 2018 at 11:53 AM, Andrius Benokraitis <
notifications@github.com> wrote:

> @privateip <https://github.com/privateip> @IPvSean
> <https://github.com/IPvSean> As Ansible moves to more role-driven
> development, would this still be required? I would assume ping actions
> would be covered under future network operations roles.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ansible/ansible/issues/35869#issuecomment-382044111>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AGPB0JDLOOlOgEQ_wHfgn4Pgxb60ITUSks5tpg_pgaJpZM4R9e6L>
> .
>

imo, `fail_on_missing_module: yes` should be set as default. 
No module should not be `failed: false` since there is no icmp packets to sent out actually and it could be the cause of misunderstanding of the result.

We can prevent misunderstanding of that behavior by few changes on `net_ping` module.

Commit message: Fix net_base default behaviour

Signed-off-by: NilashishC <nilashishchakraborty8@gmail.com>

Patch to review:
diff --git a/lib/ansible/plugins/action/net_base.py b/lib/ansible/plugins/action/net_base.py
index be8f8d438e3b97..662c223c458a1f 100644
--- a/lib/ansible/plugins/action/net_base.py
+++ b/lib/ansible/plugins/action/net_base.py
@@ -107,18 +107,12 @@ def run(self, tmp=None, task_vars=None):
                 display.vvvv('wrong context, sending exit to device', self._play_context.remote_addr)
                 conn.send_command('exit')
 
-        if 'fail_on_missing_module' not in self._task.args:
-            self._task.args['fail_on_missing_module'] = False
-
         result = super(ActionModule, self).run(task_vars=task_vars)
 
         module = self._get_implementation_module(play_context.network_os, self._task.action)
 
         if not module:
-            if self._task.args['fail_on_missing_module']:
-                result['failed'] = True
-            else:
-                result['failed'] = False
+            result['failed'] = True
 
             result['msg'] = ('Could not find implementation module %s for %s' %
                              (self._task.action, play_context.network_os))
@@ -130,8 +124,6 @@ def run(self, tmp=None, task_vars=None):
             if 'network_os' in new_module_args:
                 del new_module_args['network_os']
 
-            del new_module_args['fail_on_missing_module']
-
             display.vvvv('Running implementation module %s' % module)
             result.update(self._execute_module(module_name=module,
                           module_args=new_module_args, task_vars=task_vars,


Code review comments:
diff_hunk: @@ -107,18 +107,12 @@ def run(self, tmp=None, task_vars=None):
                 display.vvvv('wrong context, sending exit to device', self._play_context.remote_addr)
                 conn.send_command('exit')
 
-        if 'fail_on_missing_module' not in self._task.args:
-            self._task.args['fail_on_missing_module'] = False
path: lib/ansible/plugins/action/net_base.py
text: @user1:
Could we not just change this to `True` instead? I get erroring by default, but there are clearly cases where one might want the current functionality.

@author:
That seems like a good idea.

@author:
Fixed

@author:
Fixed