Skip to content

Surface the manifest filenames for higher level logic #893

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

Closed
fgiloux wants to merge 1 commit into from
Closed

Surface the manifest filenames for higher level logic #893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ require (
golang.org/x/mod v0.4.2
golang.org/x/net v0.0.0-20210825183410-e898025ed96a
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
google.golang.org/grpc v1.41.0
google.golang.org/grpc v1.40.0
Copy link
Member

@timflannagan timflannagan Feb 3, 2022

Choose a reason for hiding this comment

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

Is there a reason we need to downgrade this dependency outside of OLM doesn't work well with the latest v1.41.x version? Can we instead replace pin on the OLM side-of-things while we work through the changes?

Copy link
Author

@fgiloux fgiloux Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Some explanation in these issue comments and in this article. TL;DR; you don't pin a version but specify a minimum requirement.
I wish the issue with grpc v1.41.0 would have been further investigated when discovered. It is not a bug in the grpc library but a conscious behavior change. Now if you don't use keepalive it is the normal behavior for a connection to get IDLE if no rpc call is made. The issue is that you are only testing for READY:

catalog.Status.GRPCConnectionState.LastConnectTime.After(after.LastUpdateTime.Time) && catalog.Status.GRPCConnectionState.LastObservedState == "READY" {

I could change the line and test for IDLE as well. The one thing is that when connections have been IDLE for a defined time (OS setting) they can be garbage collected. The grpc client would have two options:

  1. use keepalive
  2. gracefully handle disconnection and reestablish a connection when required

Option 1 is better if it is important not to incur any delay due to the reestablishment of a TCP connection. Option 2 is generally advised as it allows freeing up unused resources (network connections, filedescriptors, memory on the host) and don't unnecessarily create additional network bandwidth consumption (the keepalive packets)

I don't know enough of the way you use the grpc server to say whether the behavior change would break other things than the tests, hence the downgrade of the grpc library. This is however no sustainable approach, just building up some additional technical debt.

Copy link
Contributor

@exdx exdx Feb 4, 2022

Choose a reason for hiding this comment

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

That gRPC explanation makes a lot of sense -- I also started testing for IDLE when trying to address test flakes which did help the situation. It looks like the version bump on the registry side did affect performance.

I think Option 2 would be better since the client (OLM) needs to talk to the servers (catalog pods) only intermittently, when doing a sync, instead of all the time. It would also help reduce the resource consumption of OLM somewhat versus keeping connections open all the time.

Copy link
Member

@timflannagan timflannagan Feb 4, 2022

Choose a reason for hiding this comment

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

Thanks for the explanation - I think downgrading is reasonable to me, but I wonder whether this points to a need for a simple integration tests with OLM so we can catch this kind of regression quicker? That said, that would increase the coupling between the two repositories, and they both should have their own lifecycle for managing dependencies, so maybe the value add there is watered down.

Copy link
Member

@timflannagan timflannagan Mar 30, 2022

Choose a reason for hiding this comment

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

Just closing the loop on this: we ended up downgrading this dependency to v1.40.0 to mitigate a flake in the OLM e2e testing suite. It looks like github is still showing this change in the diff - do we need to rebase against the master branch?

Copy link
Author

@fgiloux fgiloux Mar 30, 2022

Choose a reason for hiding this comment

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

@timflannagan I am waiting for final review of operator-framework/operator-lifecycle-manager#2552. With the new approach we would not need this PR anymore as the match is now based on the resource identification through group, kind, namespace, name rather than filename. If things go as planned I would then close this PR

google.golang.org/grpc/cmd/protoc-gen-go-grpc v0.0.0-20200709232328-d8193ee9cc3e
google.golang.org/protobuf v1.27.1
gopkg.in/yaml.v2 v2.4.0
Expand Down
5 changes: 1 addition & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGX
github.com/cncf/udpa/go v0.0.0-20200629203442-efcf912fb354/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk=
github.com/cncf/udpa/go v0.0.0-20201120205902-5459f2c99403/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk=
github.com/cncf/xds/go v0.0.0-20210312221358-fbca930ec8ed/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/cncf/xds/go v0.0.0-20210805033703-aa0b78936158/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/cockroachdb/apd v1.1.0/go.mod h1:8Sl8LxpKi29FqWXR16WEFZRNSz3SoPzUzeMeY4+DwBQ=
github.com/cockroachdb/cockroach-go v0.0.0-20181001143604-e0a95dfd547c/go.mod h1:XGLbWH/ujMcbPbhZq52Nv6UrCghb1yGn//133kEsvDk=
github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:zn76sxSg3SzpJ0PPJaLDCu+Bu0Lg3sKTORVIj19EIF8=
Expand Down Expand Up @@ -251,7 +250,6 @@ github.com/envoyproxy/go-control-plane v0.9.7/go.mod h1:cwu0lG7PUMfa9snN8LXBig5y
github.com/envoyproxy/go-control-plane v0.9.9-0.20201210154907-fd9021fe5dad/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk=
github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk=
github.com/envoyproxy/go-control-plane v0.9.9-0.20210512163311-63b5d3c536b0/go.mod h1:hliV/p42l8fGbc6Y9bQ70uLwIvmJyVE5k4iMKlh8wCQ=
github.com/envoyproxy/go-control-plane v0.9.10-0.20210907150352-cf90f659a021/go.mod h1:AFq3mo9L8Lqqiid3OhADV3RfLJnjiw63cSpi+fDTRC0=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMikGIFbs31qQ=
github.com/evanphx/json-patch v4.9.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
Expand Down Expand Up @@ -1295,9 +1293,8 @@ google.golang.org/grpc v1.36.0/go.mod h1:qjiiYl8FncCW8feJPdyg3v6XW24KsRHe+dy9BAG
google.golang.org/grpc v1.36.1/go.mod h1:qjiiYl8FncCW8feJPdyg3v6XW24KsRHe+dy9BAGRRjU=
google.golang.org/grpc v1.37.0/go.mod h1:NREThFqKR1f3iQ6oBuvc5LadQuXVGo9rkm5ZGrQdJfM=
google.golang.org/grpc v1.38.0/go.mod h1:NREThFqKR1f3iQ6oBuvc5LadQuXVGo9rkm5ZGrQdJfM=
google.golang.org/grpc v1.40.0 h1:AGJ0Ih4mHjSeibYkFGh1dD9KJ/eOtZ93I6hoHhukQ5Q=
google.golang.org/grpc v1.40.0/go.mod h1:ogyxbiOoUXAkP+4+xa6PZSE9DZgIHtSpzjDTB9KAK34=
google.golang.org/grpc v1.41.0 h1:f+PlOh7QV4iIJkPrx5NQ7qaNGFQ3OTse67yaDHfju4E=
google.golang.org/grpc v1.41.0/go.mod h1:U3l9uK9J0sini8mHphKoXyaqDA/8VyGnDee1zzIUK6k=
google.golang.org/grpc/cmd/protoc-gen-go-grpc v0.0.0-20200709232328-d8193ee9cc3e h1:4BwkYybqoRhPKm97iNO3ACkxj26G0hC18CaO9QXOxto=
google.golang.org/grpc/cmd/protoc-gen-go-grpc v0.0.0-20200709232328-d8193ee9cc3e/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw=
google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
Expand Down
22 changes: 15 additions & 7 deletions pkg/configmap/configmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ type BundleLoader struct {
// creates an operator registry Bundle object.
// If the Data section has a PackageManifest resource then it is also
// deserialized and included in the result.
func (l *BundleLoader) Load(cm *corev1.ConfigMap) (bundle *api.Bundle, err error) {
// The filenames contained in the ConfigMap are also returned.
// bundle.Object and filenames share the same index.
func (l *BundleLoader) Load(cm *corev1.ConfigMap) (bundle *api.Bundle, filenames []string, err error) {
if cm == nil {
err = errors.New("ConfigMap must not be <nil>")
return
Expand All @@ -42,7 +44,7 @@ func (l *BundleLoader) Load(cm *corev1.ConfigMap) (bundle *api.Bundle, err error
"configmap": fmt.Sprintf("%s/%s", cm.GetNamespace(), cm.GetName()),
})

bundle, skipped, bundleErr := loadBundle(logger, cm)
bundle, skipped, filenames, bundleErr := loadBundle(logger, cm)
if bundleErr != nil {
err = fmt.Errorf("failed to extract bundle from configmap - %v", bundleErr)
return
Expand All @@ -51,18 +53,21 @@ func (l *BundleLoader) Load(cm *corev1.ConfigMap) (bundle *api.Bundle, err error
return
}

func loadBundle(entry *logrus.Entry, cm *corev1.ConfigMap) (bundle *api.Bundle, skipped map[string]string, err error) {
// loadBundle returns an API bundle built from a ConfigMap.
// it also returns the list of filenames and a list of files that have been skipped
// bundle.Object and filenames share the same index
func loadBundle(entry *logrus.Entry, cm *corev1.ConfigMap) (bundle *api.Bundle, skipped map[string]string, filenames []string, err error) {
bundle = &api.Bundle{Object: []string{}}
skipped = map[string]string{}

filenames = []string{}
data := cm.Data
if hasGzipEncodingAnnotation(cm) {
entry.Debug("Decoding gzip-encoded bundle data")

var err error
data, err = decodeGzipBinaryData(cm)
if err != nil {
return nil, nil, err
return nil, nil, nil, err
}
}

Expand All @@ -78,19 +83,22 @@ func loadBundle(entry *logrus.Entry, cm *corev1.ConfigMap) (bundle *api.Bundle,
logger.Infof("skipping due to decode error - %v", decodeErr)

// It may not be not a kube resource, let's add it to the skipped
// list so the caller can act on ot.
// list so the caller can act on to.
skipped[name] = content
continue
}

if resource.GetKind() == "ClusterServiceVersion" {
csvBytes, err := resource.MarshalJSON()
if err != nil {
return nil, nil, err
return nil, nil, nil, err
}
bundle.CsvJson = string(csvBytes)
bundle.CsvName = resource.GetName()
}
// Surface the file name for enabling manifest specific logic at a higher level
// This cannot be added to content as it is a plain string
filenames = append(filenames, name)
bundle.Object = append(bundle.Object, content)
logger.Infof("added to bundle, Kind=%s", resource.GetKind())
}
Expand Down
32 changes: 22 additions & 10 deletions pkg/configmap/configmap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,58 +24,64 @@ func TestLoad(t *testing.T) {
tests := []struct {
name string
source string
assertFunc func(t *testing.T, bundleGot *api.Bundle)
assertFunc func(t *testing.T, bundleGot *api.Bundle, filenamesGot []string)
}{
{
name: "BundleWithCsvAndCrd",
source: "testdata/bundle.cm.yaml",
assertFunc: func(t *testing.T, bundleGot *api.Bundle) {
assertFunc: func(t *testing.T, bundleGot *api.Bundle, filenamesGot []string) {
csvGot := bundleGot.GetCsvJson()
assert.NotNil(t, csvGot)
assert.Equal(t, "etcdoperator.v0.6.1", bundleGot.GetCsvName())

crdListGot := bundleGot.GetObject()
// 1 CSV + 1 CRD = 2 objects
assert.Equal(t, 2, len(crdListGot))
assert.True(t, assert.ElementsMatch(t, []string{"crd.yaml", "csv.yaml"}, filenamesGot))
},
},
{
name: "BundleWithBuiltInKubeTypes",
source: "testdata/bundle-with-kube-resources.cm.yaml",
assertFunc: func(t *testing.T, bundleGot *api.Bundle) {
assertFunc: func(t *testing.T, bundleGot *api.Bundle, filenamesGot []string) {
objects := bundleGot.GetObject()
assert.NotNil(t, objects)
assert.Equal(t, 1, len(objects))

unst, err := unstructuredlib.FromString(objects[0])
assert.NoError(t, err)
assert.True(t, unst.GetKind() == "Foo")

assert.True(t, assert.ElementsMatch(t, []string{"my.kube.type.yaml"}, filenamesGot))
},
},
{
name: "BundleWithMultipleCsvs",
source: "testdata/bundle-with-multiple-csvs.cm.yaml",
assertFunc: func(t *testing.T, bundleGot *api.Bundle) {
assertFunc: func(t *testing.T, bundleGot *api.Bundle, filenamesGot []string) {
csvGot := bundleGot.GetCsvJson()
assert.NotNil(t, csvGot)

unst, err := unstructuredlib.FromString(csvGot)
assert.NoError(t, err)
assert.True(t, unst.GetName() == "first" || unst.GetName() == "second")

assert.True(t, assert.ElementsMatch(t, []string{"first.csv.yaml", "second.csv.yaml"}, filenamesGot))
},
},
{
name: "BundleWithBadResource",
source: "testdata/bundle-with-bad-resource.cm.yaml",
assertFunc: func(t *testing.T, bundleGot *api.Bundle) {
assertFunc: func(t *testing.T, bundleGot *api.Bundle, filenamesGot []string) {
csvGot := bundleGot.GetCsvJson()
assert.NotNil(t, csvGot)

},
},
{
name: "BundleWithAll",
source: "testdata/bundle-with-all.yaml",
assertFunc: func(t *testing.T, bundleGot *api.Bundle) {
assertFunc: func(t *testing.T, bundleGot *api.Bundle, filenamesGot []string) {
csvGot := bundleGot.GetCsvJson()
assert.NotNil(t, csvGot)
unst, err := unstructuredlib.FromString(csvGot)
Expand All @@ -85,12 +91,15 @@ func TestLoad(t *testing.T) {
objects := bundleGot.GetObject()
// 2 CRDs + 1 CSV == 3 objects
assert.Equal(t, 3, len(objects))

// kiali.package.yaml not added as it is missing Kind
assert.True(t, assert.ElementsMatch(t, []string{"kiali.crd.yaml", "kiali.monitoringdashboards.crd.yaml", "kiali.v1.4.2.clusterserviceversion.yaml"}, filenamesGot))
},
},
{
name: "BundleWithNoDefaultChannel",
source: "testdata/bundle-with-no-default-channel.yaml",
assertFunc: func(t *testing.T, bundleGot *api.Bundle) {
assertFunc: func(t *testing.T, bundleGot *api.Bundle, filenamesGot []string) {
csvGot := bundleGot.GetCsvJson()
assert.NotNil(t, csvGot)
unst, err := unstructuredlib.FromString(csvGot)
Expand All @@ -99,6 +108,9 @@ func TestLoad(t *testing.T) {

objects := bundleGot.GetObject()
assert.Equal(t, 3, len(objects))

// kiali.package.yaml not added as it is missing Kind
assert.True(t, assert.ElementsMatch(t, []string{"kiali.crd.yaml", "kiali.monitoringdashboards.crd.yaml", "kiali.v1.4.2.clusterserviceversion.yaml"}, filenamesGot))
},
},
}
Expand All @@ -108,13 +120,13 @@ func TestLoad(t *testing.T) {
cm := loadfromFile(t, tt.source)

loader := NewBundleLoader()
bundleGot, errGot := loader.Load(cm)
bundleGot, filenamesGot, errGot := loader.Load(cm)

assert.NoError(t, errGot)
assert.NotNil(t, bundleGot)

if tt.assertFunc != nil {
tt.assertFunc(t, bundleGot)
tt.assertFunc(t, bundleGot, filenamesGot)
}
})
}
Expand Down Expand Up @@ -157,7 +169,7 @@ func TestLoadWriteRead(t *testing.T) {
assert.NoError(t, err)

bundleLoader := NewBundleLoader()
bundle, err := bundleLoader.Load(cm)
bundle, _, err := bundleLoader.Load(cm)

expectedObjects, err := unstructuredlib.FromDir(tt.source + "manifests/")
assert.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/bundle_image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ var _ = Describe("Launch bundle", func() {
bundleDataConfigMap, err = kubeclient.CoreV1().ConfigMaps(namespace).Get(context.TODO(), bundleDataConfigMap.GetName(), metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())

bundle, err := configmap.NewBundleLoader().Load(bundleDataConfigMap)
bundle, _, err := configmap.NewBundleLoader().Load(bundleDataConfigMap)
Expect(err).NotTo(HaveOccurred())

expectedObjects, err := unstructuredlib.FromDir(bundleDirectory + "/manifests")
Expand Down
5 changes: 2 additions & 3 deletions vendor/google.golang.org/grpc/MAINTAINERS.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions vendor/google.golang.org/grpc/Makefile
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 0 additions & 13 deletions vendor/google.golang.org/grpc/NOTICE.txt
View file
Open in desktop

This file was deleted.

68 changes: 19 additions & 49 deletions vendor/google.golang.org/grpc/balancer/balancer.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading