registration-service: new Service to /metrics
#1051
Conversation
xcoulon
requested review from
MatousJobanek,
alexeykazakov,
rajivnathan,
ranakan19,
sbryzak and
mfrancisc
as code owners
xcoulon
changed the title
/api/metrics
I'm curius why you don't pair these PRs? |
The e2e tests are paired with the registration-service, which contains the code to collect and expose the metrics on a new port. But before we can run the e2e tests, I need an update in how the host-operator deploys the registration service (ie, we need a new |
new k8s Service to scrape the HTTP metrics from the registration service, but without exposing them to the current Route (the metrics will be exposed in a separate port in the registration service pods) see codeready-toolchain/registration-service#436 Signed-off-by: Xavier Coulon [email protected]
xcoulon
force-pushed
the
regsvc_metrics_service
branch
from
a7552f7 to
69b594f
Compare
| @@ -86,7 +86,7 @@ func TestReconcile(t *testing.T) { | |||
| Exists(). | |||
| HasConditions( | |||
| toolchainconfig.ToSyncComplete(), | |||
| toolchainconfig.ToRegServiceDeploying("updated resources: [ServiceAccount: registration-service Role: registration-service RoleBinding: registration-service Deployment: registration-service Service: registration-service Route: registration-service Service: api Route: api Service: proxy-metrics-service]")). | |||
| toolchainconfig.ToRegServiceDeploying("updated resources: [ServiceAccount: registration-service Role: registration-service RoleBinding: registration-service Deployment: registration-service Route: registration-service Service: registration-service Service: registration-service-metrics Route: api Service: api Service: proxy-metrics-service]")). | |||
There was a problem hiding this comment.
not related to your changes - I'm a bit confused with those conditions, not sure what are those for 🤔
| - containerPort: 8082 | ||
| - containerPort: 8080 # registration service | ||
| - containerPort: 8081 # proxy | ||
| - containerPort: 8082 # proxy metrics | ||
| name: metrics |
There was a problem hiding this comment.
optional - maybe we could rename this to proxy-metrics
There was a problem hiding this comment.
yes, indeed, although I did not want to bring too many changes here, in case it's used by Konflux 🤷♂️
|
/test e2e |
| @@ -168,6 +154,7 @@ objects: | |||
| namespace: ${NAMESPACE} | |||
| spec: | |||
| host: '' | |||
| path: /api # we don't want to expose anything else (eg: Prometheus metrics) | |||
There was a problem hiding this comment.
this looks like a route for the registration service.
but still, do we really need to limit it? isn't it already limited/isolated using services and ports?
apart from that - would it also allow requests on the /apis path? it's used in openshift as well.
There was a problem hiding this comment.
ohhh, that's a mistake, I thought I reverted that change! 🤦♂️
Signed-off-by: Xavier Coulon <[email protected]>
|
/test e2e |
limited to 15 characters Signed-off-by: Xavier Coulon <[email protected]>
Signed-off-by: Xavier Coulon <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc, xcoulon 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1051 +/- ##
=======================================
Coverage 84.89% 84.89%
=======================================
Files 55 55
Lines 5012 5012
=======================================
Hits 4255 4255
Misses 584 584
Partials 173 173 |
new k8s Service to scrape the HTTP metrics from the registration
service, but without exposing them to the current Route
(the metrics will be exposed in a separate port in the
registration service pods)
see codeready-toolchain/registration-service#436
note: this PR needs to be merged so that the new test in codeready-toolchain/toolchain-e2e#1001 can pass ;)
Signed-off-by: Xavier Coulon [email protected]