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

[Filebeat] Support for Deployment Kubernetes resource#822

Closed
operatorequals wants to merge 12 commits intoelastic:masterfrom
operatorequals:filebeat-deployment-support-feature
Closed

[Filebeat] Support for Deployment Kubernetes resource#822
operatorequals wants to merge 12 commits intoelastic:masterfrom
operatorequals:filebeat-deployment-support-feature

Conversation

@operatorequals
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 operatorequals requested a review from jmlrt October 21, 2020 10:54
@operatorequals
Copy link
Copy Markdown
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
Copy Markdown
Member

jmlrt commented Nov 16, 2020

jenkins test this please

Copy link
Copy Markdown
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 👍

Comment thread filebeat/templates/configmap.yaml Outdated
Comment thread filebeat/templates/configmap.yaml Outdated
Comment thread filebeat/templates/daemonset.yaml Outdated
Comment thread filebeat/templates/deployment.yaml Outdated
Comment thread filebeat/templates/deployment.yaml Outdated
Comment thread filebeat/values.yaml
Comment thread filebeat/values.yaml Outdated
Comment thread filebeat/values.yaml
Comment thread filebeat/values.yaml
Comment thread filebeat/README.md Outdated
@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
Copy Markdown
Contributor Author

On it!

operatorequals and others added 3 commits November 24, 2020 08:51
Co-authored-by: Julien Mailleret <8582351+jmlrt@users.noreply.github.com>
@operatorequals operatorequals requested a review from jmlrt November 26, 2020 17:54
@operatorequals
Copy link
Copy Markdown
Contributor Author

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

@jmlrt
Copy link
Copy Markdown
Member

jmlrt commented Dec 2, 2020

jenkins test this please

@operatorequals
Copy link
Copy Markdown
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
Copy Markdown
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 👍

Comment thread filebeat/templates/daemonset.yaml
Comment thread filebeat/templates/deployment.yaml
Comment thread filebeat/templates/daemonset.yaml
Comment thread filebeat/templates/daemonset.yaml
Comment thread filebeat/templates/daemonset.yaml
Comment thread filebeat/templates/deployment.yaml
Comment thread filebeat/templates/deployment.yaml
Comment thread filebeat/templates/daemonset.yaml
Comment thread filebeat/README.md
Comment thread filebeat/README.md
@jmlrt
Copy link
Copy Markdown
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
Copy Markdown
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