Add separate rollback parameters #1878
Conversation
aaronlehmann
force-pushed
the
rollback-parameters
branch
from
e72638e
to
b1ba935
Compare
We may use a |
Codecov Report@@ Coverage Diff @@
## master #1878 +/- ##
==========================================
+ Coverage 54.01% 54.18% +0.16%
==========================================
Files 108 108
Lines 18535 18552 +17
==========================================
+ Hits 10012 10052 +40
+ Misses 7303 7273 -30
- Partials 1220 1227 +7Continue to review full report at Codecov.
|
aaronlehmann
force-pushed
the
rollback-parameters
branch
from
b1ba935
to
963de9e
Compare
| flags.String("update-on-failure", "pause", "action on failure during update (pause|continue)") | ||
| flags.String("update-on-failure", "pause", "action on failure during update (pause|continue|rollback)") | ||
|
|
||
| flags.Uint64("rollback-parallelism", 0, "task update parallelism during rollback (0 = all at once)") |
There was a problem hiding this comment.
This default doesn't sound right. It'd interrupt service. Same with update-parallelism. How about changing default to 2 halves?
There was a problem hiding this comment.
I think this should have the same default as update-parallelism. I know that in Docker update-parallelism defaults to 1. We should probably fix it to do the same here. It's not really easy though - under our current system, the only way to do that is to hardcode those defaults in the client. I have a proposal for doing this better in #1744.
There was a problem hiding this comment.
I think #1744 is a good idea.
| if err != nil { | ||
| return err | ||
| } | ||
| if spec.Rollback == nil { |
There was a problem hiding this comment.
Since there are default values, I'd suggest always create spec.Rollback.
aaronlehmann
force-pushed
the
rollback-parameters
branch
from
963de9e
to
8149cf0
Compare
|
@aluzzardi: I've updated this to have a |
|
Design looks good to me. Please rebase. |
aaronlehmann
force-pushed
the
rollback-parameters
branch
from
8149cf0
to
def4391
Compare
|
Thanks. Rebased. |
|
Rebased again. Review ping |
Rather than reusing the values Spec.Update during a the rollback of an update, use parameters from a separate Spec.Rollback UpdateConfig. This allows configurations such as a rolling, gradual update, and a fast parallel rollback to the original version if things go wrong. If no Rollback message is provided, defaults will apply. This also adds a Rollback flag to UpdateServiceRequest so that manually triggered rollbacks (which are currently client-side operations) can be initiated server-side, and have the correct update settings applied. We also need to make sure not to trigger an automatic rollback from a failed manual rollback. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
aaronlehmann
force-pushed
the
rollback-parameters
branch
2 times, most recently
from
c1ae7e1
to
c37c646
Compare
|
LGTM |
1 similar comment
|
LGTM |
Rather than reusing the values
Spec.Updateduring a the rollback of anupdate, use parameters from a separate
Spec.RollbackUpdateConfig. Thisallows configurations such as a rolling, gradual update, and a fast
parallel rollback to the original version if things go wrong.
If no
Rollbackmessage is provided, defaults will apply.This also adds a
Rollbackflag toUpdateServiceRequestso that manuallytriggered rollbacks (which are currently client-side operations) can be
initiated server-side, and have the correct update settings applied. We
also need to make sure not to trigger an automatic rollback from a
failed manual rollback.