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

[Enhancement] Add RotateOnStartup feature flag for file output #19347

Merged
merged 19 commits into from
Feb 19, 2021
Merged

[Enhancement] Add RotateOnStartup feature flag for file output #19347

merged 19 commits into from
Feb 19, 2021

Conversation

gitck
Copy link
Contributor

@gitck gitck commented Jun 23, 2020 •
edited
Loading

What does this PR do?

File output can now be configured to disable file rotation on startup. This PR introduces the configuration option rotate_on_startup and configures the internally used FileRotator (rotator.go) accordingly.

Why is it important?

To avoid file rotation on every startup.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
    - [ ] I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • check whether the name of the added configuration option 'rotate_on_startup' conforms to guideline
  • test that default behaviour has not changed, i.e. ouput file is rotated on every startup

How to test this PR locally

Configure beat to file output. If configuration option rotate_on_startup is set to false, file may no longer be rotated on startup.

Related issues

@gitck-scch
add RotateOnStartup feature
27a077c 
enhance file output to also be able to disable file rotation on startup, which is enabled by default (see rotator)
@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?

1 similar comment
@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?

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 23, 2020
@cla-checker-service
Copy link

cla-checker-service bot commented Jun 23, 2020 •
edited
Loading

💚 CLA has been signed

@gitck gitck changed the title add RotateOnStartup feature [Enhancement] Add RotateOnStartup feature flag for file output Jun 23, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 23, 2020 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #19347 updated

  • Start Time: 2021-02-19T13:14:21.019+0000

  • Duration: 51 min 40 sec

  • Commit: bf8cb0f

Test stats 🧪

Test Results
Failed 0
Passed 46704
Skipped 4944
Total 51648

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 46704
Skipped 4944
Total 51648

@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 24, 2020
@andrewkroh andrewkroh added enhancement libbeat Team:Services (Deprecated) Label for the former Integrations-Services team and removed Team:SIEM labels Jun 24, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

@andrewkroh andrewkroh added the needs CLA User must sign the Elastic Contributor License before review. label Jun 24, 2020
@gitck
Copy link
Contributor Author

gitck commented Jun 24, 2020

I already signed the CLA, but I have not yet received a confirmation.

@andrewkroh
Copy link
Member

@gitck I think the issue is that the commits in the PR do not match the email address used to sign the CLA. You can see the email in the commits with https://patch-diff.githubusercontent.com/raw/elastic/beats/pull/19347.patch.

@fearful-symmetry
Copy link
Contributor

Code looks good. We need to get the CLA figure out, and I think we need to rebuild the docs too.

@gitck
Copy link
Contributor Author

gitck commented Jun 25, 2020

@andrewkrow Thank you for the hint. I signed the CLA again with the matching email address. I hope this is working now, sorry for the inconvenience caused.

@andrewkroh andrewkroh removed the needs CLA User must sign the Elastic Contributor License before review. label Jun 25, 2020
@andrewkroh
Copy link
Member

jenkins, run tests

@gitck
Copy link
Contributor Author

gitck commented Jul 9, 2020

I am not sure how to proceed. Who is going to review this PR finally?

@fearful-symmetry
Copy link
Contributor

@gitck Right now CI is still failing. Can you run mage notice, and go mod tidy ?

@gitck
Copy link
Contributor Author

gitck commented Jul 14, 2020 •
edited
Loading

@gitck Right now CI is still failing. Can you run mage notice, and go mod tidy ?

