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

Helm: allow GrafanaAgent tolerations #10613

Merged
merged 19 commits into from Oct 4, 2023
Merged

Helm: allow GrafanaAgent tolerations #10613

merged 19 commits into from Oct 4, 2023

Conversation

ngc4579
Copy link
Contributor

@ngc4579 ngc4579 commented Sep 16, 2023

What this PR does / why we need it:
Helm: Allow setting tolerations for GrafanaAgent pods in order to have them scheduled to tainted nodes as well.

Which issue(s) this PR fixes:
Fixes #10575

Special notes for your reviewer:
n/a

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

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@ngc4579 ngc4579 marked this pull request as ready for review September 16, 2023 15:24
@ngc4579 ngc4579 requested a review from a team as a code owner September 16, 2023 15:24
@ngc4579
Copy link
Contributor Author

ngc4579 commented Sep 16, 2023

Note that I've checked adding add-to-release-notes label yet obviously can't set that label myself.

@ngc4579
Copy link
Contributor Author

ngc4579 commented Sep 16, 2023

Hm, this has a failing check (drone) which I don't quite understand...

@JStickler
Copy link
Contributor

JStickler commented Sep 18, 2023 •
edited

Hm, this has a failing check (drone) which I don't quite understand...

Drone runs a lot of the automated check on PRs.
To view the failed check, you can click the Details link. In this case, because you've made changes to the Helm chart, you need to regenerate the Helm documentation. The Error message in Drone is Please generate Helm Chart reference by running 'make -C docs sources/setup/install/helm/reference.md'

JStickler
JStickler reviewed Sep 18, 2023
View reviewed changes
Copy link
Contributor

@JStickler JStickler left a comment

Choose a reason for hiding this comment

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

[docs team] LGTM. Engineering will still need to sign off on this.

@JStickler
Copy link
Contributor

Also, for the drone check that is failing, Please generate Helm Chart reference by running 'make -C docs sources/setup/install/helm/reference.md'

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Sep 18, 2023
@ngc4579
Copy link
Contributor Author

ngc4579 commented Sep 18, 2023

@JStickler Oops, I wasn't aware that I was supposed to execute that command... Anyway, done now. Sorry, this is my first ever PR... 🙃

@JStickler
Copy link
Contributor

@ngc4579 No worries. Sometimes it takes a while to learn your way around a particular repository and what's running as far as CI/CD checks and how to troubleshoot those build errors. And Congrats on your first Loki PR!

@ngc4579
Copy link
Contributor Author

ngc4579 commented Sep 19, 2023

@JStickler There's another one in job 'Lint Helm Chart':

Documentation not up to date. Please run helm-docs and commit changes!

What is helm-docs?

@MichelHollands
Copy link
Contributor

@ngc4579 Thanks for the contribution. Can you merge from main and update the version? The next one is 5.24.0. You also have to update the production/helm/loki/README.md file. Then the CI won't complain anymore.

@ngc4579
Copy link
Contributor Author

ngc4579 commented Sep 22, 2023 •
edited

@MichelHollands Thanks for your feedback. I was unsure what to do about the CI complaint. Do I need to manually update docs? CI mentioned helm-docs. CI seems good now.

@ngc4579
Copy link
Contributor Author

ngc4579 commented Sep 25, 2023 •
edited

Latest merge from main has introduced another CI build failure...

@MichelHollands MichelHollands merged commit 5b9de94 into grafana:main Oct 4, 2023
7 checks passed
@ngc4579 ngc4579 deleted the feature/grafanaagent-tolerations branch October 9, 2023 03:44
@ngc4579 ngc4579 mentioned this pull request Oct 9, 2023
Merged
7 tasks
MichelHollands added a commit that referenced this pull request Oct 11, 2023
@ngc4579 @MichelHollands
Helm: Fix GrafanaAgent tolerations scope (#10813)
c182940 
**What this PR does / why we need it**:
`monitoring.selfMonitoring.grafanaAgent.tolerations` was evaluated
inside a wrong scope in the `GrafanaAgent` template, thus having no
effect at all.

**Which issue(s) this PR fixes**:
n/a

**Special notes for your reviewer**:
Fixes an issue with my first (ever)
[PR](#10613) that I thought was
rather trivial but somehow turned out not to be. At least, regarding my
inability to correctly count `with` / `end` scope markers... :(

**Checklist**
- [X] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [X] `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`
- [X] 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)

---------

Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
@ngc4579 @JStickler
@MichelHollands @rhnasc
Helm: allow GrafanaAgent tolerations (grafana#10613)
69998d5 
**What this PR does / why we need it**:
Helm: Allow setting tolerations for GrafanaAgent pods in order to have
them scheduled to tainted nodes as well.

**Which issue(s) this PR fixes**:
Fixes grafana#10575

**Special notes for your reviewer**:
n/a

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] Documentation added
- [ ] Tests updated
- [x] `CHANGELOG.md` updated
- [x] 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`
- [x] 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](grafana@d10549e)

---------

Co-authored-by: J Stickler <julie.stickler@grafana.com>
Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
@ngc4579 @MichelHollands @rhnasc
Helm: Fix GrafanaAgent tolerations scope (grafana#10813)
a6937db 
**What this PR does / why we need it**:
`monitoring.selfMonitoring.grafanaAgent.tolerations` was evaluated
inside a wrong scope in the `GrafanaAgent` template, thus having no
effect at all.

**Which issue(s) this PR fixes**:
n/a

**Special notes for your reviewer**:
Fixes an issue with my first (ever)
[PR](grafana#10613) that I thought was
rather trivial but somehow turned out not to be. At least, regarding my
inability to correctly count `with` / `end` scope markers... :(

**Checklist**
- [X] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [X] `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`
- [X] 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](grafana@d10549e)

---------

Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JStickler JStickler JStickler left review comments

@MichelHollands MichelHollands MichelHollands approved these changes

Assignees
No one assigned
Labels
area/helm size/S type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Loki SSD / Helm: Allow Tolerations for GrafanaAgents
4 participants
@ngc4579 @CLAassistant @JStickler @MichelHollands