Skip to content

Commit

Permalink
fix(analysis): Make AR End When Only Dry-Run Metrics Are Defined (arg…
Browse files Browse the repository at this point in the history 
…oproj#2230)

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
  • Loading branch information
@agrawroh @jenciso
agrawroh authored and jenciso committed Oct 25, 2022
1 parent a4489af commit 3d9d43c
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 4 deletions.
5 changes: 5 additions & 0 deletions analysis/analysis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,11 @@ func (c *Controller) assessRunStatus(run *v1alpha1.AnalysisRun, metrics []v1alph
runSummary.Successful++
}
} else {
// We don't really care about the failures from dry-runs and hence, if there is no current status
// found then we just set it to `AnalysisPhaseSuccessful`
if worstStatus == "" {
worstStatus = v1alpha1.AnalysisPhaseSuccessful
}
// Update metric result message
if message != "" {
result.Message = fmt.Sprintf("Metric assessed %s due to %s", metricStatus, message)
Expand Down
117 changes: 113 additions & 4 deletions analysis/analysis_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1500,7 +1500,7 @@ func TestAssessRunStatusErrorMessageAnalysisPhaseFail(t *testing.T) {

func TestAssessRunStatusErrorMessageAnalysisPhaseFailInDryRunMode(t *testing.T) {
status, message, dryRunSummary := StartAssessRunStatusErrorMessageAnalysisPhaseFail(t, true)
assert.Equal(t, v1alpha1.AnalysisPhaseRunning, status)
assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, status)
assert.Equal(t, "", message)
expectedDryRunSummary := v1alpha1.RunSummary{
Count: 2,
Expand Down Expand Up @@ -1545,7 +1545,7 @@ func TestAssessRunStatusErrorMessageFromProvider(t *testing.T) {
func TestAssessRunStatusErrorMessageFromProviderInDryRunMode(t *testing.T) {
providerMessage := "Provider Error"
status, message, dryRunSummary := StartAssessRunStatusErrorMessageFromProvider(t, providerMessage, true)
assert.Equal(t, v1alpha1.AnalysisPhaseRunning, status)
assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, status)
assert.Equal(t, "", message)
expectedDryRunSummary := v1alpha1.RunSummary{
Count: 2,
Expand Down Expand Up @@ -1587,7 +1587,7 @@ func TestAssessRunStatusMultipleFailures(t *testing.T) {

func TestAssessRunStatusMultipleFailuresInDryRunMode(t *testing.T) {
status, message, dryRunSummary := StartAssessRunStatusMultipleFailures(t, true)
assert.Equal(t, v1alpha1.AnalysisPhaseRunning, status)
assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, status)
assert.Equal(t, "", message)
expectedDryRunSummary := v1alpha1.RunSummary{
Count: 2,
Expand All @@ -1613,6 +1613,115 @@ func StartAssessRunStatusWorstMessageInReconcileAnalysisRun(t *testing.T, isDryR
return c.reconcileAnalysisRun(run)
}

// TestAssessRunStatusWithOnlyDryRunMetrics verifies that if only dry-run metrics are getting evaluated then, the final
// status of the analysis run is always successful.
func TestAssessRunStatusWithOnlyDryRunMetrics(t *testing.T) {
f := newFixture(t)
defer f.Close()
c, _, _ := f.newController(noResyncPeriodFunc)

run := v1alpha1.AnalysisRun{
Spec: v1alpha1.AnalysisRunSpec{
Metrics: []v1alpha1.Metric{
{
Name: "success-metric",
Provider: v1alpha1.MetricProvider{
Job: &v1alpha1.JobMetric{},
},
},
},
DryRun: []v1alpha1.DryRun{{
MetricName: "success-metric",
}},
},
Status: v1alpha1.AnalysisRunStatus{
MetricResults: []v1alpha1.MetricResult{
{
Name: "success-metric",
Count: 1,
Successful: 1,
DryRun: true,
Phase: v1alpha1.AnalysisPhaseSuccessful,
Measurements: []v1alpha1.Measurement{{
Phase: v1alpha1.AnalysisPhaseSuccessful,
StartedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))),
FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))),
}},
},
},
},
}

f.provider.On("Run", mock.Anything, mock.Anything, mock.Anything).Return(newMeasurement(v1alpha1.AnalysisPhaseFailed), nil)
f.provider.On("Resume", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(newMeasurement(v1alpha1.AnalysisPhaseSuccessful), nil)

newRun := c.reconcileAnalysisRun(&run)
assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, newRun.Status.Phase)
}

// TestAssessRunStatusWithMixedMetrics verifies that the status of dry-run metrics doesn't impact the final state of the
// analysis run.
func TestAssessRunStatusWithMixedMetrics(t *testing.T) {
f := newFixture(t)
defer f.Close()
c, _, _ := f.newController(noResyncPeriodFunc)

run := v1alpha1.AnalysisRun{
Spec: v1alpha1.AnalysisRunSpec{
Metrics: []v1alpha1.Metric{
{
Name: "run-forever",
Provider: v1alpha1.MetricProvider{
Job: &v1alpha1.JobMetric{},
},
},
{
Name: "success-metric",
Provider: v1alpha1.MetricProvider{
Job: &v1alpha1.JobMetric{},
},
},
},
DryRun: []v1alpha1.DryRun{{
MetricName: "success-metric",
}},
},
Status: v1alpha1.AnalysisRunStatus{
MetricResults: []v1alpha1.MetricResult{
{
Name: "run-forever",
Inconclusive: 1,
DryRun: false,
Phase: v1alpha1.AnalysisPhaseRunning,
Measurements: []v1alpha1.Measurement{{
Phase: v1alpha1.AnalysisPhaseRunning,
StartedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))),
}},
},
{
Name: "success-metric",
Count: 1,
Failed: 1,
DryRun: true,
Phase: v1alpha1.AnalysisPhaseSuccessful,
Measurements: []v1alpha1.Measurement{{
Phase: v1alpha1.AnalysisPhaseFailed,
StartedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))),
FinishedAt: timePtr(metav1.NewTime(time.Now().Add(-60 * time.Second))),
}},
},
},
},
}

