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.

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

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

Merged
merged 2 commits into from
May 24, 2018

Conversation

thaJeztah
Copy link
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
  }
}

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 <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the fix_update_memory_cpu_limit branch from 22c9a9e to df9a0c7 Compare May 23, 2018 23:30
Copy link
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
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 😄

Copy link
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