From e6e16724aef0de0a7cde0b034a243e7e8c76d225 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Thu, 18 Jul 2024 14:54:14 -0500 Subject: [PATCH 01/13] feat: implement initial benchmark tests Signed-off-by: Edmund Ochieng --- internal/authentication/tokengetter_test.go | 62 +++++++++++++++++ .../contentmanager/contentmanager_test.go | 66 +++++++++++++++++++ 2 files changed, 128 insertions(+) diff --git a/internal/authentication/tokengetter_test.go b/internal/authentication/tokengetter_test.go index b9553cac3..1d88c057f 100644 --- a/internal/authentication/tokengetter_test.go +++ b/internal/authentication/tokengetter_test.go @@ -85,3 +85,65 @@ func TestTokenGetterGet(t *testing.T) { } } } + +func BenchmarkTokenGetterGet(b *testing.B) { + fakeClient := fake.NewSimpleClientset() + fakeClient.PrependReactor("create", "serviceaccounts/token", + func(action ctest.Action) (bool, runtime.Object, error) { + act, ok := action.(ctest.CreateActionImpl) + if !ok { + return false, nil, nil + } + tokenRequest := act.GetObject().(*authenticationv1.TokenRequest) + var err error + if act.Name == "test-service-account-1" { + tokenRequest.Status = authenticationv1.TokenRequestStatus{ + Token: "test-token-1", + ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(DefaultExpirationDuration)), + } + } + if act.Name == "test-service-account-2" { + tokenRequest.Status = authenticationv1.TokenRequestStatus{ + Token: "test-token-2", + ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(1 * time.Second)), + } + } + if act.Name == "test-service-account-3" { + tokenRequest.Status = authenticationv1.TokenRequestStatus{ + Token: "test-token-3", + ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(-10 * time.Second)), + } + } + if act.Name == "test-service-account-4" { + tokenRequest = nil + err = fmt.Errorf("error when fetching token") + } + return true, tokenRequest, err + }, + ) + + tg := NewTokenGetter(fakeClient.CoreV1(), + WithExpirationDuration(DefaultExpirationDuration), + ) + + test := struct { + serviceAccountName string + namespace string + }{ + serviceAccountName: "test-service-account-1", + namespace: "test-namespace-1", + } + + for n := 0; n < b.N; n++ { + token, err := tg.Get(context.Background(), + types.NamespacedName{ + Namespace: test.namespace, + Name: test.serviceAccountName, + }, + ) + if err != nil { + b.Logf("token creation; %v", err) + } + _ = token + } +} diff --git a/internal/contentmanager/contentmanager_test.go b/internal/contentmanager/contentmanager_test.go index 598051b59..e46ebccde 100644 --- a/internal/contentmanager/contentmanager_test.go +++ b/internal/contentmanager/contentmanager_test.go @@ -108,6 +108,48 @@ func TestWatch(t *testing.T) { } } +func BenchmarkWatch(b *testing.B) { + rcm := func(_ context.Context, _ client.Object, cfg *rest.Config) (*rest.Config, error) { + return cfg, nil + } + config := &rest.Config{} + + ce := &v1alpha1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-extension", + }, + } + objs := []client.Object{ + &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "webserver", + }, + }, + } + + for n := 0; n < b.N; n++ { + mgr, _ := manager.New(config, manager.Options{}) + ctrl, err := controller.New("test-controller", mgr, controller.Options{ + Reconciler: reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) { + return reconcile.Result{}, nil + }), + }) + if err != nil { + b.Logf("creating controller; %v", err) + } + + instance := New(rcm, config, mgr.GetRESTMapper()) + err = instance.Watch(context.Background(), ctrl, ce, objs) + if err != nil { + b.Logf("setup watcher; %v", err) + } + } +} + func TestBuildScheme(t *testing.T) { type validation struct { gvks []schema.GroupVersionKind @@ -205,3 +247,27 @@ func TestBuildScheme(t *testing.T) { }) } } + +func BenchmarkBuildScheme(b *testing.B) { + objects := []client.Object{ + &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "webserver", + }, + }, + } + + gvk := appsv1.SchemeGroupVersion.WithKind("Deployment") + + for n := 0; n < b.N; n++ { + scheme, err := buildScheme(objects) + if err != nil { + b.Logf("failed creating scheme; %v", err) + } + _ = scheme.Recognizes(gvk) + } +} From f8272a0b419e0c0a7f2dc24e9bd856b33670f291 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Wed, 24 Jul 2024 10:19:24 -0500 Subject: [PATCH 02/13] feat: add GH actions for running benchmarks Signed-off-by: Edmund Ochieng --- .github/workflows/benchmark.yaml | 46 ++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/benchmark.yaml diff --git a/.github/workflows/benchmark.yaml b/.github/workflows/benchmark.yaml new file mode 100644 index 000000000..46ac6ff58 --- /dev/null +++ b/.github/workflows/benchmark.yaml @@ -0,0 +1,46 @@ +name: performance + +on: + pull_request: + types: [opened, reopened] + +jobs: + benchmark: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + - name: Install dependencies + run: go get . + + - name: Run golang benchmarks + run: go test -run="^$" -bench=. ./internal/... + + - name: Install benchstat + run: go install golang.org/x/perf/cmd/benchstat@latest + + - name: Set Golang binaries path + run: echo "`go env GOPATH`/bin" >> $GITHUB_PATH + + - name: Download previous benchmarks + id: download + uses: actions/download-artifact@v4 + with: + name: benchmark-results + path: stats/benchmark.txt + continue-on-error: true + + - name: Compare benchmark results + if: steps.download.outcome == 'success' + run: benchstat benchmark.txt stats/benchmark.txt + + - name: Upload artifacts + uses: actions/upload-artifact@v4 + with: + name: benchmark-results + path: benchmark.txt + overwrite: true \ No newline at end of file From 8ae49f5cba8a94e3cf62b026cb7727c214eaeb71 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Thu, 25 Jul 2024 11:52:39 -0500 Subject: [PATCH 03/13] tests: Test the unpack feature Signed-off-by: Edmund Ochieng --- internal/rukpak/source/image_registry_test.go | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 internal/rukpak/source/image_registry_test.go diff --git a/internal/rukpak/source/image_registry_test.go b/internal/rukpak/source/image_registry_test.go new file mode 100644 index 000000000..fa52e8e51 --- /dev/null +++ b/internal/rukpak/source/image_registry_test.go @@ -0,0 +1,40 @@ +package source_test + +import ( + "context" + "testing" + + "github.com/operator-framework/operator-controller/internal/rukpak/source" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func BenchmarkUnpack(b *testing.B) { + b.StopTimer() + cacheDir := b.TempDir() + mgr, _ := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{}) + unpacker, _ := source.NewDefaultUnpacker(mgr, "default", cacheDir) + + logger := zap.New( + zap.UseFlagOptions( + &zap.Options{ + Development: true, + }, + ), + ) + + ctx := log.IntoContext(context.Background(), logger) + + bundleSource := &source.BundleSource{ + Type: source.SourceTypeImage, + Image: &source.ImageSource{ + Ref: "quay.io/eochieng/litmus-edge-operator-bundle@sha256:104b294fa1f4c2e45aa0eac32437a51de32dce0eff923eced44a0dddcb7f363f", + }, + } + b.StartTimer() + + for i := 0; i < b.N; i++ { + _, _ = unpacker.Unpack(ctx, bundleSource) + } +} From 47901c1441739e04f464261ff66ab170efa5d7ea Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Thu, 25 Jul 2024 15:22:09 -0500 Subject: [PATCH 04/13] test: add benchmark for RegistryV1ToHelmChart() function Signed-off-by: Edmund Ochieng --- internal/rukpak/convert/registryv1_test.go | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/internal/rukpak/convert/registryv1_test.go b/internal/rukpak/convert/registryv1_test.go index 33ed9781f..05e922476 100644 --- a/internal/rukpak/convert/registryv1_test.go +++ b/internal/rukpak/convert/registryv1_test.go @@ -1,6 +1,7 @@ package convert import ( + "context" "testing" . "github.com/onsi/ginkgo/v2" @@ -13,9 +14,13 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-controller/internal/rukpak/source" ) func TestRegistryV1Converter(t *testing.T) { @@ -451,3 +456,34 @@ func containsObject(obj unstructured.Unstructured, result []client.Object) clien } return nil } + +func BenchmarkRegistryV1ToHelmChart(b *testing.B) { + b.StopTimer() + cacheDir := b.TempDir() + mgr, _ := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{}) + unpacker, _ := source.NewDefaultUnpacker(mgr, "default", cacheDir) + + logger := zap.New( + zap.UseFlagOptions( + &zap.Options{ + Development: true, + }, + ), + ) + + ctx := log.IntoContext(context.Background(), logger) + + bundleSource := &source.BundleSource{ + Type: source.SourceTypeImage, + Image: &source.ImageSource{ + Ref: "quay.io/eochieng/litmus-edge-operator-bundle@sha256:104b294fa1f4c2e45aa0eac32437a51de32dce0eff923eced44a0dddcb7f363f", + }, + } + + unpacked, _ := unpacker.Unpack(ctx, bundleSource) + b.StartTimer() + + for i := 0; i < b.N; i++ { + _, _ = RegistryV1ToHelmChart(ctx, unpacked.Bundle, "default", []string{corev1.NamespaceAll}) + } +} From 52bea5b817b2c70aa2cb6cecfffead532348a60e Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Thu, 25 Jul 2024 15:27:27 -0500 Subject: [PATCH 05/13] Revert initial benchmark tests This reverts commit f8ae50173de601b8c79c3effd2c5f44a4cb2a06e. --- internal/authentication/tokengetter_test.go | 62 ----------------- .../contentmanager/contentmanager_test.go | 66 ------------------- 2 files changed, 128 deletions(-) diff --git a/internal/authentication/tokengetter_test.go b/internal/authentication/tokengetter_test.go index 1d88c057f..b9553cac3 100644 --- a/internal/authentication/tokengetter_test.go +++ b/internal/authentication/tokengetter_test.go @@ -85,65 +85,3 @@ func TestTokenGetterGet(t *testing.T) { } } } - -func BenchmarkTokenGetterGet(b *testing.B) { - fakeClient := fake.NewSimpleClientset() - fakeClient.PrependReactor("create", "serviceaccounts/token", - func(action ctest.Action) (bool, runtime.Object, error) { - act, ok := action.(ctest.CreateActionImpl) - if !ok { - return false, nil, nil - } - tokenRequest := act.GetObject().(*authenticationv1.TokenRequest) - var err error - if act.Name == "test-service-account-1" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-1", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(DefaultExpirationDuration)), - } - } - if act.Name == "test-service-account-2" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-2", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(1 * time.Second)), - } - } - if act.Name == "test-service-account-3" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-3", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(-10 * time.Second)), - } - } - if act.Name == "test-service-account-4" { - tokenRequest = nil - err = fmt.Errorf("error when fetching token") - } - return true, tokenRequest, err - }, - ) - - tg := NewTokenGetter(fakeClient.CoreV1(), - WithExpirationDuration(DefaultExpirationDuration), - ) - - test := struct { - serviceAccountName string - namespace string - }{ - serviceAccountName: "test-service-account-1", - namespace: "test-namespace-1", - } - - for n := 0; n < b.N; n++ { - token, err := tg.Get(context.Background(), - types.NamespacedName{ - Namespace: test.namespace, - Name: test.serviceAccountName, - }, - ) - if err != nil { - b.Logf("token creation; %v", err) - } - _ = token - } -} diff --git a/internal/contentmanager/contentmanager_test.go b/internal/contentmanager/contentmanager_test.go index e46ebccde..598051b59 100644 --- a/internal/contentmanager/contentmanager_test.go +++ b/internal/contentmanager/contentmanager_test.go @@ -108,48 +108,6 @@ func TestWatch(t *testing.T) { } } -func BenchmarkWatch(b *testing.B) { - rcm := func(_ context.Context, _ client.Object, cfg *rest.Config) (*rest.Config, error) { - return cfg, nil - } - config := &rest.Config{} - - ce := &v1alpha1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-cluster-extension", - }, - } - objs := []client.Object{ - &corev1.Pod{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Pod", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "webserver", - }, - }, - } - - for n := 0; n < b.N; n++ { - mgr, _ := manager.New(config, manager.Options{}) - ctrl, err := controller.New("test-controller", mgr, controller.Options{ - Reconciler: reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) { - return reconcile.Result{}, nil - }), - }) - if err != nil { - b.Logf("creating controller; %v", err) - } - - instance := New(rcm, config, mgr.GetRESTMapper()) - err = instance.Watch(context.Background(), ctrl, ce, objs) - if err != nil { - b.Logf("setup watcher; %v", err) - } - } -} - func TestBuildScheme(t *testing.T) { type validation struct { gvks []schema.GroupVersionKind @@ -247,27 +205,3 @@ func TestBuildScheme(t *testing.T) { }) } } - -func BenchmarkBuildScheme(b *testing.B) { - objects := []client.Object{ - &appsv1.Deployment{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "apps/v1", - Kind: "Deployment", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "webserver", - }, - }, - } - - gvk := appsv1.SchemeGroupVersion.WithKind("Deployment") - - for n := 0; n < b.N; n++ { - scheme, err := buildScheme(objects) - if err != nil { - b.Logf("failed creating scheme; %v", err) - } - _ = scheme.Recognizes(gvk) - } -} From 28609e6919e8a8e65523c095a5ed1e434d674bc4 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Fri, 26 Jul 2024 09:52:29 -0500 Subject: [PATCH 06/13] test: benchmark for the Resolve() function Signed-off-by: Edmund Ochieng --- internal/resolve/catalog_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/resolve/catalog_test.go b/internal/resolve/catalog_test.go index a067bbe79..ead055acf 100644 --- a/internal/resolve/catalog_test.go +++ b/internal/resolve/catalog_test.go @@ -589,3 +589,23 @@ func genPackage(pkg string) *declcfg.DeclarativeConfig { Deprecations: []declcfg.Deprecation{packageDeprecation(pkg)}, } } + +func BenchmarkResolve(b *testing.B) { + defer featuregatetesting.SetFeatureGateDuringTest(b, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, true)() + pkgName := randPkg() + w := staticCatalogWalker{ + "a": func() (*declcfg.DeclarativeConfig, error) { return &declcfg.DeclarativeConfig{}, nil }, + "b": func() (*declcfg.DeclarativeConfig, error) { return &declcfg.DeclarativeConfig{}, nil }, + "c": func() (*declcfg.DeclarativeConfig, error) { return genPackage(pkgName), nil }, + } + r := CatalogResolver{WalkCatalogsFunc: w.WalkCatalogs} + ce := buildFooClusterExtension(pkgName, "", "", ocv1alpha1.UpgradeConstraintPolicyEnforce) + installedBundle := &ocv1alpha1.BundleMetadata{ + Name: bundleName(pkgName, "1.0.0"), + Version: "1.0.0", + } + + for i := 0; i < b.N; i++ { + _, _, _, _ = r.Resolve(context.Background(), ce, installedBundle) + } +} From 32d43daf9e2144796dcbd36878e09f777f897af6 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Fri, 26 Jul 2024 14:41:56 -0500 Subject: [PATCH 07/13] test: benchmark FilesystemCache() function Signed-off-by: Edmund Ochieng --- internal/catalogmetadata/cache/cache_test.go | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/catalogmetadata/cache/cache_test.go b/internal/catalogmetadata/cache/cache_test.go index 05ea28ec5..a640138f9 100644 --- a/internal/catalogmetadata/cache/cache_test.go +++ b/internal/catalogmetadata/cache/cache_test.go @@ -342,3 +342,36 @@ func equalFilesystems(expected, actual fs.FS) error { } return errors.Join(cmpErrs...) } + +func BenchmarkFilesystemCache(b *testing.B) { + b.StopTimer() + catalog := &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-catalog", + }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Type: catalogd.SourceTypeImage, + Image: &catalogd.ResolvedImageSource{ + ResolvedRef: "fake/catalog@sha256:fakesha", + }, + }, + }, + } + + cacheDir := b.TempDir() + + tripper := &mockTripper{} + tripper.content = make(fstest.MapFS) + maps.Copy(tripper.content, defaultFS) + httpClient := http.DefaultClient + httpClient.Transport = tripper + b.StartTimer() + + for i := 0; i < b.N; i++ { + c := cache.NewFilesystemCache(cacheDir, func() (*http.Client, error) { + return httpClient, nil + }) + _, _ = c.FetchCatalogContents(context.Background(), catalog) + } +} From 73ee1255a8724ace7b1f3c9b664121ddcc08921c Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Wed, 31 Jul 2024 13:35:29 -0500 Subject: [PATCH 08/13] ci: parse the results of benchstats Parse the results of the GH actions and return output that has 20% decrease in performance. Signed-off-by: Edmund Ochieng --- .github/workflows/benchmark.yaml | 60 +++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/.github/workflows/benchmark.yaml b/.github/workflows/benchmark.yaml index 46ac6ff58..82523ca61 100644 --- a/.github/workflows/benchmark.yaml +++ b/.github/workflows/benchmark.yaml @@ -17,30 +17,66 @@ jobs: - name: Install dependencies run: go get . - - name: Run golang benchmarks - run: go test -run="^$" -bench=. ./internal/... + - name: Set Golang binaries path + run: echo "`go env GOPATH`/bin" >> $GITHUB_PATH - name: Install benchstat run: go install golang.org/x/perf/cmd/benchstat@latest - - name: Set Golang binaries path - run: echo "`go env GOPATH`/bin" >> $GITHUB_PATH + - name: Run golang benchmarks + run: go test -run="^$" -bench=. -count=10 ./internal/... > benchmark.txt - - name: Download previous benchmarks + - name: Download baseline benchmark id: download uses: actions/download-artifact@v4 with: - name: benchmark-results + name: baseline-benchmark path: stats/benchmark.txt continue-on-error: true + - name: Use current benchmark as reference + if: steps.download.outcome != 'success' + run: | + mkdir stats + cp benchmark.txt stats/benchmark.txt + - name: Compare benchmark results - if: steps.download.outcome == 'success' - run: benchstat benchmark.txt stats/benchmark.txt + run: | + benchstat -table col -row .name -format=csv stats/benchmark.txt benchmark.txt | grep '^[A-Z]' > stats.csv - - name: Upload artifacts + - name: Upload benchstat results uses: actions/upload-artifact@v4 with: - name: benchmark-results - path: benchmark.txt - overwrite: true \ No newline at end of file + name: benchstat-results + path: stats.csv + overwrite: true + + parse-results: + runs-on: ubuntu-latest + needs: [benchmark] + steps: + - uses: actions/setup-python@v5 + with: + python-version: '3:12' + + - name: Download benchstats result + uses: actions/download-artifact@v4 + with: + name: benchstat-results + path: stats.csv + + - name: Install dependencies + run: | + pip install --upgrade pip + pip install pandas + + - name: Evaluate the results + uses: jannekem/run-python-script-action@v1 + with: + script: | + import pandas as pd + + df = pd.read_csv('stats.csv', names=['name','reference','secs/op','benchmark','_secs/op','change','P']) + df.change = df.change.replace("~", "0") + df.change = df.change.str.rstrip("%") + print(df.loc[df.change > 20, ['name','change','P']]) From 364ba3e48f9b0ccade2b17e30493b95f27d44d3a Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Wed, 31 Jul 2024 13:37:26 -0500 Subject: [PATCH 09/13] set the logger in controller runtime Signed-off-by: Edmund Ochieng --- internal/rukpak/convert/registryv1_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/rukpak/convert/registryv1_test.go b/internal/rukpak/convert/registryv1_test.go index 05e922476..2cee1eff7 100644 --- a/internal/rukpak/convert/registryv1_test.go +++ b/internal/rukpak/convert/registryv1_test.go @@ -459,6 +459,7 @@ func containsObject(obj unstructured.Unstructured, result []client.Object) clien func BenchmarkRegistryV1ToHelmChart(b *testing.B) { b.StopTimer() + cacheDir := b.TempDir() mgr, _ := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{}) unpacker, _ := source.NewDefaultUnpacker(mgr, "default", cacheDir) @@ -471,6 +472,7 @@ func BenchmarkRegistryV1ToHelmChart(b *testing.B) { ), ) + log.SetLogger(logger) ctx := log.IntoContext(context.Background(), logger) bundleSource := &source.BundleSource{ From fe5b5ede34f69fa39a9e0f55130a485c37730c5d Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Wed, 31 Jul 2024 13:38:42 -0500 Subject: [PATCH 10/13] test: Suppress the logs when Unpack function Suppress logs from Unpack function which is causing errors to be written to the benchmark results. Signed-off-by: Edmund Ochieng --- internal/rukpak/source/image_registry.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/rukpak/source/image_registry.go b/internal/rukpak/source/image_registry.go index a6d6640d4..70ff88dda 100644 --- a/internal/rukpak/source/image_registry.go +++ b/internal/rukpak/source/image_registry.go @@ -84,6 +84,7 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Resu } authChain, err := k8schain.NewInCluster(ctx, chainOpts) if err != nil { + l.Error(err, "we encountered an issue getting auth keychain") return nil, fmt.Errorf("error getting auth keychain: %w", err) } @@ -114,7 +115,6 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Resu hexVal := strings.TrimPrefix(digest.DigestStr(), "sha256:") unpackPath := filepath.Join(i.BaseCachePath, bundle.Name, hexVal) if stat, err := os.Stat(unpackPath); err == nil && stat.IsDir() { - l.V(1).Info("found image in filesystem cache", "digest", hexVal) return unpackedResult(os.DirFS(unpackPath), bundle, digest.String()), nil } } @@ -122,9 +122,9 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Resu // always fetch the hash imgDesc, err := remote.Head(imgRef, remoteOpts...) if err != nil { + l.Error(err, "failed fetching image descriptor") return nil, fmt.Errorf("error fetching image descriptor: %w", err) } - l.V(1).Info("resolved image descriptor", "digest", imgDesc.Digest.String()) unpackPath := filepath.Join(i.BaseCachePath, bundle.Name, imgDesc.Digest.Hex) if _, err = os.Stat(unpackPath); errors.Is(err, os.ErrNotExist) { //nolint: nestif From 6fa0a59945ff749ca300297fd4a03f0c6473b903 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Wed, 31 Jul 2024 15:01:11 -0500 Subject: [PATCH 11/13] fix yaml lint error --- .github/workflows/benchmark.yaml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/benchmark.yaml b/.github/workflows/benchmark.yaml index 82523ca61..a06ff49aa 100644 --- a/.github/workflows/benchmark.yaml +++ b/.github/workflows/benchmark.yaml @@ -59,11 +59,11 @@ jobs: with: python-version: '3:12' - - name: Download benchstats result - uses: actions/download-artifact@v4 - with: - name: benchstat-results - path: stats.csv + - name: Download benchstats result + uses: actions/download-artifact@v4 + with: + name: benchstat-results + path: stats.csv - name: Install dependencies run: | @@ -76,7 +76,8 @@ jobs: script: | import pandas as pd - df = pd.read_csv('stats.csv', names=['name','reference','secs/op','benchmark','_secs/op','change','P']) + df = pd.read_csv('stats.csv', + names=['name','reference','secs/op','benchmark','_secs/op','change','P']) df.change = df.change.replace("~", "0") df.change = df.change.str.rstrip("%") print(df.loc[df.change > 20, ['name','change','P']]) From a13ba5ce89e2150d1112e44cc28e37583c8ec142 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Wed, 31 Jul 2024 15:42:07 -0500 Subject: [PATCH 12/13] fix gci lint errors Signed-off-by: Edmund Ochieng --- .github/workflows/benchmark.yaml | 86 ++++++++++--------- internal/rukpak/convert/registryv1_test.go | 1 + internal/rukpak/source/image_registry_test.go | 3 +- 3 files changed, 47 insertions(+), 43 deletions(-) diff --git a/.github/workflows/benchmark.yaml b/.github/workflows/benchmark.yaml index a06ff49aa..ea345944f 100644 --- a/.github/workflows/benchmark.yaml +++ b/.github/workflows/benchmark.yaml @@ -1,3 +1,4 @@ +--- name: performance on: @@ -8,48 +9,49 @@ jobs: benchmark: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - - uses: actions/setup-go@v5 - with: - go-version-file: go.mod - - - name: Install dependencies - run: go get . - - - name: Set Golang binaries path - run: echo "`go env GOPATH`/bin" >> $GITHUB_PATH - - - name: Install benchstat - run: go install golang.org/x/perf/cmd/benchstat@latest - - - name: Run golang benchmarks - run: go test -run="^$" -bench=. -count=10 ./internal/... > benchmark.txt - - - name: Download baseline benchmark - id: download - uses: actions/download-artifact@v4 - with: - name: baseline-benchmark - path: stats/benchmark.txt - continue-on-error: true - - - name: Use current benchmark as reference - if: steps.download.outcome != 'success' - run: | - mkdir stats - cp benchmark.txt stats/benchmark.txt - - - name: Compare benchmark results - run: | - benchstat -table col -row .name -format=csv stats/benchmark.txt benchmark.txt | grep '^[A-Z]' > stats.csv - - - name: Upload benchstat results - uses: actions/upload-artifact@v4 - with: - name: benchstat-results - path: stats.csv - overwrite: true + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + - name: Install dependencies + run: go get . + + - name: Set Golang binaries path + run: echo "`go env GOPATH`/bin" >> $GITHUB_PATH + + - name: Install benchstat + run: go install golang.org/x/perf/cmd/benchstat@latest + + - name: Run golang benchmarks + run: go test -run="^$" -bench=. -count=10 ./internal/... > benchmark.txt + + - name: Download baseline benchmark + id: download + uses: actions/download-artifact@v4 + with: + name: baseline-benchmark + path: stats/benchmark.txt + continue-on-error: true + + - name: Use current benchmark as reference + if: steps.download.outcome != 'success' + run: | + mkdir stats + cp benchmark.txt stats/benchmark.txt + + - name: Compare benchmark results + run: | + benchstat -table col -row .name -format=csv stats/benchmark.txt \ + benchmark.txt | grep '^[A-Z]' > stats.csv + + - name: Upload benchstat results + uses: actions/upload-artifact@v4 + with: + name: benchstat-results + path: stats.csv + overwrite: true parse-results: runs-on: ubuntu-latest diff --git a/internal/rukpak/convert/registryv1_test.go b/internal/rukpak/convert/registryv1_test.go index 2cee1eff7..9592ffb67 100644 --- a/internal/rukpak/convert/registryv1_test.go +++ b/internal/rukpak/convert/registryv1_test.go @@ -20,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-controller/internal/rukpak/source" ) diff --git a/internal/rukpak/source/image_registry_test.go b/internal/rukpak/source/image_registry_test.go index fa52e8e51..3b32ad3e0 100644 --- a/internal/rukpak/source/image_registry_test.go +++ b/internal/rukpak/source/image_registry_test.go @@ -4,10 +4,11 @@ import ( "context" "testing" - "github.com/operator-framework/operator-controller/internal/rukpak/source" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + + "github.com/operator-framework/operator-controller/internal/rukpak/source" ) func BenchmarkUnpack(b *testing.B) { From 1075eb334f8efe39ccd0acd826a06d534e4b7f36 Mon Sep 17 00:00:00 2001 From: Edmund Ochieng Date: Thu, 8 Aug 2024 15:49:18 -0500 Subject: [PATCH 13/13] evaluate the benchstat results for changes Signed-off-by: Edmund Ochieng --- .github/workflows/benchmark.yaml | 67 ++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/.github/workflows/benchmark.yaml b/.github/workflows/benchmark.yaml index ea345944f..5d7922f3a 100644 --- a/.github/workflows/benchmark.yaml +++ b/.github/workflows/benchmark.yaml @@ -16,14 +16,11 @@ jobs: go-version-file: go.mod - name: Install dependencies - run: go get . + run: go mod download - name: Set Golang binaries path run: echo "`go env GOPATH`/bin" >> $GITHUB_PATH - - name: Install benchstat - run: go install golang.org/x/perf/cmd/benchstat@latest - - name: Run golang benchmarks run: go test -run="^$" -bench=. -count=10 ./internal/... > benchmark.txt @@ -41,6 +38,9 @@ jobs: mkdir stats cp benchmark.txt stats/benchmark.txt + - name: Install benchstat + run: go install golang.org/x/perf/cmd/benchstat@latest + - name: Compare benchmark results run: | benchstat -table col -row .name -format=csv stats/benchmark.txt \ @@ -59,7 +59,7 @@ jobs: steps: - uses: actions/setup-python@v5 with: - python-version: '3:12' + python-version: '3.12' - name: Download benchstats result uses: actions/download-artifact@v4 @@ -70,16 +70,49 @@ jobs: - name: Install dependencies run: | pip install --upgrade pip - pip install pandas + pip install pandas numpy tabulate - - name: Evaluate the results - uses: jannekem/run-python-script-action@v1 - with: - script: | - import pandas as pd - - df = pd.read_csv('stats.csv', - names=['name','reference','secs/op','benchmark','_secs/op','change','P']) - df.change = df.change.replace("~", "0") - df.change = df.change.str.rstrip("%") - print(df.loc[df.change > 20, ['name','change','P']]) + - name: Review the results + id: results + run: | + import pandas as pd + import os + + # Threshold for increase in compute resource utilization + threshold: int = 10 + df = pd.read_csv('stats.csv', + names=['name','reference','secs/op','benchmark','_secs/op','vs_base','P']) + + # Render the benchstat output to markdown + out = df.to_markdown() + + s1 = df.vs_base.str.replace('~','0') + s1 = pd.to_numeric(s1.str.rstrip('%')) + breach = "true" if len(s1[s1 > threshold]) > 0 else "false" + + gh_output = os.environ['GITHUB_OUTPUT'] + with open(gh_output, 'a') as f: + # vs_base captures change in compute resource utilization + print(f'breach={breach}', file=f) + # capture benchstat results in markdown + print(f'report={out}', file=f) + shell: python + + - name: Post results + uses: actions/github-script@v7 + script: | + github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: ${{ steps.results.output.report }} + }) + + - name: Parse 'vs base' results + run: | + if [ "${{ steps.results.output.breach }}" == "true" ] + then + exit 1 + else + exit 0 + shell: bash