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

ansible 2.8.6 get_url repeatedly downloads file with force==no #64016

Open
gtie opened this issue Oct 28, 2019 · 9 comments · May be fixed by #64626

Comments

@gtie
Copy link

@gtie gtie commented Oct 28, 2019

SUMMARY

Unlike in version 2.8.5, get_url module repeatedly downloads an URL even if destination exists with Ansible 2.8.6

ISSUE TYPE
  • Bug Report
COMPONENT NAME

get_url

ANSIBLE VERSION
$ ansible --version
ansible 2.8.6
  config file = None
  configured module search path = [u'/Users/XXX/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/XXX/.virtualenvs/ansible2.8/lib/python2.7/site-packages/ansible
  executable location = /Users/XXX/.virtualenvs/ansible2.8/bin/ansible
  python version = 2.7.16 (default, Sep  2 2019, 11:59:44) [GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.46.4)]
CONFIGURATION
$ ansible-config dump --only-changed
$
OS / ENVIRONMENT

MacOS

STEPS TO REPRODUCE
$ touch foofile
$ cat > test_playbook.yaml
---
- hosts: localhost
  connection: local

  tasks:
    - name:           Fetch file
      get_url:
        url:          http://google.com
        dest:         ./foofile
        force:        no

$ ansible-playbook -i 'localhost,' test_playbook.yaml
EXPECTED RESULTS

No changes, since "foofile" already exists. With the same playbook, a run with Ansible 2.8.5 results in:

PLAY RECAP ********************************************************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0
ACTUAL RESULTS

File gets overwritten/downloaded every time

$ ansible-playbook -i 'localhost,' test_playbook.yaml
PLAY [localhost] **************************************************************************************************

TASK [Gathering Facts] ********************************************************************************************
ok: [localhost]

TASK [Fetch file] *************************************************************************************************
changed: [localhost]

PLAY RECAP ********************************************************************************************************
localhost                  : ok=2    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0
@ansibot

This comment has been minimized.

Copy link
Contributor

@ansibot ansibot commented Oct 28, 2019

Files identified in the description:

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

@ansibot

This comment has been minimized.

Copy link
Contributor

@ansibot ansibot commented Oct 28, 2019

@sivel

This comment has been minimized.

Copy link
Member

@sivel sivel commented Oct 28, 2019

There was a bug in <2.8.6 where we assumed no checksum was a checksum match. I think we need to further improve the logic to account for this situation too.

Maybe something like:

diff --git a/lib/ansible/modules/net_tools/basics/get_url.py b/lib/ansible/modules/net_tools/basics/get_url.py
index 3c84cb60e9..3fb72dbe84 100644
--- a/lib/ansible/modules/net_tools/basics/get_url.py
+++ b/lib/ansible/modules/net_tools/basics/get_url.py
@@ -540,7 +540,7 @@ def main():
                 checksum_mismatch = True
 
         # Not forcing redownload, unless checksum does not match
-        if not force and checksum and not checksum_mismatch:
+        if not force and (not checksum or (checksum and not checksum_mismatch)):
             # Not forcing redownload, unless checksum does not match
             # allow file attribute changes
             module.params['path'] = dest

More integration tests would need added.

@ansibot ansibot removed the needs_triage label Oct 28, 2019
@sivel sivel added easyfix P3 labels Oct 28, 2019
@telbizov

This comment has been minimized.

Copy link

@telbizov telbizov commented Oct 30, 2019

@sivel I tested the proposed patch above and it seems like the logic is fixed and matches the intended behaviour. If the file exists locally, regardless of its contents if skips the download.

What version of ansible do you envision to incorporate the fix in?

@gtie

This comment has been minimized.

Copy link
Author

@gtie gtie commented Nov 1, 2019

@sivel, the condition fix looks good - how soon can it get released? With a large server farm, this bug triggers a stampede effect, where there would be 0 requests normally.

@sivel

This comment has been minimized.

Copy link
Member

@sivel sivel commented Nov 1, 2019

I am not actively working on this issue. I have provided a proof of concept fix that someone else can validate, make modifications if necessary, provide a pull request to resolve it, and include integration tests.

@hugocartwright

This comment has been minimized.

Copy link

@hugocartwright hugocartwright commented Nov 1, 2019

@gtie @sivel I am currently looking into an implementation of @sivel 's POC and am adding integration tests for this issue too. Currently checking all previous integration tests are not impacted by the change I want to propose.

@gtie

This comment has been minimized.

Copy link
Author

@gtie gtie commented Nov 8, 2019

@hugocartwright, what needs to happen for your PR to get merged? Also, can it get backported to the 2.8 branch?

@hugocartwright

This comment has been minimized.

Copy link

@hugocartwright hugocartwright commented Nov 8, 2019

@telbizov @gtie Integration tests fail for me with @sivel 's suggestion. An extra flag required besides "not checksum"? Regardless, I am PRing it with some suggestions for progress.

The test failures seem to come from the following commit:

@mscherer @pilou- Thanks

hugocartwright pushed a commit to hugocartwright/ansible that referenced this issue Nov 8, 2019
hugocartwright pushed a commit to hugocartwright/ansible that referenced this issue Nov 8, 2019
@hugocartwright hugocartwright linked a pull request that will close this issue Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.