From 878718c2083216654b1b980c1905ddb963533ffa Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Fri, 28 Jun 2019 11:43:59 +0200 Subject: [PATCH 01/12] Add label filtering to CTS --- Gopkg.lock | 1 + .../testing_v1alpha1_clustertestsuite.yaml | 5 +- .../advanced/testsuite-select-by-labels.yaml | 14 ++ pkg/apis/testing/v1alpha1/testsuite_types.go | 7 +- .../testing/v1alpha1/zz_generated.deepcopy.go | 4 +- .../testsuite/testsuite_controller_test.go | 4 +- pkg/fetcher/definition.go | 55 ++++++- pkg/fetcher/definition_test.go | 150 +++++++++++++++++- 8 files changed, 222 insertions(+), 18 deletions(-) create mode 100644 config/samples/advanced/testsuite-select-by-labels.yaml diff --git a/Gopkg.lock b/Gopkg.lock index 7311a58..4b0f1f6 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1003,6 +1003,7 @@ "k8s.io/apimachinery/pkg/selection", "k8s.io/apimachinery/pkg/types", "k8s.io/apimachinery/pkg/util/rand", + "k8s.io/apimachinery/pkg/util/uuid", "k8s.io/client-go/kubernetes/scheme", "k8s.io/client-go/plugin/pkg/client/auth/gcp", "k8s.io/client-go/rest", diff --git a/config/crds/testing_v1alpha1_clustertestsuite.yaml b/config/crds/testing_v1alpha1_clustertestsuite.yaml index b287e19..bdfc97a 100644 --- a/config/crds/testing_v1alpha1_clustertestsuite.yaml +++ b/config/crds/testing_v1alpha1_clustertestsuite.yaml @@ -54,8 +54,9 @@ spec: all tests properties: matchLabels: - description: Find test definitions by it's labels. TestDefinition - should have AT LEAST one label listed here to be executed. + description: 'Find test definitions by its labels. TestDefinition + should match AT LEAST one expression listed here to be executed. + For a complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811' items: type: string type: array diff --git a/config/samples/advanced/testsuite-select-by-labels.yaml b/config/samples/advanced/testsuite-select-by-labels.yaml new file mode 100644 index 0000000..a7ddceb --- /dev/null +++ b/config/samples/advanced/testsuite-select-by-labels.yaml @@ -0,0 +1,14 @@ +--- +apiVersion: testing.kyma-project.io/v1alpha1 +kind: ClusterTestSuite +metadata: + labels: + controller-tools.k8s.io: "1.0" + name: testsuite-selected-by-labels +spec: + count: 1 + selectors: + matchLabelExpressions: + - "test-duration!=long" # do not execute long running tests + - "component in (frontend, backend)" # execute all tests for frontend and backend, regardless of their execution time. + diff --git a/pkg/apis/testing/v1alpha1/testsuite_types.go b/pkg/apis/testing/v1alpha1/testsuite_types.go index 10bb3f0..af67372 100644 --- a/pkg/apis/testing/v1alpha1/testsuite_types.go +++ b/pkg/apis/testing/v1alpha1/testsuite_types.go @@ -108,9 +108,10 @@ type TestSuiteSpec struct { type TestsSelector struct { // Find test definitions by it's name MatchNames []TestDefReference `json:"matchNames,omitempty"` - // Find test definitions by it's labels. - // TestDefinition should have AT LEAST one label listed here to be executed. - MatchLabels []string `json:"matchLabels,omitempty"` + // Find test definitions by its labels. + // TestDefinition should match AT LEAST one expression listed here to be executed. + // For a complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811 + MatchLabelExpressions []string `json:"matchLabels,omitempty"` } type TestDefReference struct { diff --git a/pkg/apis/testing/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/testing/v1alpha1/zz_generated.deepcopy.go index 4bd3d7e..a331b54 100644 --- a/pkg/apis/testing/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/testing/v1alpha1/zz_generated.deepcopy.go @@ -311,8 +311,8 @@ func (in *TestsSelector) DeepCopyInto(out *TestsSelector) { *out = make([]TestDefReference, len(*in)) copy(*out, *in) } - if in.MatchLabels != nil { - in, out := &in.MatchLabels, &out.MatchLabels + if in.MatchLabelExpressions != nil { + in, out := &in.MatchLabelExpressions, &out.MatchLabelExpressions *out = make([]string, len(*in)) copy(*out, *in) } diff --git a/pkg/controller/testsuite/testsuite_controller_test.go b/pkg/controller/testsuite/testsuite_controller_test.go index 33300e3..6060097 100644 --- a/pkg/controller/testsuite/testsuite_controller_test.go +++ b/pkg/controller/testsuite/testsuite_controller_test.go @@ -483,8 +483,8 @@ func (r *mockPodReconciler) getLogger() logr.Logger { func startMockPodController(mgr manager.Manager, enforceConcurrentlyRunningPods int) (*mockPodReconciler, error) { pr := &mockPodReconciler{ - cli: mgr.GetClient(), - mtx: sync.Mutex{}, + cli: mgr.GetClient(), + mtx: sync.Mutex{}, enforceNumberOfConcurrentlyRunningPods: enforceConcurrentlyRunningPods, reconcileAfter: time.Millisecond * 100, } diff --git a/pkg/fetcher/definition.go b/pkg/fetcher/definition.go index 346d070..1c7ec4e 100644 --- a/pkg/fetcher/definition.go +++ b/pkg/fetcher/definition.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/kyma-incubator/octopus/pkg/humanerr" k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "github.com/kyma-incubator/octopus/pkg/apis/testing/v1alpha1" @@ -22,12 +23,26 @@ type Definition struct { reader client.Reader } +type uniqueTestDefinitions map[types.UID]v1alpha1.TestDefinition + func (s *Definition) FindMatching(suite v1alpha1.ClusterTestSuite) ([]v1alpha1.TestDefinition, error) { ctx := context.TODO() - if len(suite.Spec.Selectors.MatchNames) > 0 { - return s.findByNames(ctx, suite) + acc := make(uniqueTestDefinitions) + + err := s.findByNames(ctx, suite, &acc) + if err != nil { + return nil, err } - // TODO(aszecowka) later so far we return all test definitions for all namespaces (https://github.com/kyma-incubator/octopus/issues/7) + + err = s.findByLabelExpressions(ctx, suite, &acc) + if err != nil { + return nil, err + } + + if len(acc) > 0 { + return acc.toList(), nil + } + var list v1alpha1.TestDefinitionList if err := s.reader.List(ctx, &client.ListOptions{Namespace: ""}, &list); err != nil { return nil, errors.Wrap(err, "while listing test definitions") @@ -35,8 +50,7 @@ func (s *Definition) FindMatching(suite v1alpha1.ClusterTestSuite) ([]v1alpha1.T return list.Items, nil } -func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTestSuite) ([]v1alpha1.TestDefinition, error) { - var list []v1alpha1.TestDefinition +func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc *uniqueTestDefinitions) error { for _, tRef := range suite.Spec.Selectors.MatchNames { def := v1alpha1.TestDefinition{} err := s.reader.Get(ctx, types.NamespacedName{Name: tRef.Name, Namespace: tRef.Namespace}, &def) @@ -44,11 +58,36 @@ func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTest switch { case err == nil: case k8serrors.IsNotFound(err): - return nil, humanerr.NewError(wrappedErr, fmt.Sprintf("Test Definition [name: %s, namespace: %s] does not exist", tRef.Name, tRef.Namespace)) + return humanerr.NewError(wrappedErr, fmt.Sprintf("Test Definition [name: %s, namespace: %s] does not exist", tRef.Name, tRef.Namespace)) default: - return nil, humanerr.NewError(wrappedErr, "Internal error") + return humanerr.NewError(wrappedErr, "Internal error") + } + (*acc)[def.UID] = def + } + return nil +} + +func (s *Definition) findByLabelExpressions(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc *uniqueTestDefinitions) error { + for _, expr := range suite.Spec.Selectors.MatchLabelExpressions { + selector, err := labels.Parse(expr) + if err != nil { + return errors.Wrapf(err, "while parsing label expression [expression: %s]", expr) + } + var list v1alpha1.TestDefinitionList + if err := s.reader.List(ctx, &client.ListOptions{LabelSelector: selector}, &list); err != nil { + return errors.Wrapf(err, "while fetching test definition from selector [expression: %s]", expr) + } + for _, def := range list.Items { + (*acc)[def.UID] = def } + } + return nil +} + +func (m uniqueTestDefinitions) toList() []v1alpha1.TestDefinition { + var list []v1alpha1.TestDefinition + for _, def := range m { list = append(list, def) } - return list, nil + return list } diff --git a/pkg/fetcher/definition_test.go b/pkg/fetcher/definition_test.go index 122b083..eed58d3 100644 --- a/pkg/fetcher/definition_test.go +++ b/pkg/fetcher/definition_test.go @@ -5,6 +5,7 @@ import ( "errors" "github.com/kyma-incubator/octopus/pkg/humanerr" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/uuid" "sigs.k8s.io/controller-runtime/pkg/client" "testing" @@ -41,18 +42,21 @@ func TestFindMatching(t *testing.T) { // GIVEN testA := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), Name: "test-a", Namespace: "test-a", }, } testB := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), Name: "test-b", Namespace: "test-b", }, } testC := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), Name: "test-c", Namespace: "test-c", }, @@ -87,6 +91,133 @@ func TestFindMatching(t *testing.T) { }) + t.Run("return tests selected by label expressions", func(t *testing.T) { + // GIVEN + testA := &v1alpha1.TestDefinition{ + ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), + Name: "test-a", + Namespace: "test-a", + Labels: map[string]string{ + "test": "true", + }, + }, + } + testB := &v1alpha1.TestDefinition{ + ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), + Name: "test-b", + Namespace: "test-b", + Labels: map[string]string{ + "test": "false", + }, + }, + } + testC := &v1alpha1.TestDefinition{ + ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), + Name: "test-c", + Namespace: "test-c", + Labels: map[string]string{ + "other": "123", + }, + }, + } + + fakeCli := fake.NewFakeClientWithScheme(sch, + testA, testB, testC, + ) + mockReader := &mockListReader{ + fakeCli: fakeCli, + listResults: [][]v1alpha1.TestDefinition{ + {*testC}, + {*testA}, + }, + } + service := fetcher.NewForDefinition(mockReader) + // WHEN + out, err := service.FindMatching(v1alpha1.ClusterTestSuite{ + Spec: v1alpha1.TestSuiteSpec{ + Selectors: v1alpha1.TestsSelector{ + MatchLabelExpressions: []string{ + "other", + "test=true", + }, + }, + }, + }) + // THEN + require.NoError(t, err) + assert.Len(t, out, 2) + assert.Contains(t, out, *testA) + assert.Contains(t, out, *testC) + }) + + t.Run("return tests returns unique results", func(t *testing.T) { + // GIVEN + testA := &v1alpha1.TestDefinition{ + ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), + Name: "test-a", + Namespace: "test-a", + Labels: map[string]string{ + "test": "true", + }, + }, + } + testB := &v1alpha1.TestDefinition{ + ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), + Name: "test-b", + Namespace: "test-b", + Labels: map[string]string{ + "test": "false", + }, + }, + } + testC := &v1alpha1.TestDefinition{ + ObjectMeta: v1.ObjectMeta{ + UID: uuid.NewUUID(), + Name: "test-c", + Namespace: "test-c", + Labels: map[string]string{ + "other": "123", + }, + }, + } + + fakeCli := fake.NewFakeClientWithScheme(sch, + testA, testB, testC, + ) + mockReader := &mockListReader{ + fakeCli: fakeCli, + listResults: [][]v1alpha1.TestDefinition{ + {*testA}, + }, + } + service := fetcher.NewForDefinition(mockReader) + // WHEN + out, err := service.FindMatching(v1alpha1.ClusterTestSuite{ + Spec: v1alpha1.TestSuiteSpec{ + Selectors: v1alpha1.TestsSelector{ + MatchNames: []v1alpha1.TestDefReference{ + { + Name: "test-a", + Namespace: "test-a", + }, + }, + MatchLabelExpressions: []string{ + "test=true", + }, + }, + }, + }) + // THEN + require.NoError(t, err) + assert.Len(t, out, 1) + assert.Contains(t, out, *testA) + }) + t.Run("return error if test selected by name does not exist", func(t *testing.T) { // GIVEN fakeCli := fake.NewFakeClientWithScheme(sch) @@ -113,7 +244,7 @@ func TestFindMatching(t *testing.T) { t.Run("return internal error when fetching selected tests failed", func(t *testing.T) { // GIVEN - errClient := &mockErrReader{err:errors.New("some error")} + errClient := &mockErrReader{err: errors.New("some error")} service := fetcher.NewForDefinition(errClient) // WHEN @@ -150,3 +281,20 @@ func (m *mockErrReader) Get(ctx context.Context, key client.ObjectKey, obj runti func (m *mockErrReader) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { return m.err } + +type mockListReader struct { + fakeCli client.Reader + listResults [][]v1alpha1.TestDefinition + calls uint +} + +func (m *mockListReader) Get(ctx context.Context, key client.ObjectKey, obj runtime.Object) error { + return m.fakeCli.Get(ctx, key, obj) +} + +func (m *mockListReader) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { + result := m.listResults[m.calls] + m.calls++ + list.(*v1alpha1.TestDefinitionList).Items = append(list.(*v1alpha1.TestDefinitionList).Items, result...) + return nil +} From 4f424aa216913659d714eb40236a1981f40309f1 Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Fri, 28 Jun 2019 11:53:32 +0200 Subject: [PATCH 02/12] Bunch of fixes --- pkg/apis/testing/v1alpha1/testsuite_types.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/apis/testing/v1alpha1/testsuite_types.go b/pkg/apis/testing/v1alpha1/testsuite_types.go index af67372..a3480bd 100644 --- a/pkg/apis/testing/v1alpha1/testsuite_types.go +++ b/pkg/apis/testing/v1alpha1/testsuite_types.go @@ -108,10 +108,10 @@ type TestSuiteSpec struct { type TestsSelector struct { // Find test definitions by it's name MatchNames []TestDefReference `json:"matchNames,omitempty"` - // Find test definitions by its labels. - // TestDefinition should match AT LEAST one expression listed here to be executed. - // For a complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811 - MatchLabelExpressions []string `json:"matchLabels,omitempty"` + // Find test definitions by their labels. + // TestDefinition must match AT LEAST one expression listed here to be executed. + // For the complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811 + MatchLabelExpressions []string `json:"matchLabelExpressions,omitempty"` } type TestDefReference struct { From 9c1802c530bdbb45962a80a5365e9e7fc304dc98 Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Fri, 28 Jun 2019 11:54:43 +0200 Subject: [PATCH 03/12] Generate CRD --- config/crds/testing_v1alpha1_clustertestsuite.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config/crds/testing_v1alpha1_clustertestsuite.yaml b/config/crds/testing_v1alpha1_clustertestsuite.yaml index bdfc97a..f0bf55b 100644 --- a/config/crds/testing_v1alpha1_clustertestsuite.yaml +++ b/config/crds/testing_v1alpha1_clustertestsuite.yaml @@ -53,10 +53,10 @@ spec: description: Decide which tests to execute. If not provided execute all tests properties: - matchLabels: - description: 'Find test definitions by its labels. TestDefinition - should match AT LEAST one expression listed here to be executed. - For a complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811' + matchLabelExpressions: + description: 'Find test definitions by their labels. TestDefinition + must match AT LEAST one expression listed here to be executed. + For the complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811' items: type: string type: array From e456d44aa62b6b4266fe459f8999b0c15aff506a Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Fri, 28 Jun 2019 12:00:35 +0200 Subject: [PATCH 04/12] Remove unnecessary pointer, rename toList --- pkg/fetcher/definition.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/fetcher/definition.go b/pkg/fetcher/definition.go index 1c7ec4e..b47bd52 100644 --- a/pkg/fetcher/definition.go +++ b/pkg/fetcher/definition.go @@ -29,18 +29,18 @@ func (s *Definition) FindMatching(suite v1alpha1.ClusterTestSuite) ([]v1alpha1.T ctx := context.TODO() acc := make(uniqueTestDefinitions) - err := s.findByNames(ctx, suite, &acc) + err := s.findByNames(ctx, suite, acc) if err != nil { return nil, err } - err = s.findByLabelExpressions(ctx, suite, &acc) + err = s.findByLabelExpressions(ctx, suite, acc) if err != nil { return nil, err } if len(acc) > 0 { - return acc.toList(), nil + return acc.getValues(), nil } var list v1alpha1.TestDefinitionList @@ -50,7 +50,7 @@ func (s *Definition) FindMatching(suite v1alpha1.ClusterTestSuite) ([]v1alpha1.T return list.Items, nil } -func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc *uniqueTestDefinitions) error { +func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc uniqueTestDefinitions) error { for _, tRef := range suite.Spec.Selectors.MatchNames { def := v1alpha1.TestDefinition{} err := s.reader.Get(ctx, types.NamespacedName{Name: tRef.Name, Namespace: tRef.Namespace}, &def) @@ -62,12 +62,12 @@ func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTest default: return humanerr.NewError(wrappedErr, "Internal error") } - (*acc)[def.UID] = def + acc[def.UID] = def } return nil } -func (s *Definition) findByLabelExpressions(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc *uniqueTestDefinitions) error { +func (s *Definition) findByLabelExpressions(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc uniqueTestDefinitions) error { for _, expr := range suite.Spec.Selectors.MatchLabelExpressions { selector, err := labels.Parse(expr) if err != nil { @@ -78,13 +78,13 @@ func (s *Definition) findByLabelExpressions(ctx context.Context, suite v1alpha1. return errors.Wrapf(err, "while fetching test definition from selector [expression: %s]", expr) } for _, def := range list.Items { - (*acc)[def.UID] = def + acc[def.UID] = def } } return nil } -func (m uniqueTestDefinitions) toList() []v1alpha1.TestDefinition { +func (m uniqueTestDefinitions) getValues() []v1alpha1.TestDefinition { var list []v1alpha1.TestDefinition for _, def := range m { list = append(list, def) From baa2a37eab0dd14c8745eba3bc2e00bc6f71799f Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Tue, 2 Jul 2019 10:29:52 +0200 Subject: [PATCH 05/12] Doc changes after review --- config/crds/testing_v1alpha1_clustertestsuite.yaml | 2 +- config/samples/advanced/testsuite-select-by-labels.yaml | 5 +++-- pkg/apis/testing/v1alpha1/testsuite_types.go | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/config/crds/testing_v1alpha1_clustertestsuite.yaml b/config/crds/testing_v1alpha1_clustertestsuite.yaml index f0bf55b..8ae8d9f 100644 --- a/config/crds/testing_v1alpha1_clustertestsuite.yaml +++ b/config/crds/testing_v1alpha1_clustertestsuite.yaml @@ -56,7 +56,7 @@ spec: matchLabelExpressions: description: 'Find test definitions by their labels. TestDefinition must match AT LEAST one expression listed here to be executed. - For the complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811' + For the complete grammar see: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels' items: type: string type: array diff --git a/config/samples/advanced/testsuite-select-by-labels.yaml b/config/samples/advanced/testsuite-select-by-labels.yaml index a7ddceb..f47a598 100644 --- a/config/samples/advanced/testsuite-select-by-labels.yaml +++ b/config/samples/advanced/testsuite-select-by-labels.yaml @@ -9,6 +9,7 @@ spec: count: 1 selectors: matchLabelExpressions: - - "test-duration!=long" # do not execute long running tests - - "component in (frontend, backend)" # execute all tests for frontend and backend, regardless of their execution time. + # This example executes all not long tests for frontend and all tests for backend + - component=frontend,test-duration!=long + - component=backend diff --git a/pkg/apis/testing/v1alpha1/testsuite_types.go b/pkg/apis/testing/v1alpha1/testsuite_types.go index a3480bd..6c75107 100644 --- a/pkg/apis/testing/v1alpha1/testsuite_types.go +++ b/pkg/apis/testing/v1alpha1/testsuite_types.go @@ -110,7 +110,7 @@ type TestsSelector struct { MatchNames []TestDefReference `json:"matchNames,omitempty"` // Find test definitions by their labels. // TestDefinition must match AT LEAST one expression listed here to be executed. - // For the complete grammar see: https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go#L811 + // For the complete grammar see: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels MatchLabelExpressions []string `json:"matchLabelExpressions,omitempty"` } From 32cdd93ee0f34ecbd5c01f7bc35f4041c1490305 Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Tue, 2 Jul 2019 10:58:19 +0200 Subject: [PATCH 06/12] Refactor definition fetching --- pkg/apis/testing/v1alpha1/testsuite_types.go | 4 ++ pkg/fetcher/definition.go | 73 ++++++++++++-------- 2 files changed, 47 insertions(+), 30 deletions(-) diff --git a/pkg/apis/testing/v1alpha1/testsuite_types.go b/pkg/apis/testing/v1alpha1/testsuite_types.go index 6c75107..dee8098 100644 --- a/pkg/apis/testing/v1alpha1/testsuite_types.go +++ b/pkg/apis/testing/v1alpha1/testsuite_types.go @@ -158,3 +158,7 @@ type TestExecution struct { func init() { SchemeBuilder.Register(&ClusterTestSuite{}, &ClusterTestSuiteList{}) } + +func (in ClusterTestSuite) HasSelector() bool { + return len(s.Spec.Selectors.MatchNames) > 0 || len(s.Spec.Selectors.MatchLabelExpressions) > 0 +} diff --git a/pkg/fetcher/definition.go b/pkg/fetcher/definition.go index b47bd52..734245c 100644 --- a/pkg/fetcher/definition.go +++ b/pkg/fetcher/definition.go @@ -23,34 +23,32 @@ type Definition struct { reader client.Reader } -type uniqueTestDefinitions map[types.UID]v1alpha1.TestDefinition - func (s *Definition) FindMatching(suite v1alpha1.ClusterTestSuite) ([]v1alpha1.TestDefinition, error) { ctx := context.TODO() - acc := make(uniqueTestDefinitions) - err := s.findByNames(ctx, suite, acc) - if err != nil { - return nil, err + if suite.HasSelector() { + return s.findBySelector(ctx, suite) } - err = s.findByLabelExpressions(ctx, suite, acc) + return s.findAll(ctx, suite) +} + +func (s *Definition) findBySelector(ctx context.Context, suite v1alpha1.ClusterTestSuite) ([]v1alpha1.TestDefinition, error) { + byNames, err := s.findByNames(ctx, suite) if err != nil { return nil, err } - if len(acc) > 0 { - return acc.getValues(), nil + byLabelExpressions, err := s.findByLabelExpressions(ctx, suite) + if err != nil { + return nil, err } - var list v1alpha1.TestDefinitionList - if err := s.reader.List(ctx, &client.ListOptions{Namespace: ""}, &list); err != nil { - return nil, errors.Wrap(err, "while listing test definitions") - } - return list.Items, nil + return s.unique(byNames, byLabelExpressions), nil } -func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc uniqueTestDefinitions) error { +func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTestSuite) ([]v1alpha1.TestDefinition, error) { + result := make([]v1alpha1.TestDefinition, 0) for _, tRef := range suite.Spec.Selectors.MatchNames { def := v1alpha1.TestDefinition{} err := s.reader.Get(ctx, types.NamespacedName{Name: tRef.Name, Namespace: tRef.Namespace}, &def) @@ -58,36 +56,51 @@ func (s *Definition) findByNames(ctx context.Context, suite v1alpha1.ClusterTest switch { case err == nil: case k8serrors.IsNotFound(err): - return humanerr.NewError(wrappedErr, fmt.Sprintf("Test Definition [name: %s, namespace: %s] does not exist", tRef.Name, tRef.Namespace)) + return nil, humanerr.NewError(wrappedErr, fmt.Sprintf("Test Definition [name: %s, namespace: %s] does not exist", tRef.Name, tRef.Namespace)) default: - return humanerr.NewError(wrappedErr, "Internal error") + return nil, humanerr.NewError(wrappedErr, "Internal error") } - acc[def.UID] = def + result = append(result, def) } - return nil + return result, nil } -func (s *Definition) findByLabelExpressions(ctx context.Context, suite v1alpha1.ClusterTestSuite, acc uniqueTestDefinitions) error { +func (s *Definition) findByLabelExpressions(ctx context.Context, suite v1alpha1.ClusterTestSuite) ([]v1alpha1.TestDefinition, error) { + result := make([]v1alpha1.TestDefinition, 0) for _, expr := range suite.Spec.Selectors.MatchLabelExpressions { selector, err := labels.Parse(expr) if err != nil { - return errors.Wrapf(err, "while parsing label expression [expression: %s]", expr) + return nil, errors.Wrapf(err, "while parsing label expression [expression: %s]", expr) } var list v1alpha1.TestDefinitionList if err := s.reader.List(ctx, &client.ListOptions{LabelSelector: selector}, &list); err != nil { - return errors.Wrapf(err, "while fetching test definition from selector [expression: %s]", expr) + return nil, errors.Wrapf(err, "while fetching test definition from selector [expression: %s]", expr) } - for _, def := range list.Items { - acc[def.UID] = def + result = append(result, list.Items...) + } + return result, nil +} + +func (s *Definition) unique(slices ...[]v1alpha1.TestDefinition) []v1alpha1.TestDefinition { + unique := make(map[types.UID]v1alpha1.TestDefinition) + for _, slice := range slices { + for _, td := range slice { + unique[td.UID] = td } } - return nil + + result := make([]v1alpha1.TestDefinition, 0, len(unique)) + for _, td := range unique { + result = append(result, td) + } + + return result } -func (m uniqueTestDefinitions) getValues() []v1alpha1.TestDefinition { - var list []v1alpha1.TestDefinition - for _, def := range m { - list = append(list, def) +func (s *Definition) findAll(ctx context.Context, suite v1alpha1.ClusterTestSuite) ([]v1alpha1.TestDefinition, error) { + var list v1alpha1.TestDefinitionList + if err := s.reader.List(ctx, &client.ListOptions{Namespace: ""}, &list); err != nil { + return nil, errors.Wrap(err, "while listing test definitions") } - return list + return list.Items, nil } From 7e1f6058eb2fafb4e984a75a4016118ee0df37e0 Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Tue, 2 Jul 2019 11:00:19 +0200 Subject: [PATCH 07/12] Get rid of uid package from tests --- pkg/fetcher/definition_test.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/fetcher/definition_test.go b/pkg/fetcher/definition_test.go index eed58d3..6b5d5c1 100644 --- a/pkg/fetcher/definition_test.go +++ b/pkg/fetcher/definition_test.go @@ -5,7 +5,6 @@ import ( "errors" "github.com/kyma-incubator/octopus/pkg/humanerr" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/uuid" "sigs.k8s.io/controller-runtime/pkg/client" "testing" @@ -42,21 +41,21 @@ func TestFindMatching(t *testing.T) { // GIVEN testA := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-a", Namespace: "test-a", }, } testB := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-b", Namespace: "test-b", }, } testC := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-c", Namespace: "test-c", }, @@ -95,7 +94,7 @@ func TestFindMatching(t *testing.T) { // GIVEN testA := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-a", Namespace: "test-a", Labels: map[string]string{ @@ -105,7 +104,7 @@ func TestFindMatching(t *testing.T) { } testB := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-b", Namespace: "test-b", Labels: map[string]string{ @@ -115,7 +114,7 @@ func TestFindMatching(t *testing.T) { } testC := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-c", Namespace: "test-c", Labels: map[string]string{ @@ -157,7 +156,7 @@ func TestFindMatching(t *testing.T) { // GIVEN testA := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-a", Namespace: "test-a", Labels: map[string]string{ @@ -167,7 +166,7 @@ func TestFindMatching(t *testing.T) { } testB := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-b", Namespace: "test-b", Labels: map[string]string{ @@ -177,7 +176,7 @@ func TestFindMatching(t *testing.T) { } testC := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), + UID: "test-uid", Name: "test-c", Namespace: "test-c", Labels: map[string]string{ From 81f59d1b559ea5e1c087ba8589e37b7429f4ef09 Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Tue, 2 Jul 2019 11:10:57 +0200 Subject: [PATCH 08/12] Document matchLabelExpressions --- docs/crd-cluster-test-suite.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/crd-cluster-test-suite.md b/docs/crd-cluster-test-suite.md index e1b3f1a..704b8e4 100644 --- a/docs/crd-cluster-test-suite.md +++ b/docs/crd-cluster-test-suite.md @@ -31,7 +31,7 @@ This table lists all the possible parameters of a given resource together with t | **metadata.name** | **YES** | Specifies the name of the CR. | | **spec.selectors** | **NO** | Defines which tests should be executed. You can define tests by specifying their names or labels. Selectors are additive. If not defined, all tests from all Namespaces are executed. | **spec.selectors.matchNames** | **NO** | Lists TestDefinitions to execute. For every element on the list, specify **name** and **namespace** that refers to a TestDefinition. | -| **spec.selectors.matchLabels** | **NO** | Lists labels that match labels of TestDefinitions to execute. A TestDefinition is selected if at least one label matches. This feature is not yet implemented. | +| **spec.selectors.matchLabelExpressions** | **NO** | Lists of label expressions that match labels of TestDefinitions to execute. A TestDefinition is selected if at least one label expression matches. For the complete grammar see: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels | | **spec.concurrency** | **NO** | Defines how many tests can be executed at the same time, which depends on cluster size and its load. The default value is `1`. | **spec.suiteTimeout** | **NO** | Defines the maximal suite duration after which test executions are interrupted and marked as **Failed**. The default value is one hour. This feature is not yet implemented. | **spec.count** | **NO** | Defines how many times every test should be executed. **Spec.Count** and **Spec.MaxRetries** are mutually exclusive. The default value is `1`. From b99192a283d80e7e2e63f3d1737828096309f759 Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Tue, 2 Jul 2019 11:43:12 +0200 Subject: [PATCH 09/12] Fix definition fetcher tests --- Gopkg.lock | 1 - pkg/apis/testing/v1alpha1/testsuite_types.go | 2 +- pkg/fetcher/definition_test.go | 80 ++++++++------------ 3 files changed, 33 insertions(+), 50 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 4b0f1f6..7311a58 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1003,7 +1003,6 @@ "k8s.io/apimachinery/pkg/selection", "k8s.io/apimachinery/pkg/types", "k8s.io/apimachinery/pkg/util/rand", - "k8s.io/apimachinery/pkg/util/uuid", "k8s.io/client-go/kubernetes/scheme", "k8s.io/client-go/plugin/pkg/client/auth/gcp", "k8s.io/client-go/rest", diff --git a/pkg/apis/testing/v1alpha1/testsuite_types.go b/pkg/apis/testing/v1alpha1/testsuite_types.go index dee8098..3dee443 100644 --- a/pkg/apis/testing/v1alpha1/testsuite_types.go +++ b/pkg/apis/testing/v1alpha1/testsuite_types.go @@ -160,5 +160,5 @@ func init() { } func (in ClusterTestSuite) HasSelector() bool { - return len(s.Spec.Selectors.MatchNames) > 0 || len(s.Spec.Selectors.MatchLabelExpressions) > 0 + return len(in.Spec.Selectors.MatchNames) > 0 || len(in.Spec.Selectors.MatchLabelExpressions) > 0 } diff --git a/pkg/fetcher/definition_test.go b/pkg/fetcher/definition_test.go index 6b5d5c1..0ab138c 100644 --- a/pkg/fetcher/definition_test.go +++ b/pkg/fetcher/definition_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "github.com/kyma-incubator/octopus/pkg/humanerr" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "testing" @@ -41,21 +42,21 @@ func TestFindMatching(t *testing.T) { // GIVEN testA := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", + UID: "test-uid-a", Name: "test-a", Namespace: "test-a", }, } testB := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", + UID: "test-uid-b", Name: "test-b", Namespace: "test-b", }, } testC := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", + UID: "test-uid-c", Name: "test-c", Namespace: "test-c", }, @@ -94,7 +95,7 @@ func TestFindMatching(t *testing.T) { // GIVEN testA := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", + UID: "test-uid-a", Name: "test-a", Namespace: "test-a", Labels: map[string]string{ @@ -104,7 +105,7 @@ func TestFindMatching(t *testing.T) { } testB := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", + UID: "test-uid-b", Name: "test-b", Namespace: "test-b", Labels: map[string]string{ @@ -114,7 +115,7 @@ func TestFindMatching(t *testing.T) { } testC := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", + UID: "test-uid-c", Name: "test-c", Namespace: "test-c", Labels: map[string]string{ @@ -126,13 +127,8 @@ func TestFindMatching(t *testing.T) { fakeCli := fake.NewFakeClientWithScheme(sch, testA, testB, testC, ) - mockReader := &mockListReader{ - fakeCli: fakeCli, - listResults: [][]v1alpha1.TestDefinition{ - {*testC}, - {*testA}, - }, - } + mockReader := &mockListReader{fakeCli: fakeCli} + service := fetcher.NewForDefinition(mockReader) // WHEN out, err := service.FindMatching(v1alpha1.ClusterTestSuite{ @@ -152,11 +148,11 @@ func TestFindMatching(t *testing.T) { assert.Contains(t, out, *testC) }) - t.Run("return tests returns unique results", func(t *testing.T) { + t.Run("return tests returns unique result across all selectors", func(t *testing.T) { // GIVEN testA := &v1alpha1.TestDefinition{ ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", + UID: "test-uid-a", Name: "test-a", Namespace: "test-a", Labels: map[string]string{ @@ -164,36 +160,11 @@ func TestFindMatching(t *testing.T) { }, }, } - testB := &v1alpha1.TestDefinition{ - ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", - Name: "test-b", - Namespace: "test-b", - Labels: map[string]string{ - "test": "false", - }, - }, - } - testC := &v1alpha1.TestDefinition{ - ObjectMeta: v1.ObjectMeta{ - UID: "test-uid", - Name: "test-c", - Namespace: "test-c", - Labels: map[string]string{ - "other": "123", - }, - }, - } fakeCli := fake.NewFakeClientWithScheme(sch, - testA, testB, testC, + testA, ) - mockReader := &mockListReader{ - fakeCli: fakeCli, - listResults: [][]v1alpha1.TestDefinition{ - {*testA}, - }, - } + mockReader := &mockListReader{fakeCli: fakeCli} service := fetcher.NewForDefinition(mockReader) // WHEN out, err := service.FindMatching(v1alpha1.ClusterTestSuite{ @@ -282,9 +253,7 @@ func (m *mockErrReader) List(ctx context.Context, opts *client.ListOptions, list } type mockListReader struct { - fakeCli client.Reader - listResults [][]v1alpha1.TestDefinition - calls uint + fakeCli client.Reader } func (m *mockListReader) Get(ctx context.Context, key client.ObjectKey, obj runtime.Object) error { @@ -292,8 +261,23 @@ func (m *mockListReader) Get(ctx context.Context, key client.ObjectKey, obj runt } func (m *mockListReader) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { - result := m.listResults[m.calls] - m.calls++ - list.(*v1alpha1.TestDefinitionList).Items = append(list.(*v1alpha1.TestDefinitionList).Items, result...) + // fakeCli has a bug fixed in controller-runtime 0.1.11 and it does not filter by labels. This mock can be removed + // when we update to new controller-runtime + // See: https://github.com/kubernetes-sigs/controller-runtime/issues/293 + if opts.LabelSelector == nil { + return m.fakeCli.List(ctx, opts, list) + } + + result := v1alpha1.TestDefinitionList{} + err := m.fakeCli.List(ctx, opts, &result) + if err != nil { + return err + } + + for _, td := range result.Items { + if opts.LabelSelector.Matches(labels.Set(td.Labels)) { + list.(*v1alpha1.TestDefinitionList).Items = append(list.(*v1alpha1.TestDefinitionList).Items, td) + } + } return nil } From 38d6204348d44ab968c99255621d55935203e4ef Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Tue, 2 Jul 2019 12:02:50 +0200 Subject: [PATCH 10/12] Add integration test for selective testing --- .../testsuite/testsuite_controller_test.go | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/pkg/controller/testsuite/testsuite_controller_test.go b/pkg/controller/testsuite/testsuite_controller_test.go index 6060097..3b9c733 100644 --- a/pkg/controller/testsuite/testsuite_controller_test.go +++ b/pkg/controller/testsuite/testsuite_controller_test.go @@ -330,6 +330,91 @@ func TestReconcileClusterTestSuite(t *testing.T) { }) + t.Run("selective testing", func(t *testing.T) { + // GIVEN + // Setup the Manager and Controller + mgr, err := manager.New(cfg, manager.Options{}) + require.NoError(t, err) + c := mgr.GetClient() + + testNs := generateTestNs() + ctx := context.Background() + + require.NoError(t, add(mgr, newReconciler(mgr))) + stopMgr, mgrStopped := StartTestManager(t, mgr) + + defer func() { + close(stopMgr) + mgrStopped.Wait() + }() + + logf.SetLogger(logf.ZapLogger(false)) + + podReconciler, err := startMockPodController(mgr, 0) + require.NoError(t, err) + + // WHEN + ns := &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: testNs, + }, + } + err = c.Create(ctx, ns) + require.NoError(t, err) + defer cleanupK8sObject(ctx, c, ns) + + testA := getSequentialTest("test-a", testNs) + err = c.Create(ctx, testA) + require.NoError(t, err) + defer cleanupK8sObject(ctx, c, testA) + + testB := getSequentialTest("test-b", testNs) + testB.Labels = map[string]string{"test": "true"} + err = c.Create(ctx, testB) + require.NoError(t, err) + defer cleanupK8sObject(ctx, c, testB) + + testC := getSequentialTest("test-c", testNs) + testC.Labels = map[string]string{"test": "false"} + err = c.Create(ctx, testC) + require.NoError(t, err) + defer cleanupK8sObject(ctx, c, testC) + + suite := &testingv1alpha1.ClusterTestSuite{ + ObjectMeta: metav1.ObjectMeta{Name: "suite-selective"}, + Spec: testingv1alpha1.TestSuiteSpec{ + Concurrency: 1, + Count: 1, + Selectors: testingv1alpha1.TestsSelector{ + MatchNames: []testingv1alpha1.TestDefReference{ + { + Name: "test-a", + Namespace: testNs, + }, + }, + MatchLabelExpressions: []string{ + "test=true", + }, + }, + }, + } + err = c.Create(ctx, suite) + require.NoError(t, err) + defer cleanupK8sObject(ctx, c, suite) + + // THEN + repeat.FuncAtMost(t, func() error { + return checkIfsuiteIsSucceeded(ctx, c, "suite-selective") + }, defaultAssertionTimeout) + + repeat.FuncAtMost(t, func() error { + return checkIfPodsWereCreated(ctx, c, testNs, []string{ + "oct-tp-suite-selective-test-a-0", + "oct-tp-suite-selective-test-b-0"}) + }, defaultAssertionTimeout) + + assertThatPodsCreatedSequentially(t, podReconciler.getAppliedChanges()) + }) } func assertThatPodsCreatedConcurrently(t *testing.T, appliedChanges []podStatusChanges) { From 1d06f497c9855757a3c9725abc51e5e475dc8e7c Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Tue, 2 Jul 2019 12:52:50 +0200 Subject: [PATCH 11/12] Typo --- pkg/controller/testsuite/testsuite_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/testsuite/testsuite_controller_test.go b/pkg/controller/testsuite/testsuite_controller_test.go index 3b9c733..c544f68 100644 --- a/pkg/controller/testsuite/testsuite_controller_test.go +++ b/pkg/controller/testsuite/testsuite_controller_test.go @@ -503,7 +503,7 @@ type mockPodReconciler struct { cli client.Client // mutex protecting access to applied changes slice mtx sync.Mutex - // chronologically list of pod changes. Use getter to access it autside the struct + // chronologically list of pod changes. Use getter to access it outside the struct appliedChanges []podStatusChanges enforceNumberOfConcurrentlyRunningPods int reconcileAfter time.Duration From 6238579e64a34bfad43e53595b8790c7dcd9df5a Mon Sep 17 00:00:00 2001 From: Szymon Janota Date: Thu, 4 Jul 2019 09:52:38 +0200 Subject: [PATCH 12/12] Update docs/crd-cluster-test-suite.md Co-Authored-By: Klaudia Grzondziel <35192450+klaudiagrz@users.noreply.github.com> --- docs/crd-cluster-test-suite.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/crd-cluster-test-suite.md b/docs/crd-cluster-test-suite.md index 704b8e4..95c56df 100644 --- a/docs/crd-cluster-test-suite.md +++ b/docs/crd-cluster-test-suite.md @@ -31,7 +31,7 @@ This table lists all the possible parameters of a given resource together with t | **metadata.name** | **YES** | Specifies the name of the CR. | | **spec.selectors** | **NO** | Defines which tests should be executed. You can define tests by specifying their names or labels. Selectors are additive. If not defined, all tests from all Namespaces are executed. | **spec.selectors.matchNames** | **NO** | Lists TestDefinitions to execute. For every element on the list, specify **name** and **namespace** that refers to a TestDefinition. | -| **spec.selectors.matchLabelExpressions** | **NO** | Lists of label expressions that match labels of TestDefinitions to execute. A TestDefinition is selected if at least one label expression matches. For the complete grammar see: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels | +| **spec.selectors.matchLabelExpressions** | **NO** | Lists label expressions that match labels of TestDefinitions to execute. A TestDefinition is selected if at least one label expression matches. See [this](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels) document for more details. | | **spec.concurrency** | **NO** | Defines how many tests can be executed at the same time, which depends on cluster size and its load. The default value is `1`. | **spec.suiteTimeout** | **NO** | Defines the maximal suite duration after which test executions are interrupted and marked as **Failed**. The default value is one hour. This feature is not yet implemented. | **spec.count** | **NO** | Defines how many times every test should be executed. **Spec.Count** and **Spec.MaxRetries** are mutually exclusive. The default value is `1`.