Skip to content

Remove metrics logic from cmd/helm-operator/main.go #3451

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.

Sign up for GitHub

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

Merged
bharathi-tenneti merged 7 commits into from
Jul 17, 2020
Merged

Remove metrics logic from cmd/helm-operator/main.go #3451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5144f4b
Remove metrics logic from cmd/helm-operator/main.go
bharathi-tenneti Jul 17, 2020
bcc99f2
Added changelog fragments, fixing sanity checks
bharathi-tenneti Jul 17, 2020
d5dc739
Remove checks for servicemonitor in test-e2e-helm.sh
bharathi-tenneti Jul 17, 2020
8e9385c
Added changelog fragment
bharathi-tenneti Jul 17, 2020
e31105b
Removed clusterrolBinding for metric service, removed env section fro…
bharathi-tenneti Jul 17, 2020
33c2b2f
reverted servcie creation
bharathi-tenneti Jul 17, 2020
f3d9da9
reverted clusterrolebinding metrics
bharathi-tenneti Jul 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions changelog/fragments/rm-helm-metrics.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# entries is a list of entries to include in
# release notes and/or the migration guide
entries:
- description: >
Remove legacy metrics generation code.
kind: "removal"
# Is this a breaking change?
breaking: true
migration:
header: Remove legacy metrics generation code from cmd/helm-operator/main.go, and test-e2e-helm.sh checks for servicemonitor.
body: TBD
80 changes: 1 addition & 79 deletions cmd/helm-operator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,13 @@
package main

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Jul 17, 2020

Choose a reason for hiding this comment

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

Is not missing remove from cmd/ansible-operator/main.go as well?
Also, should we not add the fragment here with TBD?

Copy link
Copy Markdown
Contributor Author

@bharathi-tenneti bharathi-tenneti Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@joelanford joelanford Jul 17, 2020

Choose a reason for hiding this comment

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

Yeah, we should add individual removal fragments. Those require migration sections, but we're doing this:

migration:
  header: <what got removed>
  body: TBD

Then 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.

Copy link
Copy Markdown
Contributor Author

@bharathi-tenneti bharathi-tenneti Jul 17, 2020

Choose a reason for hiding this comment

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

okay, Ill follow above template, and add fragment for this PR.

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Jul 17, 2020

Choose a reason for hiding this comment

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

e.g https://github.com/operator-framework/operator-sdk/pull/3414/files

import (
"context"
"errors"
"fmt"
"os"
"runtime"
"strings"

"github.com/spf13/pflag"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/cache"
crclient "sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -40,18 +35,11 @@ import (
"github.com/operator-framework/operator-sdk/pkg/helm/release"
"github.com/operator-framework/operator-sdk/pkg/helm/watches"
"github.com/operator-framework/operator-sdk/pkg/k8sutil"
kubemetrics "github.com/operator-framework/operator-sdk/pkg/kube-metrics"
"github.com/operator-framework/operator-sdk/pkg/log/zap"
"github.com/operator-framework/operator-sdk/pkg/metrics"
sdkVersion "github.com/operator-framework/operator-sdk/version"
)

var (
metricsHost = "0.0.0.0"
operatorMetricsPort int32 = 8686

log = logf.Log.WithName("cmd")
)
var log = logf.Log.WithName("cmd")

func printVersion() {
log.Info(fmt.Sprintf("Go Version: %s", runtime.Version()))
Expand Down Expand Up @@ -135,7 +123,6 @@ func main() {
log.Error(err, "Failed to create new manager factories.")
os.Exit(1)
}
var gvks []schema.GroupVersionKind
for _, w := range ws {
// Register the controller with the factory.
err := controller.Add(mgr, controller.WatchOptions{
Expand All @@ -151,76 +138,11 @@ func main() {
log.Error(err, "Failed to add manager factory to controller.")
os.Exit(1)
}
gvks = append(gvks, w.GroupVersionKind)
}

addMetrics(context.TODO(), cfg, gvks)

// Start the Cmd
if err = mgr.Start(signals.SetupSignalHandler()); err != nil {
log.Error(err, "Manager exited non-zero.")
os.Exit(1)
}
}

// addMetrics will create the Services and Service Monitors to allow the operator export the metrics by using
// the Prometheus operator
func addMetrics(ctx context.Context, cfg *rest.Config, gvks []schema.GroupVersionKind) {
// Get the namespace the operator is currently deployed in.
operatorNs, err := k8sutil.GetOperatorNamespace()
if err != nil {
if errors.Is(err, k8sutil.ErrRunLocal) {
log.Info("Skipping CR metrics server creation; not running in a cluster.")
return
}
}

if err := serveCRMetrics(cfg, operatorNs, gvks); err != nil {
log.Info("Could not generate and serve custom resource metrics", "error", err.Error())
}

// Add to the below struct any other metrics ports you want to expose.
servicePorts := []v1.ServicePort{
{Port: operatorMetricsPort, Name: metrics.CRPortName, Protocol: v1.ProtocolTCP,
TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: operatorMetricsPort}},
}

// Create Service object to expose the metrics port(s).
service, err := metrics.CreateMetricsService(ctx, cfg, servicePorts)
if err != nil {
log.Info("Could not create metrics Service", "error", err.Error())
}

// CreateServiceMonitors will automatically create the prometheus-operator ServiceMonitor resources
// necessary to configure Prometheus to scrape metrics from this operator.
services := []*v1.Service{service}

// The ServiceMonitor is created in the same namespace where the operator is deployed
_, err = metrics.CreateServiceMonitors(cfg, operatorNs, services)
if err != nil {
log.Info("Could not create ServiceMonitor object", "error", err.Error())
// If this operator is deployed to a cluster without the prometheus-operator running, it will return
// ErrServiceMonitorNotPresent, which can be used to safely skip ServiceMonitor creation.
if err == metrics.ErrServiceMonitorNotPresent {
log.Info("Install prometheus-operator in your cluster to create ServiceMonitor objects", "error", err.Error())
}
}
}

// serveCRMetrics gets the Operator/CustomResource GVKs and generates metrics based on those types.
// It serves those metrics on "http://metricsHost:operatorMetricsPort".
func serveCRMetrics(cfg *rest.Config, operatorNs string, gvks []schema.GroupVersionKind) error {
// The metrics will be generated from the namespaces which are returned here.
// NOTE that passing nil or an empty list of namespaces in GenerateAndServeCRMetrics will result in an error.
ns, err := kubemetrics.GetNamespacesForMetrics(operatorNs)
if err != nil {
return err
}

// Generate and serve custom resource specific metrics.
err = kubemetrics.GenerateAndServeCRMetrics(cfg, ns, gvks, metricsHost, operatorMetricsPort)
if err != nil {
return err
}
return nil
}
Copy link
Copy Markdown
Member

@joelanford joelanford Jul 17, 2020

Choose a reason for hiding this comment

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

Need to also:

  • remove the check for the service monitor in hack/tests/e2e-helm.sh
  • remove the env section for the pod spec template in internal/plugins/helm/v1/scaffolds/templates/manager/config.go

11 changes: 2 additions & 9 deletions hack/tests/e2e-helm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,7 @@ test_operator() {
exit 1
fi

header_text "verify that the servicemonitor is created"
if ! timeout 1m bash -c -- "until kubectl get servicemonitors/nginx-operator-metrics --namespace=${operator_namespace} > /dev/null 2>&1; do sleep 1; done";
then
error_text "FAIL: Failed to get service monitor"
operator_logs
exit 1
fi


release_name=$(kubectl get --namespace=${test_namespace} nginxes.helm.example.com nginx-sample -o jsonpath="{..status.deployedRelease.name}")
nginx_deployment=$(kubectl get --namespace=${test_namespace} deployment -l "app.kubernetes.io/instance=${release_name}" -o jsonpath="{..metadata.name}")
Expand Down Expand Up @@ -145,7 +139,6 @@ if echo $log | grep -q "failed to generate RBAC rules"; then
exit 1
fi

install_service_monitor_crd

sed -i".bak" -E -e 's/(FROM quay.io\/operator-framework\/helm-operator)(:.*)?/\1:dev/g' Dockerfile; rm -f Dockerfile.bak
make docker-build IMG="$DEST_IMAGE"
Expand All @@ -161,4 +154,4 @@ OPERATORDIR="$(pwd)"

deploy_operator
trap_add 'remove_operator' EXIT
test_operator
test_operator
9 changes: 0 additions & 9 deletions internal/plugins/helm/v1/scaffolds/templates/manager/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,5 @@ spec:
requests:
cpu: 100m
memory: 60Mi
env:
- name: WATCH_NAMESPACE
value: ""
- name: POD_NAME
valueFrom:
fieldRef:
fieldPath: metadata.name
- name: OPERATOR_NAME
value: "{{ .OperatorName }}"
terminationGracePeriodSeconds: 10
`