feat: Add Service field to Rollout Experiment to allow service creati…

…on (#2633) * Add Service field to Rollout Experiment to allow service creation Signed-off-by: Daniel Del Rio <[email protected]> * Change template names Signed-off-by: Daniel Del Rio <[email protected]> * Add test for service without name Signed-off-by: Daniel Del Rio <[email protected]> * Remove Service field in trafficrouting_test Signed-off-by: Daniel Del Rio <[email protected]> * Add e2e test for experiment with service name Signed-off-by: Daniel Del Rio <[email protected]> * Update specifications and documentation about setting Service in experiments Signed-off-by: Daniel Del Rio <[email protected]> * Pass Service name instead of replicaset name to CreateService func in Experiment Signed-off-by: Daniel Del Rio <[email protected]> * Modify unit/e2e tests for service name Signed-off-by: Daniel Del Rio <[email protected]> * Change app name in experiment e2e test Signed-off-by: Daniel Del Rio <[email protected]> * Add to error messages Signed-off-by: Daniel Del Rio <[email protected]> * Fix unit test for service creation Signed-off-by: Daniel Del Rio <[email protected]> * Change service.Name call in createService Signed-off-by: Daniel Del Rio <[email protected]> --------- Signed-off-by: Daniel Del Rio <[email protected]>
argoproj · Mar 23, 2023 · aa47e41 · aa47e41
1 parent 558f2f1
commit aa47e41
Show file tree

Hide file tree

Showing 21 changed files with 904 additions and 510 deletions.
diff --git a/docs/features/experiment.md b/docs/features/experiment.md
@@ -6,7 +6,8 @@ The Experiment CRD allows users to have ephemeral runs of one or more ReplicaSet
 running ephemeral ReplicaSets, the Experiment CRD can launch AnalysisRuns alongside the ReplicaSets.
 Generally, those AnalysisRun is used to confirm that new ReplicaSets are running as expected.
 
-A Service routing traffic to the Experiment ReplicaSet is also generated if a weight for that experiment is set.
+A Service routing traffic to the Experiment ReplicaSet is also generated if a weight (which requires traffic routing)
+OR the Service attribute for that experiment is set.
 
 ## Use cases of Experiments
 
@@ -51,6 +52,11 @@ spec:
   - name: purple
     # Number of replicas to run (optional). If omitted, will run a single replica
     replicas: 1
+    # Flag to create Service for this Experiment (optional)
+    # If omitted, a Service won't be created.
+    service:
+      # Name of the Service (optional). If omitted, service: {} would also be acceptable.
+      name: service-name
     selector:
       matchLabels:
         app: canary-demo
@@ -119,7 +125,8 @@ spec:
 An Experiment is intended to temporarily run one or more templates. The lifecycle of an Experiment
 is as follows:
 
-1. Create and scale a ReplicaSet for each pod template specified under `spec.templates`
+1. Create and scale a ReplicaSet for each pod template specified under `spec.templates`. If
+   `service` is specified under a pod template, a Service will also be created for that pod.
 2. Wait for all ReplicaSets reach full availability. If a ReplicaSet does not become available
    within `spec.progressDeadlineSeconds`, the Experiment will fail. Once available, the Experiment
    will transition from the `Pending` state to a `Running` state.
@@ -250,4 +257,40 @@ to `experiment-baseline`, leaving the remaining 90% of traffic to the old stack.
 
 By default, the generated Service has the name of the ReplicaSet and inherits
 ports and selector from the specRef definition. It can be accessed in using the `{{templates.baseline.replicaset.name}}`
-or `{{templates.canary.replicaset.name}}` variables respectively.
+or `{{templates.canary.replicaset.name}}` variables respectively.
+
+
+
+## Experiment Service Creation without Weight
+
+If you don't want to use traffic routing for your Experiments but still want to create
+a Service for them, you can set a Service object which takes an optional Name, without
+having to set a Weight for them.
+
+```yaml
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: guestbook
+  labels:
+    app: guestbook
+spec:
+...
+strategy:
+  canary:
+    steps:
+      - experiment:
+          duration: 1h
+          templates:
+            - name: experiment-baseline
+              specRef: stable
+              service:
+                name: test-service
+            - name: experiment-canary
+              specRef: canary
+```
+
+In the above example, during an update, the first step would start
+a baseline vs. canary experiment. This time, a service would be created
+for `experiment-baseline` even without setting a weight for it or traffic
+routing for the rollout.
diff --git a/docs/features/kustomize/rollout_cr_schema.json b/docs/features/kustomize/rollout_cr_schema.json
@@ -13032,6 +13032,11 @@
                                         "type": "object"
                                     },
                                     "service": {
+                                        "properties": {
+                                            "name": {
+                                                "type": "string"
+                                            }
+                                        },
                                         "type": "object"
                                     },
                                     "template": {

diff --git a/docs/features/specification.md b/docs/features/specification.md
@@ -337,6 +337,10 @@ spec:
           templates:
           - name: baseline
             specRef: stable
+            # optional, creates a service for the experiment if set
+            service:
+              # optional, service: {} is also acceptable if name is not included
+              name: test-service
           - name: canary
             specRef: canary
             # optional, set the weight of traffic routed to this version

diff --git a/experiments/experiment.go b/experiments/experiment.go
@@ -301,10 +301,14 @@ func (ec *experimentContext) createTemplateService(template *v1alpha1.TemplateSp
 		}
 	}
 	if (svc == nil || svc.Name != rs.Name) && len(ports) > 0 {
-		newService, err := ec.CreateService(rs.Name, *template, rs.Labels, ports)
+		serviceName := rs.Name
+		if template.Service.Name != "" {
+			serviceName = template.Service.Name
+		}
+		newService, err := ec.CreateService(serviceName, *template, rs.Labels, ports)
 		if err != nil {
 			templateStatus.Status = v1alpha1.TemplateStatusError
-			templateStatus.Message = fmt.Sprintf("Failed to create Service for template '%s': %v", template.Name, err)
+			templateStatus.Message = fmt.Sprintf("Failed to create Service %s for template '%s': %v", serviceName, template.Name, err)
 		} else {
 			ec.templateServices[template.Name] = newService
 			templateStatus.ServiceName = newService.Name

diff --git a/experiments/experiment_test.go b/experiments/experiment_test.go
@@ -399,7 +399,7 @@ func TestFailServiceCreation(t *testing.T) {
 	})
 	newStatus := exCtx.reconcile()
 	assert.Equal(t, v1alpha1.TemplateStatusError, newStatus.TemplateStatuses[0].Status)
-	assert.Contains(t, newStatus.TemplateStatuses[0].Message, "Failed to create Service for template 'bad'")
+	assert.Contains(t, newStatus.TemplateStatuses[0].Message, "Failed to create Service foo-bad for template 'bad'")
 	assert.Equal(t, v1alpha1.AnalysisPhaseError, newStatus.Phase)
 }
 
