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

fix: make sure binary install dir exists #423

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

namsic
Copy link
Contributor

@namsic namsic commented Oct 4, 2024

I'm trying to install node_exporter on a remote host with a custom node_exporter_binary_install_dir.

- hosts: all
  roles:
    - prometheus.prometheus.node_exporter
  vars:
    node_exporter_binary_local_dir: /home/namsic/downloads/node_exporter-1.6.0.linux-amd64
    node_exporter_binary_install_dir: /home/namsic/tmp

But when I ran the above playbook, I got the following result:

TASK [prometheus.prometheus.node_exporter : Propagate locally distributed node_exporter binary] ****************************************************************************************************************************************************************************************************************************************************************************
fatal: [remote001]: FAILED! => {
    "changed": false,
    "checksum": "894db4cbc1bda8d686d73f33f22c57c861836c9d",
    "msg": "Destination directory /home/namsic/tmp does not exist"
}

If there are multiple target hosts, it would be nice to automatically create the directory if it does not exist.

If these changes are not appropriate, please let me know.

Thanks.

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has now been migrated to the _common role.

@namsic
Copy link
Contributor Author

namsic commented Oct 17, 2024

Can I just add the same change to every roles?

fix: ensure binary_install_dir before copy binary

Or should I do something else?

@gardar
Copy link
Member

gardar commented Oct 17, 2024

Can I just add the same change to every roles?

fix: ensure binary_install_dir before copy binary

Or should I do something else?

You can add a task to the install tasks file of the _common role which all the roles use.

Adding this task before the last task, the one named "Propagate binaries", in _common/tasks/install.yml would probably do the trick.

- name: "Make sure binary install dir exists"
  ansible.builtin.file:
    path: "{{ _common_binary_install_dir }}"
    mode: 0755
    owner: root
    group: root
  become: true
  tags:
    - "{{ ansible_parent_role_names | first | regex_replace(ansible_collection_name ~ '.', '') }}"
    - install
    - "{{ ansible_parent_role_names | first | regex_replace(ansible_collection_name ~ '.', '') }}_install"

@namsic
Copy link
Contributor Author

namsic commented Oct 18, 2024

Thanks for the detailed explanation. I changed it according to your comment.

@namsic namsic changed the title fix: ensure node_exporter_binary_install_dir before copy binary fix: make sure binary install dir exists Oct 18, 2024
@github-actions github-actions bot added bugfix and removed bugfix labels Oct 18, 2024
Comment on lines +108 to +117
mode: 0755
owner: root
group: root
Copy link
Contributor

@SuperQ SuperQ Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should be explicitly changing these permissions, given this is /usr/local/bin by default.

For example, if this is a symlink, I'm pretty sure this will break the system.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mode/owner has to be set because of https://nvd.nist.gov/vuln/detail/CVE-2020-1736
But I agree that we should use stat first to be on the safe side.

@SuperQ
Copy link
Contributor

SuperQ commented Oct 18, 2024

I think we should use ansible.bultin.stat as a preflight check, rather than actually try and create the target directory.

@namsic namsic force-pushed the node_exporter_dir branch from 86dac65 to e7b7bd7 Compare October 19, 2024 07:59
@github-actions github-actions bot added bugfix and removed bugfix labels Oct 19, 2024
@namsic namsic force-pushed the node_exporter_dir branch from e7b7bd7 to 2fc5df0 Compare October 19, 2024 08:03
@github-actions github-actions bot added bugfix and removed bugfix labels Oct 19, 2024
Comment on lines +105 to +110
- name: "Check existence of binary install dir"
ansible.builtin.stat:
path: "{{ _common_binary_install_dir }}"
register: __common_binary_install_dir
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this with reference to below format:

- name: Check existence of TLS key file
ansible.builtin.stat:
path: "{{ systemd_exporter_tls_server_config.key_file }}"
register: __systemd_exporter_key_file
- name: Assert that TLS key and cert are present
ansible.builtin.assert:
that:
- "__systemd_exporter_cert_file.stat.exists"

