Expose metrics endpoint #486

dimakis · 2023-07-18T00:53:40Z

Using port 9090 , the conventional prom port.

this PR in odh-deployer will need to be updated also, I will get to it asap

Makefile

cmd/kar-controllers/app/metrics.go

Makefile

z103cb · 2023-07-18T10:57:14Z

@dimakis,
You might want to take a look at the error message reported by the e2e test:

kubectl kuttl test --config /home/travis/gopath/src/github.com/project-codeflare/multi-cluster-app-dispatcher/test/kuttl-test.yaml
=== RUN   kuttl
    harness.go:462: starting setup
    harness.go:252: running tests using configured kubeconfig.
    harness.go:275: Successful connection to cluster at: https://127.0.0.1:46073
    logger.go:42: 03:12:58 |  | running command: [sh -c helm upgrade  --install mcad-controller deployment/mcad-controller --namespace kube-system --wait --set loglevel=${LOG_LEVEL} --set resources.requests.cpu=1000m --set resources.requests.memory=1024Mi --set resources.limits.cpu=4000m --set resources.limits.memory=4096Mi --set image.repository=$IMAGE_REPOSITORY_MCAD --set image.tag=$IMAGE_TAG_MCAD --set image.pullPolicy=$MCAD_IMAGE_PULL_POLICY --set configMap.quotaEnabled='"true"' --set quotaManagement.rbac.apiGroup=ibm.com --set quotaManagement.rbac.resource=quotasubtrees  --set configMap.name=mcad-controller-configmap --set configMap.preemptionEnabled='"true"']
    logger.go:42: 03:12:58 |  | WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/travis/gopath/src/github.com/project-codeflare/multi-cluster-app-dispatcher/kubeconfig
    logger.go:42: 03:12:58 |  | WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/travis/gopath/src/github.com/project-codeflare/multi-cluster-app-dispatcher/kubeconfig
    logger.go:42: 03:12:59 |  | Release "mcad-controller" does not exist. Installing it now.
    logger.go:42: 03:13:02 |  | Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: ValidationError(Deployment.spec.template.spec.containers[0]): unknown field "metrics" in io.k8s.api.core.v1.Container
    logger.go:42: 03:13:02 |  | command failure, skipping 1 additional commands
    harness.go:513: cleaning up
    harness.go:570: removing temp folder: ""
    harness.go:595: fatal error running commands: exit status 1

deployment/mcad-controller/templates/deployment.yaml

asm582 · 2023-07-21T15:20:20Z

@dimakis We need to make sure that a build passes, can you please check why are the builds failing?

z103cb · 2023-08-03T09:55:44Z

/lgtm

I will approve this PR once @astefanutti has a chance to chime in.

cmd/kar-controllers/app/metrics.go

astefanutti · 2023-08-03T11:14:37Z

Makefile

+	echo "Host architecture: $(shell uname -m)"
+	@HOST_ARCH=$(shell uname -m); \
+	if [ "$$HOST_ARCH" = "arm64" ]; then \
+		if [ "$(strip $(GO_BUILD_ARGS))" = "" ]; then \


Could this get moved to a separate PR? It's not strictly related to this PR, and we would be able to iterate on that part without blocking the work on the metrics part.

you're right here, to alter this is the only way I could test my changes, hence in this PR but I can move it now 👍

cmd/kar-controllers/app/options/options.go

astefanutti · 2023-08-03T11:17:31Z

cmd/kar-controllers/app/server.go

+    RecordMetrics()
+
+    metricsHandler := http.NewServeMux()
+    metricsHandler.Handle("/metrics", promhttp.Handler())


It would be good to add the default Prometheus error handler (ErrorHandling: promhttp.HTTPErrorOnError)

cmd/kar-controllers/app/server.go

openshift-ci · 2023-08-09T11:06:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from astefanutti. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

openshift-ci · 2023-08-29T11:11:45Z

