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

Add Prometheus configuration to bundle OCP #305

Merged
merged 13 commits into from
Apr 3, 2024

Conversation

clobrano
Copy link
Contributor

Why we need this PR

To allow User Workload Prometheus (UWP) metrics scraping for NHC, we need to modify existing bundle configuration as well as and deploy new configuration.

Changes made

Currently, only in OCP we are sure to have Prometheus installed, so this PR affects the bundle created for OCP only.

  • created a config/optional/prometheus-ocp directory to hold the new configuration and documentation
    • ca-configmap.yaml: the ConfigMap which will have the CA bundle injected for usage in the client
    • monitor.yaml: the ServiceMonitor (to be installed manually by the user, see more in the readme) which will make the NHC prometheus endpoint visibile
    • readme.md: the complete UWP configuration guide
  • modified config/manifests/ocp/
    • auth_proxy_service_patch.yaml: to modify NHC Service with the annotation to create the serving-cert-secret Secret node-healthcheck-tls
    • deployment_patch.yaml: to modify NHC deployment to mount the required configuration as volumes

Which issue(s) this PR fixes

https://issues.redhat.com/browse/ECOPROJECT-1760

Test plan

To configure NHC with UWP follow the steps in config/optional/prometheus-ocp/readme.md.
To ease the test process, the script hack/ocp/test_create_bundle_ocp_image_in_quay.sh build the bundle image and pushes it to quay.io.

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
- restored `config/manifests/metrics` and `config/optional/prometheus`
  from main.
- created a new `config/manifests/metrics-ocp` and
  `config/optional/prometheus-ocp` with the new User Workload Prometheus
  (UWP) monitoring configuration from this PR.
- updated documentation under `config/manifests/readme.md` with all the
  steps to create the bundle predisposed for UWP and configure UWP
  itself.

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
- Moved config/manifests/metrics-ocp configuration files to
  config/manifests/ocp, since this configuration will always be part of
  OCP bundle
- Moved readme in config/manifests/metrics-ocp into
  config/optional/prometheus-ocp

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Copy link
Contributor

openshift-ci bot commented Mar 21, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@clobrano clobrano changed the title Add Prometheus configuration to bundle OCP [WIP} Add Prometheus configuration to bundle OCP Mar 21, 2024
@clobrano clobrano changed the title [WIP} Add Prometheus configuration to bundle OCP [WIP] Add Prometheus configuration to bundle OCP Mar 21, 2024
@clobrano clobrano force-pushed the bundle-ocp-metrics-1 branch 6 times, most recently from bf283de to acb8d03 Compare March 21, 2024 08:55
@clobrano clobrano changed the title [WIP] Add Prometheus configuration to bundle OCP Add Prometheus configuration to bundle OCP Mar 21, 2024
@clobrano
Copy link
Contributor Author

/test 4.15-openshift-e2e

The target was already building OCP bundle.
Changing the name to reflect this behavior.

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
@razo7
Copy link
Member

razo7 commented Mar 21, 2024

The test didn't start. Lets try again
/test 4.15-openshift-e2e

Add a test script to build the OCP bundle with prometheus metrics, but
pushing the images to quay.io/username (not medik8s!) for testing.

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Makefile Outdated Show resolved Hide resolved
config/manifests/ocp/deployment_patch.yaml Outdated Show resolved Hide resolved
config/optional/prometheus-ocp/readme.md Show resolved Hide resolved
config/optional/prometheus-ocp/readme.md Show resolved Hide resolved
Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
OCP Metrics configuration was moved in config/manifests/ocp and
config/manifests/metrics-ocp removed, so this target is not useful
anymore.

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Makefile Outdated
@@ -375,11 +375,6 @@ bundle-metrics: bundle-base ## Generate bundle manifests and metadata with metri
$(KUSTOMIZE) build config/manifests/metrics | $(OPERATOR_SDK) generate --verbose bundle -q --overwrite --version $(VERSION) $(BUNDLE_METADATA_OPTS)
$(MAKE) bundle-validate

.PHONY: bundle-metrics-ocp
Copy link
Member

Choose a reason for hiding this comment

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

there still is bundle-build-metrics-ocp and container-build-metrics-ocp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed also bundle-metrics 👍

Copy link
Member

Choose a reason for hiding this comment

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

removed also bundle-metrics

hm, IIRC the idea was that it can be used to manually create a k8s bundle some non OCP specific prometheus bits... 🤔

Copy link
Member

Choose a reason for hiding this comment

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

other than that lgtm! 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, let me check again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, IIUC

bundle-metrics + bundle-build-metrics + container-build-metrics

should to be restored (or at least the first two)

Copy link
Member

@razo7 razo7 left a comment

Choose a reason for hiding this comment

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

NIT: Can you be (in a few places) more specific about which build/bundle are you building or testing?
Considering that upstream we are more K8s front, thus the general tests in upstream CI might be more relevant for K8s and less to OCP Prometheus

.github/workflows/pre-submit.yaml Outdated Show resolved Hide resolved
@@ -30,7 +30,7 @@ jobs:
run: make manifests bundle-k8s bundle-reset test
Copy link
Member

Choose a reason for hiding this comment

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

bundle-k8s?
If we are testing upstream OCP then it should be bundle-metrics-ocp, otherwise the Test container build OCP Prometheus should be changed to Test container build Kubernetes and the target below as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for unittests and catching unwanted bundle changes (see ea5e7be), so it looks right to me. I'll make it clearer changing the name.

Regarding the "Test container build", while it is true we are more keen on k8s here, I think it would be better to test both, as it would be relevant to catch breaking changes at this early stage. For this reason I'd test both k8s and ocp container builds

wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

Makefile Outdated Show resolved Hide resolved
@razo7
Copy link
Member

razo7 commented Apr 2, 2024

/lgtm
Giving a chance to get more reviews
/hold

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
…unittest action

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

👍🏼 🙂

@openshift-ci openshift-ci bot added the lgtm label Apr 3, 2024
Copy link
Contributor

openshift-ci bot commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano, slintes

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:

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

@clobrano clobrano marked this pull request as ready for review April 3, 2024 08:04
@openshift-ci openshift-ci bot requested review from beekhof and mshitrit April 3, 2024 08:04
@clobrano
Copy link
Contributor Author

clobrano commented Apr 3, 2024

/retest

@clobrano
Copy link
Contributor Author

clobrano commented Apr 3, 2024

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 1133f71 into medik8s:main Apr 3, 2024
19 checks passed
@clobrano clobrano deleted the bundle-ocp-metrics-1 branch April 3, 2024 12:24
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.

None yet

3 participants