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

PWX-35477 : Support Openshift Prometheus for portworx monitoring on OCP 4.14 #1410

Merged
merged 19 commits into from
Feb 1, 2024

Conversation

nikita-bhatia
Copy link
Contributor

@nikita-bhatia nikita-bhatia commented Jan 30, 2024

Issue : OCP upgrade from 4.13.x to 4.14.5 is blocked due to PX prometheus. From OCP 4.14, PX Prometheus stopped working.

Resolution : From OCP 4.14, Portworx deployment on OCP should should use Openshift prometheus stack for monitoring Portworx metrics instead of deploying PX-Prometheus. We need to expose Portworx metrics to Openshift.
To do this, user-workload-monitoring has to be enabled on OCP cluster to use openshift's monitoring and alerting for portworx.

What this PR does / why we need it:

  1. Default autopilot URL for OCP 4.14 changed from http://px-prometheus:9090/ to https://$THANOS_QUERIER_HOST
  2. Creating autopilot-auth-token-secret to store ocp token and ca.cert
  3. Mount ocp token and ca.cert as file in autopilot pod with file names ca-certificates.crt and token.

Which issue(s) this PR fixes (optional)
Closes # https://portworx.atlassian.net/browse/PWX-35477

Testing done

  1. Tested on OCP 4.14 cluster with fresh install.
  2. Tested that secret is created in cluster namespace with correct data.
  3. Verified that default url for autopilot is correct set in stc.
  4. Verified that autopilod pod is mounted with certificate and token files

@nikita-bhatia nikita-bhatia requested a review from a team January 31, 2024 04:41
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (956b239) 75.88% compared to head (38a498d) 75.87%.

Files Patch % Lines
drivers/storage/portworx/component/autopilot.go 73.01% 9 Missing and 8 partials ⚠️
drivers/storage/portworx/util/util.go 69.38% 11 Missing and 4 partials ⚠️
drivers/storage/portworx/portworx.go 80.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1410      +/-   ##
==========================================
- Coverage   75.88%   75.87%   -0.01%     
==========================================
  Files          66       66              
  Lines       18840    18965     +125     
==========================================
+ Hits        14296    14390      +94     
- Misses       3529     3549      +20     
- Partials     1015     1026      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nikita-bhatia nikita-bhatia removed the wip label Jan 31, 2024
@zoxpx
Copy link
Collaborator

zoxpx commented Jan 31, 2024

@piyush-nimbalkar -- would you mind reviewing this?

It is a fairly complicated PR, and you were the author of the original drivers/storage/portworx/component/autopilot.go where the bulk of the changes are from.

drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
Copy link
Contributor

@piyush-nimbalkar piyush-nimbalkar left a comment

Choose a reason for hiding this comment

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

LGTM after addressing the comments.

@nikita-bhatia can we add more UTs for all corner cases like if we don't find the ocp secret or if the right data is not present in it? What happens when we disable and enable autopilot again - it should have the same deployment and the secret should have been deleted when autopilot was disabled.

Let's merge this PR after addressing comments and then add just the UTs in a different PR.

drivers/storage/portworx/portworx.go Outdated Show resolved Hide resolved
drivers/storage/portworx/util/util.go Outdated Show resolved Hide resolved
drivers/storage/portworx/component/autopilot.go Outdated Show resolved Hide resolved
Piyush Nimbalkar and others added 2 commits February 1, 2024 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants