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

fix: remove adding new resources limits and requests from 1.3 #971

Merged
merged 3 commits into from
Oct 2, 2020
Merged

fix: remove adding new resources limits and requests from 1.3 #971

merged 3 commits into from
Oct 2, 2020

Conversation

perk-sumo
Copy link
Contributor

@perk-sumo perk-sumo commented Oct 2, 2020 •
edited
Loading

I propose to remove resources from Prometheus in 1.3 release and reintroduce it in 2.0.
It has been added as a fix for #885

I see adding resources for Prometheus were wrong values can break the solution as a breaking change.

The same story with setup job and for Thanos.

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@perk-sumo perk-sumo added this to the v1.3 milestone Oct 2, 2020
@perk-sumo perk-sumo force-pushed the perk-remove-requests-from-prometheus branch from fe82166 to d477f13 Compare October 2, 2020 07:33
@perk-sumo perk-sumo changed the title fix: remove requests from Prometheus fix: remove resources from Prometheus Oct 2, 2020
@perk-sumo perk-sumo linked an issue Oct 2, 2020 that may be closed by this pull request
Prometheus container has no limits set by default #885
Closed
@perk-sumo perk-sumo changed the title fix: remove resources from Prometheus fix: remove adding new resources limits and requests from 1.3 Oct 2, 2020
@perk-sumo perk-sumo force-pushed the perk-remove-requests-from-prometheus branch 2 times, most recently from d953e48 to c6b5a2f Compare October 2, 2020 08:33
@perk-sumo perk-sumo force-pushed the perk-remove-requests-from-prometheus branch from c6b5a2f to 29eeed0 Compare October 2, 2020 08:35
sumo-drosiek
sumo-drosiek reviewed Oct 2, 2020
View reviewed changes
deploy/helm/prometheus-overrides.yaml
Comment on lines +174 to +176
# requests:
# cpu: 1m
# memory: 8Mi
Copy link
Contributor

Choose a reason for hiding this comment

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

❓

andrzej-stencel
andrzej-stencel approved these changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

We could just remove the data instead of commenting out.

@perk-sumo
Copy link
Contributor Author

We could just remove the data instead of commenting out.

@astencel-sumo this is a configuration file so it serves as an example.

@perk-sumo perk-sumo merged commit 1f5da11 into master Oct 2, 2020
@perk-sumo perk-sumo deleted the perk-remove-requests-from-prometheus branch October 2, 2020 10:01
@perk-sumo perk-sumo mentioned this pull request Oct 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sumo-drosiek sumo-drosiek sumo-drosiek approved these changes

@andrzej-stencel andrzej-stencel andrzej-stencel approved these changes

@frankreno frankreno Awaiting requested review from frankreno

@vsinghal13 vsinghal13 Awaiting requested review from vsinghal13

@pmalek-sumo pmalek-sumo Awaiting requested review from pmalek-sumo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

Prometheus container has no limits set by default
3 participants
@perk-sumo @sumo-drosiek @andrzej-stencel