Remove metrics logic from cmd/helm-operator/main.go #3451
Conversation
| @@ -16,17 +16,14 @@ package main | |||
|
|
|||
There was a problem hiding this comment.
Is not missing remove from cmd/ansible-operator/main.go as well?
Also, should we not add the fragment here with TBD?
There was a problem hiding this comment.
will be doing separate PR for Ansible, and regarding fragment I had a question if we are adding individual fragments for removal PRs. As in the meeting, some one was saying about doing one big Fragment with all PRs. I just need clarification on that.
There was a problem hiding this comment.
We can add many fragments in the same pr. IMO we can do all in the same PR as we are doing in the other cases to be easier to check that and we do not miss anything.
- remove addmetrics impl from anisble and helm
- remove the pkg/kube-metrics
- remove what is not long used from pkg/k8sutil
- add 4 fragments so far 1 for ansible, 1 for helm, 1 for pkg/kube-metrics, 1 for the pkg/utils removed funcs
There was a problem hiding this comment.
Yeah, we should add individual removal fragments. Those require migration sections, but we're doing this:
migration:
header: <what got removed>
body: TBDThen when we go to do the release, we'll run the generator and then rewrite the release notes, using what's there to remind us what to cover.
There was a problem hiding this comment.
okay, Ill follow above template, and add fragment for this PR.
There was a problem hiding this comment.
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Need to also:
- remove the check for the service monitor in
hack/tests/e2e-helm.sh - remove the
envsection for the pod spec template ininternal/plugins/helm/v1/scaffolds/templates/manager/config.go
openshift-ci-robot
added
the
needs-rebase
…m podspec template from helm plugin scaffold
bharathi-tenneti
force-pushed
the
helm-metrics
branch
from
053a754 to
e31105b
Compare
openshift-ci-robot
removed
the
needs-rebase
hack/tests/e2e-helm.sh
Outdated
| metrics_service="nginx-operator-controller-manager-metrics-service" | ||
|
|
||
| # verify that metrics service was created | ||
| if ! timeout 60s bash -c -- "until kubectl get service/${metrics_service} --namespace=${operator_namespace} > /dev/null 2>&1; do sleep 1; done"; | ||
| then | ||
| error_text "Failed to get metrics service" | ||
| operator_logs | ||
| exit 1 | ||
| fi | ||
|
|
||
|
|
||
| serviceaccount_secret=$(kubectl get serviceaccounts default -n ${operator_namespace} -o jsonpath='{.secrets[0].name}') | ||
| token=$(kubectl get secret ${serviceaccount_secret} -n ${operator_namespace} -o jsonpath={.data.token} | base64 -d) | ||
|
|
||
| # verify that the metrics endpoint exists | ||
| if ! timeout 60s bash -c -- "until kubectl run --attach --rm --restart=Never --namespace=${operator_namespace} test-metrics --image=${metrics_test_image} -- -sfkH \"Authorization: Bearer ${token}\" https://${metrics_service}:8443/metrics; do sleep 1; done"; | ||
| then | ||
| error_text "Failed to verify that metrics endpoint exists" | ||
| operator_logs | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
All of this should be kept. We still have a metrics service in the Kubebuilder layout, but it is created alongside the operator as part of the kustomize manifests.
The only thing we need to remove from the e2e is the test for the presence of the servicemonitors/nginx-operator-metrics (which you've already done)
Description: Remove kubemetrics package reference logic from
cmd/helm-operator/main.go.Motivation: Now that we have handler to setup metrics on primary resources using
handler.InstrumentedEnqueueRequestForObject{}, we do not separateKubeMetricsto setup metrics.Partially closes, #3354