[sdk-metrics] add sdk-stamps to bundle resources #21

varshaprasad96 · 2020-05-20T17:46:55Z

Proposal to add sdk-stamps to bundle image.

Description
This proposal defines the places in bundle image where SDK labels can be added. This will be helpful for OLM to identify the operators which use Operator SDK and would indirectly provide us with metrics to evaluate the reachability of SDK among RedHat community operators.

joelanford

For each resource type (e.g. bundle image, operator image, CSV, CRD, etc.), we should include:

A description of how that particular label is going to be propagated to the telemetry dashboard (is there a standard way to propagate these already or will OLM need to add code to handle these?)
A short description of how it will be used and what value it provides. I think the overall objective is clear ("measure reachability of SDK"), but why is each particular label necessary (i.e. what does a bundle image label tell us that a CRD annotation does not?)

joelanford · 2020-05-20T19:30:04Z

enhancements/sdk-metrics.md

+The dockerfile will include the sdk stamp in the form of `LABEL` which will follow the following format:
+
+```
+LABEL operators.operatorframework.io.bundle.sdk.v1 = operator_type:sdk_version


@ecordell Is the operators.operatorframework.io.bundle prefix reserved for the bundle media type?

We need to coordinate what labels are safe to add to bundle images since they have special meaning for OLM.

Related to this is #18, which is seeking to add a separate media type related to scorecard metadata and image contents. Do we need to define a new media type for SDK metadata and do something like the following:

sdk.operatorframework.io.metadata.mediatype = "sdk+v1" sdk.operatorframework.io.metadata.version.v1 = "v0.17.0" sdk.operatorframework.io.metadata.layout.v1 = "go.kubebuilder.io/v2.0.0"

Do we need version number suffixes on the version and layout keys (this seems to be a pattern in other label keys)?

+1 on the proposed metadata - that seems valuable and will avoid collisions

Something to consider: if/when we split up the SDK into component repos (CLI/Go plugin, Helm, Ansible, libraries), which repo version will we use for sdk.operatorframework.io.metadata.version.v1?

Because we're versioning label keys/mediatype, we can punt on this. If we decide to use a version other than that of the operator-sdk binary, we can create a version.v2 label.

joelanford · 2020-05-20T19:33:41Z

enhancements/sdk-metrics.md

+			Namespace: "placeholder",
+			Annotations: map[string]string{
+				"capabilities": "Basic Install",
+                "csv-builder": "sdk-go-v0.17.0",


It would probably make sense to make this align more closely with the labels being inserted in other places. i.e.:

KB-based projects:

Annotations: map[string]string{ "capabilities": "Basic Install", "sdk.operatorframework.io/version": "v0.17.0", "sdk.operatorframework.io/operator-type": "go.kubebuilder.io/v2.0.0", }

Legacy projects:

Annotations: map[string]string{ "capabilities": "Basic Install", "sdk.operatorframework.io/version": "v0.17.0", "sdk.operatorframework.io/operator-type": "go", }

If we version labels, we should version annotation keys too:

"sdk.operatorframework.io/version/v1": "v0.17.0"

or

"sdk.operatorframework.io/v1/version": "v0.17.0"

joelanford · 2020-05-20T19:47:49Z

enhancements/sdk-metrics.md

+LABEL operators.operatorframework.io.bundle.sdk.v1 = go:v0.17.0
+```
+
+3. Cluster service version


Does OLM's upcoming support for CSV-less bundles impact the approach for SDK stamping?

In that future scenario, SDK will not be involved in CSV generation so we'll have no opportunity to inject anything directly into the CSV.

Will OLM have the necessary metadata to inject these annotations on behalf of SDK? If not, does it make sense to include these stamps on the CSV now only to have them removed in the near-ish future? Are stamps on other resource types sufficient?

We don't have concrete a plan for operators that don't ultimately create a CSV on cluster, though it's likely we'll migrate our on-cluster representation to something else once CSVs are no longer shipping in bundles.

Will OLM have the necessary metadata to inject these annotations on behalf of SDK?

We don't have any such mechanism today (bundle -> runtime object projection), that would be net-new. We can pair on adding something like that to this enhancement?

If not, does it make sense to include these stamps on the CSV now only to have them removed in the near-ish future?

If we want to punt on that discussion, we could write the label into the bundle and the CSV for now?

we could write the label into the bundle and the CSV for now

Agreed. We can annotate CSVs generated for current and new projects because operator-sdk controls CSV generation.

When CSV-less bundles become a thing it would be nice to set arbitrary annotations (and, tangentially, labels) in olm.yaml that are applied to the internal operator representation that OLM constructs, whether that be a CSV or something else.

We should also consider annotating CRDs, in case we need to use those labels in the future. We could do this with kustomize patches for new projects; we control CRD generation for current projects.

ecordell · 2020-05-20T21:34:27Z

enhancements/sdk-metrics.md

