Skip to content

fix(CFP & Enhancement): remove service monitor enable values #36013

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
Nov 25, 2024
Merged

fix(CFP & Enhancement): remove service monitor enable values #36013

merged 1 commit into from
Nov 25, 2024

Conversation

saiaunghlyanhtet
Copy link
Member

@saiaunghlyanhtet saiaunghlyanhtet commented Nov 18, 2024 •
edited
Loading

Title: Decouple Metrics Service Creation from ServiceMonitor in Cilium Helm Chart

Description: This pull request addresses the issue where the creation of metrics services for cilium-agent and cilium-operator is tied to the creation of ServiceMonitors in the Cilium Helm chart. Currently, enabling Prometheus with the default Helm values results in both the creation of metrics services and ServiceMonitors. This tight coupling prevents use cases where only metrics services are required without the ServiceMonitor.

Changes:

Updated the Cilium Helm chart templates for cilium-agent and cilium-operator to decouple the creation of metrics services from the creation of ServiceMonitors.
Modified the condition that checks for the prometheus.serviceMonitor.enabled flag, ensuring the creation of metrics services is independent of ServiceMonitor creation.
Motivation:

Users may want to deploy the metrics service without relying on ServiceMonitors, especially in environments where a custom Prometheus setup is used or where ServiceMonitors are not needed.
The change allows for more flexible Prometheus integration without forcing the creation of ServiceMonitors.
Fixes: #34466

Decouples the creation of metrics services from ServiceMonitors in the Cilium Helm chart, providing greater flexibility for Prometheus integration.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 18, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 18, 2024
@saiaunghlyanhtet saiaunghlyanhtet marked this pull request as ready for review November 18, 2024 13:06
@saiaunghlyanhtet saiaunghlyanhtet requested review from a team as code owners November 18, 2024 13:06
@gandro
Copy link
Member

gandro commented Nov 18, 2024

Thanks! While I understand the use-case, this will now create a service for users who are using metrics, but don't care about service monitor, but also don't want or need services.

Would it make sense to instead introduce a new variable prometheus.metricsService? And then gate the service object via (or .Values.prometheus.serviceMonitor.enabled .Values.prometheus.metricsService)?

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. area/helm Impacts helm charts and user deployment experience labels Nov 18, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 18, 2024
@saiaunghlyanhtet
Copy link
Member Author

Thanks! While I understand the use-case, this will now create a service for users who are using metrics, but don't care about service monitor, but also don't want or need services.

Would it make sense to instead introduce a new variable prometheus.metricsService? And then gate the service object via (or .Values.prometheus.serviceMonitor.enabled .Values.prometheus.metricsService)?

Thanks for the review. I also think it makes sense to introduce a new variable to help some users who want more control over service creation. Can I try it now?

@gandro
Copy link
Member

gandro commented Nov 18, 2024

Thanks! While I understand the use-case, this will now create a service for users who are using metrics, but don't care about service monitor, but also don't want or need services.
Would it make sense to instead introduce a new variable prometheus.metricsService? And then gate the service object via (or .Values.prometheus.serviceMonitor.enabled .Values.prometheus.metricsService)?

Thanks for the review. I also think it makes sense to introduce a new variable to help some users who want more control over service creation. Can I try it now?

Sure, you are more than welcome to give it a shot.

Since I see you are a first time contributor, please note the following when adding new Helm values:

  • Add the Helm value to the values.yaml.tmpl (not values.yaml)
  • Re-generate the auto-generated files using the following two commands:
    1. make -C install/kubernetes
    2. make -C Documentation update-helm-values

Thanks!

@gandro
Copy link
Member

gandro commented Nov 19, 2024

@saiaunghlyanhtet I see you pushed to the branch, but no changes adding the values seem to have been made?

@maintainer-s-little-helper
Copy link

Commit 5330e84 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 19, 2024
@maintainer-s-little-helper
Copy link

Commits 5330e84, 5a382bc do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@saiaunghlyanhtet
Copy link
Member Author

@saiaunghlyanhtet I see you pushed to the branch, but no changes adding the values seem to have been made?

@gandro I am sorry. I forgot and synced my forked repo via GitHub. Now, I have merge commits that are not signed off. But I made the changes in my latest commit.

@saiaunghlyanhtet
Copy link
Member Author

@gandro Can you review the latest changes? Because of unsigned merge commits, do I need to create a new pull request so that history can be clear?

@gandro
Copy link
Member

gandro commented Nov 19, 2024 •
edited
Loading

I think we should set the metricsService variable to false by default, and gate the creation of the services with (or .Values.prometheus.serviceMonitor.enabled .Values.prometheus.metricsService). Other than that this looks good.

You should be able to clean up the git history via git rebase or git reset and a force push, without having to create a new PR.

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 19, 2024
@saiaunghlyanhtet
Copy link
Member Author

I think we should set the metricsService variable to false by default, and gate the creation of the services with (or .Values.prometheus.serviceMonitor.enabled .Values.prometheus.metricsService). Other than that this looks good.

You should be able to clean up the git history via git rebase or git reset and a force push, without having to create a new PR.

@gandro I made changes as requested. Can you check it again, please?

gandro
gandro reviewed Nov 19, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks, seems like the rebase worked. There are still some inconsistencies in the value use that I didn't spot before

gandro
gandro reviewed Nov 19, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Starting to look good. One issue still and once done please squash all your commits into one (e.g. with git rebase -i main)

gandro
gandro approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thank you!

@gandro
Copy link
Member

gandro commented Nov 21, 2024

/test

@gandro
Copy link
Member

gandro commented Nov 21, 2024

CI is red because of an unrelated issue: #36100

I'll rebase and restart CI

@gandro
Copy link
Member

gandro commented Nov 21, 2024

/test

@saiaunghlyanhtet
fix: add a new metricsSerice var for better control of service creation
ca71750 
Signed-off-by: saiaunghlyanhtet <saiaunghlyanhtet2003@gmail.com>
@gandro
Copy link
Member

gandro commented Nov 25, 2024

/test

@gandro
Copy link
Member

gandro commented Nov 25, 2024

@saiaunghlyanhtet Just a heads-up, there is no need to rebase unless there are issues with CI. In fact, rebasing invalidates the last CI run, so we will have to re-run CI again after each rebase.

@gandro gandro enabled auto-merge November 25, 2024 12:30
@gandro gandro added this pull request to the merge queue Nov 25, 2024
Merged via the queue into cilium:main with commit a89a36c Nov 25, 2024
64 checks passed
@lucasfcnunes
Copy link
Contributor

FYI @saiaunghlyanhtet @gandro

There are at least 6 **.serviceMonitor.enabled gates. This PR only did 2 of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@joamaki joamaki Awaiting requested review from joamaki joamaki is a code owner automatically assigned from cilium/sig-k8s

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience area/metrics Impacts statistics / metrics gathering, eg via Prometheus. kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Creating metrics services is tied to the creation of service monitors
3 participants
@saiaunghlyanhtet @gandro @lucasfcnunes