Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

Update: make delegate_to configurable / add version check to avoid re-install each time #73

Closed
wants to merge 3 commits into from
Closed

Update: make delegate_to configurable / add version check to avoid re-install each time #73

wants to merge 3 commits into from

Conversation

till
Copy link
Contributor

@till till commented Feb 22, 2019 •
edited
Loading

Objectives

  • make delegate_to configurable
  • introduce a version check to avoid redoing things each time this role is invoked
  • made node_exporter location configurable

Ticket

Resolves: #67

@paulfantom paulfantom changed the title Update: make delegate_to configurable [wip] Update: make delegate_to configurable Feb 22, 2019
@till till force-pushed the fix/creates-version-check branch from 07f0061 to e422b5b Compare February 22, 2019 16:24
paulfantom
paulfantom reviewed Feb 22, 2019
View reviewed changes
tasks/main.yml Outdated Show resolved Hide resolved
@till till changed the title [wip] Update: make delegate_to configurable [wip] Update: make delegate_to configurable / add version check to avoid re-install each time Feb 25, 2019
@till
Copy link
Contributor Author

till commented Feb 25, 2019

@paulfantom Can I, or may I, redirect stderr to stdout? Or is that also not wanted?

@till till force-pushed the fix/creates-version-check branch from 6979791 to 65d0f2a Compare February 26, 2019 11:34
@till till changed the title [wip] Update: make delegate_to configurable / add version check to avoid re-install each time Update: make delegate_to configurable / add version check to avoid re-install each time Feb 26, 2019
till
till commented Feb 26, 2019
View reviewed changes
tasks/main.yml Outdated Show resolved Hide resolved
till
till commented Feb 26, 2019
View reviewed changes
tasks/main.yml
- import_tasks: install.yml
become: true
when: (not __node_exporter_is_installed.stat.exists) or (__node_exporter_current_version_output.stderr_lines[0].split(" ")[2] != node_exporter_version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stderr_lines — likely to change in node_exporter, either directly or later via kingpin.v3. ;)

Copy link
Member

@paulfantom paulfantom Feb 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Will patch it when they change. That issue in kingpin is 3 years old and we don't know when or if node_exporter will follow with that change :)

@SuperQ any insight?

@paulfantom
Copy link
Member

paulfantom commented Feb 26, 2019 •
edited
Loading

Can I, or may I, redirect stderr to stdout?

What for? You are using stdout_lines[0] so instead use stderr_lines[0] :)

Also command won't allow for redirects. Froms command module docs:

The given command will be executed on all selected nodes. It will not be processed through the shell, so variables like $HOME and operations like "<", ">", "|", ";" and "&" will not work (use the shell module if you need these features).

@till
Copy link
Contributor Author

till commented Feb 26, 2019

@paulfantom thanks, good to know. PR is done so far, let me know your thoughts!

paulfantom
paulfantom reviewed Feb 26, 2019
View reviewed changes
defaults/main.yml
@@ -3,6 +3,7 @@ node_exporter_version: 0.17.0
node_exporter_web_listen_address: "0.0.0.0:9100"

node_exporter_textfile_dir: "/var/lib/node_exporter"
node_exporter_destination: "/usr/local/bin/node_exporter"
Copy link
Member

Choose a reason for hiding this comment

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

This is static on purpose as we are trying to follow dir structure defined in hier(7).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In hier?

Copy link
Member

Choose a reason for hiding this comment

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

man hier manual for a layout of linux system directory tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulfantom ah, I had seen node_exporter in /opt on a few systems. Do you mind if I keep this? Since the default is what you need.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we put it into /opt in the beginning, then there was a transition period which took couple of months, where we had this configurable and did some tricks to cleanup after ourselves. And now it is over and I don't see any reason to go back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulfantom I'll see if I can finish this PR this week and remove it then.

paulfantom
paulfantom reviewed Feb 26, 2019
View reviewed changes
tasks/main.yml Outdated Show resolved Hide resolved
paulfantom
paulfantom reviewed Feb 26, 2019
View reviewed changes
tasks/main.yml Outdated Show resolved Hide resolved
till added 3 commits April 1, 2019 15:02
@till
Update: make delegate_to configurable
c29aebd
@till
Update: node_exporter version check
f1142ef 
 - gather remote version
 - skip/avoid install when we are already up to date
@till
Refactor: move to preflight
9abc006
@till till force-pushed the fix/creates-version-check branch from 65d0f2a to 9abc006 Compare April 1, 2019 13:06
@paulfantom
Copy link
Member

I was rethinking this and got to some conclusions:

  1. Configurable installation location won't be included in any cloudalchemy role. This decision was taken to reduce potential support problems and align with man hier(7). But this was already said in code review.

  2. I am not so keen on having configurable delegation. Ansible best practices for production deployments assume there is a special host which is used to manage deployment from. Such host allows to persistently store artifacts and distribute files from.
    On the other hand if someone does not use this approach it might be beneficial to delegate artifacts to one of the hosts in deployment pool. However this in turn changes that hosts into some half-baked controller host.

  3. Version check should be executed in preflight.yml and if version specified in ansible role is the same as one on system, then we could skip whole install.yml (as it is already done in this PR).

@till
Copy link
Contributor Author

till commented Apr 1, 2019

@paulfantom I think everything but the install location is taken care off, I was gonna do that now. Is that okay?

@paulfantom
Copy link
Member

Could you split delegation to another PR?

@stale
Copy link

stale bot commented May 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label May 16, 2019
@stale stale bot closed this May 30, 2019
@rwos rwos mentioned this pull request Jun 19, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paulfantom paulfantom paulfantom left review comments

Assignees
No one assigned
Labels
wontfix This will not be worked on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimizations
2 participants
@till @paulfantom