render: add alpha support for filtering #1231

joelanford · 2024-03-14T17:43:08Z

Description of the change:

This commit adds two alpha flags to opm render and updates FBC library code to support efficient filtering of FBC:

--alpha-filter-config=./path/to/config
--alpha-keep-packages=pkg1,pkg2

To support the filter config option, this commits defines a new configuration file format that supports:

filtering by package
within a package, filtering by channel
within a package, selecting a new default channel (required if the original default channel is filtered out)
within each channel, filtering by a version range

In order to support correct version range filtering, some bundles that fall outside the version range may remain in the channel to ensure that the filtered channel is still valid and includes all bundles that did fall in the range

Included in this commit is an update to declcfg.WalkMetasFS to enable concurrency, similar to recent changes in declcfg.LoadFS. declcfg.LoadFS is also refactored internally to use the new declcfg.WalkMetasFS implementation.

Motivation for the change:

There are two motivations:

Filtering by individual versions is an often requested feature, especially in workflows that requiring mirroring catalogs to disconnected environments. In OLM, filtering versions requires semantic knowledge of the upgrade graph rules, so it is not as simple as keeping or removing arbitrary bundles and channel entries. This PR includes that semantic knowledge.
When rendering an existing FBC, render is significantly faster than rendering an entire catalog and then using a post-render tool like jq or yq to filter after the fact.

I ran the existing benchmark to confirm that this update does not regress performance, even in the case of no filtering being applied. I will post those results in a comment.

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

codecov · 2024-03-14T17:47:49Z

Codecov Report

Attention: Patch coverage is 92.27053% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 55.08%. Comparing base (e7ca8e3) to head (b498760).
Report is 4 commits behind head on master.

❗ Current head b498760 differs from pull request most recent head eab0be1

Please upload reports for the commit eab0be1 to get more accurate results.

Files	Patch %	Lines
alpha/declcfg/load.go	74.71%	16 Missing and 6 partials ⚠️
alpha/declcfg/filter/config/v1alpha1/filter.go	95.07%	4 Missing and 3 partials ⚠️
alpha/action/render.go	89.47%	1 Missing and 1 partial ⚠️
alpha/declcfg/filter.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
+ Coverage   50.06%   55.08%   +5.01%     
==========================================
  Files         135      113      -22     
  Lines       12995    11577    -1418     
==========================================
- Hits         6506     6377     -129     
+ Misses       5405     4207    -1198     
+ Partials     1084      993      -91

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

everettraven · 2024-03-14T20:34:34Z

alpha/declcfg/filter/filter.go

+type Filterer interface {
+	FilterCatalog(ctx context.Context, fbc *declcfg.DeclarativeConfig) (*declcfg.DeclarativeConfig, error)
+}
+
+type MetaFilterer interface {
+	KeepMeta(meta *declcfg.Meta) bool
+}


Nit: Andy did a great job of getting this stuck in my head over the course of a few PRs: https://go.dev/wiki/CodeReviewComments#interfaces

The gist is that the interfaces should be defined where the values of the interface are used and not by the package that implements them. I'm not strongly opinionated on this (yet) but is something that crossed my mind while reviewing so figured I would point it out.

Yep, this is an outcome of the way this code evolved before I submitted it. I'll move it to the declcfg package.

everettraven · 2024-03-14T20:38:46Z

alpha/action/render.go

+	if r.Filterer != nil {
+		if metaFilterer, ok := r.Filterer.(filter.MetaFilterer); ok {
+			keepMeta = metaFilterer.KeepMeta
+		}
+	}


Why not have an explicit field for the MetaFilterer?

At the time, I think it was because I was actually implementing inlined functions with that same signature, so it was lighter weight than implementing a full struct.

I'll add a function type that implements the interface (sorta like http.Handler and http.HandlerFunc).

I'm also thinking about names... the age old problem.

alpha/declcfg/filter/config/v1alpha1/config.go

everettraven · 2024-03-14T20:45:52Z

alpha/declcfg/filter/config/v1alpha1/filter.go

