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.

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

chore(promtail): Use minimal init config in rpm/deb packaging. #11511

Merged
merged 12 commits into from
Jan 14, 2024

Conversation

kavirajk
Copy link
Contributor

What this PR does / why we need it:
Related issue: #11398

This minimal config scrape only single file thus not overloading the systems as described in the issue

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

Related issue: #11398

This minimal config scrape only single file thus not overloading the systems as described in the issue

Signed-off-by: Kaviraj <[email protected]>
@kavirajk kavirajk requested a review from a team as a code owner December 18, 2023 11:43
Signed-off-by: Kaviraj <[email protected]>
Copy link
Contributor

github-actions bot commented Dec 18, 2023

Trivy scan found the following vulnerabilities:

  • HIGH, Target: docker.io/grafana/loki:main-4421efe (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.3-r0. Fixed in v3.1.4-r0
  • HIGH, Target: docker.io/grafana/loki:main-4421efe (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.3-r0. Fixed in v3.1.4-r0
    \nTo see more details on these vulnerabilities, and how/where to fix them, please run docker build -t grafana/loki:main-4421efe -f cmd/loki/Dockerfile .
    trivy i grafana/loki:main-4421efe on your branch. If these were not introduced by your PR, please considering fixing them in via a subsequent PR. Thanks!

Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

Should we move the file into tools/packaging/ as it is intended for packaging purposes only.

@kavirajk kavirajk requested a review from chaudum December 18, 2023 13:27
CHANGELOG.md Outdated Show resolved Hide resolved
tools/packaging/promtail-minimal-config.yaml Outdated Show resolved Hide resolved
kavirajk and others added 2 commits December 18, 2023 15:07
@kavirajk kavirajk changed the title chore(promtail): Use minimal init config in npm/deb packaging. chore(promtail): Use minimal init config in rpm/deb packaging. Dec 18, 2023
@kavirajk kavirajk enabled auto-merge (squash) December 18, 2023 14:22
@janfickler
Copy link

@kavirajk, seems the package testing for deb is failing and doesn´t continue with rpm package testing :-)

@janfickler
Copy link

@chaudum, maybe you can help ?

@janfickler
Copy link

janfickler commented Jan 3, 2024

any update @chaudum / @kavirajk / @MichelHollands ?

@kavirajk
Copy link
Contributor Author

kavirajk commented Jan 9, 2024

Apologies for the delay. Got distracted with PTO and other works. I will take a look ASAP.

filename: /tmp/positions.yaml

clients:
- url: http://localhost:3100/loki/api/v1/push
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use consistent list indentation (compare with scrape_configs).

CHANGELOG.md Outdated Show resolved Hide resolved
- localhost
labels:
job: varlogs
__path__: /var/log/messages
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment that this section should be changed by the user after installation?

Choose a reason for hiding this comment

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

just a hint :-) - there is now more failing in the build-pipeline - https://drone.grafana.net/grafana/loki/32891/25/8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Release stage" on drone pipeline seems flaky.

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 think found the issue. The packaging test pipeline generate test log and writes to /var/log/test.log and checks if its scraped fine, By using minimal config we scrape only /var/log/messages and doesn't include /var/log/test.log.

I changed the filename from /var/log/test.log -> /var/log/messages instead in the scripts so test logs are properly scraped.

I think that should fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did.

kavirajk and others added 5 commits January 10, 2024 20:19
Co-authored-by: Christian Haudum <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
Previously we are writing logs to /var/log/test.log. But with minimal config we
scrape only /var/log/messages. So write to /var/log/messages instead

Signed-off-by: Kaviraj <[email protected]>
@kavirajk
Copy link
Contributor Author

@chaudum addressed your comments. Also CI passes.

@kavirajk kavirajk requested a review from chaudum January 11, 2024 05:08
@grafanabot
Copy link
Collaborator

Hello @chaudum!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@janfickler
Copy link

still problems here, seems the builds are ok.
is the grafanabot really complaining about labels - @kavirajk / @chaudum

@kavirajk kavirajk merged commit 86f2001 into main Jan 14, 2024
8 checks passed
@kavirajk kavirajk deleted the kavirajk/sane-init-config-packaging branch January 14, 2024 14:10
grafanabot pushed a commit that referenced this pull request Jan 14, 2024
Related issue: #11398

This minimal config scrape only single file thus not overloading the
systems as described in the issue

(cherry picked from commit 86f2001)
@janfickler
Copy link

any updates on this ? @kavirajk / @chaudum ?

poyzannur added a commit that referenced this pull request Jan 17, 2024
…ackaging. (#11676)

Backport 86f2001 from #11511

---

**What this PR does / why we need it**:
Related issue: #11398

This minimal config scrape only single file thus not overloading the
systems as described in the issue

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] Documentation added
- [ ] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/setup/upgrade/_index.md`
- [ ] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](d10549e)
- [ ] If the change is deprecating or removing a configuration option,
update the `deprecated-config.yaml` and `deleted-config.yaml` files
respectively in the `tools/deprecated-config-checker` directory.
[Example
PR](0d4416a)

Co-authored-by: Kaviraj Kanagaraj <[email protected]>
Co-authored-by: Poyzan <[email protected]>
@janfickler
Copy link

@poyzannur / @kavirajk - there is still no new version 2.9.4 released ?

rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
…na#11511)

Related issue: grafana#11398

This minimal config scrape only single file thus not overloading the
systems as described in the issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants