[WIP] (proposal) Make bundle accessible to a cluster by tkashem · Pull Request #1054 · operator-framework/operator-lifecycle-manager

tkashem · 2019-10-01T14:11:28Z

Make bundle accessible to a cluster

jpeeler · 2019-10-01T15:03:05Z

+    └── annotations.yaml
+
+$ cat /annotations.yaml
+annotations:


I'm thinking that we don't need this line. More importantly, I think the below lines should be in the format of <key>="value" to mirror how annotations are projected to the filesystem.

We discussed this will remain in yaml format to allow extending with additional sections later on.

to allow extending with additional sections later on.

Could you elaborate what you mean by this?

What I was led to believe is another section like:

annotations: key: value moredata: ...

But I don't understand how that makes sense in the context of a file called "annotations.yaml".

awgreene

Great work! I left a few nits and general questions.

awgreene · 2019-10-02T13:14:56Z

+    └── annotations.yaml
+
+$ cat /annotations.yaml
+annotations:


to allow extending with additional sections later on.

Could you elaborate what you mean by this?

jpeeler · 2019-10-02T19:37:37Z

+
+## Serving the bundle data
+
+New code will be made in operator-registry to provide functionality for traversing the directies of the bundle image for writing to a configmap (example above is "operator-registry serve"), the format for which is discussed in more detail below. The configmap will also use a generated name to avoid collisions and will be labeled to match the requested bundle image. It will be the responsibility of the caller to 1) launch the job, 2) watch for the target configmap to be created 3) to delete the job (until ttlSecondsAfterFinished is available) 4) delete the configmap after reading.


Alternatively, instead of generating a name for the configmap, could use a sanitized version of the bundle image name and use that (can't use colons, but maybe that's the only unacceptable character. also 253 character limit might be worth worrying about). Either way, I don't think specifying the name of the configmap by the caller is the correct approach.