Trying to run mage notice in libbeat folder results in 'Unknown target specified: notice' message. Running mage -l does not list notice as target. (I'm on Windows 10)
Please could you explain what might be the cause of the build failure. I do not understand the failing check and what's causing it.

@gitck
Copy link
Contributor Author

gitck commented Dec 16, 2020

This looks fine from my perspective, my only concern is the default setting of true since that could be considered a breaking change. I'd like someone who knows the output module a bit more to look at it.

'true' reflects the behaviour before my changes, i.e. rotation on startup is the 'default' behaviour (at it is now) and my changes does not change it

@fearful-symmetry
Copy link
Contributor

Ah, okay, that makes sense.

@urso can you take a quick look at this? Not familiar with the output modules.

@urso
Copy link

urso commented Dec 17, 2020

/test

@fearful-symmetry
Copy link
Contributor

Alright, tested, looks good to me.

@fearful-symmetry
Copy link
Contributor

@gitck just one last thing I noticed. Can you add the new config option to /libbeat/_meta/config/output-file.reference.yml.tmpl?

@fearful-symmetry
Copy link
Contributor

You'll need to re-run mage update after.

@gitck
Copy link
Contributor Author

gitck commented Feb 14, 2021

@fearful-symmetry I added the new config option to the mentioned file,
but I was not able to execute mage update since this is an unknown target (libbeats folder). In addition the exection of one test failed during the build (https://beats-ci.elastic.co/job/Beats/job/beats/job/PR-19347/3/).
Please give me some advice on how to proceed.

@gitck just one last thing I noticed. Can you add the new config option to /libbeat/_meta/config/output-file.reference.yml.tmpl?

@fearful-symmetry
Copy link
Contributor

The fact that it's in fileout.asciidoc suggests that it was updated properly? I have no idea what's wrong with CI, it looks like an upstream issue?

@gitck
Copy link
Contributor Author

gitck commented Feb 16, 2021

How to proceed, any suggestion?

@fearful-symmetry
Copy link
Contributor

You might need to merge from master, there could be some CI changes we're missing.

@fearful-symmetry
Copy link
Contributor

Okay, now I'm seeing a few actual failures:

Error: some files are not up-to-date. Run 'make update' then review and commit the changes. Modified: [auditbeat/auditbeat.reference.yml]

You might need to run that from the root beats directory, or from individual directories.

@gitck
Copy link
Contributor Author

gitck commented Feb 17, 2021 •
edited
Loading

Ok, thanks for the hint. Unfortunately, it seems that there is a problem with my dev environment under Windows. As already mentioned earlier, I receive 'Unknown target specified: update' message. Any idea why this is not working, seems like a basic setup issue?

@fearful-symmetry
Copy link
Contributor

I think you might need to do mage update (or mage -l to see if it's another target) in a given directory. That should work on Windows.

@gitck
Copy link
Contributor Author

gitck commented Feb 17, 2021

Obviously it does not work in my case ☹️. I am going to try it again on a Linux machine.

@gitck
Copy link
Contributor Author

gitck commented Feb 19, 2021

Finally, I managed to execute make update and merged with master again which fixed the build issues. Thanks for your hints and patience.

@fearful-symmetry fearful-symmetry merged commit 3319f5b into elastic:master Feb 19, 2021
@fearful-symmetry
Copy link
Contributor

@urso should we backport this to 7.x?

@zube zube bot added [zube]: Done and removed [zube]: Inbox labels Feb 19, 2021
v1v added a commit to v1v/beats that referenced this pull request Feb 22, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/e2e-fleet
2747498 
* upstream/master:
  [Elastic Agent] Fix docker entrypoint for elastic-agent. (elastic#24155)
  [PACKAGING] Push docker images with the architecture in the version (elastic#24121)
  [Agent] Add agent standalone manifests for system module & Pod's log collection (elastic#23938)
  indicator type url is in upper case (elastic#24152)
  [Filebeat] Document netflow internal_networks and set default (elastic#24110)
  [Filebeat] Adding fixes to the TI module (elastic#24133)
  [Enhancement] Add RotateOnStartup feature flag for file output (elastic#19347)
  [Ingest Manager] Fix: Successfully installed and enrolled agent running standalone (elastic#24128)
  Set Elastic licence type for APM server Beats update job (elastic#24122)
  Add logrotation section on Running Filebeat on k8s (elastic#24120)
  [CI] Run if manual UI (elastic#24116)
  [CI] enable x-pack/heartbeat in the CI (elastic#23873)
v1v added a commit to v1v/beats that referenced this pull request Feb 23, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/retry-win…
3ea53ee 
…dows-7

* upstream/master:
  Remove OSS reference for kibana and elasticsearch (elastic#24164)
  Skip flaky TestActions on MacOSx (elastic#23966)
  [Filebeat][AWS] Fix vpcflow pipeline exception: Cannot invoke "Object.getClass()" because "receiver" is null (elastic#24167)
  [Elastic Agent] Fix docker entrypoint for elastic-agent. (elastic#24155)
  [PACKAGING] Push docker images with the architecture in the version (elastic#24121)
  [Agent] Add agent standalone manifests for system module & Pod's log collection (elastic#23938)
  indicator type url is in upper case (elastic#24152)
  [Filebeat] Document netflow internal_networks and set default (elastic#24110)
  [Filebeat] Adding fixes to the TI module (elastic#24133)
  [Enhancement] Add RotateOnStartup feature flag for file output (elastic#19347)
  [Ingest Manager] Fix: Successfully installed and enrolled agent running standalone (elastic#24128)
  Set Elastic licence type for APM server Beats update job (elastic#24122)
  Add logrotation section on Running Filebeat on k8s (elastic#24120)
  [CI] Run if manual UI (elastic#24116)
  [CI] enable x-pack/heartbeat in the CI (elastic#23873)
  chore: comment out the E2E (elastic#24109)
  chore: add-backport-next (elastic#24098)
  Adjust the position of the architecture name in Dockerlogbeat tarball (elastic#24095)
  Update dependencies for M1 support in System (elastic#24019)
v1v added a commit to v1v/beats that referenced this pull request Feb 23, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/packaging…
2e57ac2 
…-arm

* upstream/master: (24 commits)
  Add example input autodsicover config (elastic#24157)
  Empty configuration options generate `<no value>` string for azure-eventhub input (elastic#24156)
  Remove OSS reference for kibana and elasticsearch (elastic#24164)
  Skip flaky TestActions on MacOSx (elastic#23966)
  [Filebeat][AWS] Fix vpcflow pipeline exception: Cannot invoke "Object.getClass()" because "receiver" is null (elastic#24167)
  [Elastic Agent] Fix docker entrypoint for elastic-agent. (elastic#24155)
  [PACKAGING] Push docker images with the architecture in the version (elastic#24121)
  [Agent] Add agent standalone manifests for system module & Pod's log collection (elastic#23938)
  indicator type url is in upper case (elastic#24152)
  [Filebeat] Document netflow internal_networks and set default (elastic#24110)
  [Filebeat] Adding fixes to the TI module (elastic#24133)
  [Enhancement] Add RotateOnStartup feature flag for file output (elastic#19347)
  [Ingest Manager] Fix: Successfully installed and enrolled agent running standalone (elastic#24128)
  Set Elastic licence type for APM server Beats update job (elastic#24122)
  Add logrotation section on Running Filebeat on k8s (elastic#24120)
  [CI] Run if manual UI (elastic#24116)
  [CI] enable x-pack/heartbeat in the CI (elastic#23873)
  chore: comment out the E2E (elastic#24109)
  chore: add-backport-next (elastic#24098)
  Adjust the position of the architecture name in Dockerlogbeat tarball (elastic#24095)
  ...
@bmorelli25 bmorelli25 mentioned this pull request Apr 28, 2021
Merged
3 tasks
@zube zube bot removed the [zube]: Done label May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urso urso urso approved these changes

Assignees

@fearful-symmetry fearful-symmetry

Labels
enhancement libbeat Team:Automation Label for the Observability productivity team Team:Services (Deprecated) Label for the former Integrations-Services team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add rotateonstartup configuration for output.file
7 participants
@gitck @elasticmachine @andrewkroh @fearful-symmetry @urso @andresrc @gitck-scch