Repository navigation
Suggest closest package when trying to create or scale a cluster - #235
bashofmann wants to merge 1 commit into
Conversation
When scaling or creating a cluster and the picked ressources do not match a package, a few closest packages are suggested.
Davidonium
left a comment
Davidonium
left a comment
There was a problem hiding this comment.
The commit subject and PR title need to be Conventional Commits for releaser-pleaser (AGENTS.md). It's a single commit, so the subject is what lands on main. Suggested: feat(cluster): suggest closest packages when no package matches. Also a typo in the description and commit body: "ressources".
| return matches[0], nil | ||
| case 0: | ||
| return nil, fmt.Errorf("no package found matching %s; use 'package list' to see available packages", desc) | ||
| suggestions := closestPackages(q, resp.GetItems(), maxPackageSuggestions) |
There was a problem hiding this comment.
When MultiAz is false, the request leaves req.MultiAz unset (line 175). booking.proto says unset means "all packages are returned", so a single-AZ create or scale gets multi-AZ packages in its suggestions. There's no multi-AZ column, so they show up as duplicate rows. Can we send req.MultiAz = new(q.MultiAz)? false excludes multi-AZ. Exact matching (line 175) and ResolvePackageByName (line 132) have the same gap. That's older code, but the suggestions table now makes it visible.
Also, for scale, the cluster's current package can show up as a suggestion. Should it be excluded?
| // one dimension exactly ranks above one that is off in all of them. | ||
| // The next biggest package, the closest one that has at least the requested | ||
| // amount of every resource, always comes first. | ||
| func closestPackages(q PackageResourceQuery, pkgs []*bookingv1.Package, limit int) []packageSuggestion { |
There was a problem hiding this comment.
Packages also have a tier field. Standard and premium packages with the same CPU/RAM come out as identical rows in whatever order the API returns them. Either filter by tier or add a TIER column so they can be told apart.
| var distance float64 | ||
| missing := false | ||
| fits := true | ||
| for _, d := range []struct{ want, have int64 }{ |
There was a problem hiding this comment.
Disk isn't part of the distance, so packages that differ only in disk always tie and fall back to API order. If that's intentional, adding disk to the tie-breakers in the sort at line 312 would at least make the order stable.
| ) | ||
| }) | ||
|
|
||
| if i := slices.IndexFunc(suggestions, func(sg packageSuggestion) bool { return sg.fits }); i > 0 { |
There was a problem hiding this comment.
"Next biggest" is the fitting package with the smallest log distance, not the smallest package that fits. Asking for 2 CPU / 8GiB with A = 2/64GiB and B = 4/16GiB picks B. Also, fits only looks at the flags passed, so create --cpu 4 treats every 4+ CPU package as fitting regardless of RAM. Either pick the smallest fitting package explicitly or rename the label to something like "closest larger package".
|
|
||
| sb.WriteString("closest available packages:\n") | ||
|
|
||
| w := tabwriter.NewWriter(&sb, 0, 0, 2, ' ', 0) |
There was a problem hiding this comment.
This adds a second table renderer next to output.NewTable. I see why: the table is indented inside an error string. Did you try output.NewTable with a writer that adds the prefix? Not blocking.
| } | ||
|
|
||
| disk := sg.pkg.GetResourceConfiguration().GetDisk() | ||
| if parsed, err := resource.ParseByteQuantity(disk); err == nil { |
There was a problem hiding this comment.
Disk is normalised here (16GiB), but package list prints the raw value (16Gi). Same for CPU/RAM through .String(). The two commands should show a package's resources the same way. If this formatting stays, it could move to internal/cmd/output and package list could use it too.
| ) | ||
| require.Error(t, err) | ||
| msg := err.Error() | ||
| for _, name := range []string{"pkg-1 ", "pkg-2 ", "pkg-3 ", "pkg-4 ", "pkg-5 "} { |
There was a problem hiding this comment.
The trailing space in "pkg-1 " is what keeps it from matching pkg-10, which breaks easily. Asserting on whole table rows, or on the name followed by a tab or column gap, would be more reliable.
| use 'package list' to see all available packages`, err.Error()) | ||
| } | ||
|
|
||
| func TestCreateCluster_PackageByResourcesNoMatchNothingBigEnough(t *testing.T) { |
There was a problem hiding this comment.
No test covers the fallback where every package lacks a requested dimension and the old one-line error is returned (resolve.go:237-239). Can you add one? For example, request --gpu while every package has no GPU.
When scaling or creating a cluster and the picked ressources do not match a package, a few closest packages are suggested.