Skip to content
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

fix(opm diff): handle cyclic dependency graph #937

Merged

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Mar 21, 2022

While generating a diff from a catalog that has operators that specify dependencies
that are cyclic in nature, eg a->b, b->a, the opm alpha diff command hangs. This
was happening because while generating the diff, the command does a breadth-first
search of the dependency graph generated by the operator bundles, but did not keep
a track of the already visited bundles. As a result, when there was a cycle in the
dependency graph, the command was stuck in an infinite loop.

This PR fixes the issue by keeping track of the already visited bundles during the
search, and moving the search forward with only the bundles that haven't been visited
before.

Fixes #936

Description of the change:

Motivation for the change:

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
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #937 (87c3538) into master (b4264e2) will increase coverage by 0.04%.
The diff coverage is 95.00%.

❗ Current head 87c3538 differs from pull request most recent head 45f486d. Consider uploading reports for the commit 45f486d to get more accurate results

@@            Coverage Diff             @@
##           master     #937      +/-   ##
==========================================
+ Coverage   52.21%   52.25%   +0.04%     
==========================================
  Files         103      103              
  Lines        9117     9126       +9     
==========================================
+ Hits         4760     4769       +9     
  Misses       3451     3451              
  Partials      906      906              
Impacted Files Coverage Δ
alpha/action/diff.go 77.04% <87.50%> (ø)
alpha/declcfg/diff.go 78.77% <100.00%> (+0.80%) ⬆️

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 b4264e2...45f486d. Read the comment docs.

Copy link
Member

@exdx exdx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the implementation and I'm not 100% sure I follow everything, but it seems intuitive that ensuring that the bundles in the dependency graph are unique should not impact the diff ultimately. Also the CI is green so more confidence there.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2022
alpha/declcfg/diff.go Outdated Show resolved Hide resolved
alpha/declcfg/diff.go Show resolved Hide resolved
While generating a diff from a catalog that has operators that specify dependencies
that are cyclic in nature, eg a->b, b->a, the `opm alpha diff` command hangs. This
was happening because while generating the diff, the command does a breadth-first
search of the dependency graph generated by the operator bundles, but did not keep
a track of the already visited bundles. As a result, when there was a cycle in the
dependency graph, the command was stuck in an infinite loop.

This PR fixes the issue by keeping track of the already visited bundles during the
search, and moving the search forward with only the bundles that haven't been visited
before.

Fixes operator-framework#936

Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2022
@joelanford
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022
@exdx
Copy link
Member

exdx commented Mar 24, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2022
@openshift-merge-robot openshift-merge-robot merged commit de36104 into operator-framework:master Mar 24, 2022
grokspawn pushed a commit that referenced this pull request Apr 19, 2022
While generating a diff from a catalog that has operators that specify dependencies
that are cyclic in nature, eg a->b, b->a, the `opm alpha diff` command hangs. This
was happening because while generating the diff, the command does a breadth-first
search of the dependency graph generated by the operator bundles, but did not keep
a track of the already visited bundles. As a result, when there was a cycle in the
dependency graph, the command was stuck in an infinite loop.

This PR fixes the issue by keeping track of the already visited bundles during the
search, and moving the search forward with only the bundles that haven't been visited
before.

Fixes #936

Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
grokspawn pushed a commit that referenced this pull request Apr 20, 2022
While generating a diff from a catalog that has operators that specify dependencies
that are cyclic in nature, eg a->b, b->a, the `opm alpha diff` command hangs. This
was happening because while generating the diff, the command does a breadth-first
search of the dependency graph generated by the operator bundles, but did not keep
a track of the already visited bundles. As a result, when there was a cycle in the
dependency graph, the command was stuck in an infinite loop.

This PR fixes the issue by keeping track of the already visited bundles during the
search, and moving the search forward with only the bundles that haven't been visited
before.

Fixes #936

Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] opm alpha diff hangs when processing the Red Hat 4.10 catalog
4 participants