pkg/generator: Generate all manifests needed for Prometheus integration #241

etiennecoutaud · 2018-05-14T17:17:49Z

Refer to #222

Generate kubernetes manifest files to integrate operator with Prometheus

etiennecoutaud · 2018-05-14T18:12:23Z

fanminshi · 2018-05-14T18:28:54Z

pkg/generator/deploy_tmpl.go

 spec:
  replicas: 1
  selector:
    matchLabels:
-      name: {{.ProjectName}}
+      k8s-app: {{.ProjectName}}


why change name to k8s-app?

I introduced k8s-app label for service and servicemonitor. I just changed to have the same label everywhere.

If needed I can change to use name label in service and servicemonitor

I think we should keep the the original label name for now. if change is indeed needed, we can change the label to something else in a future pr.

cc/ @spahl @hasbro17 on label change.

Okay for using name label.
Should I use name label in service and servicemonitor too ?

fanminshi · 2018-05-14T18:29:58Z

Could you update the commit message title format as shown in https://github.com/operator-framework/operator-sdk/blob/master/CONTRIBUTING.MD#format-of-the-commit-message?

eparis · 2018-05-15T18:17:19Z

pkg/generator/deploy_tmpl.go

    spec:
      containers:
        - name: {{.ProjectName}}
          image: {{.Image}}
+          ports:
+          - containerPort: 9090


can we name this port? and then use that in the service as targetPort: name. That leaves us flexibility to change the port in the future without having to change the service.

Sure ! 👍

containerPort is now name metrics and use in the service as targetPort: metrics

Make the port number a variable like {{.MetricsPort}} and define a new parameter in gen_deploy.go

type OperatorYaml struct { ProjectName string Image string MetricsPort int }

That way we can set MetricsPort to a constant defined elsewhere.

etiennecoutaud · 2018-05-16T09:34:15Z

finally change all k8s-app labels to name

eparis · 2018-05-16T12:05:38Z

This PR LGTM, though it does bother me a little that we have "9090" hard coded in 4 places. Not sure it's worth putting in the template data, but if we really wanted to do a followup.

etiennecoutaud · 2018-05-16T12:15:40Z

I agree with you, an another way could be to add a config file pass to the generator when operator-sdk new is running.
We could enable or not prometheus integration, define metrics ports, git init etc ...
something like

$ cat operator-sdk.cfg
prometheus_integration = enabled
metrics_port = 9090
git_init = true

$ operator-sdk new app-operator --api-version=app.example.com/v1alpha1 --kind=App --config=operator-sdk.cfg

eparis · 2018-05-16T12:18:50Z

I'd personally lean pretty hard on 'only give users 1 way to do it and make sure that way is the right way'. I'm definitely in favor of us putting the port into the code and not making it optional. My only (tiny) bit of unease is that it's in the code 4 times instead of 1.

Fewer options make users less able to get into weird situations we don't test or aren't able to help them with.

chancez · 2018-05-16T18:22:06Z

I feel like hard-coding ports is going to a problem if you ever need to run another program that could use that port as a side-car to the operator.

etiennecoutaud · 2018-05-16T18:33:21Z

In that case, we cannot prevent futur usage , so user will have to change promport in main.go and metrics port in manifest files

hasbro17 · 2018-05-18T19:43:02Z

We should follow up with the discussion at #222 (comment) before we decide if we want to move forward with this approach.

etiennecoutaud · 2018-06-01T09:25:56Z

Update done according to #222

hasbro17 · 2018-06-07T20:00:43Z

pkg/generator/gen_main_test.go

 	"github.com/sirupsen/logrus"
 )

+// Prometheus metrics port
+const promPort = ":9090"


Can we use a less common port number at a higher range like 90000.
Also can you define the port number as a global constant somewhere other than the test file like in k8sutil/constants.go. That way we can use it everywhere in the generator package without redefining it.

Or something that's actually in the ephemeral port range like 60K. 90K is out of range.

hasbro17 · 2018-06-07T21:57:39Z

pkg/generator/deploy_tmpl.go

@@ -54,6 +59,10 @@ spec:
              valueFrom:
                fieldRef:
                  fieldPath: metadata.namespace
+            - name: OPERATOR_NAME


Similarly can you make the OPERATOR_NAME and WATCH_NAMESPACE strings as variables in the template so we can set them using the namespace constants defined in k8sutil/constants.go.

type OperatorYaml struct { ProjectName string Image string NameEnv string NamespaceEnv string MetricsPort int }

etiennecoutaud · 2018-06-11T11:44:05Z

@hasbro17 according to your review:

constant MetricsPort is defined in k8sutil/constant and set to 60000
OperatorYaml struct has been updated to embedded environment vars value defined in k8sutil/constant
Main struct has also been updated with MetricsPort value from constant to use it in template

…der-func pkg/generator: Create generic file rendering function

hasbro17 · 2018-06-13T20:51:36Z

pkg/generator/templates.go

@@ -529,14 +551,19 @@ spec:
      containers:
        - name: {{.ProjectName}}
          image: {{.Image}}
