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.
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
Add 'GetAvailablePackageSummaries' impl #3784
Changes from all commits
2e722e1
eb85992
ffcf3cc
d1f10f7
dbae04f
8ea324d
a54f759
7c1f4f0
b958aaf
c12a3f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,3 +11,164 @@ See the License for the specific language governing permissions and | |
limitations under the License. | ||
*/ | ||
package main | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"sync" | ||
|
||
corev1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/core/packages/v1alpha1" | ||
datapackagingv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging/v1alpha1" | ||
"google.golang.org/grpc/codes" | ||
"google.golang.org/grpc/status" | ||
log "k8s.io/klog/v2" | ||
) | ||
|
||
// GetAvailablePackageSummaries returns the available packages managed by the 'kapp_controller' plugin | ||
func (s *Server) GetAvailablePackageSummaries(ctx context.Context, request *corev1.GetAvailablePackageSummariesRequest) (*corev1.GetAvailablePackageSummariesResponse, error) { | ||
log.Infof("+kapp-controller GetAvailablePackageSummaries") | ||
|
||
// Retrieve the proper parameters from the request | ||
namespace := request.GetContext().GetNamespace() | ||
cluster := request.GetContext().GetCluster() | ||
pageSize := request.GetPaginationOptions().GetPageSize() | ||
pageOffset, err := pageOffsetFromPageToken(request.GetPaginationOptions().GetPageToken()) | ||
if err != nil { | ||
return nil, status.Errorf(codes.InvalidArgument, "unable to intepret page token %q: %v", request.GetPaginationOptions().GetPageToken(), err) | ||
} | ||
// Assume the default cluster if none is specified | ||
if cluster == "" { | ||
cluster = s.globalPackagingCluster | ||
} | ||
// fetch all the package metadatas | ||
pkgMetadatas, err := s.getPkgMetadatas(ctx, cluster, namespace) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've quickly analyzed the elapsed times of each part:
If I execute the Kubectl equivalent:
So, a bit slow, but not -that- slow (as in up to 2s)... further investigation is required. Adding a TODO linked to this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent - thanks for the info. |
||
if err != nil { | ||
return nil, errorByStatus("get", "PackageMetadata", "", err) | ||
} | ||
|
||
// paginate the list of results | ||
availablePackageSummaries := make([]*corev1.AvailablePackageSummary, len(pkgMetadatas)) | ||
|
||
// create the waiting group for processing each item aynchronously | ||
var wg sync.WaitGroup | ||
|
||
// TODO(agamez): DRY up this logic (cf GetInstalledPackageSummaries) | ||
if len(pkgMetadatas) > 0 { | ||
startAt := -1 | ||
if pageSize > 0 { | ||
startAt = int(pageSize) * pageOffset | ||
} | ||
for i, pkgMetadata := range pkgMetadatas { | ||
wg.Add(1) | ||
if startAt <= i { | ||
go func(i int, pkgMetadata *datapackagingv1alpha1.PackageMetadata) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the same/similar method you're using elsewhere? (sorry, it was hours ago in a different review, not 100%), in which case, perhaps we can DRY it up (eventually, no problem if it's just a comment for now). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I've used this pagination logic (inspired by Greg's code :P) also in the GetInstalledPackageSummaries. Adding a comment in both places for future improvement. Thanks! |
||
defer wg.Done() | ||
// fetch the associated packages | ||
// Use the field selector to return only Package CRs that match on the spec.refName. | ||
// TODO(agamez): perhaps we better fetch all the packages and filter ourselves to reduce the k8s calls | ||
fieldSelector := fmt.Sprintf("spec.refName=%s", pkgMetadata.Name) | ||
pkgs, err := s.getPkgsWithFieldSelector(ctx, cluster, namespace, fieldSelector) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes me wonder, given that the package resources are pretty lean (no inlined SVG!), whether it'd be faster here to do a single request for all packages and just filter it down based on the metadata that we're dealing with. If you've got your cluster setup with the bitnami (carvel) repo, you can just try That'd mean no need for go-routines here, if so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed that delegating the pkg search thing back to kubernetes was quicker than doing it by ourselves, but you are possibly right... we better fetch the whole list -in memory: future issues??- and then we filter.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, though if the inline SVG isn't the issue, my original thought above is moot. |
||
if err != nil { | ||
return errorByStatus("get", "Package", pkgMetadata.Name, err) | ||
} | ||
pkgVersionsMap, err := getPkgVersionsMap(pkgs) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// generate the availablePackageSummary from the fetched information | ||
availablePackageSummary, err := s.buildAvailablePackageSummary(pkgMetadata, pkgVersionsMap, cluster) | ||
if err != nil { | ||
return status.Errorf(codes.Internal, fmt.Sprintf("unable to create the AvailablePackageSummary: %v", err)) | ||
} | ||
|
||
// append the availablePackageSummary to the slice | ||
availablePackageSummaries[i] = availablePackageSummary | ||
return nil | ||
}(i, pkgMetadata) | ||
} | ||
// if we've reached the end of the page, stop iterating | ||
if pageSize > 0 && len(availablePackageSummaries) == int(pageSize) { | ||
break | ||
} | ||
} | ||
} | ||
wg.Wait() // Wait until each goroutine has finished | ||
|
||
// TODO(agamez): the slice with make is filled with <nil>, in case of an error in the | ||
// i goroutine, the i-th <nil> stub will remain. Check if 'errgroup' works here, but I haven't | ||
// been able so far. | ||
// An alternative is using channels to perform a fine-grained control... but not sure if it worths | ||
// However, should we just return an error if so? See https://github.com/kubeapps/kubeapps/pull/3784#discussion_r754836475 | ||
// filter out <nil> values | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment to the related PR (for getinstalled? not sure): why are we not returning an error if there was an error while fetching the result, rather than returning incomplete results (which it sounds like the case for values is). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to do it as fail-tolerant as possible; especially, when developing I had some inconsistencies (like in manually deleting CRs) so I opted for being lenient and letting packages have errors. A reason for that is we currently propagate that error through the chain of calls, so, a maybe temporary inconsistency or error in a single package will prevent the whole packages core API from working until the problem is manually fixed. Is that the intended behavior? I don't know, but, as a user, I don't want to break Kubeapps for a single corrupted package. Let me leave this TODO as is right now so that we can discuss it in the future. Adding a link to the comment anyway. |
||
availablePackageSummariesNilSafe := []*corev1.AvailablePackageSummary{} | ||
categories := []string{} | ||
for _, availablePackageSummary := range availablePackageSummaries { | ||
if availablePackageSummary != nil { | ||
availablePackageSummariesNilSafe = append(availablePackageSummariesNilSafe, availablePackageSummary) | ||
categories = append(categories, availablePackageSummary.Categories...) | ||
|
||
} | ||
} | ||
// if no results whatsoever, throw an error | ||
if len(availablePackageSummariesNilSafe) == 0 { | ||
return nil, status.Errorf(codes.NotFound, fmt.Sprintf("no available packages: %v", err)) | ||
} | ||
|
||
// Only return a next page token if the request was for pagination and | ||
// the results are a full page. | ||
nextPageToken := "" | ||
if pageSize > 0 && len(availablePackageSummariesNilSafe) == int(pageSize) { | ||
nextPageToken = fmt.Sprintf("%d", pageOffset+1) | ||
} | ||
response := &corev1.GetAvailablePackageSummariesResponse{ | ||
AvailablePackageSummaries: availablePackageSummariesNilSafe, | ||
// TODO(agamez): populate this field | ||
Categories: categories, | ||
NextPageToken: nextPageToken, | ||
} | ||
return response, nil | ||
} | ||
|
||
// GetAvailablePackageVersions returns the package versions managed by the 'kapp_controller' plugin | ||
func (s *Server) GetAvailablePackageVersions(ctx context.Context, request *corev1.GetAvailablePackageVersionsRequest) (*corev1.GetAvailablePackageVersionsResponse, error) { | ||
log.Infof("+kapp-controller GetAvailablePackageVersions") | ||
|
||
// Retrieve the proper parameters from the request | ||
namespace := request.GetAvailablePackageRef().GetContext().GetNamespace() | ||
cluster := request.GetAvailablePackageRef().GetContext().GetCluster() | ||
identifier := request.GetAvailablePackageRef().GetIdentifier() | ||
|
||
// Validate the request | ||
if namespace == "" || identifier == "" { | ||
return nil, status.Errorf(codes.InvalidArgument, "Required context or identifier not provided") | ||
} | ||
|
||
if cluster == "" { | ||
cluster = s.globalPackagingCluster | ||
} | ||
|
||
// Use the field selector to return only Package CRs that match on the spec.refName. | ||
fieldSelector := fmt.Sprintf("spec.refName=%s", identifier) | ||
pkgs, err := s.getPkgsWithFieldSelector(ctx, cluster, namespace, fieldSelector) | ||
if err != nil { | ||
return nil, errorByStatus("get", "Package", "", err) | ||
} | ||
pkgVersionsMap, err := getPkgVersionsMap(pkgs) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// TODO(minelson): support configurable version summary for kapp-controller pkgs | ||
// as already done for Helm (see #3588 for more info). | ||
versions := make([]*corev1.PackageAppVersion, len(pkgVersionsMap[identifier])) | ||
for i, v := range pkgVersionsMap[identifier] { | ||
versions[i] = &corev1.PackageAppVersion{ | ||
PkgVersion: v.version.String(), | ||
} | ||
} | ||
|
||
return &corev1.GetAvailablePackageVersionsResponse{ | ||
PackageAppVersions: versions, | ||
}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thought as earlier... why do we not just require cluster in the request now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could throw an error, that's true... however, let's assume a user only has a single cluster: do we really want to force them to write "default" or whichever name in the kubeapps
clusters
section?However, the reason why I wrote it was simply for the sake of parity between different plugins. See this example of the Helm one:
https://github.com/kubeapps/kubeapps/blob/9f3b99a9e94d49039dd41b49549220725a624e7e/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go#L683-L684
That said, I don't have a strong opinion here, happy to revert it to a 400-alike error (albeit I'd like to ensure the same behavior across plugins first)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, all good. I'd mistakenly thought that we'd updated already elsewhere, but what it was was that we updated the flux plugin to accept and use it when set (since the dashboard always sends it). Better to have it consistent with the other plugins. Sorry for the noise.