+	}
+}
+
+func (f *filterer) FilterCatalog(_ context.Context, fbc *declcfg.DeclarativeConfig) (*declcfg.DeclarativeConfig, error) {


Is it anticipated that the context will be required by some implementation of the filterer interface? If not, we can probably remove it from the interface and as a parameter from this function

I've seen context.Context get added to so many functions at this point that I pretty much add it defensively for functions I expect:

could be long running

could need to call other functions that accept a context

Ok, I'm going to leave it in. One possibility I thought of is an interaction with an OCI registry to get the labels of bundle images and filter based on them. I'm considering adding a context to the meta filter signature as well for the same reason. WDYT?

IMHO, we should have it for consistency reasons (most of the declcfg code follows this convention), but it's also proven really helpful when we refactored for parallelism previously, and the downside ... ?

I think it's fine to include if we are anticipating the addition of a filtering method that will use it in the near future. Just calling out that it may be a premature optimization that could be added in the future when needed.

I also am not strongly opinionated here so if folks feel strongly we should keep it - fine by me

everettraven · 2024-03-14T20:51:29Z

alpha/declcfg/filter/packages.go

+	keepPackages sets.Set[string]
+}
+
+func (f *packageFilter) FilterCatalog(ctx context.Context, fbc *declcfg.DeclarativeConfig) (*declcfg.DeclarativeConfig, error) {


My eyes could be deceiving me, but looks like this function doesn't use the ctx parameter.

everettraven · 2024-03-14T20:58:10Z

alpha/declcfg/load.go

+type pathMetas struct {
+	path  string
+	metas []pathMeta
+}
+
+type pathMeta struct {
+	meta *Meta
+	err  error
+}


What are these for? I can't seem to see where they are being used, am I just missing it?

Oh good catch. PR went through some optimization iteration and these are now unused.

everettraven · 2024-03-14T21:02:13Z

cmd/opm/render/cmd.go

+	var obj struct {
+		metav1.TypeMeta `json:",inline"`
+	}
+	if err := yaml.Unmarshal(data, &obj); err != nil {
+		return nil, err
+	}
+
+	if obj.Kind != "FilterConfiguration" {
+		return nil, fmt.Errorf("unexpected kind %q", obj.Kind)
+	}
+
+	switch obj.APIVersion {
+	case "olm.operatorframework.io/v1alpha1":
+		cfg, err := configv1alpha1.LoadFilterConfiguration(data)
+		if err != nil {
+			return nil, err
+		}
+		return configv1alpha1.NewFilterer(cfg, configv1alpha1.WithLogger(log)), nil
+	default:
+		return nil, fmt.Errorf("unexpected API version %q", obj.APIVersion)
+	}


It looks like the configv1alpha1.LoadFilterConfiguration function already performs validation. Could this whole thing just be simplified to something like:

if filterFile == "" { return nil, nil } data, err := os.ReadFile(filterFile) if err != nil { return nil, err } cfg, err := configv1alpha1.LoadFilterConfiguration(data) if err != nil { return nil, err } return configv1alpha1.NewFilterer(cfg, configv1alpha1.WithLogger(log)), nil

?

I do realize that maybe this isn't as extensible because it only validates against the specific config version but this could be updated when/if we add another version

+1, this is premature optimization. I'll reduce and add some comments about what we might do if we add new config file versions.

joelanford · 2024-03-26T21:36:17Z

Here's the promised benchstat output comparing master and this PR.

goos: darwin
goarch: arm64
                      │ master.out  │           filter-fbc.out           │
                      │   sec/op    │   sec/op     vs base               │
LoadFS/1_routines-10    718.5m ± 0%   717.7m ± 0%       ~ (p=0.353 n=10)
LoadFS/10_routines-10   110.7m ± 2%   110.5m ± 1%       ~ (p=0.684 n=10)
LoadFS/20_routines-10   110.9m ± 0%   110.9m ± 1%       ~ (p=0.971 n=10)
geomean                 206.6m        206.4m       -0.09%

                      │  master.out  │           filter-fbc.out            │
                      │     B/op     │     B/op      vs base               │
LoadFS/1_routines-10    234.8Mi ± 0%   233.6Mi ± 0%  -0.49% (p=0.000 n=10)
LoadFS/10_routines-10   237.7Mi ± 0%   236.6Mi ± 0%  -0.43% (p=0.000 n=10)
LoadFS/20_routines-10   237.1Mi ± 0%   236.0Mi ± 0%  -0.47% (p=0.000 n=10)
geomean                 236.5Mi        235.4Mi       -0.47%

                      │ master.out  │           filter-fbc.out           │
                      │  allocs/op  │  allocs/op   vs base               │
LoadFS/1_routines-10    2.561M ± 0%   2.559M ± 0%  -0.09% (p=0.000 n=10)
LoadFS/10_routines-10   2.562M ± 0%   2.559M ± 0%  -0.09% (p=0.000 n=10)
LoadFS/20_routines-10   2.561M ± 0%   2.559M ± 0%  -0.09% (p=0.000 n=10)
geomean                 2.561M        2.559M       -0.09%

openshift-ci · 2024-03-28T19:50:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, 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:

~~OWNERS~~ [joelanford]

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

This commit adds two alpha flags to `opm render` and updates FBC library code to support efficient filtering of FBC: - `--alpha-filter-config=./path/to/config` - `--alpha-keep-packages=pkg1,pkg2` To support the filter config option, this commits defines a new configuration file format that supports: - filtering by package - within a package, filtering by channel - within a package, selecting a new default channel (required if the original default channel is filtered out) - within each channel, filtering by a version range In order to support correct version range filtering, some bundles that fall outside the version range may remain in the channel to ensure that the filtered channel is still valid and includes all bundles that did fall in the range Included in this commit is an update to declcfg.WalkMetasFS to enable concurrency, similar to recent changes in declcfg.LoadFS. declcfg.LoadFS is also refactored internally to use the new declcfg.WalkMetasFS implementation. Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

perdasilva · 2024-04-08T12:31:23Z

/override go-apidiff

openshift-ci · 2024-04-08T12:31:28Z

@perdasilva: Overrode contexts on behalf of perdasilva: go-apidiff

In response to this:

/override go-apidiff

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.

grokspawn · 2024-04-10T15:05:06Z

/hold to discuss with another team

perdasilva · 2024-05-17T12:29:47Z

@joelanford I tried to resolve conflicts using the GitHub UI ~~- feel free to force push over it =D~~ it didn't work T_T

joelanford · 2024-05-17T12:49:47Z

I think we're going to eventually close this, and hand the important bits of code to a stakeholder. For now, there is some consensus among maintainers that this is beyond the scope of core opm.

openshift-merge-robot · 2024-05-21T16:00:41Z

PR needs rebase.

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-sigs/prow repository.

openshift-ci bot requested review from kevinrizza and oceanc80 March 14, 2024 17:43

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2024

everettraven reviewed Mar 14, 2024

View reviewed changes

alpha/declcfg/filter/config/v1alpha1/config.go Show resolved Hide resolved

everettraven reviewed Mar 14, 2024

View reviewed changes

joelanford force-pushed the filter-fbc branch 2 times, most recently from c47b060 to 29c9db1 Compare March 22, 2024 18:53

joelanford force-pushed the filter-fbc branch 4 times, most recently from 3a2da31 to 4e18534 Compare March 26, 2024 20:44

everettraven approved these changes Mar 28, 2024

View reviewed changes

joelanford force-pushed the filter-fbc branch from 4e18534 to 2a4d098 Compare April 3, 2024 16:31

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

joelanford force-pushed the filter-fbc branch from 2a4d098 to 5d1a245 Compare April 3, 2024 17:01

joelanford added 2 commits April 3, 2024 17:24

refactor config file filter impl and add (some) tests

b498760

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford force-pushed the filter-fbc branch from 5d1a245 to b498760 Compare April 3, 2024 21:24

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

operator-framework deleted a comment from openshift-ci bot Apr 8, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2024

Merge branch 'master' into filter-fbc

eab0be1

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

render: add alpha support for filtering #1231

render: add alpha support for filtering #1231

joelanford commented Mar 14, 2024 •

edited

codecov bot commented Mar 14, 2024 •

edited

everettraven Mar 14, 2024

joelanford Mar 15, 2024

everettraven Mar 14, 2024 •

edited

joelanford Mar 15, 2024

joelanford Mar 15, 2024

everettraven Mar 14, 2024

joelanford Mar 15, 2024

joelanford Mar 16, 2024

grokspawn Mar 22, 2024

everettraven Mar 22, 2024

everettraven Mar 14, 2024

everettraven Mar 14, 2024

joelanford Mar 15, 2024

everettraven Mar 14, 2024

everettraven Mar 14, 2024

joelanford Mar 15, 2024

joelanford commented Mar 26, 2024

openshift-ci bot commented Mar 28, 2024

perdasilva commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

grokspawn commented Apr 10, 2024

perdasilva commented May 17, 2024 •

edited

joelanford commented May 17, 2024

openshift-merge-robot commented May 21, 2024

render: add alpha support for filtering #1231

Are you sure you want to change the base?

render: add alpha support for filtering #1231

Conversation

joelanford commented Mar 14, 2024 • edited

codecov bot commented Mar 14, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everettraven Mar 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford commented Mar 26, 2024

openshift-ci bot commented Mar 28, 2024

perdasilva commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

grokspawn commented Apr 10, 2024

perdasilva commented May 17, 2024 • edited

joelanford commented May 17, 2024

openshift-merge-robot commented May 21, 2024

joelanford commented Mar 14, 2024 •

edited

codecov bot commented Mar 14, 2024 •

edited

everettraven Mar 14, 2024 •

edited

perdasilva commented May 17, 2024 •

edited