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.

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

Make specifying replicas/duration optional in the experiment step #241

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

jessesuen
Copy link
Member

@jessesuen jessesuen commented Oct 30, 2019

This simplifies manifests by making replicas and duration as optional fields. Also it gets progressDeadlineSeconds from the rollout spec.

Builds ontop of #239, so only look at last commit.

@codecov
Copy link

codecov bot commented Oct 30, 2019

Codecov Report

Merging #241 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
+ Coverage   85.24%   85.25%   +0.01%     
==========================================
  Files          49       49              
  Lines        4386     4389       +3     
==========================================
+ Hits         3739     3742       +3     
  Misses        460      460              
  Partials      187      187
Impacted Files Coverage Δ
rollout/experiment.go 83.03% <100%> (ø) ⬆️
experiments/experiment.go 93.97% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37bc1f7...59bff86. Read the comment docs.

@jessesuen jessesuen changed the title Make specifying replicas optional in the experiment step Make specifying replicas/duration optional in the experiment step Oct 30, 2019
Copy link
Member

@dthomson25 dthomson25 left a comment

Choose a reason for hiding this comment

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

LGTM, but quick question: If we have no duration, how does the experiment know when to stop and continue the rollout's steps? If the experiment has an analysis, then it should finish once the analysis completes. Otherwise, how will the experiment end and allow the rollout to continue? Is this a valid configuration? If not, we should add some validation to prevent the user from creating a never-ending experiment.

@dthomson25 dthomson25 merged commit f19e19f into argoproj:master Oct 30, 2019
@jessesuen
Copy link
Member Author

LGTM, but quick question: If we have no duration, how does the experiment know when to stop and continue the rollout's steps? If the experiment has an analysis, then it should finish once the analysis completes. Otherwise, how will the experiment end and allow the rollout to continue? Is this a valid configuration? If not, we should add some validation to prevent the user from creating a never-ending experiment.

Sorry I forgot to answer this question. Never ending/indefinite experiments are a desired use case. To proceed to the next step of the rollout, a manual termination of the Experiment would need to be performed.

If the experiment has an analysis, then it should finish once the analysis completes.

Actually we may need to support both use cases. Currently stopping an experiment because all analysis completed, is not supported. In this scenario, the experiment will continue to run (to it's duration, or forever).

We will need new syntax to indicate that an Experiment should stop when analysis completes. This is a decision that might need to be made for individual analyses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants