Adding support for memory swap settings for services#37872
Adding support for memory swap settings for services#37872wk8 wants to merge 2 commits intomoby:masterfrom
Conversation
|
Let's be careful about how we expose these settings. |
24dcec3 to
79035b9
Compare
|
Please sign your commits following these rules: $ git clone -b "wk8/memory_flags_for_swarm" git@github.com:wk8/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354426360
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
79035b9 to
6e825ea
Compare
478cc07 to
1a29d63
Compare
bf27614 to
d7c123b
Compare
b03fc60 to
a3ecf6e
Compare
a3ecf6e to
7d740c0
Compare
7d740c0 to
7a1f141
Compare
7a1f141 to
1aa7385
Compare
1aa7385 to
2a6f9ea
Compare
2a6f9ea to
2cb44ac
Compare
2cb44ac to
4a05d9b
Compare
integration/service/create_test.go
Outdated
There was a problem hiding this comment.
@anshulpundir that's the failing test. No timing issue at all :( it's set to -1 when failing...
4a05d9b to
f744db0
Compare
62b57e5 to
22dbd05
Compare
There was a problem hiding this comment.
Thinking if we should still keep them separate 🤔 (even though they should be set together)
Also, using *int64 may be better, to prevent having to set this functional argument "optionally" (see the discussion on #38793 (comment)), i.e.;
// ServiceWithMemorySwap sets the memory swap value of the service's TaskSpec
// it also requires setting a memory limit, since the engine enforces that
func ServiceWithMemorySwap(swap *int64, memoryLimit int64) ServiceSpecOpt {
return withEnsureResourceLimits(func(spec *swarmtypes.ServiceSpec) {
spec.TaskTemplate.Resources.SwapBytes = swap
spec.TaskTemplate.Resources.Limits.MemoryBytes = memoryLimit
})
}There was a problem hiding this comment.
It doesn't really make sense to use one without the other, so I'm not sure it's a good idea to keep them separate.
Changing to a pointer for swap though.
integration/service/create_test.go
Outdated
There was a problem hiding this comment.
Should probably use testEnv.DaemonInfo, but is there any reason to not skip the test in this case?
skip.If(t, !testEnv.DaemonInfo.SwapLimit)There was a problem hiding this comment.
It's still worth checking that the info made it down to the service and task specs?
api/types/swarm/task.go
Outdated
There was a problem hiding this comment.
Reading back the earlier conversations #37872 (comment)
- adding these 2 fields to the existing
Limitsstruct
Weren't these fields supposed to go into Limits ?
There was a problem hiding this comment.
Except Limits shares the same struct as Reservations, and there is a case to be made to keep it that way: it seemed to make more sense to keep the symmetry there, and take fields that don't make sense as both a limit and a reservation out in the Requirements.
Besides, you reviewed and approved this in https://github.com/docker/swarmkit/pull/2816/files :) ?
There was a problem hiding this comment.
I think we could keep a base version of Resources for sharing between Limits and Reservations and let each diverge as needed.
Other than this, the changes in this PR look great.
There was a problem hiding this comment.
If r.SwapBytes is nil, should we still calculate it's value?
i.e.;
if r.SwapBytes == nil {
resources.MemorySwap = 2 * resources.Memory
}There was a problem hiding this comment.
That would be duplicating the logic that already does this at
Lines 340 to 343 in dc52692
|
@thaJeztah : rebased, and amended as requested; could you please take another look? |
5338d46 to
04f66be
Compare
With integration tests Relevant Swarmkit PR: moby/swarmkit#2816 (updated the vendored version of Swarkit to that) Signed-off-by: Jean Rouge <rougej+github@gmail.com>
|
@thaJeztah : could you please take another look? Thanks. |
|
@thaJeztah Could we revisit this again? Thanks! |
|
As per our discussion today with @thaJeztah & @cpuguy83 :
|
|
@thaJeztah & @cpuguy83 : before I do the changes outlined in #37872 (comment), could you please validate that https://github.com/docker/swarmkit/pull/2865/files#diff-2825f8c43cd6822a89834c265124fe46 looks good to you, and that this shape for the Limits/Resources structs should be the final one, and won't need to be changed another time? Thanks. |
Signed-off-by: Jean Rouge <rougej+github@gmail.com>
|
Is this ready to merge? |
|
Any update on this one? |
|
Hi, is this still being worked on? |
|
Any progress here? |
|
Hi, just wondering if this PR is still in progress as it would be useful to be able to deactivate swapping in swarm mode ? |
|
Any update? |
|
Still not possible to deactivate swap ? |
|
Any update? |
|
Anybody still out here....? |
|
Is anything happening with this? |
|
Has there been any further traction on this? |
|
Has there been any further traction on this till now?? |
- What I did
Adds support for memory swap options in services. That includes the API
plumbing, reflected in the API's doc. Added unit tests on the new feature.
- How I did it
Adds 2 new fields to service container specs, equivalent to
docker run's--memory-swapand--memory-swappiness.- How to verify it
Includes integration tests.
- Related issues
#34654
- Description for the changelog
Adds support for memory swap options in services.