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.

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

fix error to create service metrics when run operator-sdk up local #2190

Merged
merged 2 commits into from
Jan 6, 2020
Merged

fix error to create service metrics when run operator-sdk up local #2190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c074e5
fix: metrics errors msg when run locally
camilamacedo86 Dec 12, 2019
7a7eb9a
update changelog as suggested
camilamacedo86 Jan 6, 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
3 changes: 1 addition & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,17 @@
- Fail `operator-sdk olm-catalog gen-csv` if it is not run from a project's root, which the command already assumes is the case. ([#2322](https://github.com/operator-framework/operator-sdk/pull/2322))
- **Breaking Change:** Extract custom Ansible module `k8s_status`, which is now provided by the `operator_sdk.util` Ansible collection. See [developer_guide](https://github.com/operator-framework/operator-sdk/blob/master/doc/ansible/dev/developer_guide.md#custom-resource-status-management) for new usage. ([#2310](https://github.com/operator-framework/operator-sdk/pull/2310))
- Upgrade minimal Ansible version in the init projects from `2.6` to `2.9` for collections support. ([#2310](https://github.com/operator-framework/operator-sdk/pull/2310))
- Improve skip metrics logs when running the operator locally in order to make clear the information. ([#2190](https://github.com/operator-framework/operator-sdk/pull/2190))

### Deprecated

- Deprecated `github.com/operator-framework/operator-sdk/pkg/restmapper` in favor of the `DynamicRESTMapper` implementation in [controller-runtime](https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/client/apiutil#NewDiscoveryRESTMapper). ([#2309](https://github.com/operator-framework/operator-sdk/pull/2309))

### Removed


### Bug Fixes
- Fix `operator-sdk build`'s `--image-build-args` to support spaces within quotes like `--label some.name="First Last"`. ([#2312](https://github.com/operator-framework/operator-sdk/pull/2312))


## v0.13.0

### Added
Expand Down
53 changes: 33 additions & 20 deletions internal/scaffold/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const cmdTmpl = `package main

import (
"context"
"errors"
"flag"
"fmt"
"os"
Expand Down Expand Up @@ -149,7 +150,26 @@ func main() {
os.Exit(1)
}

if err = serveCRMetrics(cfg); err != nil {
// Add the Metrics Service
addMetrics(ctx, cfg, namespace)
Comment on lines +153 to +154
Copy link
Member

Choose a reason for hiding this comment

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

It looks like addMetrics() is a small wrapper around serveCRMetrics() that does some extra logging and error handling, right?

IMO, another function in the main.go template for this is a bit much, and I think we could add the extra error handling in line in the body of the main function.

If I understand correctly, it seems like this PR could be reduced to:

        if err = serveCRMetrics(cfg); err != nil {
-               log.Info("Could not generate and serve custom resource metrics", "error", err.Error())
+               if errors.Is(err, k8sutil.ErrRunLocal) {
+                       log.Info("Skipping CR metrics server creation; not running in a cluster.")
+               } else {
+                       log.Info("Could not generate and serve custom resource metrics", "error", err.Error())
+               }
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logs/msgs came from more than one point in the original code (See serveCRMetrics, CreateMetricsService, CreateServiceMonitors). The idea was centralized/encapsulate the logic to add the metrics and do its improvement.

{"level":"info","ts":1563485392.3758454,"logger":"cmd","msg":"Could not generate and serve custom resource metrics","error":"operator run mode forced to local"}
{"level":"info","ts":1563485392.4013908,"logger":"cmd","msg":"Could not create metrics Service","error":"failed to initialize service object for metrics: OPERATOR_NAME must be set"}
{"level":"info","ts":1563485392.4066937,"logger":"cmd","msg":"Could not create ServiceMonitor object","error":"no ServiceMonitor registered with the API"}

In this way, just the above suggestion still not attending the expectations.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see now. The diff was confusing me :)


log.Info("Starting the Cmd.")

// 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, namespace string) {
if err := serveCRMetrics(cfg); err != nil {
if errors.Is(err, k8sutil.ErrRunLocal) {
log.Info("Skipping CR metrics server creation; not running in a cluster.")
return
}
log.Info("Could not generate and serve custom resource metrics", "error", err.Error())
}

Expand All @@ -158,31 +178,24 @@ func main() {
{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
{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}
_, err = metrics.CreateServiceMonitors(cfg, namespace, 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())
}
}

log.Info("Starting the Cmd.")

// Start the Cmd
if err := mgr.Start(signals.SetupSignalHandler()); err != nil {
log.Error(err, "Manager exited non-zero")
os.Exit(1)
// CreateServiceMonitors will automatically create the prometheus-operator ServiceMonitor resources
// necessary to configure Prometheus to scrape metrics from this operator.
services := []*v1.Service{service}
_, err = metrics.CreateServiceMonitors(cfg, namespace, 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())
}
}
}

Expand Down
31 changes: 22 additions & 9 deletions internal/scaffold/cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const cmdExp = `package main

import (
"context"
"errors"
"flag"
"fmt"
"os"
Expand Down Expand Up @@ -148,7 +149,26 @@ func main() {
os.Exit(1)
}

if err = serveCRMetrics(cfg); err != nil {
// Add the Metrics Service
addMetrics(ctx, cfg, namespace)

log.Info("Starting the Cmd.")

// 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, namespace string) {
if err := serveCRMetrics(cfg); err != nil {
if errors.Is(err, k8sutil.ErrRunLocal) {
log.Info("Skipping CR metrics server creation; not running in a cluster.")
return
}
log.Info("Could not generate and serve custom resource metrics", "error", err.Error())
}

Expand All @@ -157,6 +177,7 @@ func main() {
{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
{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 {
Expand All @@ -175,14 +196,6 @@ func main() {
log.Info("Install prometheus-operator in your cluster to create ServiceMonitor objects", "error", err.Error())
}
}

log.Info("Starting the Cmd.")

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

// serveCRMetrics gets the Operator/CustomResource GVKs and generates metrics based on those types.
Expand Down