-
Notifications
You must be signed in to change notification settings - Fork 85
fix(agent): separate download and upload paths for agent package transfer #1166
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
base: devel
Are you sure you want to change the base?
Changes from all commits
1c57110
bc7a723
336c519
5a75689
544d668
451552a
4c1d3e3
f7daa6b
286414e
ea847e1
e708f57
60c9d10
60b9c2e
3f13d71
551bb7e
115b2c0
d70db1e
cb1f81c
bdbb5ee
1875d0e
4c4ea1c
17ee279
a6d0475
21c3ebc
1e7b2fe
2cf3ce9
0e4c184
4258974
5510686
41d5d4f
b2a2000
f77e459
59ce22b
9952970
dbc576f
d85114d
b9fd594
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,30 @@ | |
| - install-package | ||
| - download-package | ||
|
|
||
| - name: "{{ ansible_facts['system'] }}: Include agent package download tasks." | ||
| ansible.builtin.include_tasks: "{{ ansible_facts['system'] }}-files.yml" | ||
| - name: "Preflight - Make sure facts for the delegate Host (Controller or Target Host) are present." | ||
| ansible.builtin.setup: | ||
| delegate_to: "{{ 'localhost' if checkmk_agent_delegate_download else inventory_hostname }}" | ||
| delegate_facts: true | ||
| tags: | ||
| - download-package | ||
|
|
||
| - name: "Preflight - Set facts for the delegate Host (Controller or target Host)" | ||
| ansible.builtin.set_fact: | ||
| __checkmk_agent_delegate_is_windows: >- | ||
| {{ hostvars['localhost']['ansible_facts']['os_family'] == 'Windows' }} | ||
| tags: | ||
| - download-package | ||
|
|
||
| - name: "{{ ansible_facts['system'] }}: Include agent package download tasks (Linux)." | ||
| ansible.builtin.include_tasks: "Linux-files.yml" | ||
| when: not __checkmk_agent_delegate_is_windows | ||
| tags: | ||
| - include-system-tasks | ||
| - download-package | ||
|
|
||
| - name: "{{ ansible_facts['system'] }}: Include agent package download tasks (Windows)." | ||
| ansible.builtin.include_tasks: "Win32NT-files.yml" | ||
| when: __checkmk_agent_delegate_is_windows | ||
|
Comment on lines
+30
to
+53
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary? This feels awefully complex and seems to do the same as the original code.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just came to notice that I left the original inclusion of |
||
| tags: | ||
| - include-system-tasks | ||
| - download-package | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,16 @@ | ||
| --- | ||
| __checkmk_agent_host_tmp_dir: "/tmp" | ||
| __checkmk_agent_lin_tmp_dir: "/tmp" | ||
|
|
||
| __checkmk_agent_agent: | ||
| url: | ||
| community: "{{ __checkmk_agent_site_url }}/check_mk/agents/check-mk-agent_{{ checkmk_agent_version }}-1_all.deb" | ||
| pro: "{{ __checkmk_agent_site_url }}/check_mk/api/1.0/domain-types/agent/actions/download_by_host/invoke" | ||
| file: | ||
| community: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent_{{ checkmk_agent_version }}-vanilla.deb" | ||
| pro: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent_{{ checkmk_agent_version }}-generic.deb" | ||
| host: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent_{{ checkmk_agent_version }}-{{ inventory_hostname }}.deb" | ||
| download: | ||
| community: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent_{{ checkmk_agent_version }}-vanilla.deb" | ||
| pro: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent_{{ checkmk_agent_version }}-generic.deb" | ||
| host: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent_{{ checkmk_agent_version }}-{{ inventory_hostname }}.deb" | ||
| upload: | ||
| community: "{{ __checkmk_agent_lin_tmp_dir }}/check-mk-agent_{{ checkmk_agent_version }}-vanilla.deb" | ||
| pro: "{{ __checkmk_agent_lin_tmp_dir }}/check-mk-agent_{{ checkmk_agent_version }}-generic.deb" | ||
| host: "{{ __checkmk_agent_lin_tmp_dir }}/check-mk-agent_{{ checkmk_agent_version }}-{{ inventory_hostname }}.deb" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,16 @@ | ||
| --- | ||
| __checkmk_agent_host_tmp_dir: "/tmp" | ||
| __checkmk_agent_lin_tmp_dir: "/tmp" | ||
|
|
||
| __checkmk_agent_agent: | ||
| url: | ||
| community: "{{ __checkmk_agent_site_url }}/check_mk/agents/check-mk-agent-{{ checkmk_agent_version }}-1.noarch.rpm" | ||
| pro: "{{ __checkmk_agent_site_url }}/check_mk/api/1.0/domain-types/agent/actions/download_by_host/invoke" | ||
| file: | ||
| community: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-vanilla.rpm" | ||
| pro: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-generic.rpm" | ||
| host: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-{{ inventory_hostname }}.rpm" | ||
| download: | ||
| community: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent-{{ checkmk_agent_version }}-1.noarch-vanilla.rpm" | ||
| pro: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent-{{ checkmk_agent_version }}-1.noarch-generic.rpm" | ||
| host: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent-{{ checkmk_agent_version }}-1.noarch-{{ inventory_hostname }}.rpm" | ||
| upload: | ||
| community: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-vanilla.rpm" | ||
| pro: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-generic.rpm" | ||
| host: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-{{ inventory_hostname }}.rpm" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,16 @@ | ||
| --- | ||
| __checkmk_agent_host_tmp_dir: "/tmp" | ||
| __checkmk_agent_lin_tmp_dir: "/tmp" | ||
|
|
||
| __checkmk_agent_agent: | ||
| url: | ||
| community: "{{ __checkmk_agent_site_url }}/check_mk/agents/check-mk-agent-{{ checkmk_agent_version }}-1.noarch.rpm" | ||
| pro: "{{ __checkmk_agent_site_url }}/check_mk/api/1.0/domain-types/agent/actions/download_by_host/invoke" | ||
| file: | ||
| community: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-vanilla.rpm" | ||
| pro: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-generic.rpm" | ||
| host: "{{ __checkmk_agent_host_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-{{ inventory_hostname }}.rpm" | ||
| download: | ||
| community: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent-{{ checkmk_agent_version }}-1.noarch-vanilla.rpm" | ||
| pro: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent-{{ checkmk_agent_version }}-1.noarch-generic.rpm" | ||
| host: "{{ __checkmk_agent_host_tmp_dir }}check-mk-agent-{{ checkmk_agent_version }}-1.noarch-{{ inventory_hostname }}.rpm" | ||
| upload: | ||
| community: "{{ __checkmk_agent_lin_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-vanilla.rpm" | ||
| pro: "{{ __checkmk_agent_lin_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-generic.rpm" | ||
| host: "{{ __checkmk_agent_lin_tmp_dir }}/check-mk-agent-{{ checkmk_agent_version }}-1.noarch-{{ inventory_hostname }}.rpm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. Can we somehow avoid it?
Coming back to this after reviewing further: I think we can avoid this, as the conditionals you introduced further down are unnecessary, if you keep this filed reserved for the hostname to delegate to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is, that this configuration option leaves the impression one can randomly choose a delegate to have the files downloaded and then copied from to the target host.
This violates ansibles own assumption for the modules
ansible.builtin.copyandansible.windows.win_copy, whose expectations are that the file to copy has to reside either on the controller or on the target host.One can only download files directly to the target host, or the controller, to then be able to copy the files to their destination.
So I don't think it would be good to leave the logic behind the delegation as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see your point and I remember considering the option to actually delegate things to a third party host, so it can act as an intermediary. But I am still very hesitent to introduce such a breaking change. Especially, as the issue you describe has not surficed yet. I would rather extend on documentation, instead of an unnecessary breaking change.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully understand your point.
But hear me out: Just because nobody has encountered the error yet, doesn't mean it's not there. And I am the first person then to have hit that problem.
I am targeting 1700+ hosts with this Playbook. Windows and Linux hosts. I tried delegating the operations for Windows hosts to a third (not controller nor target) Windows machine. It fails.
This is my approach to fix this misconception and the resulting error.
To me this wont break any setup already using this collection in their setups. Because they have not had this error yet.
There is no way this would be working if you choose a third host as delegate for the download and copy operation. Try it yourself.
Besides that Problem, if the controller is a Linux host, you only will be able to transfer files to Linux hosts. The current state does not work when cross-transferring files from Linux to Windows and vice versa.
This will also be fixed with my approach.
I'll fix and commit the outstanding things.
I would appreciate it if you could reconsider this once more.