-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
render: improve olm.bundle.object rendering for bundles #1094
render: improve olm.bundle.object rendering for bundles #1094
Conversation
d59200a
to
0cb93c9
Compare
Looks like a deadlock I introduced in the LoadFS concurrency commit: https://github.com/operator-framework/operator-registry/actions/runs/4992141512/jobs/8940057783?pr=1094#step:4:172 Investigating now. |
0cb93c9
to
5f2e380
Compare
Codecov Report
@@ Coverage Diff @@
## master #1094 +/- ##
==========================================
- Coverage 52.93% 52.38% -0.55%
==========================================
Files 107 107
Lines 9527 9688 +161
==========================================
+ Hits 5043 5075 +32
- Misses 3548 3671 +123
- Partials 936 942 +6
|
Alright, I think the concurrency deadlock issue is fixed up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks reasonable to me, just have a couple questions
2cacf8e
to
94eeb61
Compare
I've uncovered and fixed a few more issues:
With these changes, I:
The last test I want to do is to put the |
Ok I was able to confirm that the packagemanifest output of the existing operatorhub catalog using the opm binary from this branch is identical. So I think this is ready to go from a back-compat standpoint. /hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON bits continue to break my brain but the parallel stuff looks mostly good!
A Go benchmark would be sweet to capture this improvement. |
I think I understand it much better now that I know that |
94eeb61
to
476fb40
Compare
When rendering individual bundles, only generate olm.bundle.object properties for the CSV if there is an image reference for the bundle. Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
When rendering sqlite-based catalogs, only generate olm.bundle.object properties for the CSV if there is an image reference for the bundle. Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
476fb40
to
fc25695
Compare
I added a go benchmark in 4cfabfe, just before the changes that added concurrency. Then I:
TL;DR
|
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
1. When rendering sqlite DBs and bundle images, generate an "olm.csv.metadata" property instead of a full CSV (so long as there is a bundle image reference associated with the corresponding bundle) 2. When serving the GRPC interface and a full CSV is not present in an "olm.bundle.object" property, generate a CSV from (a) the "olm.csv.metadata" property. Also include the bundle's related images, and the package's icon, if defined. If there is no description in the CSV metadata, also include the package's description in the generated CSV. Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
fc25695
to
2e13a0a
Compare
4fba454
to
66db3fd
Compare
alpha/declcfg/declcfg.go
Outdated
return err | ||
} | ||
for key, ptr := range map[string]*string{"schema": &m.Schema, "package": &m.Package, "name": &m.Name} { | ||
if _, ok := blobMap[key]; !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to consider doing a Fold
on key before doing the existence check, here and pre-L121?
import (
"golang.org/x/text/cases"
)
foldKey := cases.Fold()
if _, ok := blobMap[foldKey]; !ok {
...
That way at least we'll prevent simultaneous schema
and Schema
keys in the blobs.
6d00945
to
f74583f
Compare
Blergh - we're still using an old version of Go that doesn't have |
It turns out that straight byte-based replacements of unicode escape characters back to their ascii representations is invalid if the unicode escape character itself is escaped (e.g. "\u003c" => "\\u003c" => "\<"). To solve this, we will instead unmarshal Meta objects to map[string]interface{}, extract the expected Meta fields from the map, and then use a JSON encoder with SetEscapeHTML(false) to re-encode the map back to JSON to be stored in Meta.Blob. Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
…m.bundle.object properties Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
f74583f
to
9c782fc
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, grokspawn, joelanford The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I think we have more conversation to explore the case-insensitivity of schema keys (FWIW, folks filed a bug with go/json about it, and they basically said "too late... Hyrum's law"), but I don't want to hold this heavy PR up because of that discussion. We just need to not forgot about it. Waiting for the CI to finish to push in. |
/lgtm |
2ee231b
into
operator-framework:master
@joelanford Hello, for some reason, for the catalog of our operator, upgrading operator-registry from 1.27.1 to 1.28.0 and testing in OpenShift 4.12.7, I observe the following:
Can this be a side-effect of the changes brought by this PR? We do observe (with
|
The changes require the opm server to be using 1.28 as well. Can you confirm the version of opm used in the catalog image to serve the new FBC? |
@joelanford Thanks a lot for the feedback.
So build date June 7, but on https://github.com/openshift/operator-framework-olm/tree/master/staging/operator-registry. , while https://github.com/operator-framework/operator-registry/releases/tag/v1.28.0 is from June 9 and the code base is different. I don't know what is the policy of keeping both in sync. Anyway, we'll use your 1.28 in our catalog too, and report back the testing results. |
@joelanford I confirm using operator-registry 1.28.0 in the catalog image and at our build time does the trick! All good. Thanks a lot! |
Awesome! Thanks for confirming! |
This pull request is a series of commits that progressively improves FBC parsing. Commits are:
olm.bundle.object
just for CSVs withopm render <bundleImage>
olm.bundle.object
just for CSVs withopm render <sqliteImageOrFile>
(but only if the bundle has an image reference in the database).olm.csv.metadata
property, and use it in place of theolm.bundle.object
properties in the scenarios covered in commits 1 and 2. When serving the GRPC API, this property is used to generate a synthetic CSV to be served.Meta
object unmarshaling that was made apparent when testing commit 4 on real-world catalogsA few notes:
olm.bundle.object
properties for just CSVs after commit 4).olm.bundle.object
properties)olm.bundle.object
propertiesolm.csv.metadata
property.olm.package
into synthetic CSVs, if defined in the package.olm.package
into synthetic CSVs, if not defined in theolm.csv.metadata
object.Open Questions:
olm.csv.metadata
object belongs in the bundle vs. the package (e.g. display name, keywords, maintainers, provider)? In this PR, I'm hesitant to add new root level fields or package properties, but maybe that's an unfounded fear. It would be a further optimization because catalogs wouldn't have to repeat those same things over and over again in each bundle.render
andmigrate
commands to do any conversion ofolm.bundle.object
toolm.csv.metadata
for existing FBCs? It seems like this would be pretty useful, so I've got another commit in a separate branch that would do this. I can include it in this PR or make a separate PR for it after we finalize this PR. Thoughts?opm migrate
intentionally loses fidelity, that invariant no longer holds. Are we okay with this? IMO, I think this is okay. SQLite serving has been deprecated for several years, so I think it is okay to start diverging. Perhaps we even consider dropping the SQLite server prior to the SQLite-based catalog building commands?/hold
There's a bunch here to discuss, evaluate, and manually test. I updated our tests as best I could, and I think I've covered old code paths + new code paths. But this is a pretty significant change, so it would be good to get some detail-oriented eyes on this.
Signed-off-by: Joe Lanford joe.lanford@gmail.com
Reviewer Checklist
/docs