f.provider.On("Run", mock.Anything, mock.Anything, mock.Anything).Return(newMeasurement(v1alpha1.AnalysisPhaseFailed), nil)
f.provider.On("Resume", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(newMeasurement(v1alpha1.AnalysisPhaseSuccessful), nil)

newRun := c.reconcileAnalysisRun(&run)
assert.Equal(t, v1alpha1.AnalysisPhaseInconclusive, newRun.Status.Phase)
assert.Equal(t, "Metric \"run-forever\" assessed Inconclusive due to inconclusive (1) > inconclusiveLimit (0)", newRun.Status.Message)
}

// TestAssessRunStatusWorstMessageInReconcileAnalysisRun verifies that the worstMessage returned by assessRunStatus is set as the
// status of the AnalysisRun returned by reconcileAnalysisRun
func TestAssessRunStatusWorstMessageInReconcileAnalysisRun(t *testing.T) {
Expand All @@ -1623,7 +1732,7 @@ func TestAssessRunStatusWorstMessageInReconcileAnalysisRun(t *testing.T) {

func TestAssessRunStatusWorstMessageInReconcileAnalysisRunInDryRunMode(t *testing.T) {
newRun := StartAssessRunStatusWorstMessageInReconcileAnalysisRun(t, true)
assert.Equal(t, v1alpha1.AnalysisPhaseRunning, newRun.Status.Phase)
assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, newRun.Status.Phase)
assert.Equal(t, "", newRun.Status.Message)
expectedDryRunSummary := v1alpha1.RunSummary{
Count: 2,
Expand Down

0 comments on commit 3d9d43c

Please sign in to comment.