feat(controller): don't hardcode experiment ports; always create service #2397

alexef · 2022-11-04T16:07:30Z

superseeds: #2357

also relates to: #2396

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.

Fixes: argoproj#2233 Signed-off-by: Alex Eftimie <[email protected]>

Create a service even though a weight is not set (when the name is provided) Signed-off-by: Alex Eftimie <[email protected]>

Signed-off-by: Alex Eftimie <[email protected]>

…periment-service-name

disable service where service is not needed or expected set ports where service must be created Signed-off-by: Alex Eftimie <[email protected]>

Signed-off-by: Alex Eftimie <[email protected]>

…service. update docs Signed-off-by: Alex Eftimie <[email protected]>

docs/features/experiment.md

github-actions · 2022-11-04T17:14:06Z

E2E Tests Published Test Results

    2 files     2 suites 1h 30m 30s ⏱️
  89 tests   84 ✔️ 3 💤 2 ❌
180 runs 172 ✔️ 6 💤 2 ❌

For more details on these failures, see this check.

Results for commit 31cf3d5.

♻️ This comment has been updated with latest results.

…ot nil Signed-off-by: Alex Eftimie <[email protected]>

docs/features/experiment.md

Signed-off-by: Alex Eftimie <[email protected]>

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

zachaller · 2022-11-05T14:25:06Z

In regards to the e2e tests, can you confirm for me that if we modify the code to not create the service if there is no ports defined on the pod spec. I think that should fix all the e2e tests and not require a port on the container which I think we should support

alexef · 2022-11-05T15:09:07Z

if we modify the code to not create the service if there is no ports defined on the pod spec

correct, I forgot about that. let me do this change

Signed-off-by: Alex Eftimie <[email protected]>

sonarcloud · 2022-11-05T16:13:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

…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]>

