Skip to content

Commit

Permalink
fix(opm diff): handle cyclic dependency graph (#937)
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@anik120
anik120 committed Mar 24, 2022
1 parent b4264e2 commit de36104
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 52 deletions.
18 changes: 9 additions & 9 deletions alpha/action/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ type Diff struct {
Logger *logrus.Entry
}

func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
if err := a.validate(); err != nil {
func (diff Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
if err := diff.validate(); err != nil {
return nil, err
}

Expand All @@ -42,8 +42,8 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {

// Heads-only mode does not require an old ref, so there may be nothing to render.
var oldModel model.Model
if len(a.OldRefs) != 0 {
oldRender := Render{Refs: a.OldRefs, Registry: a.Registry, AllowedRefMask: mask}
if len(diff.OldRefs) != 0 {
oldRender := Render{Refs: diff.OldRefs, Registry: diff.Registry, AllowedRefMask: mask}
oldCfg, err := oldRender.Run(ctx)
if err != nil {
if errors.Is(err, ErrNotAllowed) {
Expand All @@ -57,7 +57,7 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
}
}

newRender := Render{Refs: a.NewRefs, Registry: a.Registry, AllowedRefMask: mask}
newRender := Render{Refs: diff.NewRefs, Registry: diff.Registry, AllowedRefMask: mask}
newCfg, err := newRender.Run(ctx)
if err != nil {
if errors.Is(err, ErrNotAllowed) {
Expand All @@ -71,10 +71,10 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
}

g := &declcfg.DiffGenerator{
Logger: a.Logger,
SkipDependencies: a.SkipDependencies,
Includer: convertIncludeConfigToIncluder(a.IncludeConfig),
IncludeAdditively: a.IncludeAdditively,
Logger: diff.Logger,
SkipDependencies: diff.SkipDependencies,
Includer: convertIncludeConfigToIncluder(diff.IncludeConfig),
IncludeAdditively: diff.IncludeAdditively,
}
diffModel, err := g.Run(oldModel, newModel)
if err != nil {
Expand Down
27 changes: 22 additions & 5 deletions alpha/declcfg/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,24 +242,27 @@ func bundlesEqual(b1, b2 *model.Bundle) (bool, error) {
func addAllDependencies(newModel, oldModel, outputModel model.Model) error {
// Get every oldModel's bundle's dependencies, and their dependencies, etc. by BFS.
providingBundlesByPackage := map[string][]*model.Bundle{}
for curr := getBundles(outputModel); len(curr) != 0; {
reqGVKs, reqPkgs, err := findDependencies(curr)
var visitedBundles []*model.Bundle
for currentList := getBundles(outputModel); len(currentList) != 0; {
visitedBundles = append(visitedBundles, currentList...)
reqGVKs, reqPkgs, err := findDependencies(currentList)
if err != nil {
return err
}
// Break early so the entire source model is not iterated through unnecessarily.
if len(reqGVKs) == 0 && len(reqPkgs) == 0 {
break
}
curr = nil
currentList = nil
// Get bundles that provide dependencies from newModel, which should have
// the latest bundles of each dependency package.
for _, pkg := range newModel {
providingBundles := getBundlesThatProvide(pkg, reqGVKs, reqPkgs)
curr = append(curr, providingBundles...)
unvisitedProvidingBundles := difference(visitedBundles, providingBundles)
currentList = append(currentList, unvisitedProvidingBundles...)

oldPkg, oldHasPkg := oldModel[pkg.Name]
for _, b := range providingBundles {
for _, b := range unvisitedProvidingBundles {
// If the bundle is not in oldModel, add it to the set.
// outputModel is checked below.
add := true
Expand Down Expand Up @@ -341,6 +344,20 @@ func addAllDependencies(newModel, oldModel, outputModel model.Model) error {
return nil
}

func difference(a, b []*model.Bundle) []*model.Bundle {
aMap := make(map[*model.Bundle]struct{})
for _, bd := range a {
aMap[bd] = struct{}{}
}
uniqueBundles := make([]*model.Bundle, 0)
for _, bd := range b {
if _, present := aMap[bd]; !present {
uniqueBundles = append(uniqueBundles, bd)
}
}
return uniqueBundles
}

// getBundles collects all bundles specified by m. Since each bundle
// references its package, their uniqueness property holds in a flat list.
func getBundles(m model.Model) (bundles []*model.Bundle) {
Expand Down
80 changes: 80 additions & 0 deletions alpha/declcfg/diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,86 @@ func TestDiffHeadsOnly(t *testing.T) {
},
},
},
{
name: "HasDiff/CyclicDependencyGraph",
newCfg: DeclarativeConfig{
Packages: []Package{
{Schema: schemaPackage, Name: "foo", DefaultChannel: "stable"},
{Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"},
},
Channels: []Channel{
{Schema: schemaChannel, Name: "stable", Package: "bar", Entries: []ChannelEntry{
{Name: "bar.v4.9.3"},
}},
{Schema: schemaChannel, Name: "stable", Package: "foo", Entries: []ChannelEntry{
{Name: "foo.v4.9.3"},
}},
},
Bundles: []Bundle{
{
Schema: schemaBundle,
Name: "foo.v4.9.3",
Package: "foo",
Image: "reg/foo:latest",
Properties: []property.Property{
property.MustBuildPackage("foo", "0.1.0"),
property.MustBuildGVK("foo", "v1alpha1", "Foo"),
property.MustBuildGVKRequired("bar", "v1alpha1", "Bar"),
},
},
{
Schema: schemaBundle,
Name: "bar.v4.9.3",
Package: "bar",
Image: "reg/bar:latest",
Properties: []property.Property{
property.MustBuildPackage("bar", "4.9.3"),
property.MustBuildGVK("bar", "v1alpha1", "Bar"),
property.MustBuildGVKRequired("foo", "v1alpha1", "Foo"),
},
},
},
},
g: &DiffGenerator{},
expCfg: DeclarativeConfig{
Packages: []Package{
{Schema: schemaPackage, Name: "bar", DefaultChannel: "stable"},
{Schema: schemaPackage, Name: "foo", DefaultChannel: "stable"},
},
Channels: []Channel{
{Schema: schemaChannel, Name: "stable", Package: "bar", Entries: []ChannelEntry{
{Name: "bar.v4.9.3"},
}},
{Schema: schemaChannel, Name: "stable", Package: "foo", Entries: []ChannelEntry{
{Name: "foo.v4.9.3"},
}},
},
Bundles: []Bundle{
{
Schema: schemaBundle,
Name: "bar.v4.9.3",
Package: "bar",
Image: "reg/bar:latest",
Properties: []property.Property{
property.MustBuildGVK("bar", "v1alpha1", "Bar"),
property.MustBuildGVKRequired("foo", "v1alpha1", "Foo"),
property.MustBuildPackage("bar", "4.9.3"),
},
},
{
Schema: schemaBundle,
Name: "foo.v4.9.3",
Package: "foo",
Image: "reg/foo:latest",
Properties: []property.Property{
property.MustBuildGVK("foo", "v1alpha1", "Foo"),
property.MustBuildGVKRequired("bar", "v1alpha1", "Bar"),
property.MustBuildPackage("foo", "0.1.0"),
},
},
},
},
},
{
// Testing SkipDependencies only really makes sense in heads-only mode,
// since new dependencies are always added.
Expand Down
76 changes: 38 additions & 38 deletions cmd/opm/alpha/diff/cmd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const (
timeout = time.Minute * 1
)

type diff struct {
type input struct {
oldRefs []string
newRefs []string
skipDeps bool
Expand All @@ -51,7 +51,7 @@ var includeFileExample = fmt.Sprintf(`packages:
%[1]s - 0.2.0-alpha.0`, templates.Indentation)

func NewCmd() *cobra.Command {
a := diff{
in := input{
logger: logrus.NewEntry(logrus.New()),
}
cmd := &cobra.Command{
Expand Down Expand Up @@ -115,91 +115,91 @@ docker push registry.org/my-catalog:diff-latest
`), includeFileExample),
Args: cobra.RangeArgs(1, 2),
PreRunE: func(cmd *cobra.Command, args []string) error {
if a.debug {
a.logger.Logger.SetLevel(logrus.DebugLevel)
if in.debug {
in.logger.Logger.SetLevel(logrus.DebugLevel)
}
a.logger.Logger.SetOutput(os.Stderr)
in.logger.Logger.SetOutput(os.Stderr)
return nil
},
RunE: a.addFunc,
RunE: in.diffFunc,
}

cmd.Flags().BoolVar(&a.skipDeps, "skip-deps", false, "do not include bundle dependencies in the output catalog")
cmd.Flags().BoolVar(&in.skipDeps, "skip-deps", false, "do not include bundle dependencies in the output catalog")

cmd.Flags().StringVarP(&a.output, "output", "o", "yaml", "Output format (json|yaml)")
cmd.Flags().StringVar(&a.caFile, "ca-file", "", "the root Certificates to use with this command")
cmd.Flags().StringVarP(&a.includeFile, "include-file", "i", "",
cmd.Flags().StringVarP(&in.output, "output", "o", "yaml", "Output format (json|yaml)")
cmd.Flags().StringVar(&in.caFile, "ca-file", "", "the root Certificates to use with this command")
cmd.Flags().StringVarP(&in.includeFile, "include-file", "i", "",
"YAML defining packages, channels, and/or bundles/versions to extract from the new refs. "+
"Upgrade graphs from individual bundles/versions to their channel's head are also included")
cmd.Flags().BoolVar(&a.includeAdditive, "include-additive", false,
cmd.Flags().BoolVar(&in.includeAdditive, "include-additive", false,
"Ref objects from --include-file are returned on top of 'heads-only' or 'latest' output")

cmd.Flags().BoolVar(&a.debug, "debug", false, "enable debug logging")
cmd.Flags().BoolVar(&in.debug, "debug", false, "enable debug logging")
return cmd
}

func (a *diff) addFunc(cmd *cobra.Command, args []string) error {
a.parseArgs(args)
func (in *input) diffFunc(cmd *cobra.Command, args []string) error {
in.parseArgs(args)

if cmd.Flags().Changed("include-additive") && a.includeFile == "" {
a.logger.Fatal("must set --include-file if --include-additive is set")
if cmd.Flags().Changed("include-additive") && in.includeFile == "" {
in.logger.Fatal("must set --include-file if --include-additive is set")
}

var write func(declcfg.DeclarativeConfig, io.Writer) error
switch a.output {
switch in.output {
case "yaml":
write = declcfg.WriteYAML
case "json":
write = declcfg.WriteJSON
default:
return fmt.Errorf("invalid --output value: %q", a.output)
return fmt.Errorf("invalid --output value: %q", in.output)
}

skipTLSVerify, useHTTP, err := util.GetTLSOptions(cmd)
if err != nil {
return err
}

rootCAs, err := certs.RootCAs(a.caFile)
rootCAs, err := certs.RootCAs(in.caFile)
if err != nil {
a.logger.Fatalf("error getting root CAs: %v", err)
in.logger.Fatalf("error getting root CAs: %v", err)
}
reg, err := containerd.NewRegistry(
containerd.SkipTLSVerify(skipTLSVerify),
containerd.WithLog(a.logger),
containerd.WithLog(in.logger),
containerd.WithRootCAs(rootCAs),
containerd.WithPlainHTTP(useHTTP),
)
if err != nil {
a.logger.Fatalf("error creating containerd registry: %v", err)
in.logger.Fatalf("error creating containerd registry: %v", err)
}
defer func() {
if err := reg.Destroy(); err != nil {
a.logger.Errorf("error destroying local cache: %v", err)
in.logger.Errorf("error destroying local cache: %v", err)
}
}()

diff := action.Diff{
Registry: reg,
OldRefs: a.oldRefs,
NewRefs: a.newRefs,
SkipDependencies: a.skipDeps,
IncludeAdditively: a.includeAdditive,
Logger: a.logger,
OldRefs: in.oldRefs,
NewRefs: in.newRefs,
SkipDependencies: in.skipDeps,
IncludeAdditively: in.includeAdditive,
Logger: in.logger,
}

if a.includeFile != "" {
f, err := os.Open(a.includeFile)
if in.includeFile != "" {
f, err := os.Open(in.includeFile)
if err != nil {
a.logger.Fatalf("error opening include file: %v", err)
in.logger.Fatalf("error opening include file: %v", err)
}
defer func() {
if cerr := f.Close(); cerr != nil {
a.logger.Error(cerr)
in.logger.Error(cerr)
}
}()
if diff.IncludeConfig, err = action.LoadDiffIncludeConfig(f); err != nil {
a.logger.Fatalf("error loading include file: %v", err)
in.logger.Fatalf("error loading include file: %v", err)
}
}

Expand All @@ -208,17 +208,17 @@ func (a *diff) addFunc(cmd *cobra.Command, args []string) error {

cfg, err := diff.Run(ctx)
if err != nil {
a.logger.Fatalf("error generating diff: %v", err)
in.logger.Fatalf("error generating diff: %v", err)
}

if err := write(*cfg, os.Stdout); err != nil {
a.logger.Fatalf("error writing diff: %v", err)
in.logger.Fatalf("error writing diff: %v", err)
}

return nil
}

func (a *diff) parseArgs(args []string) {
func (in *input) parseArgs(args []string) {
var old, new string
switch len(args) {
case 1:
Expand All @@ -229,7 +229,7 @@ func (a *diff) parseArgs(args []string) {
logrus.Panic("should never be here, CLI must enforce arg size")
}
if old != "" {
a.oldRefs = strings.Split(old, ",")
in.oldRefs = strings.Split(old, ",")
}
a.newRefs = strings.Split(new, ",")
in.newRefs = strings.Split(new, ",")
}

0 comments on commit de36104

Please sign in to comment.