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

feat: expose prometheus nodeselector and tolerations in monitoringstack CRD #293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bng0y
Copy link

@bng0y bng0y commented May 25, 2023

Allows to isolate the Prometheus pods when necessary.
Context: https://issues.redhat.com/browse/SDE-2823

@bng0y bng0y requested a review from a team as a code owner May 25, 2023 09:33
@bng0y bng0y requested review from sthaha and slashpai May 25, 2023 09:33
@vimalk78
Copy link
Contributor

If this PR changes the Prometheus managedFields, please update this test case to keep managed Fields info updated.

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.

Maybe the fields should be located under .spec rather than .spec.prometheusConfig and apply to all child resources of the MonitoringStack resource?

@sthaha @jan--f WDYT?

@sthaha
Copy link
Collaborator

sthaha commented May 25, 2023

Maybe the fields should be located under .spec rather than .spec.prometheusConfig and apply to all child resources of the MonitoringStack resource?

@simonpasquier, I like the idea to have a global setting for all child resources, however should we also allow each child resource to have different setting to the global one?

@sthaha
Copy link
Collaborator

sthaha commented May 25, 2023

If this PR changes the Prometheus managedFields, please update this test case to keep managed Fields info updated.

@vimalk78 shouldn't that test fail in that case 🤔

@vimalk78
Copy link
Contributor

vimalk78 commented May 26, 2023

@vimalk78 shouldn't that test fail in that case

it would, but the new parameters are optional, so unless explicitly set in the object the managedFields wont change.

@simonpasquier
Copy link
Contributor

@sthaha my gut feeling is that in most cases, you want the monitoring pods to run on the same set of nodes (e.g. infra) so having a top-level field is probably what makes the most sense. We can argue later if there's a need for individual settings for each component but I'd rather make the mainstream situation easy to configure.

@sthaha
Copy link
Collaborator

sthaha commented May 29, 2023

@simonpasquier +1 to having it global for now

We can argue later if there's a need for individual settings for each component but I'd rather make the mainstream situation easy to configure.

+1 to YAGNI :)

Comment on lines +175 to +176
NodeSelector: config.NodeSelector,
Tolerations: config.Tolerations,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bng0y , Like @simonpasquier suggested, lets make this global and apply the same to all resources that ObO creates.

Copy link
Collaborator

@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.

Could you please consider implementing the suggestion.

@sthaha
Copy link
Collaborator

sthaha commented Jun 19, 2023

@bng0y , It would be great to have this feature added to ObO. Do you need help implementing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants