Performance improvement for odo catalog list components when using de…

…vfiles (#3112)

* feat: parallel fetching of component types to improve performance

Fix #3105 as we now also fetch devfiles for completion, which was
previously prohibitively slow.

* fix: add missing mutex, rename existing one for clarity

* refactor: simplify getDevfileWith by computing link within function

* docs: add documentation, update existing one

* refactor: rename wg to devfileWG for clarity

* fix: improper test setup

* fix: only filter components if there's no error

* fix: re-implement using channels, handle error and timeout better

* fix: no need to pass error

* fix: populate base in GetDevfileIndex, use registry URL, hiding path

* fix: properly retrieve all devfiles

Performance is not that great so investigating better options.

* fix: use new ConcurrentTasks support, allowing proper error reporting

* fix: adapt to dynamic registries

* feat: simplify ConcurrentTask by hiding WaitGroup

It was too easy to forget calling Done in ToRun leading to deadlocks.

* fix: broken tests

* fix: wording

[skip ci]

* fix: remove unused function, added doc

[skip ci]

* fix: remove unused ErrorWrapper

[skip ci]

* fix: rename package import

[skip ci]

* fix: test getDevfileIndexEntries directly, remove now unused function

pkg/catalog/catalog.go

@@ -3,13 +3,14 @@ package catalog
 import (
 	"encoding/json"
 	"fmt"
+	"github.com/openshift/odo/pkg/preference"
 	"sort"
 	"strings"
+	"sync"
 
 	imagev1 "github.com/openshift/api/image/v1"
 	"github.com/openshift/odo/pkg/log"
 	"github.com/openshift/odo/pkg/occlient"
-	"github.com/openshift/odo/pkg/preference"
 	"github.com/openshift/odo/pkg/util"
 	"github.com/pkg/errors"
 	"gopkg.in/yaml.v2"
@@ -29,23 +30,30 @@ var DevfileRegistries = []string{
 
 // GetDevfileRegistries gets devfile registries from preference file,
 // if registry name is specified return the specific registry, otherwise return all registries
-func GetDevfileRegistries(registryName string) (map[string]string, error) {
-	devfileRegistries := make(map[string]string)
+func GetDevfileRegistries(registryName string) (map[string]Registry, error) {
+	devfileRegistries := make(map[string]Registry)
 
 	cfg, err := preference.New()
 	if err != nil {
 		return nil, err
 	}
 
+	hasName := len(registryName) != 0
 	if cfg.OdoSettings.RegistryList != nil {
 		for _, registry := range *cfg.OdoSettings.RegistryList {
-			if len(registryName) != 0 {
+			if hasName {
 				if registryName == registry.Name {
-					devfileRegistries[registry.Name] = registry.URL
+					devfileRegistries[registry.Name] = Registry{
+						Name: registry.Name,
+						URL:  registry.URL,
+					}
 					return devfileRegistries, nil
 				}
 			} else {
-				devfileRegistries[registry.Name] = registry.URL
+				devfileRegistries[registry.Name] = Registry{
+					Name: registry.Name,
+					URL:  registry.URL,
+				}
 			}
 		}
 	} else {
@@ -55,18 +63,24 @@ func GetDevfileRegistries(registryName string) (map[string]string, error) {
 	return devfileRegistries, nil
 }
 
-// GetDevfileIndex loads the devfile registry index.json
-func GetDevfileIndex(devfileIndexLink string) ([]DevfileIndexEntry, error) {
-	var devfileIndex []DevfileIndexEntry
+const indexPath = "/devfiles/index.json"
 
-	jsonBytes, err := util.HTTPGetRequest(devfileIndexLink)
+// getDevfileIndexEntries retrieves the devfile entries associated with the specified registry
+func getDevfileIndexEntries(registry Registry) ([]DevfileIndexEntry, error) {
+	var devfileIndex []DevfileIndexEntry
+	indexLink := registry.URL + indexPath
+	jsonBytes, err := util.HTTPGetRequest(indexLink)
 	if err != nil {
-		return nil, errors.Wrapf(err, "Unable to download the devfile index.json from %s", devfileIndexLink)
+		return nil, errors.Wrapf(err, "Unable to download the devfile index.json from %s", indexLink)
 	}
 
 	err = json.Unmarshal(jsonBytes, &devfileIndex)
 	if err != nil {
-		return nil, errors.Wrapf(err, "Unable to unmarshal the devfile index.json from %s", devfileIndexLink)
+		return nil, errors.Wrapf(err, "Unable to unmarshal the devfile index.json from %s", indexLink)
+	}
+
+	for i := range devfileIndex {
+		devfileIndex[i].Registry = registry
 	}
 
 	return devfileIndex, nil
@@ -138,63 +152,83 @@ func IsDevfileComponentSupported(devfile Devfile) bool {
 
 // ListDevfileComponents lists all the available devfile components
 func ListDevfileComponents(registryName string) (DevfileComponentTypeList, error) {
-	var catalogDevfileList DevfileComponentTypeList
+	catalogDevfileList := &DevfileComponentTypeList{}
 	var err error
 
+	// TODO: consider caching registry information for better performance since it should be fairly stable over time
 	// Get devfile registries
 	catalogDevfileList.DevfileRegistries, err = GetDevfileRegistries(registryName)
 	if err != nil {
-		return catalogDevfileList, err
+		return *catalogDevfileList, err
 	}
 	if catalogDevfileList.DevfileRegistries == nil {
-		return catalogDevfileList, nil
+		return *catalogDevfileList, nil
 	}
 
-	for registryName, registryURL := range catalogDevfileList.DevfileRegistries {
+	// first retrieve the indices for each registry, concurrently
+	registryIndices := make([]DevfileIndexEntry, 0, 20)
+	devfileIndicesMutex := &sync.Mutex{}
+	retrieveRegistryIndices := util.NewConcurrentTasks(len(catalogDevfileList.DevfileRegistries))
+	for _, reg := range catalogDevfileList.DevfileRegistries {
 		// Load the devfile registry index.json
-		devfileIndexLink := registryURL + "/devfiles/index.json"
-		devfileIndex, err := GetDevfileIndex(devfileIndexLink)
-		if err != nil {
-			log.Warningf("Registry %s is not set up properly with error: %v", registryName, err)
-			break
-		}
+		registry := reg // needed to prevent the lambda from capturing the value
+		retrieveRegistryIndices.Add(util.ConcurrentTask{ToRun: func(errChannel chan error) {
+			indexEntries, e := getDevfileIndexEntries(registry)
+			if e != nil {
+				log.Warningf("Registry %s is not set up properly with error: %v", registryName, err)
+				errChannel <- e
+				return
+			}
 
-		// 1. Load devfiles that indexed in devfile registry index.json
-		// 2. Populate devfile components with devfile data
-		// 3. Form devfile component list
-		for _, devfileIndexEntry := range devfileIndex {
-			devfileIndexEntryLink := devfileIndexEntry.Links.Link
-
-			// Load the devfile
-			devfileLink := registryURL + devfileIndexEntryLink
-			// TODO: We send http get resquest in this function multiple times
-			// since devfile registry uses different links to host different devfiles,
-			// this can reduce the performance especially when we load devfiles from
-			// big registry. We may need to rethink and optimize this in the future
-			devfile, err := GetDevfile(devfileLink)
+			devfileIndicesMutex.Lock()
+			registryIndices = append(registryIndices, indexEntries...)
+			devfileIndicesMutex.Unlock()
+		}})
+	}
+	if err := retrieveRegistryIndices.Run(); err != nil {
+		return *catalogDevfileList, err
+	}
+
+	// 1. Load each devfile concurrently from the previously retrieved devfile index entries
+	// 2. Populate devfile components with devfile data
+	// 3. Add devfile component types to the catalog devfile list
+	retrieveDevfiles := util.NewConcurrentTasks(len(registryIndices))
+	devfileMutex := &sync.Mutex{}
+	for _, index := range registryIndices {
+		// Load the devfile
+		devfileIndex := index // needed to prevent the lambda from capturing the value
+		link := devfileIndex.Registry.URL + devfileIndex.Links.Link
+		retrieveDevfiles.Add(util.ConcurrentTask{ToRun: func(errChannel chan error) {
+
+			// Note that this issues an HTTP get per devfile entry in the catalog, while doing it concurrently instead of
+			// sequentially improves the performance, caching that information would improve the performance even more
+			devfile, err := GetDevfile(link)
 			if err != nil {
-				log.Warningf("Registry %s is not set up properly with error: %v", registryName, err)
-				break
+				log.Warningf("Registry %s is not set up properly with error: %v", devfileIndex.Registry.Name, err)
+				errChannel <- err
+				return
 			}
 
 			// Populate devfile component with devfile data and form devfile component list
 			catalogDevfile := DevfileComponentType{
 				Name:        strings.TrimSuffix(devfile.MetaData.GenerateName, "-"),
-				DisplayName: devfileIndexEntry.DisplayName,
-				Description: devfileIndexEntry.Description,
-				Link:        devfileIndexEntryLink,
+				DisplayName: devfileIndex.DisplayName,
+				Description: devfileIndex.Description,
+				Link:        devfileIndex.Links.Link,
 				Support:     IsDevfileComponentSupported(devfile),
-				Registry: Registry{
-					Name: registryName,
-					URL:  registryURL,
-				},
+				Registry:    devfileIndex.Registry,
 			}
 
+			devfileMutex.Lock()
 			catalogDevfileList.Items = append(catalogDevfileList.Items, catalogDevfile)
-		}
+			devfileMutex.Unlock()
+		}})
+	}
+	if err := retrieveDevfiles.Run(); err != nil {
+		return *catalogDevfileList, err
 	}
 
-	return catalogDevfileList, nil
+	return *catalogDevfileList, nil
 }
 
 // ListComponents lists all the available component types

pkg/catalog/catalog_test.go

@@ -199,21 +199,30 @@ OdoSettings:
 	tests := []struct {
 		name         string
 		registryName string
-		want         map[string]string
+		want         map[string]Registry
 	}{
 		{
 			name:         "Case 1: Test get all devfile registries",
 			registryName: "",
-			want: map[string]string{
-				"CheDevfileRegistry":     "https://che-devfile-registry.openshift.io/",
-				"DefaultDevfileRegistry": "https://raw.githubusercontent.com/elsony/devfile-registry/master",
+			want: map[string]Registry{
+				"CheDevfileRegistry": {
+					Name: "CheDevfileRegistry",
+					URL:  "https://che-devfile-registry.openshift.io/",
+				},
+				"DefaultDevfileRegistry": {
+					Name: "DefaultDevfileRegistry",
+					URL:  "https://raw.githubusercontent.com/elsony/devfile-registry/master",
+				},
 			},
 		},
 		{
 			name:         "Case 2: Test get specific devfile registry",
 			registryName: "CheDevfileRegistry",
-			want: map[string]string{
-				"CheDevfileRegistry": "https://che-devfile-registry.openshift.io/",
+			want: map[string]Registry{
+				"CheDevfileRegistry": {
+					Name: "CheDevfileRegistry",
+					URL:  "https://che-devfile-registry.openshift.io/",
+				},
 			},
 		},
 	}
@@ -232,7 +241,7 @@ OdoSettings:
 	}
 }
 
-func TestGetDevfileIndex(t *testing.T) {
+func TestGetDevfileIndexEntries(t *testing.T) {
 	// Start a local HTTP server
 	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		// Send response to be tested
@@ -263,14 +272,15 @@ func TestGetDevfileIndex(t *testing.T) {
 	// Close the server when test finishes
 	defer server.Close()
 
+	const registryName = "some registry"
 	tests := []struct {
-		name             string
-		devfileIndexLink string
-		want             []DevfileIndexEntry
+		name     string
+		registry Registry
+		want     []DevfileIndexEntry
 	}{
 		{
-			name:             "Test NodeJS devfile index",
-			devfileIndexLink: server.URL,
+			name:     "Test NodeJS devfile index",
+			registry: Registry{Name: registryName, URL: server.URL},
 			want: []DevfileIndexEntry{
 				{
 					DisplayName: "NodeJS Angular Web Application",
@@ -282,6 +292,10 @@ func TestGetDevfileIndex(t *testing.T) {
 					},
 					Icon:              "/images/angular.svg",
 					GlobalMemoryLimit: "2686Mi",
+					Registry: Registry{
+						Name: registryName,
+						URL:  server.URL,
+					},
 					Links: struct {
 						Link string `json:"self"`
 					}{
@@ -294,7 +308,7 @@ func TestGetDevfileIndex(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := GetDevfileIndex(tt.devfileIndexLink)
+			got, err := getDevfileIndexEntries(tt.registry)
 
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("Got: %v, want: %v", got, tt.want)

pkg/catalog/types.go

@@ -35,6 +35,7 @@ type DevfileIndexEntry struct {
 	Tags              []string `json:"tags"`
 	Icon              string   `json:"icon"`
 	GlobalMemoryLimit string   `json:"globalMemoryLimit"`
+	Registry          Registry `json:"registry"`
 	Links             struct {
 		Link string `json:"self"`
 	} `json:"links"`
@@ -72,7 +73,7 @@ type ComponentTypeList struct {
 
 // DevfileComponentTypeList lists all the DevfileComponentType's
 type DevfileComponentTypeList struct {
-	DevfileRegistries map[string]string
+	DevfileRegistries map[string]Registry
 	Items             []DevfileComponentType
 }
 

pkg/odo/cli/catalog/list/components.go

@@ -2,20 +2,20 @@ package list
 
 import (
 	"fmt"
-	"io"
-	"os"
-	"strings"
-	"text/tabwriter"
-
 	"github.com/openshift/odo/pkg/catalog"
 	"github.com/openshift/odo/pkg/log"
 	"github.com/openshift/odo/pkg/machineoutput"
-	"github.com/openshift/odo/pkg/odo/cli/catalog/util"
+	catalogutil "github.com/openshift/odo/pkg/odo/cli/catalog/util"
 	"github.com/openshift/odo/pkg/odo/genericclioptions"
 	"github.com/openshift/odo/pkg/odo/util/experimental"
 	"github.com/openshift/odo/pkg/odo/util/pushtarget"
+	"github.com/openshift/odo/pkg/util"
 	"github.com/spf13/cobra"
+	"io"
 	"k8s.io/klog"
+	"os"
+	"strings"
+	"text/tabwriter"
 )
 
 const componentsRecommendedCommandName = "components"
@@ -42,32 +42,39 @@ func NewListComponentsOptions() *ListComponentsOptions {
 
 // Complete completes ListComponentsOptions after they've been created
 func (o *ListComponentsOptions) Complete(name string, cmd *cobra.Command, args []string) (err error) {
+
+	tasks := util.NewConcurrentTasks(2)
+
 	if !pushtarget.IsPushTargetDocker() {
 		o.Context = genericclioptions.NewContext(cmd)
-		o.catalogList, err = catalog.ListComponents(o.Client)
-		if err != nil {
-			if experimental.IsExperimentalModeEnabled() {
-				klog.V(4).Info("Please log in to an OpenShift cluster to list OpenShift/s2i components")
+
+		tasks.Add(util.ConcurrentTask{ToRun: func(errChannel chan error) {
+			o.catalogList, err = catalog.ListComponents(o.Client)
+			if err != nil {
+				if experimental.IsExperimentalModeEnabled() {
+					klog.V(4).Info("Please log in to an OpenShift cluster to list OpenShift/s2i components")
+				} else {
+					errChannel <- err
+				}
 			} else {
-				return err
+				o.catalogList.Items = catalogutil.FilterHiddenComponents(o.catalogList.Items)
 			}
-		}
-
-		o.catalogList.Items = util.FilterHiddenComponents(o.catalogList.Items)
+		}})
 	}
 
 	if experimental.IsExperimentalModeEnabled() {
-		o.catalogDevfileList, err = catalog.ListDevfileComponents("")
-		if err != nil {
-			return err
-		}
-
-		if o.catalogDevfileList.DevfileRegistries == nil {
-			log.Warning("Please run 'odo registry add <registry name> <registry URL>' to add registry for listing devfile components\n")
-		}
+		tasks.Add(util.ConcurrentTask{ToRun: func(errChannel chan error) {
+			o.catalogDevfileList, err = catalog.ListDevfileComponents("")
+			if o.catalogDevfileList.DevfileRegistries == nil {
+				log.Warning("Please run 'odo registry add <registry name> <registry URL>' to add registry for listing devfile components\n")
+			}
+			if err != nil {
+				errChannel <- err
+			}
+		}})
 	}
 
-	return
+	return tasks.Run()
 }
 
 // Validate validates the ListComponentsOptions based on completed values