alexef · 2023-02-24T02:16:43Z

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 {


Feels like we can use weight: 0 to keep creating the service, if automated creation is rolled back

alexef added 27 commits October 21, 2022 15:23

Copy ports from replicaset rather than hardcoding them

952d043

Fixes: argoproj#2233 Signed-off-by: Alex Eftimie <[email protected]>

Allow user to set the experiment service name.

69e9061

Create a service even though a weight is not set (when the name is provided) Signed-off-by: Alex Eftimie <[email protected]>

Run codegen

d6ae5ad

Signed-off-by: Alex Eftimie <[email protected]>

Run codegen again, service name is optional

666d888

Signed-off-by: Alex Eftimie <[email protected]>

Fix conversion and tests

431f894

Signed-off-by: Alex Eftimie <[email protected]>

Fix codegen

49b01f1

Signed-off-by: Alex Eftimie <[email protected]>

Fix codegen again

21adb4a

Signed-off-by: Alex Eftimie <[email protected]>

white space

f534f9c

Signed-off-by: Alex Eftimie <[email protected]>

Extend from corev1.ServiceSpec

2b22947

Signed-off-by: Alex Eftimie <[email protected]>

make lint

0f28e2b

Signed-off-by: Alex Eftimie <[email protected]>

Merge branch 'master' into experiment-service-name

6525b3f

infer ports from ReplicaSet. this should fix e2e tests

d805441

Signed-off-by: Alex Eftimie <[email protected]>

Merge remote-tracking branch 'origin/experiment-service-name' into ex…

5b66607

…periment-service-name

fix e2e

303b29e

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

d1371cd

Signed-off-by: Alex Eftimie <[email protected]>

Add codegen openapi_generated.go

e1247e1

Signed-off-by: Alex Eftimie <[email protected]>

fix codegen, protobuf is needed

b8c4157

Signed-off-by: Alex Eftimie <[email protected]>

Merge branch 'master' into experiment-service-name

8c671cc

Add unit test

de6b858

Signed-off-by: Alex Eftimie <[email protected]>

Add unit test to new not tested code - copy pods from RS

bc379f7

Signed-off-by: Alex Eftimie <[email protected]>

Merge branch 'master' into experiment-service-name

9437f6d

Merge branch 'master' into experiment-service-name

9a179d4

Address PR comment: set entire spec

c32cbe7

Signed-off-by: Alex Eftimie <[email protected]>

docs: update experiment and rollout spec

0f5ed7a

Signed-off-by: Alex Eftimie <[email protected]>

Merge branch 'master' into experiment-service-name

8f46d8c

Merge branch 'master' into experiment-service-name

95b48ca

redo pr. drop spec changes. only inherit ports and always create the …

09ac6e1

…service. update docs Signed-off-by: Alex Eftimie <[email protected]>

alexef mentioned this pull request Nov 4, 2022

feat(controller): allow setting the Experiment's service name; don't hardcode ports #2357

Closed

6 tasks

Merge branch 'master' into experiment-ports

9e77224

alexef commented Nov 4, 2022

View reviewed changes

docs/features/experiment.md Show resolved Hide resolved

revert changes. only enable service if weighted or template service n…

fcbb31a

…ot nil Signed-off-by: Alex Eftimie <[email protected]>

alexef force-pushed the experiment-ports branch from 006cb4f to fcbb31a Compare November 4, 2022 18:52

alexef commented Nov 4, 2022

View reviewed changes

docs/features/experiment.md Show resolved Hide resolved

alexef added 5 commits November 4, 2022 19:55

typo in types

0c1984d

Signed-off-by: Alex Eftimie <[email protected]>

go fmt. need pre-commit hook

2a073c5

Signed-off-by: Alex Eftimie <[email protected]>

no more spec changes

9d2ce03

Signed-off-by: Alex Eftimie <[email protected]>

drop test which is not relevant any more

80eea63

Signed-off-by: Alex Eftimie <[email protected]>

Merge branch 'master' into experiment-ports

4a1fa58

alexef commented Nov 4, 2022

View reviewed changes

test/e2e/functional/experiment-with-service.yaml Show resolved Hide resolved

alexef requested a review from zachaller November 4, 2022 20:07

alexef added 2 commits November 5, 2022 16:35

fix e2e - only generate service if RS has ports

8771a96

Signed-off-by: Alex Eftimie <[email protected]>

fix e2e

31cf3d5

Signed-off-by: Alex Eftimie <[email protected]>

zachaller approved these changes Nov 8, 2022

View reviewed changes

zachaller merged commit 40e0166 into argoproj:master Nov 8, 2022

This was referenced Nov 8, 2022

feat: Expose metric for rollout abort with analysisrun failure #2149

Open

fix(experiment): Allow service creation with configurable ports. Fixes #2233 #2257

Closed

alexef deleted the experiment-ports branch November 21, 2022 11:39

zachaller mentioned this pull request Dec 8, 2022

Rollout experiment validate dupplicated containerPort with different name on template specs #2271

Closed

daniddelrio mentioned this pull request Feb 7, 2023

feat: Add name attribute to ServicePort #2572

Merged

6 tasks

This was referenced Feb 23, 2023

Breaking change with v1.4: experiment k8s service created even if steps are not weighted #2622

Closed

Experiment service names too long for DNS conventions #1637

Open

alexef commented Feb 24, 2023

View reviewed changes

daniddelrio mentioned this pull request Feb 24, 2023

fix: Rollback change on service creation with weightless experiments #2624

Merged

6 tasks

farid-akhoundov mentioned this pull request Mar 6, 2024

Add ability to configure custom ports for experiment services that differ from ReplicaSet container ports. #3428

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(controller): don't hardcode experiment ports; always create service #2397

feat(controller): don't hardcode experiment ports; always create service #2397

alexef commented Nov 4, 2022 •

edited

Loading

github-actions bot commented Nov 4, 2022 •

edited

Loading

zachaller commented Nov 5, 2022 •

edited

Loading

alexef commented Nov 5, 2022

sonarcloud bot commented Nov 5, 2022

alexef Feb 24, 2023

feat(controller): don't hardcode experiment ports; always create service #2397

feat(controller): don't hardcode experiment ports; always create service #2397

Conversation

alexef commented Nov 4, 2022 • edited Loading

github-actions bot commented Nov 4, 2022 • edited Loading

E2E Tests Published Test Results

zachaller commented Nov 5, 2022 • edited Loading

alexef commented Nov 5, 2022

sonarcloud bot commented Nov 5, 2022

alexef Feb 24, 2023

Choose a reason for hiding this comment

alexef commented Nov 4, 2022 •

edited

Loading

github-actions bot commented Nov 4, 2022 •

edited

Loading

zachaller commented Nov 5, 2022 •

edited

Loading