Fix 6323: compress the bundle configMap #6408
Conversation
There was a problem hiding this comment.
@nunnatsa Thanks a ton for taking a look at this and making this contribution! Overall this PR looks good, but there is a couple changes I'd like to see before I give this a thumbs up:
- For readability purposes I would prefer to see a single config map writer implementation that has configurable options. In the implementation we should try to generalize as much of the process as possible and only branch off when necessary for doing logic specific to an option.
- Use an init container to decompress the gzipped files before they are mounted to the regular container running
opm serve ...- this helps us ensure thatgzipis always available instead of assuming it is available within the index image.
I'll do another round of reviews once these changes are addressed 😄
| type defaultCMWriter struct { | ||
| cmName string | ||
| namespace string | ||
| template *template.Template | ||
| } |
There was a problem hiding this comment.
I noticed this writer and the gzipCMWriter structs. Is there a reason we have to have two separate writers? Would it be possible to get away with having one config map writer that has a gzip option that can be set?
I think this would reduce code duplication making it easier to read and understand what different steps occur if we are using compression.
| type configMapWriter interface { | ||
| newConfigMap() *corev1.ConfigMap | ||
| writeYaml([]string) ([]*corev1.ConfigMap, error) | ||
| getFilePath() string | ||
| getCommandTemplate() *template.Template | ||
| } |
There was a problem hiding this comment.
If we use a single writer that takes in configurable options this interface is likely unnecessary
| `serverDir=/var/tmp/{{ .FBCIndexRootDir }}/;` + | ||
| `mkdir ${serverDir};` + | ||
| `for dir in {{ .FBCIndexRootDir }}/*configmap-partition-*;` + | ||
| `do targetDir="/var/tmp/${dir}";` + | ||
| `mkdir ${targetDir};` + | ||
| `for f in ${dir}/*gz; ` + | ||
| `do cat $f | gzip -d -c > "/var/tmp/${f%.*}";` + | ||
| `done;` + | ||
| `done;` + |
There was a problem hiding this comment.
I'm a bit hesitant to require the use of gzip within the registry pod main container command. If we need to use gzip I'd like to see us use an init container with a gzip image (ideally one that is as small as possible) to decompress all the contents in the volume before it is used by the container running opm serve.
|
Hi @everettraven! Thanks for your comments. I think that if I'll implement the gzip writer and the default writer on the same method, the result will be not readable nor maintainable. We'll need to use In the previous implementation, I used the "strategy" design pattern. I think it's great for our case, but I get your point about code duplication. So I now changed it to use the "template method" design pattern. I think it now gives us a good balance between readability and avoiding code duplication. I also implemented your request about init container. Again, manually tested on kind cluster: $ build/operator-sdk run bundle --gzip-configmap=true quay.io/webcenter/elasticsearch-operator-bundle:v0.0.1 -n default
INFO[0010] Creating a File-Based Catalog of the bundle "quay.io/webcenter/elasticsearch-operator-bundle:v0.0.1"
INFO[0012] Generated a valid File-Based Catalog
INFO[0015] Created registry pod: quay-io-webcenter-elasticsearch-operator-bundle-v0-0-1
INFO[0015] Created CatalogSource: elasticsearch-operator-catalog
INFO[0015] OperatorGroup "operator-sdk-og" created
INFO[0015] Created Subscription: elasticsearch-operator-v0-0-1-sub
INFO[0020] Approved InstallPlan install-w5kc7 for the Subscription: elasticsearch-operator-v0-0-1-sub
INFO[0020] Waiting for ClusterServiceVersion "default/elasticsearch-operator.v0.0.1" to reach 'Succeeded' phase
INFO[0020] Waiting for ClusterServiceVersion "default/elasticsearch-operator.v0.0.1" to appear
INFO[0031] Found ClusterServiceVersion "default/elasticsearch-operator.v0.0.1" phase: Pending
INFO[0033] Found ClusterServiceVersion "default/elasticsearch-operator.v0.0.1" phase: InstallReady
INFO[0034] Found ClusterServiceVersion "default/elasticsearch-operator.v0.0.1" phase: Installing
INFO[0054] Found ClusterServiceVersion "default/elasticsearch-operator.v0.0.1" phase: Succeeded
INFO[0054] OLM has successfully installed "elasticsearch-operator.v0.0.1" |
There was a problem hiding this comment.
@nunnatsa Thanks for bearing with my delay in getting a review in. I took another look at the configMapWriter implementation and left some suggestions. I took a look at the rest of the changes and they seem reasonable but I was having some trouble with the GitHub diff formatting. I have to drop off for the day but I will circle back around and do another round of review on Monday.
Thanks again for your patience!
| }) | ||
| } | ||
|
|
||
| postfix := cm.Name + "-volume" |
There was a problem hiding this comment.
Does this need the cm.Name? This seems unnecessary if we are also explicitly adding it on line 279
| postfix := cm.Name + "-volume" | |
| postfix := "-volume" |
| if !f.cfg.GzipCM { | ||
| vm.SubPath = cm.Name | ||
| } |
There was a problem hiding this comment.
The VolumeMount.SubPath is already cm.Name. I would remove this if block all together
| containerVolumeMount := make([]corev1.VolumeMount, len(sharedVolumeMounts)) | ||
| copy(containerVolumeMount, sharedVolumeMounts) |
There was a problem hiding this comment.
Why do we need to do this copy? Can we just pass sharedVolumeMounts directly to wherever we are passing containerVolumeMount?
There was a problem hiding this comment.
We don't. it's a leftover from a previous implementation. Removed.
There was a problem hiding this comment.
@nunnatsa Thanks a ton for your help and effort on this contribution! This looks good to me - great work!
/lgtm
|
@nunnatsa Once the branch conflict is resolved and tests have run I'll work on getting this merged (some CI tests are likely to fail but I'll need to verify they are failing for the expected reason) |
|
Thanks @everettraven - now we have conflicts. |
When creating the bundle confogMap(s), compress the content using gzip. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
|
@everettraven: Overrode contexts on behalf of everettraven: docs, e2e-molecule In response to this:
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. |
everettraven
merged commit 6a6edfd
into
operator-framework:master
Description of the change
Add the
--gzip-configmap=trueCLI parameter in order to compress the configMap, if it exeeds the max length.Motivation for the change
Fix #6323
It is currently not possible to run bundles with files larger than 1MB (the configMap limit). In these cases, the existing partitioning mechanism is not enough, and the action fails.
OLM solved this issue by compressing the configMap content. This PR motivation is to implement compression mechanism in operator-sdk as well.
By adding the
--gzip-configmap=trueparameter, operator-sdk now will compress the content of the config map. The commad of the pod is also modified in this case. It is not possible to extract the files in the configMap itself. The command now extracts the files to/var/tmpwhich is writable, and then run the opm server form this directory.Successfully manually tested on a local kind cluster, with:
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments(seechangelog/fragments/00-template.yaml)website/content/en/docs