Skip to content

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

Merged
vdemeester merged 2 commits intodocker:masterfrom
thaJeztah:fix_update_memory_cpu_limit
May 24, 2018
Merged

Fix cpu/memory limits and reservations being reset on service update#1079
vdemeester merged 2 commits intodocker:masterfrom
thaJeztah:fix_update_memory_cpu_limit

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

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

fixes moby/moby#37036
fixes moby/moby#37037

Before this change:

Create a service with reservations and limits for memory and cpu:

docker service create \
  --limit-memory=100M \
  --limit-cpu=1 \
  --reserve-memory=100M \
  --reserve-cpu=1 \
  --name test \
  nginx:alpine

Verify the configuration

docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test | jq . 
{
  "Limits": {
    "NanoCPUs": 1000000000,
    "MemoryBytes": 104857600
  },
  "Reservations": {
    "NanoCPUs": 1000000000,
    "MemoryBytes": 104857600
  }
}

Update just CPU limit and reservation:

docker service update --limit-cpu=2 --reserve-cpu=2 test

Notice that the memory limit and reservation is not preserved:

docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test | jq .
{
  "Limits": {
    "NanoCPUs": 2000000000
  },
  "Reservations": {
    "NanoCPUs": 2000000000
  }
}

Update just Memory limit and reservation:

docker service update --limit-memory=200M --reserve-memory=200M test

Notice that the CPU limit and reservation is not preserved:

docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test | jq .
{
  "Limits": {
    "MemoryBytes": 209715200
  },
  "Reservations": {
    "MemoryBytes": 209715200
  }
}

After this change:

Create a service with reservations and limits for memory and cpu:

docker service create \
  --limit-memory=100M \
  --limit-cpu=1 \
  --reserve-memory=100M \
  --reserve-cpu=1 \
  --name test \
  nginx:alpine

Verify the configuration

docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test | jq . 
{
  "Limits": {
    "NanoCPUs": 1000000000,
    "MemoryBytes": 104857600
  },
  "Reservations": {
    "NanoCPUs": 1000000000,
    "MemoryBytes": 104857600
  }
}

Update just CPU limit and reservation:

docker service update --limit-cpu=2 --reserve-cpu=2 test

Confirm that the CPU limits/reservations are updated, but memory limit and reservation are preserved:

docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test | jq .
{
  "Limits": {
    "NanoCPUs": 2000000000,
    "MemoryBytes": 104857600
  },
  "Reservations": {
    "NanoCPUs": 2000000000,
    "MemoryBytes": 104857600
  }
}

Update just Memory limit and reservation:

docker service update --limit-memory=200M --reserve-memory=200M test

Confirm that the Mempry limits/reservations are updated, but CPU limit and reservation are preserved:

docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test | jq .
{
  "Limits": {
    "NanoCPUs": 2000000000,
    "MemoryBytes": 209715200
  },
  "Reservations": {
    "NanoCPUs": 2000000000,
    "MemoryBytes": 209715200
  }
}

thaJeztah added 2 commits May 24, 2018 01:30
Before this change:
----------------------------------------------------

Create a service with reservations and limits for memory and cpu:

    docker service create --name test \
      --limit-memory=100M --limit-cpu=1 \
      --reserve-memory=100M --reserve-cpu=1 \
      nginx:alpine

Verify the configuration

    docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test
    {
      "Limits": {
        "NanoCPUs": 1000000000,
        "MemoryBytes": 104857600
      },
      "Reservations": {
        "NanoCPUs": 1000000000,
        "MemoryBytes": 104857600
      }
    }

Update just CPU limit and reservation:

    docker service update --limit-cpu=2 --reserve-cpu=2 test

Notice that the memory limit and reservation is not preserved:

    docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test
    {
      "Limits": {
        "NanoCPUs": 2000000000
      },
      "Reservations": {
        "NanoCPUs": 2000000000
      }
    }

Update just Memory limit and reservation:

    docker service update --limit-memory=200M --reserve-memory=200M test

Notice that the CPU limit and reservation is not preserved:

    docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test
    {
      "Limits": {
        "MemoryBytes": 209715200
      },
      "Reservations": {
        "MemoryBytes": 209715200
      }
    }

After this change:
----------------------------------------------------

Create a service with reservations and limits for memory and cpu:

    docker service create --name test \
      --limit-memory=100M --limit-cpu=1 \
      --reserve-memory=100M --reserve-cpu=1 \
      nginx:alpine

Verify the configuration

    docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test
    {
      "Limits": {
        "NanoCPUs": 1000000000,
        "MemoryBytes": 104857600
      },
      "Reservations": {
        "NanoCPUs": 1000000000,
        "MemoryBytes": 104857600
      }
    }

Update just CPU limit and reservation:

    docker service update --limit-cpu=2 --reserve-cpu=2 test

Confirm that the CPU limits/reservations are updated, but memory limit and reservation are preserved:

    docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test
    {
      "Limits": {
        "NanoCPUs": 2000000000,
        "MemoryBytes": 104857600
      },
      "Reservations": {
        "NanoCPUs": 2000000000,
        "MemoryBytes": 104857600
      }
    }

Update just Memory limit and reservation:

    docker service update --limit-memory=200M --reserve-memory=200M test

Confirm that the Mempry limits/reservations are updated, but CPU limit and reservation are preserved:

    docker service inspect --format '{{json .Spec.TaskTemplate.Resources}}' test
    {
      "Limits": {
        "NanoCPUs": 2000000000,
        "MemoryBytes": 209715200
      },
      "Reservations": {
        "NanoCPUs": 2000000000,
        "MemoryBytes": 209715200
      }
    }

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the fix_update_memory_cpu_limit branch from 22c9a9e to df9a0c7 Compare May 23, 2018 23:30
Copy link
Copy Markdown
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🦁

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 😄

Copy link
Copy Markdown
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@vdemeester vdemeester merged commit 57ce5aa into docker:master May 24, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.06.0 milestone May 24, 2018
@thaJeztah thaJeztah deleted the fix_update_memory_cpu_limit branch May 24, 2018 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

5 participants