Repository: clouditor/clouditor
Pull request #667 (2022-05-06 08:04:20)

Add implementation for `UpdateMetricConfiguration`
It seems that this API endpoint was actually forgotten before.

codecov[bot]: # [Codecov](https://codecov.io/gh/clouditor/clouditor/pull/667?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) Report
> Merging [#667](https://codecov.io/gh/clouditor/clouditor/pull/667?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (e4b3a0a) into [main](https://codecov.io/gh/clouditor/clouditor/commit/01f153aa5ec7f5bc778f0172f0d0ab27e3470dcb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (01f153a) will **increase** coverage by `0.12%`.
> The diff coverage is `91.89%`.

```diff
@@            Coverage Diff             @@
##             main     #667      +/-   ##
==========================================
+ Coverage   81.31%   81.44%   +0.12%     
==========================================
  Files          64       64              
  Lines        5737     5771      +34     
==========================================
+ Hits         4665     4700      +35     
+ Misses        830      829       -1     
  Partials      242      242              
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `81.44% <91.89%> (+0.12%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/clouditor/clouditor/pull/667?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) | Coverage Δ | |
|---|---|---|
| [service/orchestrator/metrics.go](https://codecov.io/gh/clouditor/clouditor/pull/667/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#diff-c2VydmljZS9vcmNoZXN0cmF0b3IvbWV0cmljcy5nbw==) | `87.59% <91.89%> (+0.52%)` | :arrow_up: |
| [service/discovery/azure/azure.go](https://codecov.io/gh/clouditor/clouditor/pull/667/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#diff-c2VydmljZS9kaXNjb3ZlcnkvYXp1cmUvYXp1cmUuZ28=) | `84.41% <0.00%> (+5.19%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/clouditor/clouditor/pull/667?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/clouditor/clouditor/pull/667?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor). Last update [01f153a...e4b3a0a](https://codecov.io/gh/clouditor/clouditor/pull/667?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor).


Patch:
diff --git a/service/orchestrator/metrics.go b/service/orchestrator/metrics.go
index 2473cc85..2d3ecae8 100644
--- a/service/orchestrator/metrics.go
+++ b/service/orchestrator/metrics.go
@@ -150,6 +150,10 @@ func (svc *Service) CreateMetric(_ context.Context, req *orchestrator.CreateMetr
 		return nil, status.Errorf(codes.InvalidArgument, "%v", newError)
 	}
 
+	// Lock metrics mutex
+	svc.mm.Lock()
+	defer svc.mm.Unlock()
+
 	// Check, if metric id already exists
 	if _, ok := svc.metrics[req.Metric.Id]; ok {
 		return nil, status.Error(codes.AlreadyExists, "metric already exists")
@@ -184,6 +188,10 @@ func (svc *Service) UpdateMetric(_ context.Context, req *orchestrator.UpdateMetr
 		return nil, status.Errorf(codes.InvalidArgument, "%v", newError)
 	}
 
+	// Lock metrics mutex
+	svc.mm.Lock()
+	defer svc.mm.Unlock()
+
 	// Check, if metric exists according to req.MetricId
 	if metric, ok = svc.metrics[req.MetricId]; !ok {
 		newError := fmt.Errorf("metric with identifier %s does not exist", req.MetricId)
@@ -218,6 +226,10 @@ func (svc *Service) UpdateMetricImplementation(_ context.Context, req *orchestra
 
 	// TODO(oxisto): Validate the metric implementation request
 
+	// Lock metrics mutex
+	svc.mm.Lock()
+	defer svc.mm.Unlock()
+
 	// Check, if metric exists according to req.MetricId
 	if metric, ok = svc.metrics[req.MetricId]; !ok {
 		err := fmt.Errorf("metric with identifier %s does not exist", req.MetricId)
@@ -250,6 +262,10 @@ func (svc *Service) UpdateMetricImplementation(_ context.Context, req *orchestra
 func (svc *Service) ListMetrics(_ context.Context, req *orchestrator.ListMetricsRequest) (res *orchestrator.ListMetricsResponse, err error) {
 	res = new(orchestrator.ListMetricsResponse)
 
+	// Lock metrics mutex
+	svc.mm.Lock()
+	defer svc.mm.Unlock()
+
 	// Paginate the metrics according to the request
 	res.Metrics, res.NextPageToken, err = service.PaginateMapValues(req, svc.metrics, func(a *assessment.Metric, b *assessment.Metric) bool {
 		return a.Id < b.Id
@@ -265,6 +281,10 @@ func (svc *Service) ListMetrics(_ context.Context, req *orchestrator.ListMetrics
 func (svc *Service) GetMetric(_ context.Context, req *orchestrator.GetMetricRequest) (metric *assessment.Metric, err error) {
 	var ok bool
 
+	// Lock metrics mutex
+	svc.mm.Lock()
+	defer svc.mm.Unlock()
+
 	if metric, ok = svc.metrics[req.MetricId]; !ok {
 		return nil, status.Errorf(codes.NotFound, "could not find metric with id %s", req.MetricId)
 	}
@@ -272,9 +292,13 @@ func (svc *Service) GetMetric(_ context.Context, req *orchestrator.GetMetricRequ
 	return
 }
 
-func (s *Service) GetMetricConfiguration(_ context.Context, req *orchestrator.GetMetricConfigurationRequest) (response *assessment.MetricConfiguration, err error) {
+func (svc *Service) GetMetricConfiguration(_ context.Context, req *orchestrator.GetMetricConfigurationRequest) (response *assessment.MetricConfiguration, err error) {
+	// Lock metrics mutex
+	svc.mm.Lock()
+	defer svc.mm.Unlock()
+
 	// Check, if we have a specific configuration for the metric
-	if config, ok := s.metricConfigurations[req.ServiceId][req.MetricId]; ok {
+	if config, ok := svc.metricConfigurations[req.ServiceId][req.MetricId]; ok {
 		return config, nil
 	}
 
@@ -283,10 +307,57 @@ func (s *Service) GetMetricConfiguration(_ context.Context, req *orchestrator.Ge
 		return config, nil
 	}
 
-	newError := fmt.Errorf("could not find metric configuration for metric %s in service %s", req.MetricId, req.ServiceId)
-	log.Error(newError)
+	err = fmt.Errorf("could not find metric configuration for metric %s in service %s", req.MetricId, req.ServiceId)
+	log.Error(err)
+
+	return nil, status.Errorf(codes.NotFound, "%v", err)
+}
+
+// UpdateMetricConfiguration updates the configuration for a metric, specified by the identifier in req.MetricId.
+func (svc *Service) UpdateMetricConfiguration(_ context.Context, req *orchestrator.UpdateMetricConfigurationRequest) (res *assessment.MetricConfiguration, err error) {
+	var (
+		ok  bool
+		cld orchestrator.CloudService
+	)
+
+	// TODO(oxisto): Validate the metric configuration request
 
-	return nil, status.Errorf(codes.NotFound, "%v", newError)
+	// Lock metrics mutex
+	svc.mm.Lock()
+	defer svc.mm.Unlock()
+
+	// Check, if metric exists according to req.MetricId
+	if _, ok = svc.metrics[req.MetricId]; !ok {
+		return nil, status.Errorf(codes.NotFound, "could not find metric with id %s", req.MetricId)
+	}
+
+	err = svc.storage.Get(&cld, "Id = ?", req.ServiceId)
+	if errors.Is(err, persistence.ErrRecordNotFound) {
+		return nil, status.Errorf(codes.NotFound, "service not found")
+	} else if err != nil {
+		return nil, status.Errorf(codes.Internal, "database error: %s", err)
+	}
+
+	// Update config
+	res = req.Configuration
+
+	// Make sure, that metric configuration exists for the service
+	if _, ok = svc.metricConfigurations[req.ServiceId]; !ok {
+		svc.metricConfigurations[req.ServiceId] = make(map[string]*assessment.MetricConfiguration)
+	}
+
+	// Store it
+	svc.metricConfigurations[req.ServiceId][req.MetricId] = res
+
+	// Notify event listeners
+	go func() {
+		svc.events <- &orchestrator.MetricChangeEvent{
+			Type:     orchestrator.MetricChangeEvent_CONFIG_CHANGED,
+			MetricId: req.MetricId,
+		}
+	}()
+
+	return
 }
 
 // ListMetricConfigurations retrieves a list of MetricConfiguration objects for a particular target
diff --git a/service/orchestrator/orchestrator.go b/service/orchestrator/orchestrator.go
index 86b39dd9..c2e62c71 100644
--- a/service/orchestrator/orchestrator.go
+++ b/service/orchestrator/orchestrator.go
@@ -61,6 +61,12 @@ type Service struct {
 	// metricConfigurations holds a double-map of metric configurations associated first by service ID and then metric ID
 	metricConfigurations map[string]map[string]*assessment.MetricConfiguration
 
+	// metrics contains map of our metric definitions
+	metrics map[string]*assessment.Metric
+
+	// mm is a mutex for metric related maps
+	mm sync.Mutex
+
 	// Currently only in-memory
 	results map[string]*assessment.AssessmentResult
 
@@ -71,9 +77,6 @@ type Service struct {
 
 	storage persistence.Storage
 
-	// metrics contains map of our metric definitions
-	metrics map[string]*assessment.Metric
-
 	metricsFile string
 
 	// loadMetricsFunc is a function that is used to initially load metrics at the start of the orchestrator


Test patch:
diff --git a/service/orchestrator/metrics_test.go b/service/orchestrator/metrics_test.go
index 9a905b81..d2b3657c 100644
--- a/service/orchestrator/metrics_test.go
+++ b/service/orchestrator/metrics_test.go
@@ -709,3 +709,88 @@ func TestService_ListMetricConfigurations(t *testing.T) {
 		})
 	}
 }
+
+func TestService_UpdateMetricConfiguration(t *testing.T) {
+	type fields struct {
+		metricConfigurations  map[string]map[string]*assessment.MetricConfiguration
+		results               map[string]*assessment.AssessmentResult
+		AssessmentResultHooks []func(result *assessment.AssessmentResult, err error)
+		storage               persistence.Storage
+		metrics               map[string]*assessment.Metric
+		metricsFile           string
+		loadMetricsFunc       func() ([]*assessment.Metric, error)
+		requirements          []*orchestrator.Requirement
+		events                chan *orchestrator.MetricChangeEvent
+	}
+	type args struct {
+		in0 context.Context
+		req *orchestrator.UpdateMetricConfigurationRequest
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		args    args
+		wantRes *assessment.MetricConfiguration
+		wantErr bool
+	}{
+		{
+			name: "metric does not exist",
+			fields: fields{
+				metrics: map[string]*assessment.Metric{},
+			},
+			args: args{
+				req: &orchestrator.UpdateMetricConfigurationRequest{ServiceId: DefaultTargetCloudServiceId, MetricId: "MyMetric"},
+			},
+			wantErr: true,
+		},
+		{
+			name: "service does not exist",
+			fields: fields{
+				metrics: map[string]*assessment.Metric{"MyMetric": {}},
+				storage: testutil.NewInMemoryStorage(t),
+			},
+			args: args{
+				req: &orchestrator.UpdateMetricConfigurationRequest{ServiceId: "MyService", MetricId: "MyMetric"},
+			},
+			wantErr: true,
+		},
+		{
+			name: "first metric in service",
+			fields: fields{
+				metrics: map[string]*assessment.Metric{"MyMetric": {}},
+				storage: testutil.NewInMemoryStorage(t, func(s persistence.Storage) {
+					_ = s.Create(&orchestrator.CloudService{Id: DefaultTargetCloudServiceId})
+				}),
+				metricConfigurations: map[string]map[string]*assessment.MetricConfiguration{},
+			},
+			args: args{
+				req: &orchestrator.UpdateMetricConfigurationRequest{ServiceId: DefaultTargetCloudServiceId, MetricId: "MyMetric", Configuration: &assessment.MetricConfiguration{Operator: "<"}},
+			},
+			wantErr: false,
+			wantRes: &assessment.MetricConfiguration{Operator: "<"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			svc := &Service{
+				metricConfigurations:  tt.fields.metricConfigurations,
+				results:               tt.fields.results,
+				AssessmentResultHooks: tt.fields.AssessmentResultHooks,
+				storage:               tt.fields.storage,
+				metrics:               tt.fields.metrics,
+				metricsFile:           tt.fields.metricsFile,
+				loadMetricsFunc:       tt.fields.loadMetricsFunc,
+				requirements:          tt.fields.requirements,
+				events:                tt.fields.events,
+			}
+			gotRes, err := svc.UpdateMetricConfiguration(tt.args.in0, tt.args.req)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Service.UpdateMetricConfiguration() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(gotRes, tt.wantRes) {
+				t.Errorf("Service.UpdateMetricConfiguration() = %v, want %v", gotRes, tt.wantRes)
+			}
+		})
+	}
+}