Improve ManagementCommand status conditions #983
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances ManagementCommand status tracking by introducing granular condition types for criteria evaluation and execution states. The changes align ManagementCommand status handling with existing patterns used for Functions.
Key changes include:
- New condition types (
ManagementCommandCriteriaMatch,ManagementCommandExecuted) for tracking different lifecycle stages - Extracted shared criteria evaluation logic into reusable helpers (
NewCriteriaCondition,FormatClusterLabels) - Updated status reporting to persist conditions even when commands are skipped due to criteria mismatches
- Enhanced error messages with cluster context for better debugging
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1/managementcommand_types.go | Adds new condition constants and helper methods (SetCriteriaMatchCondition, SetExecutionCondition) for granular status tracking |
| api/v1/managementcommand_types_test.go | Tests for new condition setting methods on ManagementCommand |
| api/v1/criteria_conditions.go | Extracts shared criteria condition logic (NewCriteriaCondition, FormatClusterLabels) into reusable functions |
| api/v1/criteria_conditions_test.go | Tests for shared criteria condition helpers |
| api/v1/function_types.go | Refactors to use shared NewCriteriaCondition helper, removes duplicate FormatClusterLabels |
| api/v1/conditions.go | Adds ReasonProgressing constant for in-progress state |
| ingestor/adx/tasks.go | Updates ManagementCommand task execution to set granular conditions for criteria matching and execution states |
| ingestor/adx/tasks_test.go | Updates tests to verify new condition behavior including criteria match and execution conditions |
| parsed := kustoutil.ParseError(err) | ||
| logger.Errorf("Failed to execute management command %s/%s: %v", command.Namespace, command.Name, err) | ||
| command.SetExecutionCondition(metav1.ConditionFalse, v1.ReasonManagementCommandExecutionFailed, parsed) | ||
| if updErr := t.store.UpdateStatus(ctx, command, fmt.Errorf("%s", parsed)); updErr != nil { |
There was a problem hiding this comment.
Wrapping a string error message with fmt.Errorf(\"%s\", parsed) is redundant. Since parsed is already a string from kustoutil.ParseError(err), consider creating the error once and reusing it, or passing the original err to maintain the error chain.
| logger.Infof("Successfully executed management command %s.%s", command.Spec.Database, command.Name) | ||
| if err := t.store.UpdateStatus(ctx, &command, nil); err != nil { | ||
| message := "Management command executed successfully" | ||
| if endpoint := strings.TrimSpace(t.kustoCli.Endpoint()); endpoint != "" { |
There was a problem hiding this comment.
I'm not sure about this conditional. I think the kustoCli instance will pretty much always have an endpoint set on it if we're using it, and that's not really the criteria to know if we're executing against a specific db or not.
There was a problem hiding this comment.
StatementExecutor does not guarantee that the Endpoint is set, so the check is valid. We could short-circuit the check by hoisting it up the chain; however, the risk is that we leave all ManagementCommand CRDs with their last execution status. So if we have dozens of CRDs in some previous execution state, and we short-circuit the kusto client endpoint check, we leave the CRDs in their previous status condition state, which is misleading. I think the intent with controllers is that each CRD is suppose to reflect its own state.
Summary
Testing