+          ports:
+          - containerPort: {{.MetricsPort}}
+            name: metrics


The string metrics is being used here and in InitOperatorService().
We should make this a constant in k8sutil/constants.go.
And then pass it as a parameter name: {{.MetricsPortName}}.

hasbro17 · 2018-06-13T21:21:51Z

pkg/generator/gen_main.go

@@ -55,6 +58,7 @@ func renderMainFile(w io.Writer, repo, apiVersion, kind string) error {
 		SDKVersionImport:  versionImport,
 		APIVersion:        apiVersion,
 		Kind:              kind,
+		MetricsPort:       k8sutil.GetPrometheusMetricsPort(),


I don't think we need to convert this to a string. We can just use k8sutil.PrometheusMetricsPort.
And in the template down below we can change it to:

go http.ListenAndServe(":{{.MetricsPort}}", nil)

hasbro17 · 2018-06-13T21:52:12Z

Gopkg.toml

@@ -6,6 +6,10 @@
  name = "github.com/spf13/cobra"
  version = "0.0.2"

+[[override]]
+  name = "github.com/prometheus/client_golang"


This package isn't really used in the SDK itself right now so we can remove this. We'll add it back in later once we actually use this to record default metrics inside the SDK.
For now we can add it to the Gopkg.toml template of the operator:
https://github.com/operator-framework/operator-sdk/blob/master/pkg/generator/templates.go#L372

hasbro17 · 2018-06-13T22:22:15Z

pkg/generator/gen_main_test.go

-	sdk "github.com/operator-framework/operator-sdk/pkg/sdk"
-	k8sutil "github.com/operator-framework/operator-sdk/pkg/util/k8sutil"
-	sdkVersion "github.com/operator-framework/operator-sdk/version"
+  stub "github.com/example-inc/app-operator/pkg/stub"


We should keep the tab size to 4 as it is for all the other templates.

hasbro17 · 2018-06-13T22:23:20Z

pkg/generator/templates.go

-	sdk "{{.OperatorSDKImport}}"
-	k8sutil "{{.K8sutilImport}}"
-	sdkVersion "{{.SDKVersionImport}}"
+  stub "{{.StubImport}}"


Change tab size back to 4

hasbro17 · 2018-06-13T22:31:00Z

pkg/generator/templates.go

+  }
+  err = sdk.Create(service)
+  if err != nil {
+    logrus.Infof("Failed to create operator service: %v", err)


We should ignore the error if the service already exists as it's very likely that the operator pod can restart.

import "k8s.io/apimachinery/pkg/api/errors" ... if err != nil && !errors.IsAlreadyExists(err) { logrus.Infof("Failed to create operator service: %v", err) return }

hasbro17 · 2018-06-13T22:35:52Z

pkg/generator/templates.go

+    logrus.Infof("Failed to create operator service: %v", err)
+    return
+  }
+  logrus.Infof("Service %s have been created", service.Name)


logrus.Infof("Metrics service %s created", service.Name)

hasbro17 · 2018-06-13T22:45:37Z

@etiennecoutaud Thanks for the update. I've tested this out locally and it works fine.
If you can address the above changes we should be good. Also you'll need to rebase since we refactored the generator in #310

pkg/generator: final cleanup of generator.go

etiennecoutaud · 2018-06-15T13:16:40Z

@hasbro17 I completley failed my rebase 😄, anyway the refactor was too heavy. I will close this PR and reopen a clean one.

Is it normally that generator refactor remove all unit tests ?

hasbro17 · 2018-06-15T18:26:16Z

@etiennecoutaud No that was my mistake. We removed them by accident as part of the refactor. We're going to revert that #315

etiennecoutaud · 2018-06-15T19:03:28Z

No problem, I close this PR and wait for #315.

hasbro17 · 2018-06-19T20:55:14Z

@etiennecoutaud Can you reopen this in another PR. I think #315 will only change the main template unit test which should be easy to rebase once it's merged.

Also looking back I think it's best if we keep the generated code to a minimum and keep the port exposing and service creation utility in the SDK.

So the generated main.go should look like:

func main() {
	printVersion()

	sdk.ExposeMetricsPort()

	resource := "app.example.com/v1alpha1"
	kind := "App"
	namespace, err := k8sutil.GetWatchNamespace()
	if err != nil {
		logrus.Fatalf("Failed to get watch namespace: %v", err)
	}
	resyncPeriod := 5
	logrus.Infof("Watching %s, %s, %s, %d", resource, kind, namespace, resyncPeriod)
	sdk.Watch(resource, kind, namespace, resyncPeriod)
	sdk.Handle(stub.NewHandler())
	sdk.Run(context.TODO())
}

And sdk.ExposeMetricsPort() can be defined in pkg/sdk/metrics.go as:

func ExposeMetricsPort() {
  http.Handle("/"+k8sutil.PrometheusMetricsPortName, promhttp.Handler())
  go http.ListenAndServe(":"+k8sutil. PrometheusMetricsPort, nil)

  service, err := k8sutil.InitOperatorService()
  if err != nil {
    logrus.Fatalf("Failed to init operator service: %v", err)
  }
  err = sdk.Create(service)
  if err != nil && !errors.IsAlreadyExists(err) {
    logrus.Infof("Failed to create operator service: %v", err)
    return
  }
  logrus.Infof("Metrics service %s created", service.Name)
}

This way we can actually use the constants for the metrics port and not have them generated as a number and string in the operator's main.go. It also keeps main.go small.
Later on we can change sdk.ExposeMetricsPort() to be configurable by taking the port number as an arg if needed.

etiennecoutaud · 2018-06-20T18:37:22Z

Yes sure, I will before the end of the week.

fanminshi reviewed May 14, 2018

View reviewed changes

etiennecoutaud force-pushed the master branch from be96d03 to ddef3f5 Compare May 15, 2018 09:27

etiennecoutaud changed the title ~~Prometheus integration with serviceMonitor~~ pkg/generator: Generate all manifests needed for Prometheus integration May 15, 2018

eparis mentioned this pull request May 15, 2018

pkg/sdk: Add /debug and /metrics http endpoints #248

Closed

eparis reviewed May 15, 2018

View reviewed changes

etiennecoutaud force-pushed the master branch from ddef3f5 to 37422af Compare May 16, 2018 09:33

etiennecoutaud force-pushed the master branch 2 times, most recently from 32862b0 to 00a2432 Compare May 18, 2018 10:01

hasbro17 mentioned this pull request May 18, 2018

Prometheus Integration #222

Closed

etiennecoutaud force-pushed the master branch from 00a2432 to 792abfc Compare June 1, 2018 09:20

hasbro17 reviewed Jun 7, 2018

View reviewed changes

Make template data struct, directory generator, and generic file writer

20d59a6

hasbro17 reviewed Jun 7, 2018

View reviewed changes

etiennecoutaud and others added 5 commits June 8, 2018 16:15

commands/up: Add OPERATOR_NAME env var when run locally

3d8865a

generic render function working for versioning

f862a62

Merge remote-tracking branch 'upstream/master' into generic-render-func

4f32317

Removed redundant render functions and files

e1a7a5e

pkg/util: Init service object + GetOperatorName env var and test

cea3b57

etiennecoutaud force-pushed the master branch from 792abfc to cea3b57 Compare June 11, 2018 11:34

Ish Shah and others added 8 commits June 11, 2018 13:31

Added kind plural suffix to const strings

e57b5b7

Add singular/plural comments

6090040

Typo in comments

2522bcc

Declare template data struct before passing into render func

56a6851

Comment spacing

6edde16

Merge pull request operator-framework#310 from theishshah/generic-ren…

5db5df0

…der-func pkg/generator: Create generic file rendering function

Clean up directory generation

7d880e0

Combined file write and render func

50d2d4e

hasbro17 reviewed Jun 13, 2018

View reviewed changes

Use new function in place of buf, render, write

ee20d21

hasbro17 reviewed Jun 13, 2018

View reviewed changes

Ish Shah and others added 6 commits June 14, 2018 12:56

Fix executable file generation

518b633

Merge pull request operator-framework#313 from theishshah/clean-gen-file

03f94a2

pkg/generator: final cleanup of generator.go

commands/up: Add OPERATOR_NAME env var when run locally

f7b70b8

pkg/util: Init service object + GetOperatorName env var and test

6e7ff06

Merge remote-tracking branch 'origin/master'

392575a

pkg/generator update after refactor

083f432

etiennecoutaud closed this Jun 15, 2018

etiennecoutaud mentioned this pull request Jun 22, 2018

pkg/{sdk,generator} Expose Prometheus metrics port #323

Merged

pkg/generator: Generate all manifests needed for Prometheus integration #241

pkg/generator: Generate all manifests needed for Prometheus integration #241

Conversation

etiennecoutaud commented May 14, 2018

etiennecoutaud commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etiennecoutaud May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etiennecoutaud commented May 16, 2018

eparis commented May 16, 2018

etiennecoutaud commented May 16, 2018 • edited Loading

eparis commented May 16, 2018

chancez commented May 16, 2018

etiennecoutaud commented May 16, 2018

hasbro17 commented May 18, 2018

etiennecoutaud commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etiennecoutaud commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasbro17 Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasbro17 commented Jun 13, 2018

etiennecoutaud commented Jun 15, 2018 • edited Loading

hasbro17 commented Jun 15, 2018

etiennecoutaud commented Jun 15, 2018 • edited Loading

hasbro17 commented Jun 19, 2018

etiennecoutaud commented Jun 20, 2018

etiennecoutaud May 16, 2018 •

edited

Loading

etiennecoutaud commented May 16, 2018 •

edited

Loading

hasbro17 Jun 13, 2018 •

edited

Loading

etiennecoutaud commented Jun 15, 2018 •

edited

Loading

etiennecoutaud commented Jun 15, 2018 •

edited

Loading