feat(controller): don't hardcode experiment ports; always create serv…

…ice (argoproj#2397) * Copy ports from replicaset rather than hardcoding them Fixes: argoproj#2233 Signed-off-by: Alex Eftimie <[email protected]> * Allow user to set the experiment service name. Create a service even though a weight is not set (when the name is provided) Signed-off-by: Alex Eftimie <[email protected]> * Run codegen Signed-off-by: Alex Eftimie <[email protected]> * Run codegen again, service name is optional Signed-off-by: Alex Eftimie <[email protected]> * Fix conversion and tests Signed-off-by: Alex Eftimie <[email protected]> * Fix codegen Signed-off-by: Alex Eftimie <[email protected]> * Fix codegen again Signed-off-by: Alex Eftimie <[email protected]> * white space Signed-off-by: Alex Eftimie <[email protected]> * Extend from corev1.ServiceSpec Signed-off-by: Alex Eftimie <[email protected]> * make lint Signed-off-by: Alex Eftimie <[email protected]> * infer ports from ReplicaSet. this should fix e2e tests Signed-off-by: Alex Eftimie <[email protected]> * fix e2e disable service where service is not needed or expected set ports where service must be created Signed-off-by: Alex Eftimie <[email protected]> * use json inline so that crd reflects ports Signed-off-by: Alex Eftimie <[email protected]> * Add codegen openapi_generated.go Signed-off-by: Alex Eftimie <[email protected]> * fix codegen, protobuf is needed Signed-off-by: Alex Eftimie <[email protected]> * Add unit test Signed-off-by: Alex Eftimie <[email protected]> * Add unit test to new not tested code - copy pods from RS Signed-off-by: Alex Eftimie <[email protected]> * Address PR comment: set entire spec Signed-off-by: Alex Eftimie <[email protected]> * docs: update experiment and rollout spec Signed-off-by: Alex Eftimie <[email protected]> * redo pr. drop spec changes. only inherit ports and always create the service. update docs Signed-off-by: Alex Eftimie <[email protected]> * Ran go fmt Signed-off-by: Alex Eftimie <[email protected]> * revert changes. only enable service if weighted or template service not nil Signed-off-by: Alex Eftimie <[email protected]> * typo in types Signed-off-by: Alex Eftimie <[email protected]> * go fmt. need pre-commit hook Signed-off-by: Alex Eftimie <[email protected]> * no more spec changes Signed-off-by: Alex Eftimie <[email protected]> * drop test which is not relevant any more Signed-off-by: Alex Eftimie <[email protected]> * fix e2e - only generate service if RS has ports Signed-off-by: Alex Eftimie <[email protected]> * fix e2e Signed-off-by: Alex Eftimie <[email protected]> Signed-off-by: Alex Eftimie <[email protected]>
jandersen-plaid · Nov 8, 2022 · bf8caf4 · bf8caf4
1 parent c6c837c
commit bf8caf4
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 20 deletions.
diff --git a/docs/features/experiment.md b/docs/features/experiment.md
@@ -6,6 +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.
+
 ## Use cases of Experiments
 
 - A user wants to run two versions of an application for a specific duration to enable Kayenta-style
@@ -243,5 +245,9 @@ to `experiment-baseline`, leaving the remaining 90% of traffic to the old stack.
 
 !!! note
     When a weighted experiment step with traffic routing is used, a
-    service is auto-created for each experiment template. The traffic routers use
-    this service to send traffic to the experiment pods.
+    Service is auto-created for each experiment template. The traffic routers use
+    this service to send traffic to the experiment pods.
+
+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.
diff --git a/docs/features/specification.md b/docs/features/specification.md
@@ -328,6 +328,8 @@ spec:
             specRef: stable
           - name: canary
             specRef: canary
+            # optional, set the weight of traffic routed to this version
+            weight: 10
           analyses:
           - name : mann-whitney
             templateName: mann-whitney

diff --git a/experiments/experiment.go b/experiments/experiment.go
@@ -22,6 +22,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/kubernetes"
 	appslisters "k8s.io/client-go/listers/apps/v1"
 	v1 "k8s.io/client-go/listers/core/v1"
@@ -285,8 +286,19 @@ func (ec *experimentContext) createTemplateService(template *v1alpha1.TemplateSp
 	// Create service with has same name, podTemplateHash, and labels as RS
 	podTemplateHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 	svc := ec.templateServices[template.Name]
-	if svc == nil || svc.Name != rs.Name {
-		newService, err := ec.CreateService(rs.Name, *template, rs.Labels)
+	var ports []corev1.ServicePort
+	for _, ctr := range rs.Spec.Template.Spec.Containers {
+		for _, port := range ctr.Ports {
+			servicePort := corev1.ServicePort{
+				Protocol:   port.Protocol,
+				Port:       port.ContainerPort,
+				TargetPort: intstr.FromInt(int(port.ContainerPort)),
+			}
+			ports = append(ports, servicePort)
+		}
+	}
+	if (svc == nil || svc.Name != rs.Name) && len(ports) > 0 {
+		newService, err := ec.CreateService(rs.Name, *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)

diff --git a/experiments/experiment_test.go b/experiments/experiment_test.go
@@ -65,6 +65,17 @@ func newTestContext(ex *v1alpha1.Experiment, objects ...runtime.Object) *experim
 		func(obj interface{}, duration time.Duration) {},
 	)
 }
+
+func setExperimentService(template *v1alpha1.TemplateSpec) {
+	template.Service = &v1alpha1.TemplateService{}
+	template.Template.Spec.Containers[0].Ports = []corev1.ContainerPort{
+		{
+			ContainerPort: 80,
+			Protocol:      "TCP",
+		},
+	}
+}
+
 func TestSetExperimentToPending(t *testing.T) {
 	templates := generateTemplates("bar")
 	e := newExperiment("foo", templates, "")
@@ -375,7 +386,7 @@ func TestFailReplicaSetCreation(t *testing.T) {
 
 func TestFailServiceCreation(t *testing.T) {
 	templates := generateTemplates("bad")
-	templates[0].Service = &v1alpha1.TemplateService{}
+	setExperimentService(&templates[0])
 	e := newExperiment("foo", templates, "")
 
 	exCtx := newTestContext(e)
@@ -444,7 +455,7 @@ func TestFailAddScaleDownDelayIsConflict(t *testing.T) {
 // TestDeleteOutdatedService verifies that outdated service for Template in templateServices map is deleted and new service is created
 func TestDeleteOutdatedService(t *testing.T) {
 	templates := generateTemplates("bar")
-	templates[0].Service = &v1alpha1.TemplateService{}
+	setExperimentService(&templates[0])
 	ex := newExperiment("foo", templates, "")
 
 	wrongService := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: "wrong-service"}}
@@ -496,3 +507,26 @@ func TestDeleteServiceIfServiceFieldNil(t *testing.T) {
 	assert.Equal(t, "", exStatus.TemplateStatuses[0].ServiceName)
 	assert.Nil(t, exCtx.templateServices["bar"])
 }
+
+func TestServiceInheritPortsFromRS(t *testing.T) {
+	templates := generateTemplates("bar")
+	templates[0].Service = &v1alpha1.TemplateService{}
+	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, "foo-bar")
+	assert.Equal(t, exCtx.templateServices["bar"].Spec.Ports[0].Port, int32(80))
+}
diff --git a/experiments/service.go b/experiments/service.go
@@ -12,7 +12,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
 var experimentKind = v1alpha1.SchemeGroupVersion.WithKind("Experiment")
@@ -59,9 +58,9 @@ func GetServiceForExperiment(experiment *v1alpha1.Experiment, svc *corev1.Servic
 	return nil
 }
 
-func (ec *experimentContext) CreateService(serviceName string, template v1alpha1.TemplateSpec, selector map[string]string) (*corev1.Service, error) {
+func (ec *experimentContext) CreateService(serviceName string, template v1alpha1.TemplateSpec, selector map[string]string, ports []corev1.ServicePort) (*corev1.Service, error) {
 	ctx := context.TODO()
-	serviceAnnotations := newServiceSetAnnotations(ec.ex.Name, template.Name)
+	serviceAnnotations := newServiceAnnotations(ec.ex.Name, template.Name)
 	newService := &corev1.Service{
 		TypeMeta: metav1.TypeMeta{},
 		ObjectMeta: metav1.ObjectMeta{
@@ -74,11 +73,7 @@ func (ec *experimentContext) CreateService(serviceName string, template v1alpha1
 		},
 		Spec: corev1.ServiceSpec{
 			Selector: selector,
-			Ports: []corev1.ServicePort{{
-				Protocol:   "TCP",
-				Port:       int32(80),
-				TargetPort: intstr.FromInt(8080),
-			}},
+			Ports:    ports,
 		},
 	}
 
@@ -112,7 +107,7 @@ func (ec *experimentContext) deleteService(service corev1.Service) error {
 	return nil
 }
 
-func newServiceSetAnnotations(experimentName, templateName string) map[string]string {
+func newServiceAnnotations(experimentName, templateName string) map[string]string {
 	return map[string]string{
 		v1alpha1.ExperimentNameAnnotationKey:         experimentName,
 		v1alpha1.ExperimentTemplateNameAnnotationKey: templateName,

diff --git a/rollout/experiment.go b/rollout/experiment.go
@@ -64,9 +64,7 @@ func GetExperimentFromTemplate(r *v1alpha1.Rollout, stableRS, newRS *appsv1.Repl
 			Name:     templateStep.Name,
 			Replicas: templateStep.Replicas,
 		}
-		if templateStep.Weight != nil {
-			template.Service = &v1alpha1.TemplateService{}
-		}
+		template.Service = &v1alpha1.TemplateService{}
 		templateRS := &appsv1.ReplicaSet{}
 		switch templateStep.SpecRef {
 		case v1alpha1.CanarySpecRef:

diff --git a/rollout/experiment_test.go b/rollout/experiment_test.go
@@ -791,7 +791,7 @@ func TestRolloutCreateExperimentWithService(t *testing.T) {
 					Replicas: pointer.Int32Ptr(1),
 					Weight:   pointer.Int32Ptr(5),
 				},
-				// Service should NOT be created for "canary-template"
+				// Service should also be created for "canary-template"
 				{
 					Name:     "canary-template",
 					SpecRef:  v1alpha1.CanarySpecRef,
@@ -818,5 +818,5 @@ func TestRolloutCreateExperimentWithService(t *testing.T) {
 	assert.NotNil(t, ex.Spec.Templates[0].Service)
 
 	assert.Equal(t, "canary-template", ex.Spec.Templates[1].Name)
-	assert.Nil(t, ex.Spec.Templates[1].Service)
+	assert.NotNil(t, ex.Spec.Templates[1].Service)
 }
diff --git a/test/e2e/functional/experiment-with-service.yaml b/test/e2e/functional/experiment-with-service.yaml
@@ -25,3 +25,6 @@ spec:
                 requests:
                   memory: 16Mi
                   cpu: 1m
+              ports:
+                - protocol: TCP
+                  containerPort: 8080