-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
swarm: Add update/rollback order #30261
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -175,6 +175,115 @@ func (s *DockerSwarmSuite) TestAPISwarmServicesUpdate(c *check.C) { | |
map[string]int{image1: instances}) | ||
} | ||
|
||
func (s *DockerSwarmSuite) TestAPISwarmServicesUpdateStartFirst(c *check.C) { | ||
d := s.AddDaemon(c, true, true) | ||
|
||
// service image at start | ||
image1 := "busybox:latest" | ||
// target image in update | ||
image2 := "testhealth" | ||
|
||
// service started from this image won't pass health check | ||
_, _, err := d.BuildImageWithOut(image2, | ||
`FROM busybox | ||
HEALTHCHECK --interval=1s --timeout=1s --retries=1024\ | ||
CMD cat /status`, | ||
true) | ||
c.Check(err, check.IsNil) | ||
|
||
// create service | ||
instances := 5 | ||
parallelism := 2 | ||
rollbackParallelism := 3 | ||
id := d.CreateService(c, serviceForUpdate, setInstances(instances), setUpdateOrder(swarm.UpdateOrderStartFirst), setRollbackOrder(swarm.UpdateOrderStartFirst)) | ||
|
||
checkStartingTasks := func(expected int) []swarm.Task { | ||
var startingTasks []swarm.Task | ||
waitAndAssert(c, defaultReconciliationTimeout, func(c *check.C) (interface{}, check.CommentInterface) { | ||
tasks := d.GetServiceTasks(c, id) | ||
startingTasks = nil | ||
for _, t := range tasks { | ||
if t.Status.State == swarm.TaskStateStarting { | ||
startingTasks = append(startingTasks, t) | ||
} | ||
} | ||
return startingTasks, nil | ||
}, checker.HasLen, expected) | ||
|
||
return startingTasks | ||
} | ||
|
||
makeTasksHealthy := func(tasks []swarm.Task) { | ||
for _, t := range tasks { | ||
containerID := t.Status.ContainerStatus.ContainerID | ||
d.Cmd("exec", containerID, "touch", "/status") | ||
} | ||
} | ||
|
||
// wait for tasks ready | ||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image1: instances}) | ||
|
||
// issue service update | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is admittedly quite nit picky, but I personally don't find these inline comments provide much value. Many of them just echo the function name ("make it healthy", "wait for tasks ready", "update service"). These can lead to confusion when the code is modified and the comments no longer reflect reality. I subscribe to the school of thought that inline comments should generally be avoided. They can be used sparingly when there is a very unexpected line that needs explanation. Instead of inline comments, code should be structured to reveal it's intent. Comments on functions and structs on the other hand are great, because they can be used to generate documentation, and they apply to a well defined block of code (the function body or struct definition). Applying this rule here, some of these comments could be deleted (as the function names are already explicit enough), others (like "wait for tasks ready") could be replaced by extracting a new function, which would also remove duplication: func waitForTasksReady(c *check.C, tasks map[string]int) {
...
} Are there benefits to this style of commenting that I'm missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mostly agree. Note that the comments were copied from |
||
service := d.GetService(c, id) | ||
d.UpdateService(c, service, setImage(image2)) | ||
|
||
// first batch | ||
|
||
// The old tasks should be running, and the new ones should be starting. | ||
startingTasks := checkStartingTasks(parallelism) | ||
|
||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image1: instances}) | ||
|
||
// make it healthy | ||
makeTasksHealthy(startingTasks) | ||
|
||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image1: instances - parallelism, image2: parallelism}) | ||
|
||
// 2nd batch | ||
|
||
// The old tasks should be running, and the new ones should be starting. | ||
startingTasks = checkStartingTasks(parallelism) | ||
|
||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image1: instances - parallelism, image2: parallelism}) | ||
|
||
// make it healthy | ||
makeTasksHealthy(startingTasks) | ||
|
||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image1: instances - 2*parallelism, image2: 2 * parallelism}) | ||
|
||
// 3nd batch | ||
|
||
// The old tasks should be running, and the new ones should be starting. | ||
startingTasks = checkStartingTasks(1) | ||
|
||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image1: instances - 2*parallelism, image2: 2 * parallelism}) | ||
|
||
// make it healthy | ||
makeTasksHealthy(startingTasks) | ||
|
||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image2: instances}) | ||
|
||
// Roll back to the previous version. This uses the CLI because | ||
// rollback is a client-side operation. | ||
out, err := d.Cmd("service", "update", "--rollback", id) | ||
c.Assert(err, checker.IsNil, check.Commentf(out)) | ||
|
||
// first batch | ||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image2: instances - rollbackParallelism, image1: rollbackParallelism}) | ||
|
||
// 2nd batch | ||
waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskImages, checker.DeepEquals, | ||
map[string]int{image1: instances}) | ||
} | ||
|
||
func (s *DockerSwarmSuite) TestAPISwarmServicesFailedUpdate(c *check.C) { | ||
const nodeCount = 3 | ||
var daemons [nodeCount]*daemon.Swarm | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stating default in
service update
command is a little misleading. If you don't specify the flag, it keeps existing value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, however the other
--update-*
flags have the same problem.Let's deal with this in a separate PR. Would you mind filing an issue?