feat: Add name attribute to ServicePort #2572

daniddelrio · 2023-02-07T13:52:43Z

We encountered this error when we tried to run an Experiment for a service with multiple named ports:

Failed to create Service for template 'smoketest': Service
    is invalid: [spec.ports[0].name: Required value,
    spec.ports[1].name: Required value, spec.ports[2].name:
    Required value]

Argo Rollouts currently only copies the port number and protocol from the Rollout to the service it creates during experiments, but not the name. This PR addresses this issue by also including the port name when copying.

Relates to: #2397

fixes: #2599

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed my commits with DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

Signed-off-by: Daniel Del Rio <[email protected]>

github-actions · 2023-02-07T14:11:19Z

Go Published Test Results

1 850 tests 1 850 ✔️ 2m 31s ⏱️
  112 suites       0 💤
      1 files       0 ❌

Results for commit 96a9313.

♻️ This comment has been updated with latest results.

github-actions · 2023-02-07T14:36:57Z

E2E Tests Published Test Results

    2 files     2 suites 1h 35m 9s ⏱️
  96 tests   91 ✔️ 3 💤 2 ❌
194 runs 186 ✔️ 6 💤 2 ❌

For more details on these failures, see this check.

Results for commit 96a9313.

♻️ This comment has been updated with latest results.

Signed-off-by: Daniel Del Rio <[email protected]>

codecov · 2023-02-07T14:51:40Z

Codecov Report

Base: 81.60% // Head: 81.60% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (96a9313) compared to base (24c9621).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2572   +/-   ##
=======================================
  Coverage   81.60%   81.60%           
=======================================
  Files         130      130           
  Lines       19481    19483    +2     
=======================================
+ Hits        15898    15900    +2     
  Misses       2766     2766           
  Partials      817      817

Impacted Files	Coverage Δ
experiments/experiment.go	`89.93% <100.00%> (+0.04%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

alexef · 2023-02-07T18:25:17Z

experiments/experiment.go

@@ -290,6 +290,7 @@ func (ec *experimentContext) createTemplateService(template *v1alpha1.TemplateSp
 	for _, ctr := range rs.Spec.Template.Spec.Containers {
 		for _, port := range ctr.Ports {
 			servicePort := corev1.ServicePort{
+				Name:       port.Name,


I believe ports can also be unnamed. what is the default value here, is it compatible? (ideally type's default which is "")

Documentation says it will be omitted if the name is not given. Should we make it by default an empty string if not given?

I would not set it at all

alexef · 2023-02-07T18:25:57Z

test/e2e/functional/experiment-with-service.yaml

@@ -26,5 +26,6 @@ spec:
                  memory: 16Mi
                  cpu: 1m
              ports:
-                - protocol: TCP
+                - name: testport


does the test fail when name is not present? can we rather have two tests, one with single, unnamed port, another one with two named ports?

Yes, test fails when name isn't present. I'll try that out

okay, so let's make sure the test doesn't fails without name first

Signed-off-by: Daniel Del Rio <[email protected]>

alexef · 2023-02-08T13:45:42Z

experiments/experiment_test.go

@@ -70,6 +70,7 @@ func setExperimentService(template *v1alpha1.TemplateSpec) {
 	template.Service = &v1alpha1.TemplateService{}
 	template.Template.Spec.Containers[0].Ports = []corev1.ContainerPort{
 		{
+			Name:          "testport",


this is not needed any more

Signed-off-by: Daniel Del Rio <[email protected]>

zachaller · 2023-02-08T14:54:17Z

test/e2e/experiment_test.go

+	g.ApplyManifests("@functional/experiment-with-multiport-service.yaml")
+	g.When().
+		WaitForExperimentPhase("experiment-with-multiport-service", "Running").
+		Sleep(time.Second*5).


Suggested change

Sleep(time.Second*5).

WaitForExperimentCondition("experiment-with-service", func(ex *rov1.Experiment) bool {

return s.GetReplicaSetFromExperiment(ex, "test").Status.Replicas == 1

}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).

I saw a few test have these long waits I would rather not slow down the e2e tests any more, I think this is what you where trying to accomplish if so could you switch to it?

@alexef what do you think about this?

the proposed change makes sense, most of the time rs gets ready in less than 5 seconds

zachaller · 2023-02-08T14:54:43Z

test/e2e/experiment_test.go

+		ExpectExperimentServiceCount("experiment-with-multiport-service", 1).
+		When().
+		WaitForExperimentPhase("experiment-with-multiport-service", "Successful").
+		Sleep(time.Second*15).


Suggested change

Sleep(time.Second*15).

WaitForExperimentCondition("experiment-with-service", func(ex *rov1.Experiment) bool {

return s.GetReplicaSetFromExperiment(ex, "test").Status.Replicas == 0

}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).

I saw a few test have these long waits I would rather not slow down the e2e tests any more, I think this is what you where trying to accomplish if so could you switch to it?

Signed-off-by: Daniel Del Rio <[email protected]>

sonarcloud · 2023-02-09T09:34:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
5.1% Duplication

zachaller · 2023-02-09T15:04:27Z

Thanks for contributing!

* Add name attribute to ServicePort Signed-off-by: Daniel Del Rio <[email protected]> * Format with gofmt Signed-off-by: Daniel Del Rio <[email protected]> * Update experiment test by adding port name Signed-off-by: Daniel Del Rio <[email protected]> * Include port name only when given Signed-off-by: Daniel Del Rio <[email protected]> * Implement separate e2e tests for single unnamed port and two named ports Signed-off-by: Daniel Del Rio <[email protected]> * Remove port name in experiment_test.go Signed-off-by: Daniel Del Rio <[email protected]> * Replace Sleep in new multiport service experiment Signed-off-by: Daniel Del Rio <[email protected]> --------- Signed-off-by: Daniel Del Rio <[email protected]> Signed-off-by: kaldorn <[email protected]>

* Add name attribute to ServicePort Signed-off-by: Daniel Del Rio <[email protected]> * Format with gofmt Signed-off-by: Daniel Del Rio <[email protected]> * Update experiment test by adding port name Signed-off-by: Daniel Del Rio <[email protected]> * Include port name only when given Signed-off-by: Daniel Del Rio <[email protected]> * Implement separate e2e tests for single unnamed port and two named ports Signed-off-by: Daniel Del Rio <[email protected]> * Remove port name in experiment_test.go Signed-off-by: Daniel Del Rio <[email protected]> * Replace Sleep in new multiport service experiment Signed-off-by: Daniel Del Rio <[email protected]> --------- Signed-off-by: Daniel Del Rio <[email protected]>

daniddelrio changed the title ~~Add name attribute to ServicePort~~ feat: Add name attribute to ServicePort Feb 7, 2023

Add name attribute to ServicePort

6cd8202

Signed-off-by: Daniel Del Rio <[email protected]>

daniddelrio force-pushed the named-experiment-ports branch from e54d4cd to 6cd8202 Compare February 7, 2023 13:57

Format with gofmt

9d1b667

Signed-off-by: Daniel Del Rio <[email protected]>

Update experiment test by adding port name

ce7d56a

Signed-off-by: Daniel Del Rio <[email protected]>

alexef reviewed Feb 7, 2023

View reviewed changes

daniddelrio added 3 commits February 8, 2023 11:12

Include port name only when given

cd1c970

Signed-off-by: Daniel Del Rio <[email protected]>

Implement separate e2e tests for single unnamed port and two named ports

1821988

Signed-off-by: Daniel Del Rio <[email protected]>

Merge branch 'master' into named-experiment-ports

eba9bf1

daniddelrio marked this pull request as ready for review February 8, 2023 13:16

daniddelrio requested a review from alexef February 8, 2023 13:16

alexef reviewed Feb 8, 2023

View reviewed changes

alexef approved these changes Feb 8, 2023

View reviewed changes

alexef requested a review from zachaller February 8, 2023 13:45

alexef added the enhancement New feature or request label Feb 8, 2023

Remove port name in experiment_test.go

912ca6c

Signed-off-by: Daniel Del Rio <[email protected]>

zachaller reviewed Feb 8, 2023

View reviewed changes

daniddelrio added 2 commits February 9, 2023 10:28

Merge branch 'master' into named-experiment-ports

7b2a96a

Replace Sleep in new multiport service experiment

96a9313

Signed-off-by: Daniel Del Rio <[email protected]>

daniddelrio requested a review from zachaller February 9, 2023 10:11

zachaller approved these changes Feb 9, 2023

View reviewed changes

zachaller merged commit 0450778 into argoproj:master Feb 9, 2023

zachaller mentioned this pull request Feb 17, 2023

v1.4.0 Failed to create Service for template -- port.Name not being passed to CreateService #2599

Closed

2 tasks

zachaller added cherry-pick/release-1.4 cherry-pick-completed Used once we have cherry picked the PR to all requested releases labels Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add name attribute to ServicePort #2572

feat: Add name attribute to ServicePort #2572

daniddelrio commented Feb 7, 2023 •

edited by zachaller

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

codecov bot commented Feb 7, 2023 •

edited

Loading

alexef Feb 7, 2023

daniddelrio Feb 8, 2023

alexef Feb 8, 2023

alexef Feb 7, 2023

daniddelrio Feb 8, 2023

alexef Feb 8, 2023

alexef Feb 8, 2023

zachaller Feb 8, 2023 •

edited

Loading

daniddelrio Feb 9, 2023

alexef Feb 9, 2023

zachaller Feb 8, 2023 •

edited

Loading

sonarcloud bot commented Feb 9, 2023

zachaller commented Feb 9, 2023

-		Sleep(time.Second*5).
+                WaitForExperimentCondition("experiment-with-service", func(ex *rov1.Experiment) bool {
+			return s.GetReplicaSetFromExperiment(ex, "test").Status.Replicas == 1
+		}, "number-of-rs-pods-meet", fixtures.E2EWaitTimeout).

feat: Add name attribute to ServicePort #2572

feat: Add name attribute to ServicePort #2572

Conversation

daniddelrio commented Feb 7, 2023 • edited by zachaller Loading

github-actions bot commented Feb 7, 2023 • edited Loading

Go Published Test Results

github-actions bot commented Feb 7, 2023 • edited Loading

E2E Tests Published Test Results

codecov bot commented Feb 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachaller Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachaller Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Feb 9, 2023

zachaller commented Feb 9, 2023

daniddelrio commented Feb 7, 2023 •

edited by zachaller

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

codecov bot commented Feb 7, 2023 •

edited

Loading

zachaller Feb 8, 2023 •

edited

Loading

zachaller Feb 8, 2023 •

edited

Loading