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.

Sign up for GitHub

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

🌱 refactor catalogmetadata types used for resolution #599

Closed
wants to merge 3 commits into from
Closed

🌱 refactor catalogmetadata types used for resolution #599

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e37bb1c
refactor catalogmetadata types used for resolution
everettraven Jan 31, 2024
94fe3df
add lazy loading/caching back
everettraven Feb 2, 2024
32466fa
better type checking
everettraven Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,10 @@ func main() {
}

if err = (&controllers.ClusterExtensionReconciler{
Client: cl,
BundleProvider: catalogClient,
Scheme: mgr.GetScheme(),
Resolver: resolver,
Client: cl,
CatalogProvider: catalogClient,
Scheme: mgr.GetScheme(),
Resolver: resolver,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
os.Exit(1)
Expand Down
82 changes: 61 additions & 21 deletions cmd/resolutioncli/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,22 @@ import (
"context"

"github.com/operator-framework/operator-registry/alpha/action"
"github.com/operator-framework/operator-registry/alpha/declcfg"

"github.com/operator-framework/operator-controller/internal/catalogmetadata"
"github.com/operator-framework/operator-controller/internal/catalogmetadata/client"
"github.com/operator-framework/operator-controller/internal/controllers"
)

type indexRefClient struct {
renderer action.Render
bundlesCache []*catalogmetadata.Bundle
renderer action.Render
bundlesCache []*catalogmetadata.Bundle
channelsCache []*catalogmetadata.Channel
packagesCache []*catalogmetadata.Package
}

var _ controllers.CatalogProvider = &indexRefClient{}
everettraven marked this conversation as resolved.
Show resolved Hide resolved

func newIndexRefClient(indexRef string) *indexRefClient {
return &indexRefClient{
renderer: action.Render{
Expand All @@ -39,47 +45,81 @@ func newIndexRefClient(indexRef string) *indexRefClient {
}
}

func (c *indexRefClient) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error) {
if c.bundlesCache == nil {
func (c *indexRefClient) CatalogContents(ctx context.Context) (*client.Contents, error) {
if c.bundlesCache == nil || c.channelsCache == nil || c.packagesCache == nil {
cfg, err := c.renderer.Run(ctx)
if err != nil {
return nil, err
}

var (
channels []*catalogmetadata.Channel
bundles []*catalogmetadata.Bundle
deprecations []*catalogmetadata.Deprecation
channels []*catalogmetadata.Channel
bundles []*catalogmetadata.Bundle
packages []*catalogmetadata.Package
)

// TODO: update fake catalog name string to be catalog name once we support multiple catalogs in CLI
catalogName := "offline-catalog"

for i := range cfg.Packages {
packages = append(packages, &catalogmetadata.Package{
Package: cfg.Packages[i],
Catalog: catalogName,
})
}

for i := range cfg.Channels {
channels = append(channels, &catalogmetadata.Channel{
Channel: cfg.Channels[i],
Catalog: catalogName,
})
}

for i := range cfg.Bundles {
bundles = append(bundles, &catalogmetadata.Bundle{
Bundle: cfg.Bundles[i],
Bundle: cfg.Bundles[i],
Catalog: catalogName,
})
}

for i := range cfg.Deprecations {
deprecations = append(deprecations, &catalogmetadata.Deprecation{
Deprecation: cfg.Deprecations[i],
})
}

// TODO: update fake catalog name string to be catalog name once we support multiple catalogs in CLI
catalogName := "offline-catalog"

bundles, err = client.PopulateExtraFields(catalogName, channels, bundles, deprecations)
if err != nil {
return nil, err
for _, deprecation := range cfg.Deprecations {
for _, entry := range deprecation.Entries {
switch entry.Reference.Schema {
case declcfg.SchemaPackage:
for _, pkg := range packages {
if pkg.Name == deprecation.Package {
pkg.Deprecation = &declcfg.DeprecationEntry{
Reference: entry.Reference,
Message: entry.Message,
}
}
}
case declcfg.SchemaChannel:
for _, channel := range channels {
if channel.Package == deprecation.Package && channel.Name == entry.Reference.Name {
channel.Deprecation = &declcfg.DeprecationEntry{
Reference: entry.Reference,
Message: entry.Message,
}
}
}
case declcfg.SchemaBundle:
for _, bundle := range bundles {
if bundle.Package == deprecation.Package && bundle.Name == entry.Reference.Name {
bundle.Deprecation = &declcfg.DeprecationEntry{
Reference: entry.Reference,
Message: entry.Message,
}
}
}
}
}
}

c.bundlesCache = bundles
c.channelsCache = channels
c.packagesCache = packages
}

return c.bundlesCache, nil
return &client.Contents{}, nil
}
4 changes: 2 additions & 2 deletions cmd/resolutioncli/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func run(ctx context.Context, packageName, packageChannel, packageVersionRange,

cl := clientBuilder.Build()
catalogClient := newIndexRefClient(indexRef)
allBundles, err := catalogClient.Bundles(ctx)
contents, err := catalogClient.CatalogContents(ctx)
if err != nil {
return err
}
Expand All @@ -162,7 +162,7 @@ func run(ctx context.Context, packageName, packageChannel, packageVersionRange,
if err := cl.List(ctx, &bundleDeploymentList); err != nil {
return err
}
variables, err := controllers.GenerateVariables(allBundles, clusterExtensionList.Items, bundleDeploymentList.Items)
variables, err := controllers.GenerateVariables(contents, clusterExtensionList.Items, bundleDeploymentList.Items)
if err != nil {
return err
}
Expand Down
107 changes: 54 additions & 53 deletions internal/catalogmetadata/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,31 @@ type Client struct {
fetcher Fetcher
}

func (c *Client) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error) {
var allBundles []*catalogmetadata.Bundle
type Contents struct {
Packages []*catalogmetadata.Package
Channels []*catalogmetadata.Channel
Bundles []*catalogmetadata.Bundle
}

func (c *Client) CatalogContents(ctx context.Context) (*Contents, error) {
var catalogList catalogd.CatalogList
if err := c.cl.List(ctx, &catalogList); err != nil {
return nil, err
}

contents := &Contents{
Packages: []*catalogmetadata.Package{},
Channels: []*catalogmetadata.Channel{},
Bundles: []*catalogmetadata.Bundle{},
}

for _, catalog := range catalogList.Items {
// if the catalog has not been successfully unpacked, skip it
if !meta.IsStatusConditionPresentAndEqual(catalog.Status.Conditions, catalogd.TypeUnpacked, metav1.ConditionTrue) {
continue
}

packages := []*catalogmetadata.Package{}
channels := []*catalogmetadata.Channel{}
bundles := []*catalogmetadata.Bundle{}
deprecations := []*catalogmetadata.Deprecation{}
Expand All @@ -70,93 +83,81 @@ func (c *Client) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error)
return fmt.Errorf("error was provided to the WalkMetasReaderFunc: %s", err)
}
switch meta.Schema {
case declcfg.SchemaPackage:
var content catalogmetadata.Package
if err := json.Unmarshal(meta.Blob, &content); err != nil {
return fmt.Errorf("error unmarshalling channel from catalog metadata: %s", err)
}
content.Catalog = catalog.Name
packages = append(packages, &content)
case declcfg.SchemaChannel:
var content catalogmetadata.Channel
if err := json.Unmarshal(meta.Blob, &content); err != nil {
return fmt.Errorf("error unmarshalling channel from catalog metadata: %s", err)
}
content.Catalog = catalog.Name
channels = append(channels, &content)
case declcfg.SchemaBundle:
var content catalogmetadata.Bundle
if err := json.Unmarshal(meta.Blob, &content); err != nil {
return fmt.Errorf("error unmarshalling bundle from catalog metadata: %s", err)
}
content.Catalog = catalog.Name
bundles = append(bundles, &content)
case declcfg.SchemaDeprecation:
var content catalogmetadata.Deprecation
if err := json.Unmarshal(meta.Blob, &content); err != nil {
return fmt.Errorf("error unmarshalling deprecation from catalog metadata: %s", err)
}
content.Catalog = catalog.Name
deprecations = append(deprecations, &content)
}

return nil
})
if err != nil {
return nil, fmt.Errorf("error processing response: %s", err)
}

bundles, err = PopulateExtraFields(catalog.Name, channels, bundles, deprecations)
if err != nil {
return nil, err
}

allBundles = append(allBundles, bundles...)
}

return allBundles, nil
}

func PopulateExtraFields(catalogName string, channels []*catalogmetadata.Channel, bundles []*catalogmetadata.Bundle, deprecations []*catalogmetadata.Deprecation) ([]*catalogmetadata.Bundle, error) {
bundlesMap := map[string]*catalogmetadata.Bundle{}
for i := range bundles {
bundleKey := fmt.Sprintf("%s-%s", bundles[i].Package, bundles[i].Name)
bundlesMap[bundleKey] = bundles[i]

bundles[i].CatalogName = catalogName
}

for _, ch := range channels {
for _, chEntry := range ch.Entries {
bundleKey := fmt.Sprintf("%s-%s", ch.Package, chEntry.Name)
bundle, ok := bundlesMap[bundleKey]
if !ok {
return nil, fmt.Errorf("bundle %q not found in catalog %q (package %q, channel %q)", chEntry.Name, catalogName, ch.Package, ch.Name)
}

bundle.InChannels = append(bundle.InChannels, ch)
return nil, fmt.Errorf("error processing response: %s", err)
}
}

// According to https://docs.google.com/document/d/1EzefSzoGZL2ipBt-eCQwqqNwlpOIt7wuwjG6_8ZCi5s/edit?usp=sharing
// the olm.deprecations FBC object is only valid when either 0 or 1 instances exist
// for any given package
deprecationMap := make(map[string]*catalogmetadata.Deprecation, len(deprecations))
for _, deprecation := range deprecations {
deprecationMap[deprecation.Package] = deprecation
}

for i := range bundles {
if dep, ok := deprecationMap[bundles[i].Package]; ok {
for _, entry := range dep.Entries {
for _, deprecation := range deprecations {
for _, entry := range deprecation.Entries {
switch entry.Reference.Schema {
case declcfg.SchemaPackage:
bundles[i].Deprecations = append(bundles[i].Deprecations, entry)
for _, pkg := range packages {
if pkg.Name == deprecation.Package {
pkg.Deprecation = &declcfg.DeprecationEntry{
Reference: entry.Reference,
Message: entry.Message,
}
}
}
case declcfg.SchemaChannel:
for _, ch := range bundles[i].InChannels {
if ch.Name == entry.Reference.Name {
bundles[i].Deprecations = append(bundles[i].Deprecations, entry)
break
for _, channel := range channels {
if channel.Package == deprecation.Package && channel.Name == entry.Reference.Name {
channel.Deprecation = &declcfg.DeprecationEntry{
Reference: entry.Reference,
Message: entry.Message,
}
}
}
case declcfg.SchemaBundle:
if bundles[i].Name == entry.Reference.Name {
bundles[i].Deprecations = append(bundles[i].Deprecations, entry)
for _, bundle := range bundles {
if bundle.Package == deprecation.Package && bundle.Name == entry.Reference.Name {
bundle.Deprecation = &declcfg.DeprecationEntry{
Reference: entry.Reference,
Message: entry.Message,
}
Comment on lines +147 to +150
Copy link
Member

Choose a reason for hiding this comment

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

If I remember our conversation from January correctly, we decided to get rid of fields like InChannels and try to keep original FBC/declcfg stucts as much as possible. I see that InChannels is now gone, but Deprecation field on Bundle and other structs was added. I think it is the same same concept.

Should we explore adding some maps in places where we need to do lookups of deprecation entries? Or something similar what you did to channels (using filters).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I similarly recall the conversation as keeping the wrapper as simple as possible if one is necessary (at the very least we need one for that catalog name since the declcfg.* representations don't store catalog information). I recall @joelanford mentioning adding the deprecation entry to the wrapper as well in previous conversations so I took that approach as it was one less set of information to deal with after the fact. I'm not opposed to either of the approaches though so this is more or less a "let's pick one and I'll implement it" situation.

The benefit of this approach is that there is no lookup cost after the set of packages, channels, and bundles are returned but there are definitely other solutions where the lookup cost is negligible that I'm open to exploring.

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember the context of my suggestion of putting the deprecation entry in the wrapper, but I think I'd suggest separating it out and keeping the structure similar to the FBC structure.

}
}
}
}
}

contents.Packages = append(contents.Packages, packages...)
contents.Channels = append(contents.Channels, channels...)
contents.Bundles = append(contents.Bundles, bundles...)
}

return bundles, nil
return contents, nil
}
Loading
Loading