Skip to content

MON-2193: pkg/manifests: Expose retention size settings for Platform Prometheus #1579

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
Merged
merged 1 commit into from
Apr 19, 2022
Merged

MON-2193: pkg/manifests: Expose retention size settings for Platform Prometheus #1579

merged 1 commit into from
Apr 19, 2022

Conversation

slashpai
Copy link
Member

@slashpai slashpai commented Mar 3, 2022

MON-2216: Expose retention size settings for Platform Prometheus in the CMO configuration

Introduce a new "retentionSize" field under the prometheusK8s key
When neither retention nor retentionSize are defined, a default of 15d for time retention applies.
When "retentionSize" is defined and "retention" isn't, only size retention applies.
When "retentionSize" isn't defined and "retention" is, only time retention applies
When both time and size retention fields are defined, both apply.

Signed-off-by: Jayapriya Pai [email protected]

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2022
@openshift-ci openshift-ci bot requested review from bison and philipgough March 3, 2022 10:20
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2022
@philipgough
Copy link
Contributor

/skip

philipgough
philipgough reviewed Mar 3, 2022
View reviewed changes
@slashpai slashpai force-pushed the retentionSize branch 2 times, most recently from d79216b to 5d0a75c Compare March 3, 2022 11:00
@slashpai
Copy link
Member Author

slashpai commented Mar 3, 2022

/retest

2 similar comments
@slashpai
Copy link
Member Author

slashpai commented Mar 3, 2022

/retest

@slashpai
Copy link
Member Author

slashpai commented Mar 4, 2022

/retest

@slashpai slashpai changed the title WIP: pkg/manifests: Expose retention size settings for Platform Prometheus pkg/manifests: Expose retention size settings for Platform Prometheus Mar 4, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2022
@slashpai
Copy link
Member Author

slashpai commented Mar 4, 2022

@philipgough @simonpasquier Please review when you get a chance

philipgough
philipgough approved these changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@slashpai
Copy link
Member Author

slashpai commented Mar 4, 2022

cc: @openshift/team-monitoring-qe-approvers

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@philipgough
Copy link
Contributor

/skip

@philipgough
Copy link
Contributor

/retest-required

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@slashpai
Copy link
Member Author

slashpai commented Mar 4, 2022

/retest

simonpasquier
simonpasquier reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

Eventually CMO should run more validations on the user inputs to avoid pushing bad configuration but it's something that we should address globally so I'm mostly fine with this PR.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2022
@slashpai
Copy link
Member Author

Eventually CMO should run more validations on the user inputs to avoid pushing bad configuration but it's something that we should address globally so I'm mostly fine with this PR.

Once we add validating webhook in Prometheus operator for the fields it should be enough? Or should we add validations for user inputs on top of it?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2022
@slashpai
Copy link
Member Author

Apart from small comment it looks good 👍

Thank you :), I have updated the test

sthaha
sthaha reviewed Apr 14, 2022
View reviewed changes
Copy link
Contributor

@sthaha sthaha left a comment

Choose a reason for hiding this comment

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

/lgtm

I think string comparison == "" or " != ""` should make use of Trim(). Applicable also when you set it.

RetentionSize = strings.Trim(config.RetentionSize)

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2022
@slashpai
Copy link
Member Author

/lgtm

I think string comparison == "" or " != ""` should make use of Trim(). Applicable also when you set it.

RetentionSize = strings.Trim(config.RetentionSize)

Good suggestion, I will update :)

@slashpai
Copy link
Member Author

/lgtm
I think string comparison == "" or " != ""` should make use of Trim(). Applicable also when you set it.

RetentionSize = strings.Trim(config.RetentionSize)

Good suggestion, I will update :)

Also we have other fields as well which will be good to use trim, can do that in separate PR?

simonpasquier
simonpasquier reviewed Apr 19, 2022
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

/lgtm

@sthaha suggestion would make sense but IMHO it should be addressed consistently since it affects not only retention settings. As such I'm fine with the PR as it is and we can have a follow-up ticket to track the suggested improvement.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2022
@slashpai
pkg/manifests: Expose retention size settings for Platform Prometheus
44a1f2b 
Introduce a new "retentionSize" field under the prometheusK8s key
When neither retention nor retentionSize are defined, a default of 15d for time retention applies.
When "retentionSize" is defined and "retention" isn't, only size retention applies.
When "retentionSize" isn't defined and "retention" is, only time retention applies
When both time and size retention fields are defined, both apply.

Signed-off-by: Jayapriya Pai <[email protected]>
@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 19, 2022
@slashpai
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2022
@simonpasquier
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PhilipGough, simonpasquier, slashpai, sthaha

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [PhilipGough,simonpasquier,slashpai,sthaha]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@slashpai
Copy link
Member Author

/retest

1 similar comment
@slashpai
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@slashpai
Copy link
Member Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 19, 2022

@slashpai: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 21da4f1 into openshift:master Apr 19, 2022
@slashpai slashpai deleted the retentionSize branch October 13, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpasquier simonpasquier simonpasquier left review comments

@sthaha sthaha sthaha left review comments

@JoaoBraveCoding JoaoBraveCoding JoaoBraveCoding left review comments

@philipgough philipgough philipgough approved these changes

@bison bison Awaiting requested review from bison

Assignees

@simonpasquier simonpasquier

@sthaha sthaha

@philipgough philipgough

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@slashpai @philipgough @openshift-bot @lihongyan1 @simonpasquier @Senthamilarasu-STA @jan--f @sthaha @JoaoBraveCoding @openshift-merge-robot