gemini report

Author: 照微

cmd/llmctl/config/config.go

@@ -42,7 +42,7 @@ Examples:
   llmctl config view
 
   # Add a new storage configuration
-  llmctl config add-storage my-pvc --type pvc --config claimName=model-pvc
+  llmctl config add-storage my-pvc --type pvc --config pvcName=model-pvc
 
   # Add a new model source
   llmctl config add-source huggingface --type huggingface --config token=hf_xxx

cmd/llmctl/svc/run.go

@@ -66,7 +66,7 @@ func resolveEngine(engineType string, cfg *cliconfig.Config) (*cliconfig.EngineC
 	}
 
 	// 3. Use default (empty config) - plugin will use its built-in defaults
-	fmt.Printf("INFO: Using default configuration for engine '%s'. Run 'llmctl config add-engine %s' to customize.\n", engineType, engineType)
+	fmt.Printf("INFO: Using default configuration for engine '%s'. Run 'llmctl config set-engine %s' to customize.\n", engineType, engineType)
 	return &cliconfig.EngineConfig{
 		Type:   engineType,
 		Config: map[string]interface{}{},

pkg/autobenchmark/config/parse.go

@@ -144,8 +144,8 @@ func validateScenario(cfg *AutoBenchmarkConfig) []string {
 	} else if err := ValidateWorkload(cfg.Scenario.Workload); err != nil {
 		errs = append(errs, fmt.Sprintf("scenario.workload: %v", err))
 	}
-	if cfg.Scenario.MaxRequests <= 0 {
-		errs = append(errs, "scenario.maxRequests: must be positive")
+	if cfg.Scenario.MaxRequests < 0 {
+		errs = append(errs, "scenario.maxRequests: must not be negative (0 means use tool default)")
 	}
 	if cfg.Scenario.Concurrency <= 0 {
 		errs = append(errs, "scenario.concurrency: must be positive")

pkg/autobenchmark/controller/controller.go

@@ -195,11 +195,10 @@ func (ctrl *Controller) Run(ctx context.Context) error {
 		// template should be resumable on the next run.
 		if ctx.Err() != nil {
 			logger.Info("Experiment timeout during template, not marking completed", "template", tmplRef.Name)
-			ts.BestTrial = SelectBest(ts.Trials)
 		} else {
 			ts.Completed = true
-			ts.BestTrial = SelectBest(ts.Trials)
 		}
+		ts.BestTrial = SelectBest(ts.Trials)
 
 		// Update global best
 		if ts.BestTrial != nil {
@@ -357,11 +356,8 @@ func (ctrl *Controller) executeTrial(
 			return
 		}
 		cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 60*time.Second)
-		err := wait.ExponentialBackoffWithContext(cleanupCtx, wait.Backoff{
-			Duration: 5 * time.Second,
-			Factor:   1,
-			Steps:    3,
-		}, func(ctx context.Context) (bool, error) {
+		defer cleanupCancel()
+		err := wait.PollUntilContextTimeout(cleanupCtx, 5*time.Second, 20*time.Second, true, func(ctx context.Context) (bool, error) {
 			if delErr := ctrl.manager.Delete(ctx, trialName); delErr != nil {
 				if apierrors.IsNotFound(delErr) {
 					return true, nil
@@ -374,7 +370,6 @@ func (ctrl *Controller) executeTrial(
 		if err != nil {
 			logger.Error(err, "Failed to cleanup trial RBG after retries", "rbgName", trialName)
 		}
-		cleanupCancel()
 	}()
 
 	// Create trial RBG
@@ -400,7 +395,7 @@ func (ctrl *Controller) executeTrial(
 		ctrl.collectFailureLogs(logger, trialName, resultDir, &result)
 		return result
 	}
-	modelName := extractServedModelName(baseRBG, ctrl.cfg.Backend)
+	modelName := extractServedModelName(baseRBG)
 
 	// Result directory: {reportDir}/{scenario}/{templateName}/trial-{idx}
 	scenario := ctrl.cfg.Scenario

pkg/autobenchmark/controller/early_termination.go

@@ -74,7 +74,7 @@ func CheckEarlyTermination(trials []abtypes.TrialResult, spec config.EarlyTermin
 		consecutive := 0
 		for i := len(trials) - 1; i >= 0; i-- {
 			if IsExecutionError(&trials[i]) {
-				break
+				continue
 			}
 			if !trials[i].IsSLAFeasible() {
 				consecutive++

pkg/autobenchmark/controller/endpoint.go

@@ -47,8 +47,8 @@ func (ctrl *Controller) resolveEndpoint(trialRBG *v1alpha2.RoleBasedGroup) strin
 		}
 		return lifecycle.GetServiceEndpoint(trialRBG, &role, ctrl.namespace, port)
 	}
-	// Last resort fallback: assume a default worker role at port 8000.
-	return lifecycle.GetServiceEndpoint(trialRBG, &v1alpha2.RoleSpec{Name: "worker"}, ctrl.namespace, 8000)
+	// Last resort fallback: assume a default worker role at the engine's default port.
+	return lifecycle.GetServiceEndpoint(trialRBG, &v1alpha2.RoleSpec{Name: "worker"}, ctrl.namespace, defaultEnginePort(ctrl.cfg.Backend))
 }
 
 // resolveRolePort extracts the inference port for a role.
@@ -94,20 +94,20 @@ func defaultEnginePort(backend string) int {
 	}
 }
 
-// extractServedModelName reads --served-model-name from the base template's container args.
+// extractServedModelName reads the served-model-name flag from the base template's container args.
 // Falls back to the RBG metadata name if the flag is not found.
-func extractServedModelName(rbg *v1alpha2.RoleBasedGroup, backend string) string {
-	flag := "--served-model-name"
-	if backend == "vllm" {
-		flag = "--served-model-name"
-	}
+func extractServedModelName(rbg *v1alpha2.RoleBasedGroup) string {
+	// Both vllm and sglang use --served-model-name.
+	const flag = "--served-model-name"
 	for _, role := range rbg.Spec.Roles {
 		podSpec := getRolePodSpec(&role)
 		if podSpec == nil {
 			continue
 		}
 		for _, c := range podSpec.Containers {
-			allArgs := append(c.Command, c.Args...)
+			allArgs := make([]string, 0, len(c.Command)+len(c.Args))
+			allArgs = append(allArgs, c.Command...)
+			allArgs = append(allArgs, c.Args...)
 			for i, arg := range allArgs {
 				if arg == flag && i+1 < len(allArgs) {
 					return allArgs[i+1]
@@ -147,6 +147,9 @@ func (ctrl *Controller) waitRBGFullyReady(
 	rbgReady := false
 	httpClient := &http.Client{Timeout: 5 * time.Second}
 
+	// Resolve endpoint once — trialRBG is immutable for the duration of the wait.
+	endpoint = ctrl.resolveEndpoint(trialRBG)
+
 	err = wait.PollUntilContextTimeout(ctx, 10*time.Second, timeout, true, func(ctx context.Context) (bool, error) {
 		if !rbgReady {
 			rbg, err := ctrl.manager.Get(ctx, trialName)
@@ -161,9 +164,12 @@ func (ctrl *Controller) waitRBGFullyReady(
 			rbgReady = true
 		}
 
-		endpoint = ctrl.resolveEndpoint(trialRBG)
 		healthURL := endpoint + "/health"
-		resp, err := httpClient.Get(healthURL)
+		req, reqErr := http.NewRequestWithContext(ctx, http.MethodGet, healthURL, nil)
+		if reqErr != nil {
+			return false, nil
+		}
+		resp, err := httpClient.Do(req)
 		if err != nil {
 			logger.V(2).Info("Endpoint not ready yet", "error", err.Error())
 			return false, nil // retry
@@ -200,7 +206,7 @@ func sanitizeLabelValue(name string) string {
 	// Replace invalid characters with '-'
 	invalidChars := regexp.MustCompile(`[^a-zA-Z0-9._-]`)
 	name = invalidChars.ReplaceAllString(name, "-")
-	name = strings.Trim(name, "-")
+	name = strings.Trim(name, "-._")
 	if name == "" {
 		name = "default"
 	}

pkg/autobenchmark/controller/endpoint_test.go

@@ -65,6 +65,8 @@ func TestSanitizeLabelValue(t *testing.T) {
 		{"special chars replaced", "exp name@v1!", "exp-name-v1"},
 		{"empty after sanitize", "@@@@", "default"},
 		{"leading and trailing dashes trimmed", "-my-exp-", "my-exp"},
+		{"trailing dot trimmed", "my-exp.", "my-exp"},
+		{"trailing underscore trimmed", "my-exp_", "my-exp"},
 		{"all dashes becomes default", "---", "default"},
 	}
 
@@ -198,10 +200,9 @@ func TestIsRBGReady(t *testing.T) {
 
 func TestExtractServedModelName(t *testing.T) {
 	tests := []struct {
-		name    string
-		rbg     *v1alpha2.RoleBasedGroup
-		backend string
-		want    string
+		name string
+		rbg  *v1alpha2.RoleBasedGroup
+		want string
 	}{
 		{
 			name: "flag in args",
@@ -217,8 +218,7 @@ func TestExtractServedModelName(t *testing.T) {
 					},
 				},
 			},
-			backend: "vllm",
-			want:    "llama-3",
+			want: "llama-3",
 		},
 		{
 			name: "flag in command",
@@ -237,8 +237,7 @@ func TestExtractServedModelName(t *testing.T) {
 					},
 				},
 			},
-			backend: "vllm",
-			want:    "gpt-custom",
+			want: "gpt-custom",
 		},
 		{
 			name: "flag not found - fallback to rbg name",
@@ -254,22 +253,20 @@ func TestExtractServedModelName(t *testing.T) {
 					},
 				},
 			},
-			backend: "vllm",
-			want:    "my-rbg",
+			want: "my-rbg",
 		},
 		{
 			name: "no roles",
 			rbg: &v1alpha2.RoleBasedGroup{
 				ObjectMeta: metav1.ObjectMeta{Name: "empty-rbg"},
 			},
-			backend: "sglang",
-			want:    "empty-rbg",
+			want: "empty-rbg",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			assert.Equal(t, tt.want, extractServedModelName(tt.rbg, tt.backend))
+			assert.Equal(t, tt.want, extractServedModelName(tt.rbg))
 		})
 	}
 }

pkg/autobenchmark/controller/failure_logs.go

@@ -66,6 +66,30 @@ func (ctrl *Controller) snapshotRestartCounts(logger logr.Logger, trialName stri
 	return snap
 }
 
+// identifyFailedPods filters pods that actually crashed during the benchmark:
+//   - Pod in Failed phase → indicates pod crash
+//   - Any container RestartCount increased vs. preRunRestarts snapshot → OOM or similar crash
+//   - Neither → benchmark tool's own problem, not a pod crash
+func identifyFailedPods(pods []corev1.Pod, preRunRestarts podRestartSnapshot) []*corev1.Pod {
+	var failedPods []*corev1.Pod
+	for i := range pods {
+		pod := &pods[i]
+		if pod.Status.Phase == corev1.PodFailed {
+			failedPods = append(failedPods, pod)
+			continue
+		}
+		if preRunRestarts != nil {
+			for _, cs := range append(pod.Status.InitContainerStatuses, pod.Status.ContainerStatuses...) {
+				if cs.RestartCount > preRunRestarts[pod.Name+"/"+cs.Name] {
+					failedPods = append(failedPods, pod)
+					break
+				}
+			}
+		}
+	}
+	return failedPods
+}
+
 // collectBenchmarkFailureLogs checks pod state after eval.Run failure and
 // collects logs only when pods actually crashed during the benchmark:
 //   - Pod in Failed phase → collect current logs (before RBG controller deletes it)
@@ -84,27 +108,9 @@ func (ctrl *Controller) collectBenchmarkFailureLogs(logger logr.Logger, trialNam
 		return
 	}
 
-	var needLogs bool
-	for i := range podList.Items {
-		pod := &podList.Items[i]
-		if pod.Status.Phase == corev1.PodFailed {
-			needLogs = true
-			break
-		}
-		if preRunRestarts != nil {
-			for _, cs := range append(pod.Status.InitContainerStatuses, pod.Status.ContainerStatuses...) {
-				if cs.RestartCount > preRunRestarts[pod.Name+"/"+cs.Name] {
-					needLogs = true
-					break
-				}
-			}
-		}
-		if needLogs {
-			break
-		}
-	}
+	failedPods := identifyFailedPods(podList.Items, preRunRestarts)
 
-	if !needLogs {
+	if len(failedPods) == 0 {
 		return
 	}
 
@@ -114,13 +120,12 @@ func (ctrl *Controller) collectBenchmarkFailureLogs(logger logr.Logger, trialNam
 		return
 	}
 
-	for i := range podList.Items {
-		pod := &podList.Items[i]
+	for _, pod := range failedPods {
 		logPath := filepath.Join(logDir, pod.Name+".log")
 		ctrl.writePodLogs(ctx, logPath, pod)
 	}
 
-	logger.Info("Collected benchmark failure logs", "logDir", logDir)
+	logger.Info("Collected benchmark failure logs", "logDir", logDir, "pods", len(failedPods))
 }
 
 // collectFailureLogs fetches failure context from all pods belonging to the
@@ -172,8 +177,12 @@ func (ctrl *Controller) collectFailureLogs(logger logr.Logger, trialName string,
 	}
 
 	if len(pendingSummaries) > 0 {
-		result.Error = fmt.Sprintf("%s; pending pods: %s",
-			result.Error, strings.Join(pendingSummaries, "; "))
+		pendingMsg := "pending pods: " + strings.Join(pendingSummaries, "; ")
+		if result.Error != "" {
+			result.Error += "; " + pendingMsg
+		} else {
+			result.Error = pendingMsg
+		}
 	}
 
 	if logFileCount > 0 {
@@ -254,8 +263,10 @@ func (ctrl *Controller) fetchContainerLogs(ctx context.Context, sb *strings.Buil
 		fmt.Fprintf(sb, "(failed to get logs: %v)\n\n", err)
 		return
 	}
+	defer func() {
+		_ = stream.Close()
+	}()
 	logBytes, err := io.ReadAll(stream)
-	_ = stream.Close()
 	if err != nil {
 		fmt.Fprintf(sb, "(failed to read logs: %v)\n\n", err)
 		return