Add an option to store Gzip-encoded bundle data in ConfigMaps

[zcahana]

Description of the change:

This PR adds an option to compress/encode bundle data stored in a ConfigMap, so that large bundles have better chance fitting within the 1048576 bytes limit for ConfigMaps.

It's basically a stop-gap solution to operator-framework/operator-lifecycle-manager#1523, until the design proposed in operator-framework/enhancements#40 is finalized and implemented.

Some key aspects of this implementation:

• The compression option is opt-in, and is activated by a -z/--gzip flag to opm alpha bundle extract command.
• When enabled, the manifests content is compressed using Gzip and then encoded using Base64.
• Base64 isn't strictly required here, but results with some nicer looking/better debuggable ConfigMaps without garbled binary data, in the expense of a slightly larger storage size.
• ConfigMaps with compressed data are discriminated using a olm.contentEncoding: gzip+base64 annotation.
• This PR should be complemented with a PR in OLM to set the --gzip flag while invoking the bundle unpacker job.

So far I've been manually testing this with the kubevirt/hyperconverged-cluster-operator bundle.
With compression enabled, this bundle takes 180KB, compared to 1004KB uncompressed (and 152KB compressed w/o base64 encoding).
I'll be extending this PR with unit+e2e tests, given the approach taken here is acceptable by the project maintainers.

Thanks!

Motivation for the change:

There's a growing number of operators who are hitting the 1048576 bytes barrier, including kubevirt/hyperconverged-cluster-operator, cert-manager, Strimzi, OCS, ...

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Docs updated or added to /docs
• Commit messages sensible and descriptive

[openshift-ci]

Hi @zcahana. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[kevinrizza]

/ok-to-test

[kevinrizza]

IMO This looks more or less exactly how I was expecting an implementation would look. I think we had some thoughts on whether it was better to only compress the CRDs + other raw kube manifests and leave the CSVs as is because we know that a large portion of the filesize of CSVs comes from already base64 encoded image metadata -- it would probably be interesting to see if we could run this against a few other operator bundles to see what kinds of reductions we get. But I imagine even just the naive implementation that compresses all of the contents will get very good results given the bulk of the size comes from the CRD structural schemas.

[codecov]

Codecov Report

Merging #685 (416513a) into master (9114e6a) will increase coverage by 0.48%.
The diff coverage is 54.66%.

❗ Current head 416513a differs from pull request most recent head 8720e3d. Consider uploading reports for the commit 8720e3d to get more accurate results

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   48.63%   49.11%   +0.48%     
==========================================
  Files          95       96       +1     
  Lines        8124     8185      +61     
==========================================
+ Hits         3951     4020      +69     
+ Misses       3412     3388      -24     
- Partials      761      777      +16     

Impacted Files	Coverage Δ
pkg/configmap/configmap_writer.go	33.10% <48.71%> (+33.10%)	⬆️
pkg/lib/encoding/encoding.go	54.54% <54.54%> (ø)
pkg/configmap/configmap.go	75.51% <71.42%> (-2.27%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9114e6a...8720e3d. Read the comment docs.

[joelanford]

Base64 isn't strictly required here, but results with some nicer looking/better debuggable ConfigMaps without garbled binary data, in the expense of a slightly larger storage size.

Can the ConfigMap binaryData field be used to solve this problem?

[exdx]

Really impressive PR! Just have some nits and general observations.

I think this would definitely be valuable in OLM.

[zcahana]

Thanks all for this thorough review. I'll work on the suggested changes as well as unit/e2e test.

[zcahana]

Base64 isn't strictly required here, but results with some nicer looking/better debuggable ConfigMaps without garbled binary data, in the expense of a slightly larger storage size.

Can the ConfigMap binaryData field be used to solve this problem?

No, yes! I've only now realized that base64 encoding is applied to anything passed into binaryData, so we're basically doing double base64 encoding here (and wastefully increasing data size). I'll certainly look into removing our own base64 encoding.

[joelanford]

@zcahana As I understand it, binaryData fields are actually stored as raw binary (it's a map[string][]byte). It's just rendered as base64 when encoded as YAML or JSON.

[zcahana]

As I understand it, binaryData fields are actually stored as raw binary (it's a map[string][]byte). It's just rendered as base64 when encoded as YAML or JSON.

Best of both worlds :-)

[zcahana]

IMO This looks more or less exactly how I was expecting an implementation would look. I think we had some thoughts on whether it was better to only compress the CRDs + other raw kube manifests and leave the CSVs as is because we know that a large portion of the filesize of CSVs comes from already base64 encoded image metadata -- it would probably be interesting to see if we could run this against a few other operator bundles to see what kinds of reductions we get. But I imagine even just the naive implementation that compresses all of the contents will get very good results given the bulk of the size comes from the CRD structural schemas.

@kevinrizza here's a quick look at the top-5 CSVs by size, as well as their compressed sizes:

$ ll ./redis-operator/0.5.0/redis-operator.v0.5.0.clusterserviceversion.yaml*
-rw-r--r-- 1 zvic zvic 872918 Jun 21 23:15 ./redis-operator/0.5.0/redis-operator.v0.5.0.clusterserviceversion.yaml
-rw-r--r-- 1 zvic zvic 352004 Jun 21 22:37 ./redis-operator/0.5.0/redis-operator.v0.5.0.clusterserviceversion.yaml.gz

$ ll ./ember-csi-community-operator/0.9.4/ember-csi-community-operator.v0.9.4.clusterserviceversion.yaml*
-rw-r--r-- 1 zvic zvic 576990 Jun 21 23:15 ./ember-csi-community-operator/0.9.4/ember-csi-community-operator.v0.9.4.clusterserviceversion.yaml
-rw-r--r-- 1 zvic zvic  51783 Jun 21 22:37 ./ember-csi-community-operator/0.9.4/ember-csi-community-operator.v0.9.4.clusterserviceversion.yaml.gz

$ ll ./hedvig-operator/1.0.1/manifests/hedvig-operator.v1.0.1.clusterserviceversion.yaml*
-rwxr-xr-x 1 zvic zvic 254115 Jun 21 23:15 ./hedvig-operator/1.0.1/manifests/hedvig-operator.v1.0.1.clusterserviceversion.yaml*
-rwxr-xr-x 1 zvic zvic 182983 Jun 21 22:37 ./hedvig-operator/1.0.1/manifests/hedvig-operator.v1.0.1.clusterserviceversion.yaml.gz*

$ ll ./jaeger/1.20.0/jaeger.v1.20.0.clusterserviceversion.yaml*
-rw-r--r-- 1 zvic zvic 231809 Jun 21 23:15 ./jaeger/1.20.0/jaeger.v1.20.0.clusterserviceversion.yaml
-rw-r--r-- 1 zvic zvic  68659 Jun 21 22:37 ./jaeger/1.20.0/jaeger.v1.20.0.clusterserviceversion.yaml.gz

$ ll ./cockroachdb/2.0.9/manifests/cockroachdb.v2.0.9.clusterserviceversion.yaml*
-rw-r--r-- 1 zvic zvic 186843 Jun 21 23:15 ./cockroachdb/2.0.9/manifests/cockroachdb.v2.0.9.clusterserviceversion.yaml
-rw-r--r-- 1 zvic zvic 131128 Jun 21 22:37 ./cockroachdb/2.0.9/manifests/cockroachdb.v2.0.9.clusterserviceversion.yaml.gz

As you see, the compression ratios vary substantially. Lower ratios indeed correspond with CSVs with base64 encoded data inline. Still, some CSVs benefit nicely from compression (particularly, redis-operator.v0.5.0 stepping away from the red line, and ember-csi-community-operator.v0.9.4 getting a x10 reduction in size), so I think it worthwhile to keep compressing CSVs too.

[zcahana]

@ALL,

I've worked through all review comments and added unit tests.
As for e2e tests, I've taken a quick look through the existing e2e infra both here and at the OLM repo, and it seems to me that it would be easier to implement it in there, not to mention we'd be able to test the actual e2e flow (bundle image > 1mb, opm bundle extract, ConfigMap, InstallPlan, ...). What do you think?

[exdx]

Hi @zcahana,

w.r.t. e2e testing, we think its best to have a specific operator-registry e2e test where we we spin up a cluster, run the unpack job, and check that the content made it to the configmap in the right format. There is an e2e test in operator-registry that runs in-cluster already, so it should be ok to use the existing infrastructure for the test.

Although the OLM e2e suite is more robust, we don't need this test to integrate into the OLM APIs itself. The test in OLM is already there: do an install and make sure that the unpacking still works after adding the compression flag. This can be turned on in a subsequent PR (after vendoring in this change) and if CI is green then we can consider it working on the OLM side as well.

[zcahana]

As I understand it, binaryData fields are actually stored as raw binary (it's a map[string][]byte). It's just rendered as base64 when encoded as YAML or JSON.

@joelanford I've been playing with the base64 encoding/decoding back and forth and eventually realized we must base64-decode what we get back when reading the binaryData, regardless if we base64-encoded it when writing, or didn't.
As for writing, we can either pass the raw bytes in, or base64-encode first and then pass it in - the result is the same, as well as the size limit (which always apply to the un-encoded raw bytes, even if we pass in base64 encoded data).

Bottom line, we're back to gzip+base64 encoding/decoding, but at least this time I've rewritten it with better memory efficiency.

[zcahana]

w.r.t. e2e testing, we think its best to have a specific operator-registry e2e test where we we spin up a cluster, run the unpack job, and check that the content made it to the configmap in the right format. There is an e2e test in operator-registry that runs in-cluster already, so it should be ok to use the existing infrastructure for the test.

@exdx thanks for the tip about the e2e test. I've indeed extended the existing one to launch the extract job, and then read the configmap and verify we get the expected objects. The test runs twice: once for a small bundle, without applying compression, and once for a large (> 1MiB) bundle), with compression applied.

[exdx]

nice work! I'm ready to lgtm once CI is green

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[timflannagan]

Looks like there are some failures in the e2e test:

[Fail] Launch bundle Deploy bundle job should populate specified configmap [It] Small bundle, uncompressed 
/home/runner/work/operator-registry/operator-registry/test/e2e/bundle_image_test.go:198

[Fail] Launch bundle Deploy bundle job should populate specified configmap [It] Large bundle, compressed 
/home/runner/work/operator-registry/operator-registry/test/e2e/bundle_image_test.go:112

I haven't dived into those errors, but placing a hold on this PR in the meantime so the bot doesn't go crazy attempting to retest non-prow-based jobs.

/hold

[zcahana]

The current failures are still due to a timeout (120 sec) in waiting for the extract job to complete.
Unfortunately I couldn't reproduce in my local env - tried both minikube and kind, with/without TLS, whole test suite or just the single test - everything passes time after time.

I can try to increase the timeout even further, but it's hard to believe the test is running that slow on the CI.
It would be best to take a look at the bundle job's logs, but I'm not sure if it's collected somewhere by the e2e running here. Alternatively I can add test code that dumps it to the test logs, and remove it when the issue is resolved.

[timflannagan]

@zcahana We have a couple of testing flakes that are difficult to reproduce locally but will pop up frequently while in CI - it's possible there's some sort of ginko parallelization magic going on that causes these flakes, but it's difficult to track down. It's possible that the testing environment that GH actions spins up for us has limited resources, which may help explain why more frequent tests are flaking due to exceeding the configured test timeout period, but I haven't dived too much into that angle either. Increasing the test timeout to find a good medium seems like a sufficient workaround in the short term though.

[joelanford]

@zcahana

I've been playing with the base64 encoding/decoding back and forth and eventually realized we must base64-decode what we get back when reading the binaryData, regardless if we base64-encoded it when writing, or didn't.

I'm not sure I'm following the reasoning here, though its entirely possible I'm overlooking something. I checked out the PR and removed the base64.NewEncoder and base64.NewDecoder (and related) lines in pkg/lib/encoding, ran the unit tests, and everything still passed.

I would have expected the TestLoadWriteRead/BundleCompressed test in pkg/configmap/configmap_test.go to fail if base64 decoding the data from the configmap is always required.

[zcahana]

I'm not sure I'm following the reasoning here, though its entirely possible I'm overlooking something. I checked out the PR and removed the base64.NewEncoder and base64.NewDecoder (and related) lines in pkg/lib/encoding, ran the unit tests, and everything still passed.

I would have expected the TestLoadWriteRead/BundleCompressed test in pkg/configmap/configmap_test.go to fail if base64 decoding the data from the configmap is always required.

@joelanford The unit test still passes since the fake client just returns whatever was written into the in-memory ConfigMap.binaryData as-is, encoded or not. In a real cluster (and indeed I discovered this only after starting to actually test the code using a real cluster), the ConfigMap binaryData field always returns base64-encoded when read from the api-server. This happens whether I base64-encoded the data put in binaryData (as I do now), or used the "raw" gzipped bytes (as I initially did)

[joelanford]

The unit test still passes since the fake client just returns whatever was written into the in-memory ConfigMap.binaryData as-is, encoded or not. In a real cluster (and indeed I discovered this only after starting to actually test the code using a real cluster), the ConfigMap binaryData field always returns base64-encoded when read from the api-server. This happens whether I base64-encoded the data put in binaryData (as I do now), or used the "raw" gzipped bytes (as I initially did)

Oh wow, that is extremely surprising (to me at least). I wonder if this is documented somewhere... 🤔

EDIT: this is the best I could find: kubernetes/website#27066. Either way, I find it pretty surprising that the binaryData API is map[string][]byte, but it doesn't round trip actual binary data. It seems like it should fail validation if I try to send non-base64 binaryData. 🤷‍♂️

Also, this seems like a pretty obvious footgun of the fake client that I'm sure others have already run into. We may want to add a feature to the fake client to handle this peculiarity of the apiserver. Not sure if it will be accepted, but its worth a try, and at the very least it will serve as a breadcrumb for others who fall into this trap.

[zcahana]

Guys, can someone please kick off the CI? The two last commits increase the job timeout as well as dump job logs in case of timeout error. Let's see where we are with this.

[timflannagan]

/rerun-all

[dinhxuanvu]

@zcahana Would you mind rebasing your PR against latest master? We recently fixed a kind test on CI so your PR won't pass CI until it is rebased. Thanks.

[zcahana]

Would you mind rebasing your PR against latest master? We recently fixed a kind test on CI so your PR won't pass CI until it is rebased. Thanks.

Will do, thanks.
Based on the recently added debug logging, it appears that there's no schedulable node hence the bundle extract job runs into timeout (10m) and the test fails:

INFO: Displaying job status for deploy-bundle-image-zclds
&JobStatus{Conditions:[]JobCondition{},StartTime:2021-07-02 18:16:48 +0000 UTC,CompletionTime:<nil>,Active:1,Succeeded:0,Failed:0,}
INFO: Found 1 pods matching job selector job-name=deploy-bundle-image-zclds
INFO: Displaying pod status for deploy-bundle-image-zclds-rtw2s
&PodStatus{Phase:Pending,Conditions:[]PodCondition{PodCondition{Type:PodScheduled,Status:False,LastProbeTime:0001-01-01 00:00:00 +0000 UTC,LastTransitionTime:2021-07-02 18:16:48 +0000 UTC,Reason:Unschedulable,Message:0/1 nodes are available: 1 node(s) had taint {node.kubernetes.io/not-ready: }, that the pod didn't tolerate.,},},Message:,Reason:,HostIP:,PodIP:,StartTime:<nil>,ContainerStatuses:[]ContainerStatus{},QOSClass:BestEffort,InitContainerStatuses:[]ContainerStatus{},NominatedNodeName:,PodIPs:[]PodIP{},EphemeralContainerStatuses:[]ContainerStatus{},}

[joelanford]

/lgtm
/hold cancel

[zcahana]

@kevinrizza @joelanford @exdx @timflannagan @dinhxuanvu

Thank you all for the thorough review and assistance throughout this PR.
To close the loop, we'll need to set the new --gzip flag in the bundle extract job spawned by OLM.
Do you have an estimation for when there will be an operator-registry release including this patch?