Skip to content

🐛 improve poor performance of helm chart conversion #1050

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

Merged
merged 1 commit into from
Jul 15, 2024
Merged

🐛 improve poor performance of helm chart conversion #1050

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
32 changes: 7 additions & 25 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"flag"
"fmt"
"net/url"
"os"
"path/filepath"

Expand All @@ -47,11 +46,8 @@ import (
"github.com/operator-framework/operator-controller/internal/controllers"
"github.com/operator-framework/operator-controller/internal/httputil"
"github.com/operator-framework/operator-controller/internal/labels"
registryv1handler "github.com/operator-framework/operator-controller/internal/rukpak/handler"
crdupgradesafety "github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
"github.com/operator-framework/operator-controller/internal/rukpak/provisioner/registry"
"github.com/operator-framework/operator-controller/internal/rukpak/source"
"github.com/operator-framework/operator-controller/internal/rukpak/storage"
"github.com/operator-framework/operator-controller/internal/version"
"github.com/operator-framework/operator-controller/pkg/features"
"github.com/operator-framework/operator-controller/pkg/scheme"
Expand Down Expand Up @@ -95,6 +91,7 @@ func main() {
flag.StringVar(&systemNamespace, "system-namespace", "", "Configures the namespace that gets used to deploy system resources.")
opts := zap.Options{
Development: true,
TimeEncoder: zapcore.RFC3339NanoTimeEncoder,
}
opts.BindFlags(flag.CommandLine)

Expand Down Expand Up @@ -161,7 +158,12 @@ func main() {
}

cl := mgr.GetClient()
catalogClient := catalogclient.New(cl, cache.NewFilesystemCache(cachePath, httpClient))
catalogsCachePath := filepath.Join(cachePath, "catalogs")
if err := os.MkdirAll(catalogsCachePath, 0700); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing that the previous mode (for bundles) was 0755, does this make a difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't. We don't expect any other users or groups to read these files.

setupLog.Error(err, "unable to create catalogs cache directory")
os.Exit(1)
}
catalogClient := catalogclient.New(cl, cache.NewFilesystemCache(catalogsCachePath, httpClient))

installNamespaceMapper := helmclient.ObjectToStringMapper(func(obj client.Object) (string, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
Expand Down Expand Up @@ -193,7 +195,6 @@ func main() {

domain := ocv1alpha1.GroupVersion.Group
cleanupUnpackCacheKey := fmt.Sprintf("%s/cleanup-unpack-cache", domain)
deleteCachedBundleKey := fmt.Sprintf("%s/delete-cached-bundle", domain)
if err := clusterExtensionFinalizers.Register(cleanupUnpackCacheKey, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
return crfinalizer.Result{}, os.RemoveAll(filepath.Join(unpacker.BaseCachePath, ext.GetName()))
Expand All @@ -202,23 +203,6 @@ func main() {
os.Exit(1)
}

localStorageRoot := filepath.Join(cachePath, "bundles")
if err := os.MkdirAll(localStorageRoot, 0755); err != nil {
setupLog.Error(err, "unable to create local storage root directory", "root", localStorageRoot)
os.Exit(1)
}
localStorage := &storage.LocalDirectory{
RootDirectory: localStorageRoot,
URL: url.URL{},
}
if err := clusterExtensionFinalizers.Register(deleteCachedBundleKey, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
return crfinalizer.Result{}, localStorage.Delete(ctx, ext.GetName())
})); err != nil {
setupLog.Error(err, "unable to register finalizer", "finalizerKey", deleteCachedBundleKey)
os.Exit(1)
}

aeClient, err := apiextensionsv1client.NewForConfig(mgr.GetConfig())
if err != nil {
setupLog.Error(err, "unable to create apiextensions client")
Expand All @@ -234,9 +218,7 @@ func main() {
BundleProvider: catalogClient,
ActionClientGetter: acg,
Unpacker: unpacker,
Storage: localStorage,
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
Handler: registryv1handler.HandlerFunc(registry.HandleBundleDeployment),
Finalizers: clusterExtensionFinalizers,
CaCertDir: caCertDir,
Preflights: preflights,
Expand Down
1 change: 0 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ require (
github.com/google/go-containerregistry v0.20.0
github.com/google/go-containerregistry/pkg/authn/k8schain v0.0.0-20240505154900-ff385a972813
github.com/google/go-containerregistry/pkg/authn/kubernetes v0.0.0-20240505154900-ff385a972813
github.com/nlepage/go-tarfs v1.2.1
github.com/onsi/ginkgo/v2 v2.19.0
github.com/onsi/gomega v1.33.1
github.com/operator-framework/api v0.26.0
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,8 +560,6 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8m
github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=
github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f h1:y5//uYreIhSUg3J1GEMiLbxo1LJaP8RfCpH6pymGZus=
github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw=
github.com/nlepage/go-tarfs v1.2.1 h1:o37+JPA+ajllGKSPfy5+YpsNHDjZnAoyfvf5GsUa+Ks=
github.com/nlepage/go-tarfs v1.2.1/go.mod h1:rno18mpMy9aEH1IiJVftFsqPyIpwqSUiAOpJYjlV2NA=
github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A=
github.com/nxadm/tail v1.4.8 h1:nPr65rt6Y5JFSKQO7qToXr7pePgD6Gwiw05lkbyAQTE=
github.com/nxadm/tail v1.4.8/go.mod h1:+ncqLTQzXmGhMZNUePPaPqPvBxHAIsmXswZKocGu+AU=
Expand Down
35 changes: 18 additions & 17 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"helm.sh/helm/v3/pkg/postrender"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -71,10 +72,9 @@ import (
"github.com/operator-framework/operator-controller/internal/httputil"
"github.com/operator-framework/operator-controller/internal/labels"
"github.com/operator-framework/operator-controller/internal/rukpak/bundledeployment"
registryv1handler "github.com/operator-framework/operator-controller/internal/rukpak/handler"
"github.com/operator-framework/operator-controller/internal/rukpak/convert"
crdupgradesafety "github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
rukpaksource "github.com/operator-framework/operator-controller/internal/rukpak/source"
"github.com/operator-framework/operator-controller/internal/rukpak/storage"
"github.com/operator-framework/operator-controller/internal/rukpak/util"
)

Expand All @@ -88,8 +88,6 @@ type ClusterExtensionReconciler struct {
BundleProvider BundleProvider
Unpacker rukpaksource.Unpacker
ActionClientGetter helmclient.ActionClientGetter
Storage storage.Storage
Handler registryv1handler.Handler
dynamicWatchMutex sync.RWMutex
dynamicWatchGVKs sets.Set[schema.GroupVersionKind]
controller crcontroller.Controller
Expand Down Expand Up @@ -132,6 +130,8 @@ type Preflight interface {
// This has been taken from rukpak, and an issue was created before to discuss it: https://github.com/operator-framework/rukpak/issues/800.
func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
l := log.FromContext(ctx).WithName("operator-controller")
ctx = log.IntoContext(ctx, l)

l.V(1).Info("reconcile starting")
defer l.V(1).Info("reconcile ending")

Expand Down Expand Up @@ -212,6 +212,9 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool {
*/
//nolint:unparam
func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (ctrl.Result, error) {
l := log.FromContext(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to put the name of the ClusterExtension here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly, also attach the V(1) to this. e.g.

Suggested change
l := log.FromContext(ctx)
l := log.FromContext(ctx).V(1).AddValues("ClusterExtension", ext.GetName())

Copy link
Member Author

Choose a reason for hiding this comment

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

The name is already part of the context provided in the context that arrives from the caller of Reconcile.

I thought about using V(1), but that would mean that no logging (with that logger at least) could ever happen at the INFO level.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to suggest that I could do something like this:

l := log.FromContext(ctx)
debugLog := l.V(1)

But that would be misleading because levels build (i.e. .V(1).V(1) is equivalent to .V(2)), so if the caller set a .V(1) logger in the context, the this would suddenly not be a debug logger.


l.V(1).Info("handling finalizers")
finalizeResult, err := r.Finalizers.Finalize(ctx, ext)
if err != nil {
// TODO: For now, this error handling follows the pattern of other error handling.
Expand All @@ -232,6 +235,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
}

// run resolution
l.V(1).Info("resolving bundle")
bundle, err := r.resolve(ctx, *ext)
if err != nil {
// Note: We don't distinguish between resolution-specific errors and generic errors
Expand All @@ -242,6 +246,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
return ctrl.Result{}, err
}

l.V(1).Info("validating bundle")
if err := r.validateBundle(bundle); err != nil {
ext.Status.ResolvedBundle = nil
ext.Status.InstalledBundle = nil
Expand Down Expand Up @@ -269,6 +274,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
// Note: The BundleDeployment here is not a k8s API, its a simple Go struct which
// necessary embedded values.
bd := r.generateBundleDeploymentForUnpack(ctx, bundle.Image, ext)
l.V(1).Info("unpacking resolved bundle")
unpackResult, err := r.Unpacker.Unpack(ctx, bd)
if err != nil {
setStatusUnpackFailed(ext, err.Error())
Expand All @@ -282,31 +288,22 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp

return ctrl.Result{}, nil
case rukpaksource.StateUnpacked:
// TODO: Add finalizer to clean the stored bundles, after https://github.com/operator-framework/rukpak/pull/897
// merges.
if err := r.Storage.Store(ctx, ext.GetName(), unpackResult.Bundle); err != nil {
setStatusUnpackFailed(ext, err.Error())
return ctrl.Result{}, err
}
setStatusUnpacked(ext, fmt.Sprintf("unpack successful: %v", unpackResult.Message))
default:
setStatusUnpackFailed(ext, "unexpected unpack status")
// We previously exit with a failed status if error is not nil.
return ctrl.Result{}, fmt.Errorf("unexpected unpack status: %v", unpackResult.Message)
}

bundleFS, err := r.Storage.Load(ctx, ext.GetName())
if err != nil {
setInstalledStatusConditionFailed(ext, err.Error())
return ctrl.Result{}, err
}

chrt, values, err := r.Handler.Handle(ctx, bundleFS, bd)
l.V(1).Info("converting bundle to helm chart")
chrt, err := convert.RegistryV1ToHelmChart(ctx, unpackResult.Bundle, ext.Spec.InstallNamespace, []string{corev1.NamespaceAll})
if err != nil {
setInstalledStatusConditionFailed(ext, err.Error())
return ctrl.Result{}, err
}
values := chartutil.Values{}

l.V(1).Info("getting helm client")
ac, err := r.ActionClientGetter.ActionClientFor(ctx, ext)
if err != nil {
ext.Status.InstalledBundle = nil
Expand All @@ -324,12 +321,14 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
},
}

l.V(1).Info("getting current state of helm release")
rel, desiredRel, state, err := r.getReleaseState(ac, ext, chrt, values, post)
if err != nil {
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonErrorGettingReleaseState, err))
return ctrl.Result{}, err
}

l.V(1).Info("running preflight checks")
for _, preflight := range r.Preflights {
if ext.Spec.Preflight != nil && ext.Spec.Preflight.CRDUpgradeSafety != nil {
if _, ok := preflight.(*crdupgradesafety.Preflight); ok && ext.Spec.Preflight.CRDUpgradeSafety.Disabled {
Expand All @@ -354,6 +353,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
}
}

l.V(1).Info("reconciling helm release changes")
switch state {
case stateNeedsInstall:
rel, err = ac.Install(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, func(install *action.Install) error {
Expand Down Expand Up @@ -384,6 +384,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
return ctrl.Result{}, fmt.Errorf("unexpected release state %q", state)
}

l.V(1).Info("configuring watches for release objects")
relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name))
if err != nil {
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err))
Expand Down
39 changes: 0 additions & 39 deletions internal/controllers/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ package controllers_test

import (
"context"
"io/fs"
"log"
"net/http"
"os"
"path/filepath"
"testing"
Expand All @@ -40,7 +38,6 @@ import (
"github.com/operator-framework/operator-controller/internal/controllers"
bd "github.com/operator-framework/operator-controller/internal/rukpak/bundledeployment"
"github.com/operator-framework/operator-controller/internal/rukpak/source"
"github.com/operator-framework/operator-controller/internal/rukpak/storage"
"github.com/operator-framework/operator-controller/internal/testutil"
"github.com/operator-framework/operator-controller/pkg/scheme"
)
Expand All @@ -61,39 +58,6 @@ func (m *MockUnpacker) Cleanup(ctx context.Context, bundle *bd.BundleDeployment)
panic("implement me")
}

// MockStorage is a mock of Storage interface
type MockStorage struct {
mock.Mock
}

func (m *MockStorage) Load(ctx context.Context, owner string) (fs.FS, error) {
args := m.Called(ctx, owner)
if args.Get(0) == nil {
return nil, args.Error(1)
}
return args.Get(0).(fs.FS), args.Error(1)
}

func (m *MockStorage) Delete(ctx context.Context, owner string) error {
//TODO implement me
panic("implement me")
}

func (m *MockStorage) ServeHTTP(writer http.ResponseWriter, request *http.Request) {
//TODO implement me
panic("implement me")
}

func (m *MockStorage) URLFor(ctx context.Context, owner string) (string, error) {
//TODO implement me
panic("implement me")
}

func (m *MockStorage) Store(ctx context.Context, owner string, bundle fs.FS) error {
args := m.Called(ctx, owner, bundle)
return args.Error(0)
}

func newClient(t *testing.T) client.Client {
cl, err := client.New(config, client.Options{Scheme: scheme.Scheme})
require.NoError(t, err)
Expand Down Expand Up @@ -125,7 +89,6 @@ func newClientAndReconciler(t *testing.T, bundle *ocv1alpha1.BundleMetadata) (cl
BundleProvider: &fakeCatalogClient,
ActionClientGetter: helmClientGetter,
Unpacker: unpacker,
Storage: store,
InstalledBundleGetter: mockInstalledBundleGetter,
Finalizers: crfinalizer.NewFinalizers(),
}
Expand All @@ -136,7 +99,6 @@ var (
config *rest.Config
helmClientGetter helmclient.ActionClientGetter
unpacker source.Unpacker // Interface, will be initialized as a mock in TestMain
store storage.Storage
)

func TestMain(m *testing.M) {
Expand All @@ -160,7 +122,6 @@ func TestMain(m *testing.M) {
utilruntime.Must(err)

unpacker = new(MockUnpacker)
store = new(MockStorage)

code := m.Run()
utilruntime.Must(testEnv.Stop())
Expand Down
Loading