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

file.managed doesn't hash currently existing file prior to download #40479

Closed
bitskri3g opened this issue Apr 2, 2017 · 4 comments
Closed

file.managed doesn't hash currently existing file prior to download #40479

bitskri3g opened this issue Apr 2, 2017 · 4 comments
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Milestone

Comments

@bitskri3g
Copy link

bitskri3g commented Apr 2, 2017

Description of Issue/Question

I use file.managed to distribute qcow2 images across controllers in my openstack cluster. Currently, every time I run a highstate to pull down the latest set of images, it appears that salt doesn't hash the image it already has to compare it against source_hash, and instead fully downloads the file first. As our image repository grows, this causes a lot of unnecessary traffic.

It would be great if salt hashed the file it already had stored locally, compared that hash against the value stored in the file listed at source_hash, and then downloaded the entire file only if there was a hash mismatch.

Setup

file distribution

{% for os, args in pillar.get('images', {}).items() %}
/kvmfs/images/{{ args['name'] }}:
  file.managed:
    - source: {{ args['url'] }}
    - source_hash: {{ args['hash'] }}
    - require:
      - /kvmfs/images
      - sls: /states/disk/prov/controller-fs
      - sls: /states/system/refresh-pillar

relevant pillar

images:
  debian8:
    name: debian8.qcow2
    url: http://images.cybbh.space/debian8.qcow2
    hash: http://images.cybbh.space/debian8.hash
  ubuntu1604:
    name: ubuntu1604.img
    url: http://images.cybbh.space/ubuntu1604.img
    hash: http://images.cybbh.space/ubuntu1604.hash
  centos6:
    name: centos6.qcow2
    url: http://images.cybbh.space/centos6.qcow2
    hash: http://images.cybbh.space/centos6.hash
  centos7:
    name: centos7.qcow2
    url: http://10.10.3.248/centos7.qcow2
    hash: http://10.10.3.248/centos7.hash
  fedora:
    name: fedora.qcow2
    url: http://images.cybbh.space/fedora.qcow2
    hash: http://images.cybbh.space/fedora.hash
  ubuntu1404:
    name: ubuntu1404.img
    url: http://images.cybbh.space/ubuntu1404.img
    hash: http://images.cybbh.space/ubuntu1404.hash
  leap42:
    name: leap42.qcow2
    url: http://images.cybbh.space/leap42.qcow2
    hash: http://images.cybbh.space/leap42.hash

Steps to Reproduce Issue

Run highstate (or apply file.managed state individually)

Versions Report

Salt Version:
Salt: 2016.11.3

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: 2.2
gitdb: 0.5.4
gitpython: 0.3.2 RC1
ioflo: Not Installed
Jinja2: 2.9.4
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.4.2
mysql-python: 1.2.3
pycparser: Not Installed
pycrypto: 2.6.1
pygit2: Not Installed
Python: 2.7.9 (default, Jun 29 2016, 13:08:31)
python-gnupg: Not Installed
PyYAML: 3.11
PyZMQ: 14.4.0
RAET: Not Installed
smmap: 0.8.2
timelib: Not Installed
Tornado: 4.2.1
ZMQ: 4.0.5

System Versions:
dist: debian 8.7
machine: x86_64
release: 4.8.6-x86_64-linode78
system: Linux
version: debian 8.7

@Ch3LL
Copy link
Contributor

Ch3LL commented Apr 3, 2017

ping @terminalmage can I get your insight here? Any opinions on changing this approach to file.managed?

@Ch3LL Ch3LL added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Apr 3, 2017
@Ch3LL Ch3LL added this to the Blocked milestone Apr 3, 2017
@terminalmage
Copy link
Contributor

I'm not sure this should be default behavior. What if the file was supposed to have changed, and its your SLS file that needs an update? You'd never get an error message, and the problem of using an outdated file may take much longer to detect.

I think an option like this should be gated in some way, probably via an argument passed to the state.

However, I'd like to get opinions from some of our other core devs (@cachedout @thatch45 @rallytime @gtmanfred).

@JaseFace
Copy link
Contributor

I believe this is a dupe of #42681

@cachedout
Copy link
Contributor

Closed as duplicate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

No branches or pull requests

5 participants