From 6320e35cd29b8620bdb716b55f0467be9d046e54 Mon Sep 17 00:00:00 2001 From: Alex Eftimie Date: Tue, 8 Nov 2022 17:59:47 +0100 Subject: [PATCH] feat(controller): don't hardcode experiment ports; always create service (#2397) * Copy ports from replicaset rather than hardcoding them Fixes: #2233 Signed-off-by: Alex Eftimie * 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 * Run codegen Signed-off-by: Alex Eftimie * Run codegen again, service name is optional Signed-off-by: Alex Eftimie * Fix conversion and tests Signed-off-by: Alex Eftimie * Fix codegen Signed-off-by: Alex Eftimie * Fix codegen again Signed-off-by: Alex Eftimie * white space Signed-off-by: Alex Eftimie * Extend from corev1.ServiceSpec Signed-off-by: Alex Eftimie * make lint Signed-off-by: Alex Eftimie * infer ports from ReplicaSet. this should fix e2e tests Signed-off-by: Alex Eftimie * fix e2e disable service where service is not needed or expected set ports where service must be created Signed-off-by: Alex Eftimie * use json inline so that crd reflects ports Signed-off-by: Alex Eftimie * Add codegen openapi_generated.go Signed-off-by: Alex Eftimie * fix codegen, protobuf is needed Signed-off-by: Alex Eftimie * Add unit test Signed-off-by: Alex Eftimie * Add unit test to new not tested code - copy pods from RS Signed-off-by: Alex Eftimie * Address PR comment: set entire spec Signed-off-by: Alex Eftimie * docs: update experiment and rollout spec Signed-off-by: Alex Eftimie * redo pr. drop spec changes. only inherit ports and always create the service. update docs Signed-off-by: Alex Eftimie * Ran go fmt Signed-off-by: Alex Eftimie * revert changes. only enable service if weighted or template service not nil Signed-off-by: Alex Eftimie * typo in types Signed-off-by: Alex Eftimie * go fmt. need pre-commit hook Signed-off-by: Alex Eftimie * no more spec changes Signed-off-by: Alex Eftimie * drop test which is not relevant any more Signed-off-by: Alex Eftimie * fix e2e - only generate service if RS has ports Signed-off-by: Alex Eftimie * fix e2e Signed-off-by: Alex Eftimie Signed-off-by: Alex Eftimie --- docs/features/experiment.md | 10 ++++- docs/features/specification.md | 2 + experiments/experiment.go | 16 +++++++- experiments/experiment_test.go | 38 ++++++++++++++++++- experiments/service.go | 13 ++----- rollout/experiment.go | 4 +- rollout/experiment_test.go | 4 +- .../functional/experiment-with-service.yaml | 3 ++ 8 files changed, 70 insertions(+), 20 deletions(-) diff --git a/docs/features/experiment.md b/docs/features/experiment.md index 41057d866b..6740895d68 100644 --- 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. \ No newline at end of file + 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. \ No newline at end of file diff --git a/docs/features/specification.md b/docs/features/specification.md index 100ddbb430..64ee171972 100644 --- 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 index aa154d03d9..0ab5d3bb8a 100644 --- 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 index 60c04e0723..0860188f65 100644 --- 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 index 286c3b537a..3f165e76a5 100644 --- 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 index aeac6e8049..a6d3bdcd43 100644 --- 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 index e2e50c1f28..1c60e9f7c7 100644 --- 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 index d3f5a8c9eb..76cbddaeed 100644 --- 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