Add 'GetAvailablePackageSummaries' impl

[antgamdia]

Description of the change

Initial implementation of the GetAvailablePackageSummaries operation

Benefits

An initial and very limited Carvel support in Kubeapps

Possible drawbacks

N/A

Applicable issues

• related Carvel package support in Kubeapps #3221

Additional information

N/A

[absoludity]

Same thought as earlier... why do we not just require cluster in the request now?

[antgamdia]

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)

[absoludity]

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.

[absoludity]

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).

[antgamdia]

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!

[absoludity]

Given that the PackageMetadata contains the in-lined SVG icon, this call could itself be causing performance/mem issues for a repo like Bitnami? Worth checking and if so, only request the page size number of results.

[antgamdia]

I've quickly analyzed the elapsed times of each part:

• Get all package metadatas: 0.2 seconds, it's ok
• getPkgsWithFieldSelector: 2 seconds avg
  • The "problem" is in the resource.List(ctx, listOptions) code, which is taking from 0.8 to 2.4 seconds in retrieving all the packages whose FieldSelector is foo. Some examples:
    • resource.List function took 2.245123407s for (3) items for spec.refName=contour.community.tanzu.vmware.com
    • resource.List function took 1.550673668s for (3) items for spec.refName=pkg.test.carvel.dev
• getPkgVersionsMap function: microseconds, not relevant
• Build the AvailablePackageSummary object: microseconds, not relevant

If I execute the Kubectl equivalent:

> rm -rf  $HOME/.kube/cache
> time k get pkg --field-selector spec.refName=kapp-controller.tanzu.vmware.com
real    0m0,462s

> time k get pkg --field-selector spec.refName=kapp-controller.tanzu.vmware.com
real    0m0,347s

So, a bit slow, but not -that- slow (as in up to 2s)... further investigation is required.

Adding a TODO linked to this comment.

[absoludity]

Excellent - thanks for the info.

[absoludity]

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 time kubectl get packages --all-namespaces.

That'd mean no need for go-routines here, if so.

[antgamdia]

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.

> time kubectl get packages --all-namespaces
real    0m1,464s

[absoludity]

Yeah, though if the inline SVG isn't the issue, my original thought above is moot.

[absoludity]

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).

[antgamdia]

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.

[absoludity]

Similar comment here: slight preference for a name that doesn't communicate getting/fetching over a network.

[absoludity]

Same comment about Strings.Builder.

[absoludity]

Similar comment: not sure why the tests were removed to be re-added, rather than just editing them so it's clear what changed (just for next time).