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

Expose Prometheus port when custom config files are used #1827

Closed
wants to merge 6 commits into from

Conversation

suryadutta
Copy link

@suryadutta suryadutta commented Aug 30, 2023

Note

This is almost a duplicate of this PR: #1165
I'm opening this because that PR was abandoned (was opened by a former employee at my company, SpotHero)


Right now, the patch that adds the Prometheus container port when Prometheus is enabled does not run if these two configurations are provided in the spec:

  • SparkApplication.spec.monitoring.metricsPropertiesFile (link to docs)
  • SparkApplication.spec.monitoring.prometheus.configFile (link to docs)

This leads to an issue where the jmx-exporter port doesn't get exposed in either driver or executor containers if both of these configurations are set. More details here: #1053


The current behavior of the code is as follows:

If prometheus is enabled AND both metricsPropertiesFile and prometheus.configFile are not set:

  1. create a patch to create the Prometheus container port
  2. create a patch to create the VolumeMount at config.PrometheusConfigMapMountPath

However, only the second patch (VolumeMount) needs to be skipped if both metricsPropertiesFile and prometheus.configFile are set, since the default configs won't be mounted there.

This PR proposes modifying the logic to reflect this correct state.

@google-cla
Copy link

google-cla bot commented Aug 30, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@suryadutta suryadutta marked this pull request as ready for review August 30, 2023 20:03
@suryadutta
Copy link
Author

HI @liyinan926. Can you help review this PR?

Thank you!

@suryadutta
Copy link
Author

@liyinan926 - this PR is still waiting for a review.

If this project is no longer actively maintained, could you let the community know? I think this project is still being actively used by a large group of people/companies.

@liyinan926
Copy link
Collaborator

@suryadutta Thanks for the PR. I have not been able to do much for this repo in the past couple of years. We are trying to figure out what to do for this repo at this point.

@suryadutta
Copy link
Author

@suryadutta Thanks for the PR. I have not been able to do much for this repo in the past couple of years. We are trying to figure out what to do for this repo at this point.

@liyinan926 Totally understand, appreciate the response!

@suryadutta suryadutta closed this Jun 11, 2024
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

2 participants