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

Adds functionality to apply constraints to a LXD container spec #8869

Merged
merged 1 commit into from
Jun 28, 2018

Conversation

manadart
Copy link
Member

@manadart manadart commented Jun 28, 2018

Description of change

This change introduces constraints support for LXD container machines.

The currently supported constraints are:

  • Instance type
  • CPU cores
  • Memory

Instance types are congruent with AWS, GCE and Azure types. Those are listed here:
https://github.com/dustinkirkland/instance-type/tree/master/yaml

If an instance type constraint is supplied and any of CPU cores or memory are also supplied, these are ignored and a warning indicating such is logged.

Given recent networking changes, we should also be able to filter networking devices based on space constraints in a future patch.

QA steps

juju add-machine lxd:0 --constraints "cores=2 mem=1G"

  • Check the LXD config for the new container and verify that:
    • limits.cpu = "2" and;
    • limits.memory = "1024MB"

juju add-machine lxd:0 --constraints "cores=2 mem=1G instance-type=t2.micro"

  • SSH to the host machine, check that the warnings were logged.
  • Check the LXD config for the new container and verify that:
    • limits.cpu = "1" and;
    • limits.memory = "1024MB"

Documentation changes

Support for this enhancement should be documented.

Bug reference

N/A

Constraints for CPU cores, memory and instance type are supported.
@manadart
Copy link
Member Author

Ping @pmatulis to sync regarding docs.

@manadart
Copy link
Member Author

@manadart manadart requested a review from jameinel June 28, 2018 11:10
@manadart
Copy link
Member Author

Merge check failure looks related to https://bugs.launchpad.net/juju/+bug/1778986.
!!build!!

@manadart
Copy link
Member Author

Same again. !!build!!

Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

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

How hard would it be to support something like cpu affinity?
https://stgraber.org/2016/03/26/lxd-2-0-resource-control-412/

(I'm quite surprised about limits.cpu, how do you specify just the 3rd cpu, because it looks like:
limits.cpu 0,3
means only cpu 0 and 3
but
limits.cpu 2
means limit to '2' cpus.
I suppose it is whether the given item looks like a list?
Maybe
limits.cpu 2,
?

Anyway, it has been a field request to be able to constrain containers in that fashion, and it feels like this starts to open up that possibility. We should discuss syntax, etc.

@manadart
Copy link
Member Author

I recall there was some trepidation regarding using constraints (get me a machine satisfying these I.e. "at least") for container limits (at most).

There is nothing in constraints.Value that obviously does as a pass through to pinning CPUs, but we can thrash that out in said discussions.

@manadart
Copy link
Member Author

$$merge$$

@pmatulis
Copy link
Contributor

@manadart Confirm that these LXD constraints are maximums while traditional constraints are minimums?

@manadart
Copy link
Member Author

@pmatulis Indeed, confirmed.

@jujubot jujubot merged commit 4f97614 into juju:develop Jun 28, 2018
@manadart manadart deleted the lxd-machine-constraints branch June 28, 2018 13:14
@evilnick
Copy link
Member

I don't think that's really going to be easy to explain. I mean obviously, we can document it, but it means every time we mention constraints we have to say "(* not LXD)" or whatever. Unless I am misunderstanding. Also, it will actually mix the types of constraint in a single command e.g. in future when networks are added.
Would it not be better to use a more obvious syntax ( " cores<2") ?

@manadart
Copy link
Member Author

I will discuss with @jam and/or @mitechie and/or @howbazaar on how we should approach the concern here.

@manadart
Copy link
Member Author

manadart commented Jun 28, 2018

Note that the KVM container manager already implements constraints in similar fashion.

@manadart
Copy link
Member Author

@jameinel Down the page in the comments section of that post, the single core pinning question was asked. The answer is to use the range syntax.

lxc config set containerA limits.cpu 0-0

jujubot added a commit that referenced this pull request Jul 4, 2018
#8891

## Description of change

Removes the constrain validation added in #8869 and passes constraints through to LXD as config, as they are supplied.

This changes behaviour from #8869 where specifying an instance-type would override settings for CPU cores and/or memory.

LXD's behaviour, now deferred to, is for any specific CPU/mem constraints to override those implied by an instance type _even when they are lower_.

## QA steps

Only CPU and Memory
```juju add-machine lxd:0 --constraints "cores=2 mem=1G"```
- Check the LXD config for the new container and verify that:
 - limits.cpu = "2" and;
 - limits.memory = "1024MB"

Specific Overrides Instance Type
```juju add-machine lxd:0 --constraints "cores=2 mem=2G instance-type=t2.micro"```
- Check the LXD config for the new container and verify that:
 - limits.cpu = "2" and;
 - limits.memory = "2048MB"

Merged
```juju add-machine lxd:0 --constraints "mem=2G instance-type=t2.micro"```
- Check the LXD config for the new container and verify that:
 - limits.cpu = "1" and;
 - limits.memory = "2048MB"

## Documentation changes

This changes the documentation requirements from #8869.

## Bug reference

N/A
jujubot added a commit that referenced this pull request Jul 4, 2018
#8889

## Description of change

This is a backport of functionality added to the development branch, for supporting constraints and image types when provisioning LXD container machines.

It cherry picks all commits from these PRs:
- #8825
- #8838
- #8869
- #8891

## QA steps

See: #8891

## Documentation changes

Yes.

## Bug reference

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants