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

New module: remove_host #18596

Open
wants to merge 5 commits into
base: devel
from

Conversation

@jpic
Contributor

jpic commented Nov 23, 2016

ISSUE TYPE
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME

remove_host

ANSIBLE VERSION
ansible 2.3.0 (remove_host f7f455dd84) last updated 2016/11/23 11:21:43 (GMT +200)
  lib/ansible/modules/core: (detached HEAD b598611afb) last updated 2016/11/23 11:23:32 (GMT +200)
  lib/ansible/modules/extras: (detached HEAD 25292b3ebd) last updated 2016/11/23 11:23:32 (GMT +200)
  config file = /home/jpic/.ansible.cfg
  configured module search path = ['/home/jpic/ansible/library', '/usr/share/ansible']
SUMMARY

Added remove_host module. This is for when a task actually destroys a host, then it can be removed from further plays.

lib/ansible/inventory/group.py Outdated
def remove_host(self, host):
host.remove_group(self)
self.hosts.remove(host)
self.clear_hosts_cache()

This comment has been minimized.

@bcoca

bcoca Nov 23, 2016

Member

At first I thought this might be overkill, but all hosts need to rebuild their groups vars to remove this host from them. Making note in case someone else goes down same road.

This comment has been minimized.

@jpic

jpic Nov 25, 2016

Contributor

Adding this as a code comment, thank you @bcoca <3

@bcoca bcoca added this to the 2.3.0 milestone Nov 23, 2016

@bcoca

This comment has been minimized.

Member

bcoca commented Nov 23, 2016

making note of needing docs (module stub) once this is merged

jpic added a commit to jpic/ansible-modules-core that referenced this pull request Nov 25, 2016

@jpic

This comment has been minimized.

Contributor

jpic commented Nov 25, 2016

Thanks for accepting this, but the test should fail, looks like this commit misses something to have it executed, any clue please ?

@gundalow

This comment has been minimized.

Contributor

gundalow commented Nov 25, 2016

@jpic The tests need referencing from test/integration/non_destructive.yml (assuming they are non destructive.

@jpic

This comment has been minimized.

Contributor

jpic commented Nov 25, 2016

Thank you @gundalow ! Tests are running, but it can't find remote_host. Probably because it's missing from modules/core ? ansible/ansible-modules-core#5726

2016-11-25 18:10:03 ERROR! no action detected in task. This often indicates a misspelled module name, or incorrect module path.

Locally it works great:

TASK [add_host] **************************************************************************************************************************************************
task path: /home/jpic/work/novapost/ansible-boot-lxc/test.yml:8
creating host via 'add_host': hostname=testboot.lxc
changed: [localhost] => {
    "add_host": {
        "groups": [
            "testboot"
        ], 
        "host_name": "testboot.lxc", 
        "host_vars": {
            "lxc_container_config": [
                "lxc.mount.entry=/home/jpic/work/novapost/ansible-boot-lxc srv none defaults,bind,create=dir,uid=0 0 0"
            ]
        }
    }, 
    "changed": true
}

TASK [debug] *****************************************************************************************************************************************************
task path: /home/jpic/work/novapost/ansible-boot-lxc/test.yml:13
ok: [localhost] => {
    "groups": {
        "all": [
            "testboot.lxc"
        ], 
        "testboot": [
            "testboot.lxc"
        ], 
        "ungrouped": [
            "localhost"
        ]
    }
}

TASK [remove_host] ***********************************************************************************************************************************************
task path: /home/jpic/work/novapost/ansible-boot-lxc/test.yml:14
changed: [localhost] => {
    "changed": true, 
    "remove_host": {
        "host_name": "testboot.lxc"
    }
}

TASK [debug] *****************************************************************************************************************************************************
task path: /home/jpic/work/novapost/ansible-boot-lxc/test.yml:16
ok: [localhost] => {
    "groups": {
        "all": [], 
        "testboot": [], 
        "ungrouped": [
            "localhost"
        ]
    }
}

Isn't the best thing to do now creating another branch with a submodule reference to my fork of core modules ?

@gundalow

This comment has been minimized.

Contributor

gundalow commented Nov 28, 2016

Changes to lib/ansible/modules/* must be in another PR, once merger we (Ansible Core Team) will update the submodules reference

Once the above is done this you can test this PR.

Please remove the change to lib/ansible/modules/core

The git submodule pain will be going away very shortly

@gundalow

needs_revision

test/integration/targets/remove_host/tasks/main.yml Outdated
@@ -0,0 +1,34 @@
# test code for the add_host action
# (c) 2015, Matt Davis <mdavis@ansible.com>

This comment has been minimized.

@gundalow

gundalow Nov 28, 2016

Contributor

Copy and paste error?

test/integration/targets/remove_host/tasks/main.yml Outdated
that:
- hostvars['newdynamichost'] is not defined
- groups['newdynamicgroup'] is defined
- "not groups['newdynamicgroup']"

This comment has been minimized.

@gundalow

gundalow Nov 28, 2016

Contributor

After this test it would be worth doing another remove_host operation, to ensure the action is idempotent.

@mattclay

This comment has been minimized.

Member

mattclay commented Dec 12, 2016

@jpic One of the test failures is due to a yamllint test failure:

test/integration/targets/remove_host/tasks/main.yml
  36:25     error    syntax error: mapping values are not allowed here
@gundalow

This comment has been minimized.

Contributor

gundalow commented Feb 7, 2017

rebase needed

@mattclay

This comment has been minimized.

Member

mattclay commented Feb 9, 2017

CI is failing on integration tests:

2017-02-09 18:12:10 TASK [remove_host : removing an unknown host should not do anything] ***********
2017-02-09 18:12:10 task path: /root/ansible/test/integration/targets/remove_host/tasks/main.yml:19
2017-02-09 18:12:10 ERROR! Unexpected Exception: 'dict' object has no attribute '_result'
2017-02-09 18:12:10 the full traceback was:
2017-02-09 18:12:10 
2017-02-09 18:12:10 Traceback (most recent call last):
2017-02-09 18:12:10   File "/root/ansible/bin/ansible-playbook", line 103, in <module>
2017-02-09 18:12:10     exit_code = cli.run()
2017-02-09 18:12:10   File "/root/ansible/lib/ansible/cli/playbook.py", line 156, in run
2017-02-09 18:12:10     results = pbex.run()
2017-02-09 18:12:10   File "/root/ansible/lib/ansible/executor/playbook_executor.py", line 153, in run
2017-02-09 18:12:10     result = self._tqm.run(play=play)
2017-02-09 18:12:10   File "/root/ansible/lib/ansible/executor/task_queue_manager.py", line 284, in run
2017-02-09 18:12:10     play_return = strategy.run(iterator, play_context)
2017-02-09 18:12:10   File "/root/ansible/lib/ansible/plugins/strategy/linear.py", line 278, in run
2017-02-09 18:12:10     results += self._wait_on_pending_results(iterator)
2017-02-09 18:12:10   File "/root/ansible/lib/ansible/plugins/strategy/__init__.py", line 581, in _wait_on_pending_results
2017-02-09 18:12:10     results = self._process_pending_results(iterator)
2017-02-09 18:12:10   File "/root/ansible/lib/ansible/plugins/strategy/__init__.py", line 487, in _process_pending_results
2017-02-09 18:12:10     self._remove_host(old_host_info, iterator, result_item)
2017-02-09 18:12:10   File "/root/ansible/lib/ansible/plugins/strategy/__init__.py", line 602, in _remove_host
2017-02-09 18:12:10     result_item._result['changed'] = False
2017-02-09 18:12:10 AttributeError: 'dict' object has no attribute '_result'

jpic added some commits Feb 16, 2017

$ ANSIBLE_ROLES_PATH=/home/jpic/env/src/ansible/test/integration/tar…
…gets ansible-playbook non_destructive.yml -i inventory -e @integration_config.yml -vvv

    TASK [remove_host : remove unknown host] ******************************************************************************************************************************************************
    task path: /home/jpic/env/src/ansible/test/integration/targets/remove_host/tasks/main.yml:46
    ok: [testhost] => {
        "changed": false,
        "remove_host": {
            "host_name": "removedhost"
        }
    }

    TASK [remove_host : debug] ********************************************************************************************************************************************************************
    task path: /home/jpic/env/src/ansible/test/integration/targets/remove_host/tasks/main.yml:51
    ok: [testhost] => {
        "changed": false,
        "remove_host_idempotence": {
            "changed": true, # <---- WOOOOT ?
            "remove_host": {
                "host_name": "removedhost"
            }
        }
    }

@ansibot ansibot removed the ci_verified label Feb 16, 2017

@jpic

This comment has been minimized.

Contributor

jpic commented Feb 16, 2017

Thanks, I've finished the rebase. However, I'm a bit stuck at passing the test:

- name: remove unknown host
  remove_host:
    name: removedhost
  register: remove_host_idempotence

- debug: var=remove_host_idempotence

While I get the changed=false as expected from the remove_host output, the debug module call shows changed=true !


    TASK [remove_host : remove unknown host] ******************************************************************************************************************************************************
    task path: /home/jpic/env/src/ansible/test/integration/targets/remove_host/tasks/main.yml:46
    ok: [testhost] => {
        "changed": false,
        "remove_host": {
            "host_name": "removedhost"
        }
    }

    TASK [remove_host : debug] ********************************************************************************************************************************************************************
    task path: /home/jpic/env/src/ansible/test/integration/targets/remove_host/tasks/main.yml:51
    ok: [testhost] => {
        "changed": false,
        "remove_host_idempotence": {
            "changed": true, # <---- WOOOOOOOOOOOOOT ?
            "remove_host": {
                "host_name": "removedhost"
            }
        }
    }
@mattclay

This comment has been minimized.

Member

mattclay commented Feb 16, 2017

@jpic Where you seeing that error? The only CI error I see is the following:

2017-02-16 16:12:39 ============================================================================
2017-02-16 16:12:39 lib/ansible/modules/inventory/remove_host.py
2017-02-16 16:12:39 ============================================================================
2017-02-16 16:12:39 ERROR: ANSIBLE_METADATA.version: not a valid value for dictionary value @ data['version']. Got '2.4.0'
@ansibot

This comment has been minimized.

Contributor

ansibot commented Feb 16, 2017

@jpic This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibot ansibot added the needs_rebase label Mar 1, 2017

@bcoca bcoca modified the milestone: 2.3.0 Mar 7, 2017

@ansibot ansibot added the stale_ci label Apr 11, 2017

@gundalow

This comment has been minimized.

Contributor

gundalow commented Apr 27, 2017

Rebase needed

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