@namsic namsic force-pushed the node_exporter_dir branch from 2fc5df0 to 5e188a2 Compare October 19, 2024 08:11
@github-actions github-actions bot added bugfix and removed bugfix labels Oct 19, 2024
@namsic
Copy link
Contributor Author

namsic commented Oct 21, 2024

Can someone help me?

I can't figure out how my ansible.builtin.stat changes relate to the error below.

INFO     Running alternative > destroy
INFO     Sanity checks: 'docker'
[WARNING]: Collection community.docker does not support Ansible version
2.11.12.post0

PLAY [Destroy] *****************************************************************

TASK [Destroy molecule instance(s)] ********************************************
fatal: [localhost]: FAILED! => {
   "msg": "Could not find imported module support code for ansible_collections.community.docker.plugins.modules.docker_container.
           Looked for (['ansible.module_utils.compat.version.LooseVersion', 'ansible.module_utils.compat.version'])"
}

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

WARNING  Retrying execution failure 2 of: ansible-playbook --inventory /root/.cache/molecule/alertmanager/alternative/inventory --skip-tags molecule-notest,notest /usr/local/lib/python3.6/dist-packages/molecule_docker/playbooks/destroy.yml
CRITICAL Ansible return code was 2, command was: ['ansible-playbook', '--inventory', '/root/.cache/molecule/alertmanager/alternative/inventory', '--skip-tags', 'molecule-notest,notest', '/usr/local/lib/python3.6/dist-packages/molecule_docker/playbooks/destroy.yml']

@gardar gardar force-pushed the node_exporter_dir branch from 5e188a2 to 0ebd5ba Compare October 21, 2024 11:14
@github-actions github-actions bot added bugfix and removed bugfix labels Oct 21, 2024
@gardar
Copy link
Member

gardar commented Oct 23, 2024

Can someone help me?

I can't figure out how my ansible.builtin.stat changes relate to the error below.

INFO     Running alternative > destroy
INFO     Sanity checks: 'docker'
[WARNING]: Collection community.docker does not support Ansible version
2.11.12.post0

PLAY [Destroy] *****************************************************************

TASK [Destroy molecule instance(s)] ********************************************
fatal: [localhost]: FAILED! => {
   "msg": "Could not find imported module support code for ansible_collections.community.docker.plugins.modules.docker_container.
           Looked for (['ansible.module_utils.compat.version.LooseVersion', 'ansible.module_utils.compat.version'])"
}

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

WARNING  Retrying execution failure 2 of: ansible-playbook --inventory /root/.cache/molecule/alertmanager/alternative/inventory --skip-tags molecule-notest,notest /usr/local/lib/python3.6/dist-packages/molecule_docker/playbooks/destroy.yml
CRITICAL Ansible return code was 2, command was: ['ansible-playbook', '--inventory', '/root/.cache/molecule/alertmanager/alternative/inventory', '--skip-tags', 'molecule-notest,notest', '/usr/local/lib/python3.6/dist-packages/molecule_docker/playbooks/destroy.yml']

That issue is unrelated to your change, the docker ansible collection which is used for the tests was updated recently and that broke compatibility with old ansible versions.
I fixed the issue in #440

Signed-off-by: Namjae Kim <namjae.kim@namsic.dev>
@gardar gardar force-pushed the node_exporter_dir branch from 0ebd5ba to 99c72c8 Compare October 23, 2024 18:59
@github-actions github-actions bot added bugfix and removed bugfix labels Oct 23, 2024
@gardar gardar merged commit dfd5600 into prometheus-community:main Oct 24, 2024
635 checks passed
Copy link
Contributor

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main:
https://prometheus-community.github.io/ansible/branch/main

@namsic namsic deleted the node_exporter_dir branch October 25, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment