Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: change logic of analysis run to better handle errors #2695

Merged
merged 7 commits into from
Apr 28, 2023
Merged

fix: change logic of analysis run to better handle errors #2695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cf55022
change logic of analysis run to better haneld errors
zachaller Apr 4, 2023
678fd42
change logic to not call GetMetaData if not nil like the old behavior
zachaller Apr 4, 2023
488d72f
move code closer to usage
zachaller Apr 4, 2023
2e32af5
change logic to not always call GetMetadata keeps original behavior
zachaller Apr 4, 2023
40d41d3
fix logic
zachaller Apr 4, 2023
798d8c5
cleanup
zachaller Apr 4, 2023
35e21aa
add test
zachaller Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions analysis/analysis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,34 +321,18 @@ func (c *Controller) runMeasurements(run *v1alpha1.AnalysisRun, tasks []metricTa
//redact secret values from logs
logger := logutil.WithRedactor(*logutil.WithAnalysisRun(run).WithField("metric", t.metric.Name), secrets)

resultsLock.Lock()
metricResult := analysisutil.GetResult(run, t.metric.Name)
resultsLock.Unlock()

provider, err := c.newProvider(*logger, t.metric)
if err != nil {
log.Errorf("Error in getting metric provider :%v", err)
return err
}
if metricResult == nil {
metricResult = &v1alpha1.MetricResult{
Name: t.metric.Name,
Phase: v1alpha1.AnalysisPhaseRunning,
DryRun: dryRunMetricsMap[t.metric.Name],
Metadata: provider.GetMetadata(t.metric),
}
}

var newMeasurement v1alpha1.Measurement
if err != nil {
provider, providerErr := c.newProvider(*logger, t.metric)
if providerErr != nil {
log.Errorf("Error in getting metric provider :%v", providerErr)
if t.incompleteMeasurement != nil {
newMeasurement = *t.incompleteMeasurement
} else {
startedAt := timeutil.MetaNow()
newMeasurement.StartedAt = &startedAt
}
newMeasurement.Phase = v1alpha1.AnalysisPhaseError
newMeasurement.Message = err.Error()
newMeasurement.Message = providerErr.Error()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not returning to the caller which will make the function to continue the execution even after the error occurred. Is this new behaviour expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is expected and is the fix for the actual bug, this function is also in a go routing and the returned error also is not handled by letting the code actually continue we now get provider errors to end users via the MetricRun status etc. The return was there because the old logic had a requirement on the provider being valid in order to call GetMetadata() I reworked it so that is not required anymore and so it should be safe to continue the code and report the error.

} else {
if t.incompleteMeasurement == nil {
newMeasurement = provider.Run(run, t.metric)
Expand All @@ -366,12 +350,28 @@ func (c *Controller) runMeasurements(run *v1alpha1.AnalysisRun, tasks []metricTa
}
}

resultsLock.Lock()
metricResult := analysisutil.GetResult(run, t.metric.Name)
resultsLock.Unlock()
if metricResult == nil {
metricResult = &v1alpha1.MetricResult{
Name: t.metric.Name,
Phase: v1alpha1.AnalysisPhaseRunning,
DryRun: dryRunMetricsMap[t.metric.Name],
}

if provider != nil && providerErr == nil {
metricResult.Metadata = provider.GetMetadata(t.metric)
}
}

if newMeasurement.Phase.Completed() {
logger.Infof("Measurement Completed. Result: %s", newMeasurement.Phase)
if newMeasurement.FinishedAt == nil {
finishedAt := timeutil.MetaNow()
newMeasurement.FinishedAt = &finishedAt
}

switch newMeasurement.Phase {
case v1alpha1.AnalysisPhaseSuccessful:
metricResult.Successful++
Expand Down
39 changes: 39 additions & 0 deletions analysis/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package analysis
import (
"context"
"encoding/json"
"fmt"
"reflect"
"testing"
"time"
Expand Down Expand Up @@ -317,6 +318,44 @@ func TestNoReconcileForAnalysisRunWithDeletionTimestamp(t *testing.T) {
f.run(getKey(ar, t))
}

func TestFailedToCreateProviderError(t *testing.T) {
f := newFixture(t)
defer f.Close()

ar := &v1alpha1.AnalysisRun{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: metav1.NamespaceDefault,
},
Spec: v1alpha1.AnalysisRunSpec{
Metrics: []v1alpha1.Metric{
{
Name: "metric1",
Provider: v1alpha1.MetricProvider{
Plugin: map[string]json.RawMessage{"mypluginns/myplugin": []byte(`{"invalid": "json"}`)},
},
},
},
},
}
f.analysisRunLister = append(f.analysisRunLister, ar)
f.objects = append(f.objects, ar)

c, i, k8sI := f.newController(noResyncPeriodFunc)
c.newProvider = func(logCtx log.Entry, metric v1alpha1.Metric) (metric.Provider, error) {
return nil, fmt.Errorf("failed to create provider")
}

pi := f.expectPatchAnalysisRunAction(ar)

f.runController(getKey(ar, t), true, false, c, i, k8sI)

updatedAr := f.getPatchedAnalysisRun(pi)

assert.Equal(t, v1alpha1.AnalysisPhaseError, updatedAr.Status.MetricResults[0].Measurements[0].Phase)
assert.Equal(t, "failed to create provider", updatedAr.Status.MetricResults[0].Measurements[0].Message)
}

func TestRun(t *testing.T) {
f := newFixture(t)
defer f.Close()
Expand Down