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

Jump to bottom

Fix cpu/memory limits and reservations being reset on service update #1079

Merged
merged 2 commits into from
May 24, 2018
Merged

Fix cpu/memory limits and reservations being reset on service update #1079

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
df43eb9
Fix cpu/memory limits and reservations being reset on service update
thaJeztah May 23, 2018
df9a0c7
Minor refactor: use anyChanged() to detect changed flags
thaJeztah May 23, 2018
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
9 changes: 5 additions & 4 deletions cli/command/service/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,14 @@ func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags
return err
}

if flags.Changed(flagLimitCPU) || flags.Changed(flagLimitMemory) {
taskResources().Limits = &swarm.Resources{}
if anyChanged(flags, flagLimitCPU, flagLimitMemory) {
taskResources().Limits = spec.TaskTemplate.Resources.Limits
updateInt64Value(flagLimitCPU, &task.Resources.Limits.NanoCPUs)
updateInt64Value(flagLimitMemory, &task.Resources.Limits.MemoryBytes)
}
if flags.Changed(flagReserveCPU) || flags.Changed(flagReserveMemory) {
taskResources().Reservations = &swarm.Resources{}

if anyChanged(flags, flagReserveCPU, flagReserveMemory) {
taskResources().Reservations = spec.TaskTemplate.Resources.Reservations
updateInt64Value(flagReserveCPU, &task.Resources.Reservations.NanoCPUs)
updateInt64Value(flagReserveMemory, &task.Resources.Reservations.MemoryBytes)
}
Expand Down
44 changes: 44 additions & 0 deletions cli/command/service/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,50 @@ func TestUpdateIsolationValid(t *testing.T) {
assert.Check(t, is.Equal(container.IsolationProcess, spec.TaskTemplate.ContainerSpec.Isolation))
}

// TestUpdateLimitsReservations tests that limits and reservations are updated,
// and that values are not updated are not reset to their default value
func TestUpdateLimitsReservations(t *testing.T) {
spec := swarm.ServiceSpec{
TaskTemplate: swarm.TaskSpec{
ContainerSpec: &swarm.ContainerSpec{},
Resources: &swarm.ResourceRequirements{
Limits: &swarm.Resources{
NanoCPUs: 1000000000,
MemoryBytes: 104857600,
},
Reservations: &swarm.Resources{
NanoCPUs: 1000000000,
MemoryBytes: 104857600,
},
},
},
}

flags := newUpdateCommand(nil).Flags()
err := flags.Set(flagLimitCPU, "2")
assert.NilError(t, err)
err = flags.Set(flagReserveCPU, "2")
assert.NilError(t, err)
err = updateService(context.Background(), nil, flags, &spec)
assert.NilError(t, err)
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Limits.NanoCPUs, int64(2000000000)))
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Limits.MemoryBytes, int64(104857600)))
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Reservations.NanoCPUs, int64(2000000000)))
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Reservations.MemoryBytes, int64(104857600)))

flags = newUpdateCommand(nil).Flags()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: split it into 2 subtests?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean: only change (eg) reservation so that we can catch regressions where changing a reservation would reset limits?

Actually thought about that, don't know why I didn't do that; let me know if you want that updated in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, in case of failure it points directly to the right sub-test. But it's only a small nit, no need to update it 😄

err = flags.Set(flagLimitMemory, "200M")
assert.NilError(t, err)
err = flags.Set(flagReserveMemory, "200M")
assert.NilError(t, err)
err = updateService(context.Background(), nil, flags, &spec)
assert.NilError(t, err)
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Limits.NanoCPUs, int64(2000000000)))
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Limits.MemoryBytes, int64(209715200)))
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Reservations.NanoCPUs, int64(2000000000)))
assert.Check(t, is.Equal(spec.TaskTemplate.Resources.Reservations.MemoryBytes, int64(209715200)))
}

func TestUpdateIsolationInvalid(t *testing.T) {
// validation depends on daemon os / version so validation should be done on the daemon side
flags := newUpdateCommand(nil).Flags()
Expand Down