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.

Sign up for GitHub

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

Jump to bottom

Installing node exporter and filebeat as daemonsets in custom namespaces #1839

Merged
merged 9 commits into from
Nov 24, 2020
Merged

Installing node exporter and filebeat as daemonsets in custom namespaces #1839

merged 9 commits into from
Nov 24, 2020

Conversation

rpudlowski93
Copy link
Contributor

@rpudlowski93 rpudlowski93 commented Nov 9, 2020 •
edited
Loading

Fix related to bug : #1833

  • Installing Node Exporter and FileBeat as DaemonSets in custom namespaces.
  • DaemonSets for k8s as cloud service

@rpudlowski93 rpudlowski93 mentioned this pull request Nov 9, 2020
Closed
@rpudlowski93 rpudlowski93 marked this pull request as ready for review November 19, 2020 08:38
atsikham
atsikham reviewed Nov 20, 2020
View reviewed changes
core/src/epicli/data/common/ansible/playbooks/roles/filebeat/defaults/main.yml
@@ -1,3 +1,5 @@
---
filebeat_helm_chart_file_name: filebeat-7.9.2.tgz
filebeat_version: "7.9.2"
# Use custom namespace for logging charts such as filebeat in case of k8s as cloud service.
logging_chart_namespace: epi-logging
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be configurable?

Copy link
Contributor

Choose a reason for hiding this comment

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

We've decided to keep it in defaults for now. After this refactor #1756 it will be easier to properly implement upgrades for this case.

.../common/ansible/playbooks/roles/node_exporter/tasks/uninstall-node-exporter-as-daemonset.yml Outdated
become: false
run_once: true

environment: { KUBECONFIG: "{{ vault_location }}/../kubeconfig" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Recently we reduced the number of tasks where we use KUBECONFIG variable. Can't the value be taken from group_vars/all.yml and be specified on a playbook level? The same question for the left part of PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in case of just the helm feature we have to keep such construction, but for filebeat and node_exporter we have properly defined non-localhost ansible groups, @atsikham I think you're right. 🤔 Actually in both cases lines

environment: { KUBECONFIG: "{{ vault_location }}/../kubeconfig" }

can be removed because it's already defined at playbook level. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpudlowski93 any thoughts? 🤗

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've just added the kubeconfig env to upgrade playbook level and removed the env from role. Tested, works fine. Small changes also added in order to optimize code

core/src/epicli/data/common/ansible/playbooks/roles/upgrade/tasks/filebeat.yml Outdated Show resolved Hide resolved
@rpudlowski93 rpudlowski93 requested a review from sk4zuzu November 20, 2020 11:54
sk4zuzu
sk4zuzu reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@sk4zuzu sk4zuzu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

What about something like this? In this case it's not a big performance improvement, but this is a better way to do these things in general, it's faster and we get rid of useless log entries. 🤔

---
- hosts: localhost
  tasks:
    - set_fact:
        specification:
          helm_chart_name: asd2
        helm_ls:
          - {"name":"asd1","namespace":"default","revision":"20","updated":"2020-11-20 03:14:29.269115248 +0100 CET","status":"failed","chart":"asd-0.0.1","app_version":"0.0.1"}
          - {"name":"asd2","namespace":"default","revision":"20","updated":"2020-11-20 03:14:29.269115248 +0100 CET","status":"failed","chart":"asd-0.0.1","app_version":"0.0.1"}

    - set_fact:
        helm_release_exists: >-
          {{ _names | ternary(true, false) }}
      vars:
        _names: >-
          {{ helm_ls | map(attribute='name')
                     | select('eq', specification.helm_chart_name)
                     | list }}

    - debug: msg=OK
      when: helm_release_exists

Also using is defined to check boolean values is dangerous! 🙀

sk4zuzu
sk4zuzu previously approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@sk4zuzu sk4zuzu left a comment

Choose a reason for hiding this comment

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

LGTM 👍😍

core/src/epicli/data/common/ansible/playbooks/roles/upgrade/tasks/node-exporter.yml
| map(attribute='name')
| select('==', specification.helm_chart_name)
| list }}
block:
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

core/src/epicli/data/common/ansible/playbooks/roles/upgrade/tasks/filebeat.yml
environment: { KUBECONFIG: "{{ vault_location }}/../kubeconfig" }
command: helm list --output json
register: helm_list
block:
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

atsikham
atsikham previously approved these changes Nov 24, 2020
View reviewed changes
@rpudlowski93 rpudlowski93 dismissed stale reviews from atsikham and sk4zuzu via 40e684a November 24, 2020 14:33
@rpudlowski93 rpudlowski93 force-pushed the bug/daemonsets-in-custom-namespaces branch 3 times, most recently from 4524f33 to f6fcc58 Compare November 24, 2020 15:02
@rpudlowski93 rpudlowski93 force-pushed the bug/daemonsets-in-custom-namespaces branch from f6fcc58 to 4b22a13 Compare November 24, 2020 15:39
@rpudlowski93 rpudlowski93 force-pushed the bug/daemonsets-in-custom-namespaces branch from 4b22a13 to 2ce2b4e Compare November 24, 2020 15:43
sk4zuzu
sk4zuzu approved these changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@sk4zuzu sk4zuzu left a comment

Choose a reason for hiding this comment

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

🐒

atsikham
atsikham approved these changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@atsikham atsikham left a comment

Choose a reason for hiding this comment

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

🥳

@rpudlowski93 rpudlowski93 merged commit 37942b5 into hitachienergy:develop Nov 24, 2020
rafzei pushed a commit to rafzei/epiphany that referenced this pull request Nov 26, 2020
@rpudlowski93 @rafzei
Installing node exporter and filebeat as daemonsets in custom namespa…
a97089b 
…ces (hitachienergy#1839)

* Installing daemonsets in custom namespaces

* Helm Chart name added to uninstall role

* Upgrade process added

* Including vars moved on the top of task

* Added more strictly condiction

* Obtaining helm releases in different way

* Format improved

* Kubeconfig env moved to playbook level

* Changelog updated

Co-authored-by: Robert Pudłowski <[email protected]>
toszo pushed a commit to toszo/epiphany that referenced this pull request Dec 5, 2020
@rpudlowski93 @toszo
Installing node exporter and filebeat as daemonsets in custom namespa…
73e8341 
…ces (hitachienergy#1839)

* Installing daemonsets in custom namespaces

* Helm Chart name added to uninstall role

* Upgrade process added

* Including vars moved on the top of task

* Added more strictly condiction

* Obtaining helm releases in different way

* Format improved

* Kubeconfig env moved to playbook level

* Changelog updated

Co-authored-by: Robert Pudłowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sk4zuzu sk4zuzu sk4zuzu approved these changes

@atsikham atsikham atsikham approved these changes

@to-bar to-bar Awaiting requested review from to-bar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rpudlowski93 @atsikham @sk4zuzu