+The dockerfile will include the sdk stamp in the form of `LABEL` which will follow the following format:
+
+```
+LABEL operators.operatorframework.io.bundle.sdk.v1 = operator_type:sdk_version


+1 on the proposed metadata - that seems valuable and will avoid collisions

ecordell · 2020-05-20T21:48:48Z

enhancements/sdk-metrics.md

+LABEL operators.operatorframework.io.bundle.sdk.v1 = go:v0.17.0
+```
+
+3. Cluster service version


We don't have concrete a plan for operators that don't ultimately create a CSV on cluster, though it's likely we'll migrate our on-cluster representation to something else once CSVs are no longer shipping in bundles.

Will OLM have the necessary metadata to inject these annotations on behalf of SDK?

We don't have any such mechanism today (bundle -> runtime object projection), that would be net-new. We can pair on adding something like that to this enhancement?

If not, does it make sense to include these stamps on the CSV now only to have them removed in the near-ish future?

If we want to punt on that discussion, we could write the label into the bundle and the CSV for now?

ecordell · 2020-05-20T21:51:01Z

enhancements/sdk-metrics.md

+    ...
+```
+
+This can further be read by OLM while recording the metrics on the status of CSVs.


To complete this I think we should be explicit about which metrics are getting which label values.

I'm guessing we want csv_succeeded and csv_abnormal to have a an extra builder label that defaults to "" and is populated with csv.metadata.annotations["csv-builder"] if present (or whatever the final label/annotation is.

I could also see an argument for putting this on the deployment or pod itself, so that non-olm deployed sdk operators are aggregated as well. If that's the case, we will need to implement a new metric to capture the builder for any controllers we find that match.

Are csv_succeeded and csv_abnormal metrics or annotation keys?

Metrics emitted by OLM

estroz · 2020-05-20T22:22:52Z

enhancements/sdk-metrics.md

+### Limitations
+
+1. The metrics which would be collected through this process will not cover the audience which uses Operator SDK but not OLM to deploy and run their operators.
+2. Currently, `operator-sdk generate csv` officially supports only Go Operators. Ansible and Helm Operators will be fully supported in future. Hence, the collected metrics would majorly cover only Go operators.


Officially this is true. However the generator will still work (minimally) for these project types, so we can still inject annotations.

General bundle question: can Ansible and Helm operator-managed projects be bundled? I know there's a helm mediatype, but is that for a set of Helm charts and not necessarily an operator? @ecordell

If they can be, then we can treat any operator project type managed by the SDK the same, since we can generate a bundle.Dockerfile and apply our labels for those types too.

can Ansible and Helm operator-managed projects be bundled?

Yes, it's just that the features of CSV generation that use Go source code obviously won't work for Ansible and Helm operators since those projects don't contain Go source. If we haven't already we should exercise the proposed bundle generation workflow on ansible and helm operators and see what happens and if there are any major gaps.

estroz · 2020-05-20T22:29:14Z

enhancements/sdk-metrics.md

+
+1. The metrics which would be collected through this process will not cover the audience which uses Operator SDK but not OLM to deploy and run their operators.
+2. Currently, `operator-sdk generate csv` officially supports only Go Operators. Ansible and Helm Operators will be fully supported in future. Hence, the collected metrics would majorly cover only Go operators.
+3. Implementation limitation - As operator registry does not have an api to modify the bundle dockerfiles or manifests, the implementation for addition of labels will involve parsing of the relevant files and rewriting them to include the relevant labels. 


Writing an API to do this deserves an EP since we're now seeing another example outside of #18 of custom labels. I can take this on.

varshaprasad96 · 2020-05-21T03:56:28Z

enhancements/sdk-metrics.md

+LABEL sdk.operatorframework.io.metadata.layout.v1 = "go.kubebuilder.io/v2.0.0"
+```
+
+Having labels on `bundle.dockerfile` and `annotations.yaml` will help in collecting metrics regarding the use of SDK from indexed operator bundle images.


Though the enhancement proposes to add stamps to bundle images (bundle.dockerfile and annotations.yaml) in addition to CSVs, does OLM have/intend to have metrics which would evaluate bundles to extract any useful data. If not, would this be a new metric specifically added for SDK?

varshaprasad96 · 2020-05-21T03:58:00Z

enhancements/sdk-metrics.md

+
+#### Integration of SDK stamps with OLM metrics
+
+The main aim of adding SDK stamps to bundle images and CSV is to embed SDK related data with OLM metrics. Currently, OLM tracks the lifecycle of CSV and reports metrics related to its success or failure. The first step, would be to include SDK metadata in those metrics by adding labels. The two relevant metrics where we can the label currently are - `csvSucceeded` and `csvAbnormal`.


I suppose we can skip csvCount (which is Number of CSVs successfully registered).

Proposal to add sdk-stamps to image bundle

kevinrizza · 2020-05-22T11:00:23Z

