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

[Filebeat] Support for Deployment Kubernetes resource #822

Closed
wants to merge 12 commits into from
Closed

[Filebeat] Support for Deployment Kubernetes resource #822

wants to merge 12 commits into from

Conversation

operatorequals
Copy link
Contributor

@operatorequals operatorequals commented Sep 24, 2020

This commit introduces the feature of deploying a Kubernetes deployment
instead of a Daemonset using Filebeat, using a values.yaml syntax
as below:

values.yaml


[...]
deploymentType: [daemonset|deployment]
[...]

Specifically, this is used for creation of Filebeat instances not
bound to each Worker, conducting non-Worker-related work, such as
collection of AWS CloudTrail logs as described in 1.

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

Fix #821

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Sep 24, 2020

💚 CLA has been signed

John Torakis added 2 commits September 24, 2020 13:08
This commit indtroduces the feature of deploying a Kubernetes deployment
instead of a Daemonset using Filebeat, using a `values.yaml` syntax
as below:

`values.yaml`
---
```yaml
[...]
deploymentType: [daemonset|deployment]
[...]
```

Specifically, this is used for creation of Filebeat instances not
bound to each Worker, conducting non-Worker-related work, such as
collection of AWS CloudTrail logs as described in [1].

[1]:#821
This commit adds a default value test for `deploymentType`.

Additionally,
* `test_deployment_type_deployment`
Checks if a `Deployment` is created but NOT a `DaemonSet`
* `test_deployment_type_daemonset`
Checks if a `DaemonSet` is created but NOT a `Deployment`
* `test_deployment_type_case_insensitive`
Checks if `deploymentType` value is accepted in a case-insensitive way.
@operatorequals operatorequals changed the title Support for Deployment Kubernetes resource [Filebeat] Support for Deployment Kubernetes resource Sep 24, 2020
@operatorequals
Copy link
Contributor Author

Hello people!
I signed the CLA, is there something that I need to do for the PR to get tested?

Thanks!

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Hi @operatorequals, thanks for submitting this PR.

Metricbeat chart is already managing both daemonset and deployment resources using daemonset.enable and deployment.enable values.

For consistency reason between Elastic charts, I would prefer having the same behavior in Filebeat chart if we had the feature to manage Filebeat deployment resources.

Can you refactor your PR according to Metricbeat chart code?

@operatorequals
Copy link
Contributor Author

Hi @operatorequals, thanks for submitting this PR.

Can you refactor your PR according to Metricbeat chart code?

Absolutely!

This commit uses the MetricBeat Helm chart to create a
Daemonset/Deployment Helm chart for Filebeat.
Uses the
```yaml
daemonset:
  [...]
deployment:
  [...]
```
structure falling back to root key defaults.
@operatorequals
Copy link
Contributor Author

operatorequals commented Oct 21, 2020

I did refactor the PR in a way that accepts defaults from the YAML root key, and also get overriden by deployment/daemonset keys, similar to Metricbeat.

John Torakis and others added 3 commits November 2, 2020 12:29
@jmlrt
Copy link
Member

jmlrt commented Nov 16, 2020

jenkins test this please

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Thanks for this great work 👍

filebeat/templates/configmap.yaml Outdated Show resolved Hide resolved
filebeat/templates/configmap.yaml Outdated Show resolved Hide resolved
filebeat/templates/daemonset.yaml Outdated Show resolved Hide resolved
filebeat/templates/deployment.yaml Outdated Show resolved Hide resolved
filebeat/templates/deployment.yaml Outdated Show resolved Hide resolved
filebeat/values.yaml Show resolved Hide resolved
filebeat/values.yaml Outdated Show resolved Hide resolved
filebeat/values.yaml Show resolved Hide resolved
filebeat/values.yaml Show resolved Hide resolved
filebeat/README.md Outdated Show resolved Hide resolved
@jmlrt jmlrt added filebeat and removed metricbeat labels Nov 18, 2020
operatorequals and others added 2 commits November 19, 2020 14:49
Co-authored-by: Julien Mailleret <8582351+jmlrt@users.noreply.github.com>
Co-authored-by: Julien Mailleret <8582351+jmlrt@users.noreply.github.com>
@operatorequals
Copy link
Contributor Author

On it!

@operatorequals
Copy link
Contributor Author

Please revisit all your previous Comments. I think I fixed them all!
Also added the README.md explanation values.

@jmlrt
Copy link
Member

jmlrt commented Dec 2, 2020

jenkins test this please

@operatorequals
Copy link
Contributor Author

As of Jenkins the tolerations defaults is failing:

16:42:29 >       assert "tolerations" not in r["daemonset"][name]["spec"]["template"]["spec"]
16:42:29 E       AssertionError: assert 'tolerations' not in {'affinity': {}, 'containers': [{'args': ['-e', '-E', 'http.enabled=true'], 'env': [{'name': 'POD_NAMESPACE', 'valueFr...astic.co/beats/filebeat:8.0.0-SNAPSHOT', ...}], 'nodeSelector': {}, 'serviceAccountName': 'release-name-filebeat', ...}
16:42:29 

The line should be replaced with:

    assert (
        r["daemonset"][name]["spec"]["template"]["spec"]["tolerations"]
        == []
    )

(taken from Metricbeat).

Yet, I lost access to the fork due to an accident and I cannot push another commit:
εικόνα

After that, the tests will have lower coverage but they will pass.

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Ensuring that everything is fin for this PR is a very tedious work so many thanks for it 👍

filebeat/templates/daemonset.yaml Show resolved Hide resolved
filebeat/templates/deployment.yaml Show resolved Hide resolved
filebeat/templates/daemonset.yaml Show resolved Hide resolved
filebeat/templates/daemonset.yaml Show resolved Hide resolved
filebeat/templates/daemonset.yaml Show resolved Hide resolved
filebeat/templates/deployment.yaml Show resolved Hide resolved
filebeat/templates/deployment.yaml Show resolved Hide resolved
filebeat/templates/daemonset.yaml Show resolved Hide resolved
filebeat/README.md Show resolved Hide resolved
filebeat/README.md Show resolved Hide resolved
@jmlrt
Copy link
Member

jmlrt commented Dec 2, 2020

After that, the tests will have lower coverage but they will pass.

Yep tests will also need some refactoring pretty similar to what I did for metricbeat_test.py in https://github.com/elastic/helm-charts/pull/572/files?file-filters%5B%5D=.py

@operatorequals
Copy link
Contributor Author

Hello @jmlrt !
This PR is no longer usable because I lost control of the base branch (by deleting my fork).
This PR does contain all the commits from the current one, plus the suggested fixes to finalize Filebeat as deployment. I am closing this one and let's continue our work here:
#964

@jmlrt jmlrt closed this Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Filebeat] Support Kubernetes Deployment (not only Daemonset)
3 participants