From 11e58fcc41dbc47806a4a5143253cc3f34bc1d22 Mon Sep 17 00:00:00 2001 From: Hiroshi Muraoka Date: Fri, 5 Mar 2021 10:15:39 +0900 Subject: [PATCH] Manage runner with label (#355) * Update RunnerDeploymentSpec to have Selector field Signed-off-by: Hiroshi Muraoka * Update RunnerReplicaSetSpec to have Selector field Signed-off-by: Hiroshi Muraoka * Add CloneSelectorAndAddLabel to add Selector field Signed-off-by: Hiroshi Muraoka * Fix tests Signed-off-by: Hiroshi Muraoka * Use label to find RunnerReplicaSet/Runner Signed-off-by: binoue * Update controller-gen versions in CRD Signed-off-by: Hiroshi Muraoka * Update autoscaler to list Pods with labels Signed-off-by: Hiroshi Muraoka * Add debug log Signed-off-by: Hiroshi Muraoka * Modify RunnerDeployment tests Signed-off-by: binoue * Modify RunnerReplicaset test Signed-off-by: binoue * Modify integration test Signed-off-by: Hiroshi Muraoka * Use RunnerDeployment Template Labels as the default selector for backward compatibility * Fix labeling Signed-off-by: Hiroshi Muraoka * Update func in Eventually to return (int, error) Signed-off-by: Hiroshi Muraoka * Update RunnerDeployment controller not to use label selector Signed-off-by: Hiroshi Muraoka * Fix potential replicaset controller breakage on replicaset created before v0.17.0 * Fix errors on existing runner replica sets * Ensure RunnerReplicaSet Spec Selector addition does not break controller * Ensure RunnerDeployment Template.Spec.Labels change does result in template hash change * Fix comment Co-authored-by: binoue Co-authored-by: Yusuke Kuoka --- api/v1alpha1/runnerdeployment_types.go | 7 +- api/v1alpha1/runnerreplicaset_types.go | 5 +- api/v1alpha1/zz_generated.deepcopy.go | 11 + ...ions.summerwind.dev_runnerdeployments.yaml | 33 +- ...ions.summerwind.dev_runnerreplicasets.yaml | 31 ++ ...ions.summerwind.dev_runnerdeployments.yaml | 33 +- ...ions.summerwind.dev_runnerreplicasets.yaml | 31 ++ controllers/autoscaling.go | 20 +- controllers/autoscaling_test.go | 10 + controllers/integration_test.go | 47 ++- controllers/runnerdeployment_controller.go | 80 +++- .../runnerdeployment_controller_test.go | 351 +++++++++++++++--- controllers/runnerreplicaset_controller.go | 21 +- .../runnerreplicaset_controller_test.go | 48 ++- 14 files changed, 667 insertions(+), 61 deletions(-) diff --git a/api/v1alpha1/runnerdeployment_types.go b/api/v1alpha1/runnerdeployment_types.go index ba6085bd..128de533 100644 --- a/api/v1alpha1/runnerdeployment_types.go +++ b/api/v1alpha1/runnerdeployment_types.go @@ -25,13 +25,16 @@ const ( AutoscalingMetricTypePercentageRunnersBusy = "PercentageRunnersBusy" ) -// RunnerReplicaSetSpec defines the desired state of RunnerDeployment +// RunnerDeploymentSpec defines the desired state of RunnerDeployment type RunnerDeploymentSpec struct { // +optional // +nullable Replicas *int `json:"replicas,omitempty"` - Template RunnerTemplate `json:"template"` + // +optional + // +nullable + Selector *metav1.LabelSelector `json:"selector"` + Template RunnerTemplate `json:"template"` } type RunnerDeploymentStatus struct { diff --git a/api/v1alpha1/runnerreplicaset_types.go b/api/v1alpha1/runnerreplicaset_types.go index eaef7fd7..4c755a0d 100644 --- a/api/v1alpha1/runnerreplicaset_types.go +++ b/api/v1alpha1/runnerreplicaset_types.go @@ -26,7 +26,10 @@ type RunnerReplicaSetSpec struct { // +nullable Replicas *int `json:"replicas,omitempty"` - Template RunnerTemplate `json:"template"` + // +optional + // +nullable + Selector *metav1.LabelSelector `json:"selector"` + Template RunnerTemplate `json:"template"` } type RunnerReplicaSetStatus struct { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 70ce7606..f5752e73 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -22,6 +22,7 @@ package v1alpha1 import ( "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -403,6 +404,11 @@ func (in *RunnerDeploymentSpec) DeepCopyInto(out *RunnerDeploymentSpec) { *out = new(int) **out = **in } + if in.Selector != nil { + in, out := &in.Selector, &out.Selector + *out = new(metav1.LabelSelector) + (*in).DeepCopyInto(*out) + } in.Template.DeepCopyInto(&out.Template) } @@ -535,6 +541,11 @@ func (in *RunnerReplicaSetSpec) DeepCopyInto(out *RunnerReplicaSetSpec) { *out = new(int) **out = **in } + if in.Selector != nil { + in, out := &in.Selector, &out.Selector + *out = new(metav1.LabelSelector) + (*in).DeepCopyInto(*out) + } in.Template.DeepCopyInto(&out.Template) } diff --git a/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerdeployments.yaml b/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerdeployments.yaml index 29afc816..17c05808 100644 --- a/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerdeployments.yaml +++ b/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerdeployments.yaml @@ -38,11 +38,42 @@ spec: metadata: type: object spec: - description: RunnerReplicaSetSpec defines the desired state of RunnerDeployment + description: RunnerDeploymentSpec defines the desired state of RunnerDeployment properties: replicas: nullable: true type: integer + selector: + description: A label selector is a label query over a set of resources. The result of matchLabels and matchExpressions are ANDed. An empty label selector matches all objects. A null label selector matches no objects. + nullable: true + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. + properties: + key: + description: key is the label key that the selector applies to. + type: string + operator: + description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + type: object template: properties: metadata: diff --git a/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerreplicasets.yaml b/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerreplicasets.yaml index 6e7a6315..c23bfe9a 100644 --- a/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerreplicasets.yaml +++ b/charts/actions-runner-controller/crds/actions.summerwind.dev_runnerreplicasets.yaml @@ -43,6 +43,37 @@ spec: replicas: nullable: true type: integer + selector: + description: A label selector is a label query over a set of resources. The result of matchLabels and matchExpressions are ANDed. An empty label selector matches all objects. A null label selector matches no objects. + nullable: true + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. + properties: + key: + description: key is the label key that the selector applies to. + type: string + operator: + description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + type: object template: properties: metadata: diff --git a/config/crd/bases/actions.summerwind.dev_runnerdeployments.yaml b/config/crd/bases/actions.summerwind.dev_runnerdeployments.yaml index 29afc816..17c05808 100644 --- a/config/crd/bases/actions.summerwind.dev_runnerdeployments.yaml +++ b/config/crd/bases/actions.summerwind.dev_runnerdeployments.yaml @@ -38,11 +38,42 @@ spec: metadata: type: object spec: - description: RunnerReplicaSetSpec defines the desired state of RunnerDeployment + description: RunnerDeploymentSpec defines the desired state of RunnerDeployment properties: replicas: nullable: true type: integer + selector: + description: A label selector is a label query over a set of resources. The result of matchLabels and matchExpressions are ANDed. An empty label selector matches all objects. A null label selector matches no objects. + nullable: true + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. + properties: + key: + description: key is the label key that the selector applies to. + type: string + operator: + description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + type: object template: properties: metadata: diff --git a/config/crd/bases/actions.summerwind.dev_runnerreplicasets.yaml b/config/crd/bases/actions.summerwind.dev_runnerreplicasets.yaml index 6e7a6315..c23bfe9a 100644 --- a/config/crd/bases/actions.summerwind.dev_runnerreplicasets.yaml +++ b/config/crd/bases/actions.summerwind.dev_runnerreplicasets.yaml @@ -43,6 +43,37 @@ spec: replicas: nullable: true type: integer + selector: + description: A label selector is a label query over a set of resources. The result of matchLabels and matchExpressions are ANDed. An empty label selector matches all objects. A null label selector matches no objects. + nullable: true + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. + properties: + key: + description: key is the label key that the selector applies to. + type: string + operator: + description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + type: object template: properties: metadata: diff --git a/controllers/autoscaling.go b/controllers/autoscaling.go index a5fbb3ce..84390d80 100644 --- a/controllers/autoscaling.go +++ b/controllers/autoscaling.go @@ -10,6 +10,8 @@ import ( "time" "github.com/summerwind/actions-runner-controller/api/v1alpha1" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -257,11 +259,23 @@ func (r *HorizontalRunnerAutoscalerReconciler) calculateReplicasByPercentageRunn scaleDownFactor = sdf } - // return the list of runners in namespace. Horizontal Runner Autoscaler should only be responsible for scaling resources in its own ns. - var runnerList v1alpha1.RunnerList - if err := r.List(ctx, &runnerList, client.InNamespace(rd.Namespace)); err != nil { + selector, err := metav1.LabelSelectorAsSelector(rd.Spec.Selector) + if err != nil { return nil, err } + // return the list of runners in namespace. Horizontal Runner Autoscaler should only be responsible for scaling resources in its own ns. + var runnerList v1alpha1.RunnerList + if err := r.List( + ctx, + &runnerList, + client.InNamespace(rd.Namespace), + client.MatchingLabelsSelector{Selector: selector}, + ); err != nil { + if !kerrors.IsNotFound(err) { + return nil, err + } + } + runnerMap := make(map[string]struct{}) for _, items := range runnerList.Items { runnerMap[items.Name] = struct{}{} diff --git a/controllers/autoscaling_test.go b/controllers/autoscaling_test.go index 3450e273..50d458fa 100644 --- a/controllers/autoscaling_test.go +++ b/controllers/autoscaling_test.go @@ -443,7 +443,17 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { Name: "testrd", }, Spec: v1alpha1.RunnerDeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: v1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: v1alpha1.RunnerSpec{ Organization: tc.org, }, diff --git a/controllers/integration_test.go b/controllers/integration_test.go index 81274fc7..2a30bdfe 100644 --- a/controllers/integration_test.go +++ b/controllers/integration_test.go @@ -3,13 +3,14 @@ package controllers import ( "context" "fmt" - "github.com/google/go-github/v33/github" - github2 "github.com/summerwind/actions-runner-controller/github" - "k8s.io/apimachinery/pkg/runtime" "net/http" "net/http/httptest" "time" + "github.com/google/go-github/v33/github" + github2 "github.com/summerwind/actions-runner-controller/github" + "k8s.io/apimachinery/pkg/runtime" + "github.com/summerwind/actions-runner-controller/github/fake" corev1 "k8s.io/api/core/v1" @@ -184,7 +185,17 @@ var _ = Context("INTEGRATION: Inside of a new namespace", func() { }, Spec: actionsv1alpha1.RunnerDeploymentSpec{ Replicas: intPtr(1), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: actionsv1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: actionsv1alpha1.RunnerSpec{ Repository: "test/valid", Image: "bar", @@ -301,7 +312,17 @@ var _ = Context("INTEGRATION: Inside of a new namespace", func() { }, Spec: actionsv1alpha1.RunnerDeploymentSpec{ Replicas: intPtr(1), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: actionsv1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: actionsv1alpha1.RunnerSpec{ Repository: "test/valid", Image: "bar", @@ -396,7 +417,17 @@ var _ = Context("INTEGRATION: Inside of a new namespace", func() { }, Spec: actionsv1alpha1.RunnerDeploymentSpec{ Replicas: intPtr(1), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: actionsv1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: actionsv1alpha1.RunnerSpec{ Repository: "test/valid", Image: "bar", @@ -515,7 +546,17 @@ var _ = Context("INTEGRATION: Inside of a new namespace", func() { }, Spec: actionsv1alpha1.RunnerDeploymentSpec{ Replicas: intPtr(1), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: actionsv1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: actionsv1alpha1.RunnerSpec{ Repository: "test/valid", Image: "bar", diff --git a/controllers/runnerdeployment_controller.go b/controllers/runnerdeployment_controller.go index 9fc8a79f..da99622b 100644 --- a/controllers/runnerdeployment_controller.go +++ b/controllers/runnerdeployment_controller.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "hash/fnv" + "reflect" "sort" "time" @@ -143,6 +144,28 @@ func (r *RunnerDeploymentReconciler) Reconcile(req ctrl.Request) (ctrl.Result, e return ctrl.Result{RequeueAfter: 5 * time.Second}, nil } + if !reflect.DeepEqual(newestSet.Spec.Selector, desiredRS.Spec.Selector) { + updateSet := newestSet.DeepCopy() + updateSet.Spec = *desiredRS.Spec.DeepCopy() + + // A selector update change doesn't trigger replicaset replacement, + // but we still need to update the existing replicaset with it. + // Otherwise selector-based runner query will never work on replicasets created before the controller v0.17.0 + // See https://github.com/summerwind/actions-runner-controller/pull/355#discussion_r585379259 + if err := r.Client.Update(ctx, updateSet); err != nil { + log.Error(err, "Failed to update runnerreplicaset resource") + + return ctrl.Result{}, err + } + + // At this point, we are already sure that there's no need to create a new replicaset + // as the runner template hash is not changed. + // + // But we still need to requeue for the (possibly rare) cases that there are still old replicasets that needs + // to be cleaned up. + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + const defaultReplicas = 1 currentDesiredReplicas := getIntOrDefault(newestSet.Spec.Replicas, defaultReplicas) @@ -258,18 +281,70 @@ func CloneAndAddLabel(labels map[string]string, labelKey, labelValue string) map return newLabels } +// Clones the given selector and returns a new selector with the given key and value added. +// Returns the given selector, if labelKey is empty. +// +// Proudly copied from k8s.io/kubernetes/pkg/util/labels.CloneSelectorAndAddLabel +func CloneSelectorAndAddLabel(selector *metav1.LabelSelector, labelKey, labelValue string) *metav1.LabelSelector { + if labelKey == "" { + // Don't need to add a label. + return selector + } + + // Clone. + newSelector := new(metav1.LabelSelector) + + newSelector.MatchLabels = make(map[string]string) + if selector.MatchLabels != nil { + for key, val := range selector.MatchLabels { + newSelector.MatchLabels[key] = val + } + } + newSelector.MatchLabels[labelKey] = labelValue + + if selector.MatchExpressions != nil { + newMExps := make([]metav1.LabelSelectorRequirement, len(selector.MatchExpressions)) + for i, me := range selector.MatchExpressions { + newMExps[i].Key = me.Key + newMExps[i].Operator = me.Operator + if me.Values != nil { + newMExps[i].Values = make([]string, len(me.Values)) + copy(newMExps[i].Values, me.Values) + } else { + newMExps[i].Values = nil + } + } + newSelector.MatchExpressions = newMExps + } else { + newSelector.MatchExpressions = nil + } + + return newSelector +} + func (r *RunnerDeploymentReconciler) newRunnerReplicaSet(rd v1alpha1.RunnerDeployment) (*v1alpha1.RunnerReplicaSet, error) { + return newRunnerReplicaSet(&rd, r.CommonRunnerLabels, r.Scheme) +} + +func newRunnerReplicaSet(rd *v1alpha1.RunnerDeployment, commonRunnerLabels []string, scheme *runtime.Scheme) (*v1alpha1.RunnerReplicaSet, error) { newRSTemplate := *rd.Spec.Template.DeepCopy() + templateHash := ComputeHash(&newRSTemplate) // Add template hash label to selector. labels := CloneAndAddLabel(rd.Spec.Template.Labels, LabelKeyRunnerTemplateHash, templateHash) - for _, l := range r.CommonRunnerLabels { + for _, l := range commonRunnerLabels { newRSTemplate.Spec.Labels = append(newRSTemplate.Spec.Labels, l) } newRSTemplate.Labels = labels + selector := rd.Spec.Selector + if selector == nil { + selector = &metav1.LabelSelector{MatchLabels: labels} + } + newRSSelector := CloneSelectorAndAddLabel(selector, LabelKeyRunnerTemplateHash, templateHash) + rs := v1alpha1.RunnerReplicaSet{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{ @@ -279,11 +354,12 @@ func (r *RunnerDeploymentReconciler) newRunnerReplicaSet(rd v1alpha1.RunnerDeplo }, Spec: v1alpha1.RunnerReplicaSetSpec{ Replicas: rd.Spec.Replicas, + Selector: newRSSelector, Template: newRSTemplate, }, } - if err := ctrl.SetControllerReference(&rd, &rs, r.Scheme); err != nil { + if err := ctrl.SetControllerReference(rd, &rs, scheme); err != nil { return &rs, err } diff --git a/controllers/runnerdeployment_controller_test.go b/controllers/runnerdeployment_controller_test.go index 34e2b0c7..c9cee456 100644 --- a/controllers/runnerdeployment_controller_test.go +++ b/controllers/runnerdeployment_controller_test.go @@ -2,11 +2,13 @@ package controllers import ( "context" - "github.com/google/go-cmp/cmp" - "k8s.io/apimachinery/pkg/runtime" + "fmt" "testing" "time" + "github.com/google/go-cmp/cmp" + "k8s.io/apimachinery/pkg/runtime" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" @@ -36,7 +38,17 @@ func TestNewRunnerReplicaSet(t *testing.T) { Name: "example", }, Spec: actionsv1alpha1.RunnerDeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: actionsv1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: actionsv1alpha1.RunnerSpec{ Labels: []string{"project1"}, }, @@ -49,10 +61,63 @@ func TestNewRunnerReplicaSet(t *testing.T) { t.Fatalf("%v", err) } - want := []string{"project1", "dev"} - if d := cmp.Diff(want, rs.Spec.Template.Spec.Labels); d != "" { + if val, ok := rs.Labels["foo"]; ok { + if val != "bar" { + t.Errorf("foo label does not have bar but %v", val) + } + } else { + t.Errorf("foo label does not exist") + } + + hash1, ok := rs.Labels[LabelKeyRunnerTemplateHash] + if !ok { + t.Errorf("missing runner-template-hash label") + } + + runnerLabel := []string{"project1", "dev"} + if d := cmp.Diff(runnerLabel, rs.Spec.Template.Spec.Labels); d != "" { t.Errorf("%s", d) } + + rd2 := rd.DeepCopy() + rd2.Spec.Template.Spec.Labels = []string{"project2"} + + rs2, err := r.newRunnerReplicaSet(*rd2) + if err != nil { + t.Fatalf("%v", err) + } + + hash2, ok := rs2.Labels[LabelKeyRunnerTemplateHash] + if !ok { + t.Errorf("missing runner-template-hash label") + } + + if hash1 == hash2 { + t.Errorf( + "runner replica sets from runner deployments with varying labels must have different template hash, but got %s and %s", + hash1, hash2, + ) + } + + rd3 := rd.DeepCopy() + rd3.Spec.Template.Labels["foo"] = "baz" + + rs3, err := r.newRunnerReplicaSet(*rd3) + if err != nil { + t.Fatalf("%v", err) + } + + hash3, ok := rs3.Labels[LabelKeyRunnerTemplateHash] + if !ok { + t.Errorf("missing runner-template-hash label") + } + + if hash1 == hash3 { + t.Errorf( + "runner replica sets from runner deployments with varying meta labels must have different template hash, but got %s and %s", + hash1, hash3, + ) + } } // SetupDeploymentTest will set up a testing environment. @@ -112,7 +177,113 @@ var _ = Context("Inside of a new namespace", func() { Describe("when no existing resources exist", func() { It("should create a new RunnerReplicaSet resource from the specified template, add a another RunnerReplicaSet on template modification, and eventually removes old runnerreplicasets", func() { - name := "example-runnerdeploy" + name := "example-runnerdeploy-1" + + { + rs := &actionsv1alpha1.RunnerDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns.Name, + }, + Spec: actionsv1alpha1.RunnerDeploymentSpec{ + Replicas: intPtr(1), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, + Template: actionsv1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, + Spec: actionsv1alpha1.RunnerSpec{ + Repository: "foo/bar", + Image: "bar", + Env: []corev1.EnvVar{ + {Name: "FOO", Value: "FOOVALUE"}, + }, + }, + }, + }, + } + + err := k8sClient.Create(ctx, rs) + + Expect(err).NotTo(HaveOccurred(), "failed to create test RunnerReplicaSet resource") + + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + Eventually( + func() (int, error) { + selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) + if err != nil { + return 0, err + } + err = k8sClient.List( + ctx, + &runnerSets, + client.InNamespace(ns.Name), + client.MatchingLabelsSelector{Selector: selector}, + ) + if err != nil { + return 0, err + } + if len(runnerSets.Items) != 1 { + return 0, fmt.Errorf("runnerreplicasets is not 1 but %d", len(runnerSets.Items)) + } + + return *runnerSets.Items[0].Spec.Replicas, nil + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) + } + + { + // We wrap the update in the Eventually block to avoid the below error that occurs due to concurrent modification + // made by the controller to update .Status.AvailableReplicas and .Status.ReadyReplicas + // Operation cannot be fulfilled on runnersets.actions.summerwind.dev "example-runnerset": the object has been modified; please apply your changes to the latest version and try again + var rd actionsv1alpha1.RunnerDeployment + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: name}, &rd) + if err != nil { + return fmt.Errorf("failed to get test RunnerReplicaSet resource: %v\n", err) + } + rd.Spec.Replicas = intPtr(2) + + return k8sClient.Update(ctx, &rd) + }, + time.Second*1, time.Millisecond*500).Should(BeNil()) + + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + Eventually( + func() (int, error) { + selector, err := metav1.LabelSelectorAsSelector(rd.Spec.Selector) + if err != nil { + return 0, err + } + err = k8sClient.List( + ctx, + &runnerSets, + client.InNamespace(ns.Name), + client.MatchingLabelsSelector{Selector: selector}, + ) + if err != nil { + return 0, err + } + if len(runnerSets.Items) != 1 { + return 0, fmt.Errorf("runnerreplicasets is not 1 but %d", len(runnerSets.Items)) + } + + return *runnerSets.Items[0].Spec.Replicas, nil + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(2)) + } + }) + + It("should create a new RunnerReplicaSet resource from the specified template without labels and selector, add a another RunnerReplicaSet on template modification, and eventually removes old runnerreplicasets", func() { + name := "example-runnerdeploy-2" { rs := &actionsv1alpha1.RunnerDeployment{ @@ -141,29 +312,25 @@ var _ = Context("Inside of a new namespace", func() { runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} Eventually( - func() int { - err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + func() (int, error) { + selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) if err != nil { - logf.Log.Error(err, "list runner sets") + return 0, err } - - return len(runnerSets.Items) - }, - time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) - - Eventually( - func() int { - err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + err = k8sClient.List( + ctx, + &runnerSets, + client.InNamespace(ns.Name), + client.MatchingLabelsSelector{Selector: selector}, + ) if err != nil { - logf.Log.Error(err, "list runner sets") + return 0, err + } + if len(runnerSets.Items) != 1 { + return 0, fmt.Errorf("runnerreplicasets is not 1 but %d", len(runnerSets.Items)) } - if len(runnerSets.Items) == 0 { - logf.Log.Info("No runnerreplicasets exist yet") - return -1 - } - - return *runnerSets.Items[0].Spec.Replicas + return *runnerSets.Items[0].Spec.Replicas, nil }, time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) } @@ -172,13 +339,12 @@ var _ = Context("Inside of a new namespace", func() { // We wrap the update in the Eventually block to avoid the below error that occurs due to concurrent modification // made by the controller to update .Status.AvailableReplicas and .Status.ReadyReplicas // Operation cannot be fulfilled on runnersets.actions.summerwind.dev "example-runnerset": the object has been modified; please apply your changes to the latest version and try again + var rd actionsv1alpha1.RunnerDeployment Eventually(func() error { - var rd actionsv1alpha1.RunnerDeployment - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: name}, &rd) - - Expect(err).NotTo(HaveOccurred(), "failed to get test RunnerReplicaSet resource") - + if err != nil { + return fmt.Errorf("failed to get test RunnerReplicaSet resource: %v\n", err) + } rd.Spec.Replicas = intPtr(2) return k8sClient.Update(ctx, &rd) @@ -188,27 +354,126 @@ var _ = Context("Inside of a new namespace", func() { runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} Eventually( - func() int { - err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + func() (int, error) { + selector, err := metav1.LabelSelectorAsSelector(rd.Spec.Selector) if err != nil { - logf.Log.Error(err, "list runner sets") + return 0, err + } + err = k8sClient.List( + ctx, + &runnerSets, + client.InNamespace(ns.Name), + client.MatchingLabelsSelector{Selector: selector}, + ) + if err != nil { + return 0, err + } + if len(runnerSets.Items) != 1 { + return 0, fmt.Errorf("runnerreplicasets is not 1 but %d", len(runnerSets.Items)) } - return len(runnerSets.Items) - }, - time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) - - Eventually( - func() int { - err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) - if err != nil { - logf.Log.Error(err, "list runner sets") - } - - return *runnerSets.Items[0].Spec.Replicas + return *runnerSets.Items[0].Spec.Replicas, nil }, time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(2)) } }) + + It("should adopt RunnerReplicaSet created before 0.18.0 to have Spec.Selector", func() { + name := "example-runnerdeploy-2" + + { + rd := &actionsv1alpha1.RunnerDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns.Name, + }, + Spec: actionsv1alpha1.RunnerDeploymentSpec{ + Replicas: intPtr(1), + Template: actionsv1alpha1.RunnerTemplate{ + Spec: actionsv1alpha1.RunnerSpec{ + Repository: "foo/bar", + Image: "bar", + Env: []corev1.EnvVar{ + {Name: "FOO", Value: "FOOVALUE"}, + }, + }, + }, + }, + } + + createRDErr := k8sClient.Create(ctx, rd) + Expect(createRDErr).NotTo(HaveOccurred(), "failed to create test RunnerReplicaSet resource") + + Eventually( + func() (int, error) { + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + err := k8sClient.List( + ctx, + &runnerSets, + client.InNamespace(ns.Name), + ) + if err != nil { + return 0, err + } + + return len(runnerSets.Items), nil + }, + time.Second*1, time.Millisecond*500).Should(BeEquivalentTo(1)) + + var rs17 *actionsv1alpha1.RunnerReplicaSet + + Consistently( + func() (*metav1.LabelSelector, error) { + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + err := k8sClient.List( + ctx, + &runnerSets, + client.InNamespace(ns.Name), + ) + if err != nil { + return nil, err + } + if len(runnerSets.Items) != 1 { + return nil, fmt.Errorf("runnerreplicasets is not 1 but %d", len(runnerSets.Items)) + } + + rs17 = &runnerSets.Items[0] + + return runnerSets.Items[0].Spec.Selector, nil + }, + time.Second*1, time.Millisecond*500).Should(Not(BeNil())) + + // We simulate the old, pre 0.18.0 RunnerReplicaSet by updating it. + // I've tried to use controllerutil.Set{Owner,Controller}Reference and k8sClient.Create(rs17) + // but it didn't work due to missing RD UID, where UID is generated on K8s API server on k8sCLient.Create(rd) + rs17.Spec.Selector = nil + + updateRSErr := k8sClient.Update(ctx, rs17) + Expect(updateRSErr).NotTo(HaveOccurred()) + + Eventually( + func() (*metav1.LabelSelector, error) { + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + err := k8sClient.List( + ctx, + &runnerSets, + client.InNamespace(ns.Name), + ) + if err != nil { + return nil, err + } + if len(runnerSets.Items) != 1 { + return nil, fmt.Errorf("runnerreplicasets is not 1 but %d", len(runnerSets.Items)) + } + + return runnerSets.Items[0].Spec.Selector, nil + }, + time.Second*1, time.Millisecond*500).Should(Not(BeNil())) + } + }) + }) }) diff --git a/controllers/runnerreplicaset_controller.go b/controllers/runnerreplicaset_controller.go index e47b4483..ffdc4f7d 100644 --- a/controllers/runnerreplicaset_controller.go +++ b/controllers/runnerreplicaset_controller.go @@ -68,8 +68,18 @@ func (r *RunnerReplicaSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, e return ctrl.Result{}, nil } + selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) + if err != nil { + return ctrl.Result{}, err + } + // Get the Runners managed by the target RunnerReplicaSet var allRunners v1alpha1.RunnerList - if err := r.List(ctx, &allRunners, client.InNamespace(req.Namespace)); err != nil { + if err := r.List( + ctx, + &allRunners, + client.InNamespace(req.Namespace), + client.MatchingLabelsSelector{Selector: selector}, + ); err != nil { if !kerrors.IsNotFound(err) { return ctrl.Result{}, err } @@ -77,9 +87,14 @@ func (r *RunnerReplicaSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, e var myRunners []v1alpha1.Runner - var available, ready int + var ( + available int + ready int + ) for _, r := range allRunners.Items { + // This guard is required to avoid the RunnerReplicaSet created by the controller v0.17.0 or before + // to not treat all the runners in the namespace as its children. if metav1.IsControlledBy(&r, &rs) { myRunners = append(myRunners, r) @@ -106,7 +121,7 @@ func (r *RunnerReplicaSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, e // get runners that are currently not busy var notBusy []v1alpha1.Runner - for _, runner := range myRunners { + for _, runner := range allRunners.Items { busy, err := r.GitHubClient.IsRunnerBusy(ctx, runner.Spec.Enterprise, runner.Spec.Organization, runner.Spec.Repository, runner.Name) if err != nil { notRegistered := false diff --git a/controllers/runnerreplicaset_controller_test.go b/controllers/runnerreplicaset_controller_test.go index c146464b..7d8ea5d4 100644 --- a/controllers/runnerreplicaset_controller_test.go +++ b/controllers/runnerreplicaset_controller_test.go @@ -115,7 +115,17 @@ var _ = Context("Inside of a new namespace", func() { }, Spec: actionsv1alpha1.RunnerReplicaSetSpec{ Replicas: intPtr(1), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: actionsv1alpha1.RunnerTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: actionsv1alpha1.RunnerSpec{ Repository: "foo/bar", Image: "bar", @@ -135,9 +145,26 @@ var _ = Context("Inside of a new namespace", func() { Eventually( func() int { - err := k8sClient.List(ctx, &runners, client.InNamespace(ns.Name)) + selector, err := metav1.LabelSelectorAsSelector( + &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, + ) + if err != nil { + logf.Log.Error(err, "failed to create labelselector") + return -1 + } + err = k8sClient.List( + ctx, + &runners, + client.InNamespace(ns.Name), + client.MatchingLabelsSelector{Selector: selector}, + ) if err != nil { logf.Log.Error(err, "list runners") + return -1 } for i, runner := range runners.Items { @@ -176,7 +203,23 @@ var _ = Context("Inside of a new namespace", func() { Eventually( func() int { - err := k8sClient.List(ctx, &runners, client.InNamespace(ns.Name)) + selector, err := metav1.LabelSelectorAsSelector( + &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, + ) + if err != nil { + logf.Log.Error(err, "failed to create labelselector") + return -1 + } + err = k8sClient.List( + ctx, + &runners, + client.InNamespace(ns.Name), + client.MatchingLabelsSelector{Selector: selector}, + ) if err != nil { logf.Log.Error(err, "list runners") } @@ -220,6 +263,7 @@ var _ = Context("Inside of a new namespace", func() { err := k8sClient.List(ctx, &runners, client.InNamespace(ns.Name)) if err != nil { logf.Log.Error(err, "list runners") + return -1 } for i, runner := range runners.Items {