Hrm, labels are actually restricted to 63 characters. Docker registry names are limited to 256 characters. And quay names seem to be even potentially longer (https://coreos.com/quay-enterprise/releases/#2.0.4).

You could always create a shortened hash of the fully qualified image name -- that would help you meet both the length and character-set restrictions.

That's a good idea, however, I pivoted a little bit.
I ended up retracting my original aversion to specifying a configmap, at least within the bundle image. Rather than worrying about character limits I ended up putting the configmap creation code in the "launch" function, so that the caller doesn't have to worry about configmap naming. I'll update the proposal here shortly that hopefully fully clarifies the new/final direction.

ecordell · 2019-10-02T22:37:49Z

+    spec:
+      containers:
+      - name: bundle-image
+        image: &image bundle-image


This seems odd as an example?

Were you wanting a more realistic pull spec? It's what I used for testing since I was using local images.

ecordell · 2019-10-02T22:38:05Z

+        command: ['/injected/operator-registry', 'serve']
+        env:
+          - name: CONTAINER_IMAGE
+            value: *image


This also seems like an odd example

I thought it was a clever usage of repeated nodes, but the implementation won't use this.

ecordell · 2019-10-02T22:38:58Z

+        command: ['/bin/cp', '/operator-registry', '/copy-dest']
+        volumeMounts:
+        - name: copydir
+          mountPath: /copy-dest


is there a reason to do this instead of, say, /bin?

It reduces the likelihood of conflicts.

ecordell · 2019-10-02T22:42:50Z

+```
+
+Notes:
+* The resource file name needs to be manipulated if it contains special characters.


What if we made restrictions on the bundle format side?

That would be nice, but given that we aren't actually remounting the configmap data (yet) I think handling the translation instead of just failing is best. What do you think?

njhale · 2019-10-03T06:04:37Z

+
+## Serving the bundle data
+
+New code will be made in operator-registry to provide functionality for traversing the directies of the bundle image for writing to a configmap (example above is "operator-registry serve"), the format for which is discussed in more detail below. The configmap will also use a generated name to avoid collisions and will be labeled to match the requested bundle image. It will be the responsibility of the caller to 1) launch the job, 2) watch for the target configmap to be created 3) to delete the job (until ttlSecondsAfterFinished is available) 4) delete the configmap after reading.


You could always create a shortened hash of the fully qualified image name -- that would help you meet both the length and character-set restrictions.

njhale · 2019-10-03T06:07:58Z

+
+## Serving the bundle data
+
+New code will be made in operator-registry to provide functionality for traversing the directies of the bundle image for writing to a configmap (example above is "operator-registry serve"), the format for which is discussed in more detail below. The configmap will also use a generated name to avoid collisions and will be labeled to match the requested bundle image. It will be the responsibility of the caller to 1) launch the job, 2) watch for the target configmap to be created 3) to delete the job (until ttlSecondsAfterFinished is available) 4) delete the configmap after reading.


to delete the job (until ttlSecondsAfterFinished is available)

Is there any reason the caller can't just delete the job when it reaches Completed? Why wait?

delete the configmap after reading.

I think we want to keep the ConfigMap around as an on-cluster cache.

I think (3) is:

the caller will be responsible for deleting the job

when ttlSecondsAfterfinished is available, the caller will no longer be required to delete the job.

Yeah, I was referring to the ttlSecondAfterFinished feature being non-alpha. I've tried to rewrite the proposal in such a way that makes it clear that deletion is up to the caller.

njhale · 2019-10-03T06:35:01Z

+    operators.coreos.com.bundle.resources: "manifests+metadata"
+    operators.coreos.com.bundle.mediatype: "registry+v1"
+data:
+  testbackup.crd.yaml: content of testbackup.crd.yaml


I think we should shard files across multiple ConfigMaps to avoid hitting the resource size limit (RSL). A simple strategy we could use is to generate a ConfigMap per file (assume |single file| < RSL - |metadata+kind+etc...|).

Interesting idea. At a minimum the data usage should be checked. If this is a huge concern though, should configmaps even be used for storing the data?

njhale · 2019-10-03T06:42:57Z

+* The consumer of the `ConfigMap` does not use the key name in `Data` section to identify the type of resource. It should inspect the content.
+* The consumer will iterate through the `Data` section and and add each resource to the bundle.
+* The annotations from the `annotations.yaml` file is copied to `metadata.annotations` to the `ConfigMap`.
+* The `ConfigMap` may have a resource that contains a `PackageManifest` resource. The consumer needs to handle this properly.


Can you elaborate on this for me? When does this happen and is it something consumers would trip on?

Consumers being OLM applying the manifests to a cluster?

fixes a few syntax errors too

openshift-ci-robot · 2019-10-03T20:12:55Z

@tkashem: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-olm	`ca8efed`	link	`/test e2e-aws-olm`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

awgreene · 2019-10-10T13:29:55Z

+	// Data contains the configuration data.
+	// Each key must consist of alphanumeric characters, '-', '_' or '.'.
+	// Values with non-UTF-8 byte sequences must use the BinaryData field.
+	// The keys stored in Data must not overlap with the keys in
+	// the BinaryData field, this is enforced during validation process.
+	// +optional


We could use a validation webhook to do this.

ecordell · 2019-10-18T14:28:59Z

/lgtm

We may make some minor changes to this over time (esp. w.r.t. sharding) but I think this proposal is good to go, and we should merge it.

Doc-only, merging without tests.

openshift-ci-robot · 2019-10-18T14:29:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

(proposal) Make bundle accessible to a cluster

87aee11

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2019

openshift-ci-robot requested review from jpeeler and njhale October 1, 2019 14:11

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 1, 2019

jpeeler reviewed Oct 1, 2019

View reviewed changes

jpeeler mentioned this pull request Oct 1, 2019

Expose bundle data from bundle image operator-framework/operator-registry#94

Merged

awgreene reviewed Oct 2, 2019

View reviewed changes

Comment thread doc/contributors/design-proposals/pull-bundle-on-a-cluster.md Outdated

awgreene requested changes Oct 2, 2019

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2019

jpeeler reviewed Oct 2, 2019

View reviewed changes

ecordell reviewed Oct 2, 2019

View reviewed changes

njhale reviewed Oct 3, 2019

View reviewed changes

(proposal): add bundle image deployment text

ca8efed

fixes a few syntax errors too

jpeeler force-pushed the proposal branch from 86cc865 to ca8efed Compare October 3, 2019 19:33

awgreene reviewed Oct 10, 2019

View reviewed changes

openshift-ci-robot assigned ecordell Oct 18, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2019

ecordell merged commit 45f78d6 into operator-framework:master Oct 18, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2019


		## Serving the bundle data

		New code will be made in operator-registry to provide functionality for traversing the directies of the bundle image for writing to a configmap (example above is "operator-registry serve"), the format for which is discussed in more detail below. The configmap will also use a generated name to avoid collisions and will be labeled to match the requested bundle image. It will be the responsibility of the caller to 1) launch the job, 2) watch for the target configmap to be created 3) to delete the job (until ttlSecondsAfterFinished is available) 4) delete the configmap after reading.

Conversation

tkashem commented Oct 1, 2019

Uh oh!

jpeeler Oct 1, 2019 • 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!

jpeeler Oct 2, 2019 • 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!

Uh oh!

awgreene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

jpeeler Oct 3, 2019 • 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!

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!

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!

openshift-ci-robot commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecordell commented Oct 18, 2019

Uh oh!

openshift-ci-robot commented Oct 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jpeeler Oct 1, 2019 •

edited

Loading

jpeeler Oct 2, 2019 •

edited

Loading

jpeeler Oct 3, 2019 •

edited

Loading

openshift-ci-robot commented Oct 3, 2019 •

edited

Loading