New changes are detected. LGTM label has been removed.

astefanutti · 2023-08-29T13:09:03Z

/retest

astefanutti · 2023-08-29T13:10:25Z

@dimakis could you squash the refactor / review commits?

asm582 · 2023-08-29T16:32:57Z

@dimakis The build has been consistently failing on the quota management step 3 test, my guess is in server.go or during MCAD startup we are failing to init quota trees for this PR.

dimakis · 2023-09-04T15:30:06Z

@dimakis The build has been consistently failing on the quota management step 3 test, my guess is in server.go or during MCAD startup we are failing to init quota trees for this PR.

I'm not sure I know how to progress this, I'm not familiar with the quota aspect of MCAD. can I have some help here @asm582 ?

…rently this commit also edits the startHealthServer to also start collecting of default prometheus metrics. channels are used to collect potential errors and allow for graceful shutdown of servers

…shutdown once jobctrl is shut down

…rver before controller

…er minor edits from reviews

z103cb · 2023-09-19T07:35:00Z

cmd/kar-controllers/app/server.go

+	wg.Add(1)
+	g.Go(func() error {
+		defer wg.Done()
+		jobctrl.Run(gCtx.Done())


@dimakis,
The jobctrl.Run function is asysnchrouous, it will fire off a few deamon goroutines and return, thusly triggering the defer call and efectively shutting down the process. (the container is constant crashbackoff loop). I would suggest making the jobctrl.Run be synchrounous or revisiting this implementation.

cc: @astefanutti

This is what I see when running the controller:

@z103cb thanks, very good catch. That Go routine should block for the group context channel to close.

astefanutti · 2023-09-19T07:53:10Z

@dimakis @z103cb this PR has become obsolete in the context of project-codeflare/codeflare-operator#279 and should be closed. The metrics registration should moved to the CFO once project-codeflare/codeflare-operator#216 is merged, and be done using the controller-runtime registry.

dimakis commented Jul 18, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

dimakis commented Jul 18, 2023

View reviewed changes

cmd/kar-controllers/app/metrics.go Outdated Show resolved Hide resolved

dimakis commented Jul 18, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

dimakis requested a review from z103cb July 18, 2023 00:58

dimakis force-pushed the expose-metrics-endpoint branch from 5cf0992 to 4d6bf26 Compare July 18, 2023 01:02

dimakis mentioned this pull request Jul 18, 2023

Update MCAD to expose /metrics endpoint #464

Open

z103cb reviewed Jul 18, 2023

View reviewed changes

deployment/mcad-controller/templates/deployment.yaml Outdated Show resolved Hide resolved

ChristianZaccaria mentioned this pull request Aug 1, 2023

refactor: addition of codeflare components as scrape targets and availabil… red-hat-data-services/odh-deployer#365

Closed

8 tasks

dimakis force-pushed the expose-metrics-endpoint branch 2 times, most recently from 4bcf9e1 to c09181f Compare August 3, 2023 08:14

openshift-merge-robot added the needs-rebase label Aug 3, 2023

dimakis force-pushed the expose-metrics-endpoint branch from c09181f to 0db79d4 Compare August 3, 2023 08:26

openshift-merge-robot removed the needs-rebase label Aug 3, 2023

z103cb requested a review from astefanutti August 3, 2023 08:47

openshift-ci bot assigned z103cb Aug 3, 2023

openshift-ci bot added the lgtm label Aug 3, 2023

astefanutti suggested changes Aug 3, 2023

View reviewed changes

openshift-ci bot assigned astefanutti Aug 3, 2023

openshift-ci bot removed the lgtm label Aug 3, 2023

dimakis force-pushed the expose-metrics-endpoint branch from 0db79d4 to cb5f178 Compare August 9, 2023 11:05

openshift-merge-robot added needs-rebase and removed needs-rebase labels Aug 9, 2023

dimakis force-pushed the expose-metrics-endpoint branch from e87fbfa to 96ffc09 Compare August 9, 2023 11:22

openshift-merge-robot added needs-rebase and removed needs-rebase labels Aug 9, 2023

dimakis requested a review from z103cb August 9, 2023 12:42

openshift-ci bot removed the lgtm label Aug 29, 2023

ChristianZaccaria mentioned this pull request Sep 5, 2023

feat: addition of codeflare stack to monitoring red-hat-data-services/odh-deployer#380

Closed

8 tasks

dimakis added 19 commits September 15, 2023 11:57

refactor: addition of root ctx to main

f1abcdc

refactor: addition of metrics address

16ad276

refactor: edit of the deployment and service to expose metrics ports

3298bbf

refactor: edit of run to start controller and health + metrics concur…

4180e7a

…rently this commit also edits the startHealthServer to also start collecting of default prometheus metrics. channels are used to collect potential errors and allow for graceful shutdown of servers

refactor: making sure that the health and metrics servers only start …

e42175c

…shutdown once jobctrl is shut down

refactor: update health and metric port defaults from strings to ints

f35715d

refactor: addition of a generic server to use for health, metrics etc

a20d09d

refactor: use errgroup and new generic-server

3e98689

refactor: update metrics port in deployment to standard prom port

394f411

docs: add license

6cd08e7

refactor: update metrics port to use conventional prom port

fc7abaa

refactor: move metrics funcs to own file

c1518e7

refactor: add better error and use custom timeout

7941a94

refactor: remove not needed comments and context

935698a

refactor: start shutdown in their own goroutines and start metrics se…

4b6e864

…rver before controller

docs: remove duplicate copyright notice

19a5662

refactor: addition of wait groups to ensure order of shutdown and oth…

0d92fca

…er minor edits from reviews

refactor: simplify the concurrency logic

2b5987c

refactor: update after rebase

6a0e175

dimakis force-pushed the expose-metrics-endpoint branch from 4c6b5fb to 6a0e175 Compare September 15, 2023 11:33

z103cb reviewed Sep 19, 2023

View reviewed changes

dimakis closed this Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose metrics endpoint #486

Expose metrics endpoint #486

dimakis commented Jul 18, 2023 •

edited

Loading

z103cb commented Jul 18, 2023

asm582 commented Jul 21, 2023

z103cb commented Aug 3, 2023

astefanutti Aug 3, 2023

dimakis Aug 3, 2023

astefanutti Aug 3, 2023

openshift-ci bot commented Aug 9, 2023

openshift-ci bot commented Aug 29, 2023

astefanutti commented Aug 29, 2023

astefanutti commented Aug 29, 2023

asm582 commented Aug 29, 2023

dimakis commented Sep 4, 2023

z103cb Sep 19, 2023

z103cb Sep 19, 2023

astefanutti Sep 19, 2023

astefanutti commented Sep 19, 2023

Expose metrics endpoint #486

Expose metrics endpoint #486

Conversation

dimakis commented Jul 18, 2023 • edited Loading

z103cb commented Jul 18, 2023

asm582 commented Jul 21, 2023

z103cb commented Aug 3, 2023

astefanutti Aug 3, 2023

Choose a reason for hiding this comment

dimakis Aug 3, 2023

Choose a reason for hiding this comment

astefanutti Aug 3, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Aug 9, 2023

openshift-ci bot commented Aug 29, 2023

astefanutti commented Aug 29, 2023

astefanutti commented Aug 29, 2023

asm582 commented Aug 29, 2023

dimakis commented Sep 4, 2023

z103cb Sep 19, 2023

Choose a reason for hiding this comment

z103cb Sep 19, 2023

Choose a reason for hiding this comment

astefanutti Sep 19, 2023

Choose a reason for hiding this comment

astefanutti commented Sep 19, 2023

dimakis commented Jul 18, 2023 •

edited

Loading