@@ -530,3 +530,27 @@ func TestServiceInheritPortsFromRS(t *testing.T) {
 	assert.Equal(t, exCtx.templateServices["bar"].Name, "foo-bar")
 	assert.Equal(t, exCtx.templateServices["bar"].Spec.Ports[0].Port, int32(80))
 }
+
+func TestServiceNameSet(t *testing.T) {
+	templates := generateTemplates("bar")
+	templates[0].Service = &v1alpha1.TemplateService{
+		Name: "service-name",
+	}
+	templates[0].Template.Spec.Containers[0].Ports = []corev1.ContainerPort{
+		{
+			Name:          "testport",
+			ContainerPort: 80,
+			Protocol:      "TCP",
+		},
+	}
+	ex := newExperiment("foo", templates, "")
+
+	exCtx := newTestContext(ex)
+	rs := templateToRS(ex, templates[0], 0)
+	exCtx.templateRSs["bar"] = rs
+
+	exCtx.reconcile()
+
+	assert.NotNil(t, exCtx.templateServices["bar"])
+	assert.Equal(t, exCtx.templateServices["bar"].Name, "service-name")
+}
diff --git a/experiments/service.go b/experiments/service.go
@@ -81,17 +81,17 @@ func (ec *experimentContext) CreateService(serviceName string, template v1alpha1
 	if err != nil {
 		// If service already exists, get service and check that it is owned by Experiment Template. Otherwise return error.
 		if errors.IsAlreadyExists(err) {
-			svc, err := ec.kubeclientset.CoreV1().Services(ec.ex.Namespace).Get(ctx, service.Name, metav1.GetOptions{})
+			svc, err := ec.kubeclientset.CoreV1().Services(ec.ex.Namespace).Get(ctx, serviceName, metav1.GetOptions{})
 			if err != nil {
-				return nil, err
+				return nil, fmt.Errorf("did not get existing service with name %s: %v", serviceName, err)
 			}
 			controllerRef := metav1.GetControllerOf(svc)
 			if controllerRef == nil || controllerRef.UID != ec.ex.UID || svc.Annotations == nil || svc.Annotations[v1alpha1.ExperimentNameAnnotationKey] != ec.ex.Name || svc.Annotations[v1alpha1.ExperimentTemplateNameAnnotationKey] != template.Name {
 				return nil, fmt.Errorf("service %s already exists and is not owned by experiment template %s", serviceName, template.Name)
 			}
 			return svc, nil
 		} else {
-			return nil, err
+			return nil, fmt.Errorf("cannot create service: %v %v", err, newService)
 		}
 	}
 	return service, nil

diff --git a/manifests/crds/experiment-crd.yaml b/manifests/crds/experiment-crd.yaml
@@ -149,6 +149,9 @@ spec:
                           type: object
                       type: object
                     service:
+                      properties:
+                        name:
+                          type: string
                       type: object
                     template:
                       properties:

diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml
@@ -570,6 +570,11 @@ spec:
                                               type: string
                                             type: object
                                         type: object
+                                      service:
+                                        properties:
+                                          name:
+                                            type: string
+                                        type: object
                                       specRef:
                                         type: string
                                       weight:

diff --git a/manifests/install.yaml b/manifests/install.yaml
@@ -8599,6 +8599,9 @@ spec:
                           type: object
                       type: object
                     service:
+                      properties:
+                        name:
+                          type: string
                       type: object
                     template:
                       properties:
@@ -11622,6 +11625,11 @@ spec:
                                               type: string
                                             type: object
                                         type: object
+                                      service:
+                                        properties:
+                                          name:
+                                            type: string
+                                        type: object
                                       specRef:
                                         type: string
                                       weight:

diff --git a/pkg/apiclient/rollout/rollout.swagger.json b/pkg/apiclient/rollout/rollout.swagger.json
@@ -1373,6 +1373,10 @@
           "type": "integer",
           "format": "int32",
           "title": "Weight sets the percentage of traffic the template's replicas should receive"
+        },
+        "service": {
+          "$ref": "#/definitions/github.com.argoproj.argo_rollouts.pkg.apis.rollouts.v1alpha1.TemplateService",
+          "title": "Service controls the optionally generated service"
         }
       },
       "title": "RolloutExperimentTemplate defines the template used to create experiments for the Rollout's experiment canary step"
@@ -1785,6 +1789,15 @@
       },
       "description": "TLSRoute holds the information on the virtual service's TLS/HTTPS routes that are desired to be matched for changing weights."
     },
+    "github.com.argoproj.argo_rollouts.pkg.apis.rollouts.v1alpha1.TemplateService": {
+      "type": "object",
+      "properties": {
+        "name": {
+          "type": "string",
+          "title": "Name of the service generated by the experiment"
+        }
+      }
+    },
     "github.com.argoproj.argo_rollouts.pkg.apis.rollouts.v1alpha1.TraefikTrafficRouting": {
       "type": "object",
       "properties": {

diff --git a/pkg/apis/rollouts/v1alpha1/experiment_types.go b/pkg/apis/rollouts/v1alpha1/experiment_types.go
@@ -89,7 +89,10 @@ type TemplateSpec struct {
 	Service *TemplateService `json:"service,omitempty" protobuf:"bytes,6,opt,name=service"`
 }
 
-type TemplateService struct{}
+type TemplateService struct {
+	// Name of the service generated by the experiment
+	Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
+}
 
 type TemplateStatusCode string