Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae32617
Rollback change on service creation with weightless experiments
daniddelrio Feb 24, 2023
0e8ea9e
Update documentation on service creation
daniddelrio Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/features/experiment.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ 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.
A Service routing traffic to the Experiment ReplicaSet is also generated if a weight for that experiment is set.

## Use cases of Experiments

Expand Down Expand Up @@ -245,7 +245,7 @@ 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
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
Expand Down
4 changes: 3 additions & 1 deletion rollout/experiment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ func GetExperimentFromTemplate(r *v1alpha1.Rollout, stableRS, newRS *appsv1.Repl
Name: templateStep.Name,
Replicas: templateStep.Replicas,
}
template.Service = &v1alpha1.TemplateService{}
if templateStep.Weight != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if templateStep.Weight != nil {
if (templateStep.Weight != nil || templateStep.Service != nil) {

should we put this already in?

atm the spec doesn't allow templateStep.Service to be anything but:

  • nil
  • empty object

so this shouldn't be a breaking change (famous last words)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see: https://github.com/alexef/argo-rollouts/blob/master/pkg/apis/rollouts/v1alpha1/experiment_types.go#L92

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, this is not possible without a Rollout spec change.

template.Service = &v1alpha1.TemplateService{}
}
templateRS := &appsv1.ReplicaSet{}
switch templateStep.SpecRef {
case v1alpha1.CanarySpecRef:
Expand Down
4 changes: 2 additions & 2 deletions rollout/experiment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ func TestRolloutCreateExperimentWithService(t *testing.T) {
Replicas: pointer.Int32Ptr(1),
Weight: pointer.Int32Ptr(5),
},
// Service should also be created for "canary-template"
// Service should NOT be created for "canary-template"
{
Name: "canary-template",
SpecRef: v1alpha1.CanarySpecRef,
Expand All @@ -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.NotNil(t, ex.Spec.Templates[1].Service)
assert.Nil(t, ex.Spec.Templates[1].Service)
}