High level question: since metrics only get added and exist in the OpenShift project and not in upstream kube with OLM, would this enhancement be more appropriate just for downstream forks? And therefore this enhancement in general should be added to OpenShift/enhancements as well? Or are we expecting this to land in the upstream sdk project even though it won't be used by upstream OLM?

If not, can we call that out?

joelanford

LGTM

estroz · 2020-05-22T14:18:53Z

@kevinrizza ~~assuming what you say about OpenShift metrics is true~~ do operator developers who target OpenShift use the upstream or downstream SDK? From my experience they use upstream.

Additionally although less importantly these CSV/CRD annotations could be useful for cluster admins debugging operator issues, since they point to a place to start looking for solutions (the SDK repo/docs).

Edit: while the first point I made above may be true, it is orthogonal to this potentially being an OpenShift pipeline-specific proposal. However, I still see worth in this feature being upstream for tooling, metrics, and general pipeline support.

varshaprasad96 · 2020-05-27T17:03:12Z

@kevinrizza @estroz @ecordell Shall I move this proposal to OpenShift/enhancements ?

ecordell

This looks good! Just one question about the final labels we will use.

ecordell · 2020-05-29T13:49:56Z

enhancements/sdk-metrics.md

+
+```
+operators.operatorframework.io.metrics.mediatype.v1 = “metrics+v1”
+operators.operatorframework.io.metrics.builder = “operator-sdk-v0.17.0”


These examples have the metrics prefix, and the labels further down do not. Is the proposal to have different keys for image labels vs. CSV labels?

I think the intent is to keep the annotation key as short as possible so as to not approach the 63 character limit, and having metrics in the key doesn't add much information to annotation keys specifically since they should be treated opaquely by users.

kevinrizza · 2020-05-29T15:07:05Z

@estroz @varshaprasad96 Sorry, super slow to respond to this. I think that everything related to adding the labels is a reasonable thing to include in the upstream enhancement. I just think that the OLM integration section that refers specifically to the metrics that are defined in OpenShift should probably live there, perhaps in a followup? Or we should attempt to make that section more generic to describe how upstream could use these labels for metrics.

varshaprasad96 · 2020-05-29T15:43:53Z

@kevinrizza I just opened a follow up PR on adding of labels to OLM metrics in OpenShift/enhancements - openshift/enhancements#349

ecordell · 2020-06-01T17:04:50Z

/lgtm

joelanford reviewed May 20, 2020

View reviewed changes

ecordell reviewed May 20, 2020

View reviewed changes

estroz reviewed May 20, 2020

View reviewed changes

varshaprasad96 force-pushed the metrics/sdk-stamp branch 4 times, most recently from 8da1130 to b3e6227 Compare May 21, 2020 03:54

varshaprasad96 commented May 21, 2020

View reviewed changes

varshaprasad96 force-pushed the metrics/sdk-stamp branch from b3e6227 to 6359b6a Compare May 21, 2020 23:52

[sdk-metrics] add sdk-stamps to bundle resources

1e216c7

Proposal to add sdk-stamps to image bundle

varshaprasad96 force-pushed the metrics/sdk-stamp branch from 6359b6a to 1e216c7 Compare May 22, 2020 05:41

joelanford approved these changes May 22, 2020

View reviewed changes

ecordell reviewed May 29, 2020

View reviewed changes

varshaprasad96 mentioned this pull request May 29, 2020

[olm-metrics] Collect data on reachability of Operator SDK from bundle image openshift/enhancements#349

Merged

openshift-ci-robot assigned ecordell Jun 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2020

varshaprasad96 merged commit a0b290d into operator-framework:master Jun 1, 2020

estroz mentioned this pull request Jun 10, 2020

[sdk-stamps] add sdk stamps to bundle images operator-framework/operator-sdk#3120

Merged

4 tasks


		#### Integration of SDK stamps with OLM metrics

		The main aim of adding SDK stamps to bundle images and CSV is to embed SDK related data with OLM metrics. Currently, OLM tracks the lifecycle of CSV and reports metrics related to its success or failure. The first step, would be to include SDK metadata in those metrics by adding labels. The two relevant metrics where we can the label currently are - `csvSucceeded` and `csvAbnormal`.

[sdk-metrics] add sdk-stamps to bundle resources #21

[sdk-metrics] add sdk-stamps to bundle resources #21

Uh oh!

Conversation

varshaprasad96 commented May 20, 2020

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

joelanford May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrizza commented May 22, 2020

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

estroz commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varshaprasad96 commented May 27, 2020

Uh oh!

ecordell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrizza commented May 29, 2020

Uh oh!

varshaprasad96 commented May 29, 2020

Uh oh!

joelanford May 20, 2020 •

edited

Loading

estroz May 20, 2020 •

edited

Loading

joelanford May 20, 2020 •

edited

Loading

estroz May 20, 2020 •

edited

Loading

estroz May 20, 2020 •

edited

Loading

estroz May 20, 2020 •

edited

Loading

estroz commented May 22, 2020 •

edited

Loading