feat(cli): improve get output for autoscaling-policies, autoscaling-p…#1177
feat(cli): improve get output for autoscaling-policies, autoscaling-p…#1177anirudh240 wants to merge 3 commits into
Conversation
…olicy-bindings, and model-servings Signed-off-by: Anirudh <2410030013@klh.edu.in>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR enhances the kthena get CLI output by adding optional name filtering for model-servings and enriching autoscaling policy display data, plus adds unit tests for new helper formatting functions.
Changes:
- Add optional
[NAME]argument toget model-servingsto filter listed resources by name substring. - Extend
get autoscaling-policiesoutput withMETRICScount andTOLERANCEcolumns. - Add helper functions for autoscaling policy binding target/min-max formatting and corresponding unit tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cli/kthena/cmd/get.go | Adds name filtering to model-servings output; adds autoscaling policy columns; introduces helper formatting functions. |
| cli/kthena/cmd/get_test.go | Adds tests for autoscaling policy binding target and min/max helper functions. |
Comments suppressed due to low confidence (1)
cli/kthena/cmd/get.go:353
- New CLI behavior was added (optional
[NAME]filtering, plus the distinct no-match message). Since this package already has tests, consider adding unit tests that cover: (1) filtering matches (case-insensitive substring), and (2) the no-match path that printsNo ModelServings found matching ....
func runGetModelServings(cmd *cobra.Command, args []string) error {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Print matching ModelServings | ||
| for _, ms := range modelServingList.Items { | ||
| age := time.Since(ms.CreationTimestamp.Time).Truncate(time.Second) | ||
| ready := fmt.Sprintf("%d/%d", ms.Status.AvailableReplicas, ms.Status.Replicas) | ||
| status := getModelServingStatus(ms.Status.Conditions) | ||
| if getAllNamespaces { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", ms.Namespace, ms.Name, ready, status, age) | ||
| } else { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", ms.Name, ready, status, age) | ||
| if nameFilter == "" || strings.Contains(strings.ToLower(ms.Name), strings.ToLower(nameFilter)) { | ||
| age := time.Since(ms.CreationTimestamp.Time).Truncate(time.Second) | ||
| ready := fmt.Sprintf("%d/%d", ms.Status.AvailableReplicas, ms.Status.Replicas) | ||
| status := getModelServingStatus(ms.Status.Conditions) | ||
| if getAllNamespaces { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", ms.Namespace, ms.Name, ready, status, age) | ||
| } else { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", ms.Name, ready, status, age) | ||
| } | ||
| } | ||
| } |
| Use: "model-servings [NAME]", | ||
| Aliases: []string{"ms", "model-serving"}, | ||
| Short: "List model serving workloads", | ||
| Long: `List ModelServing resources in the cluster.`, | ||
| Long: `List ModelServing resources in the cluster. Optionally filter by name.`, | ||
| Args: cobra.MaximumNArgs(1), |
| // Get name filter if provided | ||
| var nameFilter string | ||
| if len(args) > 0 { | ||
| nameFilter = args[0] | ||
| } |
| if matchCount == 0 { | ||
| if nameFilter != "" { | ||
| fmt.Printf("No ModelServings found matching '%s'.\n", nameFilter) |
There was a problem hiding this comment.
Code Review
This pull request introduces name-based filtering for the get model-servings command, adds helper functions to retrieve targets and replica limits for autoscaling policy bindings, and updates the output of get autoscaling-policies to include metrics and tolerance details. The review feedback highlights that the newly added helper functions for autoscaling policy bindings are defined and tested but never actually used in the command execution. Additionally, the reviewer suggests optimizing the model-serving filtering logic to avoid redundant iterations and ensuring that empty target references consistently fall back to a "" display value.
| // Print matching ModelServings | ||
| for _, ms := range modelServingList.Items { | ||
| age := time.Since(ms.CreationTimestamp.Time).Truncate(time.Second) | ||
| ready := fmt.Sprintf("%d/%d", ms.Status.AvailableReplicas, ms.Status.Replicas) | ||
| status := getModelServingStatus(ms.Status.Conditions) | ||
| if getAllNamespaces { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", ms.Namespace, ms.Name, ready, status, age) | ||
| } else { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", ms.Name, ready, status, age) | ||
| if nameFilter == "" || strings.Contains(strings.ToLower(ms.Name), strings.ToLower(nameFilter)) { | ||
| age := time.Since(ms.CreationTimestamp.Time).Truncate(time.Second) | ||
| ready := fmt.Sprintf("%d/%d", ms.Status.AvailableReplicas, ms.Status.Replicas) | ||
| status := getModelServingStatus(ms.Status.Conditions) | ||
| if getAllNamespaces { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", ms.Namespace, ms.Name, ready, status, age) | ||
| } else { | ||
| fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", ms.Name, ready, status, age) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterate over the pre-filtered filtered slice instead of re-evaluating the filter logic for each item.
// Print matching ModelServings
for _, ms := range filtered {
age := time.Since(ms.CreationTimestamp.Time).Truncate(time.Second)
ready := fmt.Sprintf("%d/%d", ms.Status.AvailableReplicas, ms.Status.Replicas)
status := getModelServingStatus(ms.Status.Conditions)
if getAllNamespaces {
fmt.Fprintf(w, "%s\\t%s\\t%s\\t%s\\t%s\\n", ms.Namespace, ms.Name, ready, status, age)
} else {
fmt.Fprintf(w, "%s\\t%s\\t%s\\t%s\\n", ms.Name, ready, status, age)
}
}| func getAutoscalingPolicyBindingTarget(binding workload.AutoscalingPolicyBinding) string { | ||
| if binding.Spec.HomogeneousTarget != nil { | ||
| return binding.Spec.HomogeneousTarget.Target.TargetRef.Name | ||
| } | ||
| if binding.Spec.HeterogeneousTarget != nil { | ||
| var names []string | ||
| for _, p := range binding.Spec.HeterogeneousTarget.Params { | ||
| if p.Target.TargetRef.Name != "" { | ||
| names = append(names, p.Target.TargetRef.Name) | ||
| } | ||
| } | ||
| if len(names) > 0 { | ||
| return strings.Join(names, ",") | ||
| } | ||
| } | ||
| return "<none>" | ||
| } |
There was a problem hiding this comment.
If binding.Spec.HomogeneousTarget.Target.TargetRef.Name is empty, the function currently returns an empty string "" instead of "<none>". We should check if the name is non-empty before returning it, allowing it to fall back to "<none>" for consistency with HeterogeneousTarget.
func getAutoscalingPolicyBindingTarget(binding workload.AutoscalingPolicyBinding) string {
if binding.Spec.HomogeneousTarget != nil {
if name := binding.Spec.HomogeneousTarget.Target.TargetRef.Name; name != "" {
return name
}
}
if binding.Spec.HeterogeneousTarget != nil {
var names []string
for _, p := range binding.Spec.HeterogeneousTarget.Params {
if p.Target.TargetRef.Name != "" {
names = append(names, p.Target.TargetRef.Name)
}
}
if len(names) > 0 {
return strings.Join(names, ",")
}
}
return "<none>"
}Signed-off-by: Anirudh <2410030013@klh.edu.in>
…cy-bindings Signed-off-by: Anirudh <2410030013@klh.edu.in>
| func getAutoscalingPolicyBindingTarget(binding workload.AutoscalingPolicyBinding) string { | ||
| if binding.Spec.HomogeneousTarget != nil { | ||
| if binding.Spec.HomogeneousTarget.Target.TargetRef.Name != "" { | ||
| return binding.Spec.HomogeneousTarget.Target.TargetRef.Name | ||
| } | ||
| } | ||
| if binding.Spec.HeterogeneousTarget != nil { | ||
| var names []string | ||
| for _, p := range binding.Spec.HeterogeneousTarget.Params { | ||
| if p.Target.TargetRef.Name != "" { | ||
| names = append(names, p.Target.TargetRef.Name) | ||
| } | ||
| } | ||
| if len(names) > 0 { | ||
| return strings.Join(names, ",") | ||
| } | ||
| } | ||
| return "<none>" | ||
| } | ||
|
|
||
| func getAutoscalingPolicyBindingMinMax(binding workload.AutoscalingPolicyBinding) (string, string) { | ||
| if binding.Spec.HomogeneousTarget != nil { | ||
| return fmt.Sprintf("%d", binding.Spec.HomogeneousTarget.MinReplicas), | ||
| fmt.Sprintf("%d", binding.Spec.HomogeneousTarget.MaxReplicas) | ||
| } | ||
| if binding.Spec.HeterogeneousTarget != nil { | ||
| var totalMin, totalMax int32 | ||
| for _, p := range binding.Spec.HeterogeneousTarget.Params { | ||
| totalMin += p.MinReplicas | ||
| totalMax += p.MaxReplicas | ||
| } | ||
| return fmt.Sprintf("%d", totalMin), fmt.Sprintf("%d", totalMax) | ||
| } | ||
| return "-", "-" | ||
| } |
| // Get name filter if provided | ||
| var nameFilter string | ||
| if len(args) > 0 { | ||
| nameFilter = args[0] | ||
| } | ||
| filterLower := strings.ToLower(nameFilter) | ||
|
|
||
| // Filter matching items | ||
| var filtered []workload.ModelServing | ||
| for _, ms := range modelServingList.Items { | ||
| if nameFilter == "" || strings.Contains(strings.ToLower(ms.Name), filterLower) { | ||
| filtered = append(filtered, ms) | ||
| } | ||
| } |
…olicy-bindings, and model-servings
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
so currently
kthena get autoscaling-policies,kthena get autoscaling-policy-bindings, andkthena get model-servingsdisplay only NAME and AGE columns, which forces operators to run a separate describe for every resource just to understand their cluster state.PR adds METRICS and TOLERANCE columns to
get autoscaling-policies, adds POLICY/TARGET/MIN/MAX columns toget autoscaling-policy-bindings, and adds an optional NAME filter argument toget model-servings(consistent with howget model-boostersalready works).i also added UT's for the new additions
Which issue(s) this PR fixes:
Fixes #1176
Special notes for your reviewer:
Does this PR introduce a user-facing change?: