diff --git a/controllers/incarnation.go b/controllers/incarnation.go index 07bcfa72..7e90b9aa 100644 --- a/controllers/incarnation.go +++ b/controllers/incarnation.go @@ -479,48 +479,6 @@ func (i *Incarnation) deleteCanaryRules(ctx context.Context) error { }) } -func (i *Incarnation) syncTaggedServiceLevels(ctx context.Context) error { - if i.picchuConfig.ServiceLevelsNamespace != "" { - // Account for a fleet other than Delivery (old way of configuring SLOs) and Production (the only other place we ideally want SLOs to go) - err := i.controller.applyPlan( - ctx, - "Ensure Service Levels Namespace", - &rmplan.EnsureNamespace{Name: i.picchuConfig.ServiceLevelsNamespace}, - ) - if err != nil { - return err - } - return i.controller.applyPlan(ctx, "Sync Tagged Service Levels", &rmplan.SyncTaggedServiceLevels{ - App: i.appName(), - Target: i.targetName(), - Namespace: i.picchuConfig.ServiceLevelsNamespace, - Tag: i.tag, - Labels: i.defaultLabels(), - ServiceLevelObjectiveLabels: i.target().ServiceLevelObjectiveLabels, - ServiceLevelObjectives: i.target().SlothServiceLevelObjectives, - }) - } - i.log.Info("service-levels-fleet and service-levels-namespace not set, skipping SyncTaggedServiceLevels") - return nil -} - -func (i *Incarnation) deleteTaggedServiceLevels(ctx context.Context) error { - if i.picchuConfig.ServiceLevelsNamespace != "" { - return i.controller.applyPlan( - ctx, - "Delete Tagged Service Levels", - &rmplan.DeleteTaggedServiceLevels{ - App: i.appName(), - Target: i.targetName(), - Namespace: i.picchuConfig.ServiceLevelsNamespace, - Tag: i.tag, - }, - ) - } - i.log.Info("service-levels-fleet and service-levels-namespace not set, skipping DeleteTaggedServiceLevels") - return nil -} - func (i *Incarnation) genScalePlan(ctx context.Context) *rmplan.ScaleRevision { requestsRateTarget, err := i.target().Scale.TargetRequestsRateQuantity() if err != nil { diff --git a/controllers/plan/deleteTaggedServiceLevels.go b/controllers/plan/deleteTaggedServiceLevels.go deleted file mode 100644 index dafa0cb3..00000000 --- a/controllers/plan/deleteTaggedServiceLevels.go +++ /dev/null @@ -1,51 +0,0 @@ -package plan - -import ( - "context" - - "github.com/go-logr/logr" - slov1alpha1 "github.com/slok/sloth/pkg/kubernetes/api/sloth/v1" - picchuv1alpha1 "go.medium.engineering/picchu/api/v1alpha1" - "go.medium.engineering/picchu/plan" - "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/labels" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type DeleteTaggedServiceLevels struct { - App string - Target string - Namespace string - Tag string -} - -func (p *DeleteTaggedServiceLevels) Apply(ctx context.Context, cli client.Client, cluster *picchuv1alpha1.Cluster, log logr.Logger) error { - sllist := &slov1alpha1.PrometheusServiceLevelList{} - - opts := &client.ListOptions{ - Namespace: p.Namespace, - LabelSelector: labels.SelectorFromSet(map[string]string{ - picchuv1alpha1.LabelApp: p.App, - picchuv1alpha1.LabelTarget: p.Target, - picchuv1alpha1.LabelTag: p.Tag, - }), - } - - if err := cli.List(ctx, sllist, opts); err != nil { - log.Error(err, "Failed to delete tagged Service Levels") - return err - } - - for _, sl := range sllist.Items { - err := cli.Delete(ctx, &sl) - if err != nil && !errors.IsNotFound(err) { - plan.LogSync(log, "deleted", err, &sl) - return err - } - if err == nil { - plan.LogSync(log, "deleted", err, &sl) - } - } - - return nil -} diff --git a/controllers/plan/deleteTaggedServiceLevels_test.go b/controllers/plan/deleteTaggedServiceLevels_test.go deleted file mode 100644 index 8e4940a5..00000000 --- a/controllers/plan/deleteTaggedServiceLevels_test.go +++ /dev/null @@ -1,43 +0,0 @@ -package plan - -import ( - "context" - _ "runtime" - "testing" - - ktest "go.medium.engineering/kubernetes/pkg/test" - - slo "github.com/slok/sloth/pkg/kubernetes/api/sloth/v1" - testify "github.com/stretchr/testify/assert" - picchu "go.medium.engineering/picchu/api/v1alpha1" - "go.medium.engineering/picchu/test" - meta "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func TestDeleteTaggedServiceLevels(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - assert := testify.New(t) - log := test.MustNewLogger() - deleteTaggedServiceLevels := &DeleteTaggedServiceLevels{ - App: "testapp", - Namespace: "testnamespace", - Target: "target", - Tag: "v1", - } - sl := &slo.PrometheusServiceLevel{ - ObjectMeta: meta.ObjectMeta{ - Name: "test", - Namespace: "testnamespace", - Labels: map[string]string{ - picchu.LabelApp: deleteTaggedServiceLevels.App, - picchu.LabelTag: deleteTaggedServiceLevels.Tag, - picchu.LabelTarget: deleteTaggedServiceLevels.Target, - }, - }, - } - cli := fakeClient(sl) - - assert.NoError(deleteTaggedServiceLevels.Apply(ctx, cli, cluster, log), "Shouldn't return error.") - ktest.AssertNotFound(ctx, t, cli, sl) -} diff --git a/controllers/plan/syncTaggedServiceLevels.go b/controllers/plan/syncTaggedServiceLevels.go deleted file mode 100644 index 8eba310e..00000000 --- a/controllers/plan/syncTaggedServiceLevels.go +++ /dev/null @@ -1,90 +0,0 @@ -package plan - -import ( - "context" - "fmt" - - "github.com/go-logr/logr" - slov1alpha1 "github.com/slok/sloth/pkg/kubernetes/api/sloth/v1" - picchuv1alpha1 "go.medium.engineering/picchu/api/v1alpha1" - "go.medium.engineering/picchu/plan" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type SyncTaggedServiceLevels struct { - App string - Target string - Namespace string - Tag string - Labels map[string]string - ServiceLevelObjectiveLabels picchuv1alpha1.ServiceLevelObjectiveLabels - ServiceLevelObjectives []*picchuv1alpha1.SlothServiceLevelObjective -} - -func (p *SyncTaggedServiceLevels) Apply(ctx context.Context, cli client.Client, cluster *picchuv1alpha1.Cluster, log logr.Logger) error { - serviceLevels, err := p.serviceLevels(log) - if err != nil { - return err - } - if len(serviceLevels.Items) > 0 { - for i := range serviceLevels.Items { - if err := plan.CreateOrUpdate(ctx, log, cli, &serviceLevels.Items[i]); err != nil { - return err - } - } - } - - return nil -} - -func (p *SyncTaggedServiceLevels) serviceLevels(log logr.Logger) (*slov1alpha1.PrometheusServiceLevelList, error) { - sll := &slov1alpha1.PrometheusServiceLevelList{} - var sl []slov1alpha1.PrometheusServiceLevel - var slos []slov1alpha1.SLO - - for i := range p.ServiceLevelObjectives { - if p.ServiceLevelObjectives[i].Enabled { - config := SLOConfig{ - SLO: p.ServiceLevelObjectives[i], - App: p.App, - Name: sanitizeName(p.ServiceLevelObjectives[i].Name), - Tag: p.Tag, - Labels: p.ServiceLevelObjectiveLabels, - } - serviceLevelObjective := config.serviceLevelObjective(log) - - // if a grpc slo - if _, ok := p.ServiceLevelObjectives[i].ServiceLevelObjectiveLabels.ServiceLevelLabels["is_grpc"]; ok { - serviceLevelObjective.SLI.Events = config.taggedSLISourceGRPC() - } else { - serviceLevelObjective.SLI.Events = config.taggedSLISource() - } - - slos = append(slos, *serviceLevelObjective) - } - } - - if len(slos) > 0 { - serviceLevel := &slov1alpha1.PrometheusServiceLevel{ - ObjectMeta: metav1.ObjectMeta{ - Name: p.taggedServiceLevelName(), - Namespace: p.Namespace, - Labels: p.Labels, - }, - Spec: slov1alpha1.PrometheusServiceLevelSpec{ - Service: p.App, - SLOs: slos, - }, - } - sl = append(sl, *serviceLevel) - } - - sll.Items = sl - return sll, nil -} - -func (p *SyncTaggedServiceLevels) taggedServiceLevelName() string { - return fmt.Sprintf("%s-%s-%s-servicelevels", p.App, p.Target, p.Tag) -} diff --git a/controllers/plan/syncTaggedServiceLevels_test.go b/controllers/plan/syncTaggedServiceLevels_test.go deleted file mode 100644 index 364f5a1b..00000000 --- a/controllers/plan/syncTaggedServiceLevels_test.go +++ /dev/null @@ -1,176 +0,0 @@ -package plan - -import ( - "context" - _ "runtime" - "testing" - - picchuv1alpha1 "go.medium.engineering/picchu/api/v1alpha1" - "go.medium.engineering/picchu/mocks" - common "go.medium.engineering/picchu/plan/test" - "go.medium.engineering/picchu/test" - "sigs.k8s.io/controller-runtime/pkg/client" - - slov1alpha1 "github.com/slok/sloth/pkg/kubernetes/api/sloth/v1" - "github.com/stretchr/testify/assert" - "go.uber.org/mock/gomock" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" -) - -var ( - sltaggedplan = &SyncTaggedServiceLevels{ - App: "test-app", - Target: "production", - Namespace: "testnamespace", - Tag: "v1", - Labels: map[string]string{ - picchuv1alpha1.LabelApp: "test-app", - picchuv1alpha1.LabelTag: "v1", - picchuv1alpha1.LabelK8sName: "test-app", - picchuv1alpha1.LabelK8sVersion: "v1", - }, - ServiceLevelObjectiveLabels: picchuv1alpha1.ServiceLevelObjectiveLabels{ - ServiceLevelLabels: map[string]string{ - "severity": "test", - }, - }, - ServiceLevelObjectives: []*picchuv1alpha1.SlothServiceLevelObjective{ - { - Enabled: true, - Name: "test-app-availability", - Description: "test desc", - Objective: "99.999", - ServiceLevelIndicator: picchuv1alpha1.ServiceLevelIndicator{ - Canary: picchuv1alpha1.SLICanaryConfig{ - Enabled: true, - AllowancePercent: 1, - FailAfter: "1m", - }, - TagKey: "destination_workload", - AlertAfter: "1m", - ErrorQuery: "sum(rate(test_metric{job=\"test\"}[2m])) by (destination_workload)", - TotalQuery: "sum(rate(test_metric2{job=\"test\"}[2m])) by (destination_workload)", - }, - ServiceLevelObjectiveLabels: picchuv1alpha1.ServiceLevelObjectiveLabels{ - ServiceLevelLabels: map[string]string{ - "team": "test", - }, - }, - }, - { - Enabled: true, - Name: "test-app-availability-GRPC", - Description: "test desc", - Objective: "99.999", - ServiceLevelIndicator: picchuv1alpha1.ServiceLevelIndicator{ - Canary: picchuv1alpha1.SLICanaryConfig{ - Enabled: true, - AllowancePercent: 1, - FailAfter: "1m", - }, - TagKey: "destination_workload", - AlertAfter: "1m", - ErrorQuery: "sum(rate(test_metric{job=\"test\"}[2m])) by (destination_workload)", - TotalQuery: "sum(rate(test_metric2{job=\"test\"}[2m])) by (destination_workload)", - }, - ServiceLevelObjectiveLabels: picchuv1alpha1.ServiceLevelObjectiveLabels{ - ServiceLevelLabels: map[string]string{ - "team": "test", - // new label - "is_grpc": "true", - }, - }, - }, - }, - } - - sltaggedexpected = &slov1alpha1.PrometheusServiceLevelList{ - Items: []slov1alpha1.PrometheusServiceLevel{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-app-production-v1-servicelevels", - Namespace: "testnamespace", - Labels: map[string]string{ - picchuv1alpha1.LabelApp: "test-app", - picchuv1alpha1.LabelTag: "v1", - picchuv1alpha1.LabelK8sName: "test-app", - picchuv1alpha1.LabelK8sVersion: "v1", - }, - }, - Spec: slov1alpha1.PrometheusServiceLevelSpec{ - Service: "test-app", - SLOs: []slov1alpha1.SLO{ - { - Name: "test_app_availability", - Objective: 99.999, - Description: "test desc", - Labels: map[string]string{ - "severity": "test", - "team": "test", - "tag": "v1", - }, - SLI: slov1alpha1.SLI{ - Events: &slov1alpha1.SLIEvents{ - ErrorQuery: "sum(rate(test_app:test_app_availability:errors{destination_workload=\"v1\"}[{{.window}}]))", - TotalQuery: "sum(rate(test_app:test_app_availability:total{destination_workload=\"v1\"}[{{.window}}]))", - }, - }, - }, - { - Name: "test_app_availability_grpc", - Objective: 99.999, - Description: "test desc", - Labels: map[string]string{ - "severity": "test", - "team": "test", - "tag": "v1", - "is_grpc": "true", - }, - SLI: slov1alpha1.SLI{ - Events: &slov1alpha1.SLIEvents{ - ErrorQuery: "sum by (grpc_method) (rate(test_app:test_app_availability_grpc:errors{destination_workload=\"v1\"}[{{.window}}]))", - TotalQuery: "sum by (grpc_method) (rate(test_app:test_app_availability_grpc:total{destination_workload=\"v1\"}[{{.window}}]))", - }, - }, - }, - }, - }, - }, - }, - } -) - -func TestTaggedServiceLevels(t *testing.T) { - log := test.MustNewLogger() - ctrl := gomock.NewController(t) - m := mocks.NewMockClient(ctrl) - defer ctrl.Finish() - - tests := []client.ObjectKey{ - {Name: "test-app-production-v1-servicelevels", Namespace: "testnamespace"}, - } - ctx := context.TODO() - - for i := range tests { - m. - EXPECT(). - Get(ctx, mocks.ObjectKey(tests[i]), gomock.Any()). - Return(common.NotFoundError). - Times(1) - } - - for i := range sltaggedexpected.Items { - for _, obj := range []runtime.Object{ - &sltaggedexpected.Items[i], - } { - m. - EXPECT(). - Create(ctx, common.K8sEqual(obj)). - Return(nil). - AnyTimes() - } - } - - assert.NoError(t, sltaggedplan.Apply(ctx, m, cluster, log), "Shouldn't return error.") -} diff --git a/controllers/state.go b/controllers/state.go index 5ffff665..e5db2745 100644 --- a/controllers/state.go +++ b/controllers/state.go @@ -76,8 +76,6 @@ type Deployment interface { del(context.Context) error syncCanaryRules(context.Context) error deleteCanaryRules(context.Context) error - syncTaggedServiceLevels(context.Context) error - deleteTaggedServiceLevels(context.Context) error hasRevision() bool schedulePermitsRelease() bool markedAsFailed() bool @@ -360,9 +358,6 @@ func Releasing(ctx context.Context, deployment Deployment, lastUpdated *time.Tim if err := deployment.sync(ctx); err != nil { return releasing, err } - if err := deployment.syncTaggedServiceLevels(ctx); err != nil { - return releasing, err - } if deployment.peakPercent() >= 100 { return released, nil } @@ -386,9 +381,6 @@ func Retiring(ctx context.Context, deployment Deployment, lastUpdated *time.Time if deployment.isReleaseEligible() { return deploying, nil } - if err := deployment.deleteTaggedServiceLevels(ctx); err != nil { - return retiring, err - } if deployment.currentPercent() <= 0 { return retired, deployment.retire(ctx) } @@ -420,10 +412,6 @@ func Deleting(ctx context.Context, deployment Deployment, lastUpdated *time.Time return deleting, err } - if err := deployment.deleteTaggedServiceLevels(ctx); err != nil { - return deleting, err - } - if deployment.currentPercent() <= 0 { return deleted, deployment.del(ctx) } @@ -454,9 +442,6 @@ func Failing(ctx context.Context, deployment Deployment, lastUpdated *time.Time) if err := deployment.deleteCanaryRules(ctx); err != nil { return failing, err } - if err := deployment.deleteTaggedServiceLevels(ctx); err != nil { - return failing, err - } if deployment.currentPercent() <= 0 { return failed, deployment.retire(ctx) } @@ -483,9 +468,6 @@ func Canarying(ctx context.Context, deployment Deployment, lastUpdated *time.Tim if err := deployment.syncCanaryRules(ctx); err != nil { return canarying, err } - if err := deployment.syncTaggedServiceLevels(ctx); err != nil { - return canarying, err - } if err := deployment.sync(ctx); err != nil { return canarying, err diff --git a/controllers/state_test.go b/controllers/state_test.go index e3b79aee..bcae7184 100644 --- a/controllers/state_test.go +++ b/controllers/state_test.go @@ -533,8 +533,8 @@ func TestCanarying(t *tt.T) { testcase(deleting, m(false, false, true)) testcase(deleting, m(false, true, false)) testcase(deleting, m(false, true, true)) - testcase(canaried, expectSync(expectSyncTaggedServiceLevels(expectSyncCanaryRules(m(true, false, false))))) - testcase(canarying, expectSync(expectSyncTaggedServiceLevels(expectSyncCanaryRules(m(true, false, true))))) + testcase(canaried, expectSync(expectSyncCanaryRules(m(true, false, false)))) + testcase(canarying, expectSync(expectSyncCanaryRules(m(true, false, true)))) testcase(failing, m(true, true, false)) testcase(failing, m(true, true, true)) } @@ -770,7 +770,7 @@ func TestRetiring(t *tt.T) { testcase(retiring, m(true, true, false, 1)) testcase(retiring, m(true, true, true, 1)) - testcase(retired, expectRetire(expectDeleteTaggedServiceLevels(m(true, false, false, 0)))) + testcase(retired, expectRetire(m(true, false, false, 0))) testcase(retiring, m(true, false, false, 1)) testcase(retiring, m(true, false, false, 100)) @@ -826,7 +826,7 @@ func TestDeleting(t *tt.T) { testcase(deleting, m(false, 100)) testcase(deleting, m(false, 1)) - testcase(deleted, expectDeleteCanaryRules(expectDeleteTaggedServiceLevels(expectDelete(m(false, 0))))) + testcase(deleted, expectDeleteCanaryRules(expectDelete(m(false, 0)))) testcase(deploying, m(true, 0)) testcase(deleting, m(true, 100)) testcase(deleting, m(true, 1)) @@ -883,10 +883,10 @@ func TestFailing(t *tt.T) { testcase(deploying, m(true, false, ExternalTestSucceeded, 0)) testcase(failing, m(true, false, ExternalTestSucceeded, 100)) - testcase(failed, expectDeleteCanaryRules(expectDeleteTaggedServiceLevels(expectRetire(m(true, true, ExternalTestDisabled, 0))))) + testcase(failed, expectDeleteCanaryRules(expectRetire(m(true, true, ExternalTestDisabled, 0)))) testcase(failing, m(true, true, ExternalTestDisabled, 1)) testcase(failing, m(true, true, ExternalTestDisabled, 100)) - testcase(failed, expectDeleteCanaryRules(expectDeleteTaggedServiceLevels(expectRetire(m(true, false, ExternalTestFailed, 0))))) + testcase(failed, expectDeleteCanaryRules(expectRetire(m(true, false, ExternalTestFailed, 0)))) testcase(failing, m(true, false, ExternalTestFailed, 1)) testcase(failing, m(true, false, ExternalTestFailed, 100)) } @@ -949,22 +949,20 @@ func testHandler(ctx context.Context, t *tt.T, handler string, expected State, m } type responses struct { - hasRevision bool - markedAsFailed bool - isReleaseEligible bool - externalTestStatus ExternalTestStatus - isCanaryPending bool - datadogMonitoring bool - isDeployed bool - schedulePermitsRelease bool - currentPercent uint32 - peakPercent uint32 - syncCanaryRules error - deleteCanaryRules error - syncTaggedServiceLevels error - deleteTaggedServiceLevels error - isTimingOut bool - isExpired bool + hasRevision bool + markedAsFailed bool + isReleaseEligible bool + externalTestStatus ExternalTestStatus + isCanaryPending bool + datadogMonitoring bool + isDeployed bool + schedulePermitsRelease bool + currentPercent uint32 + peakPercent uint32 + syncCanaryRules error + deleteCanaryRules error + isTimingOut bool + isExpired bool } func createMockDeployment(ctrl *gomock.Controller, r responses) *MockDeployment { @@ -1083,21 +1081,3 @@ func expectDeleteCanaryRules(mock *MockDeployment) *MockDeployment { Times(1) return mock } - -func expectSyncTaggedServiceLevels(mock *MockDeployment) *MockDeployment { - mock. - EXPECT(). - syncTaggedServiceLevels(gomock.Any()). - Return(nil). - Times(1) - return mock -} - -func expectDeleteTaggedServiceLevels(mock *MockDeployment) *MockDeployment { - mock. - EXPECT(). - deleteTaggedServiceLevels(gomock.Any()). - Return(nil). - Times(1) - return mock -}