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

LXC Support #1699

Merged
merged 13 commits into from
Oct 25, 2016
Merged

LXC Support #1699

merged 13 commits into from
Oct 25, 2016

Conversation

diptanu
Copy link
Contributor

@diptanu diptanu commented Sep 12, 2016

This PR implements support for the LXC driver. The driver is enabled only in Linux.

@diptanu diptanu force-pushed the f-lxc-driver branch 2 times, most recently from 9fb4534 to be33c11 Compare September 12, 2016 02:48
return nil, fmt.Errorf("unable to start container: %v", err)
}
// Set the resource limits
if err := c.SetMemoryLimit(lxc.ByteSize(task.Resources.MemoryMB) * lxc.MB); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should this come before c.Start()?

@schmichael
Copy link
Member

Neither of my comments are blockers. LGTM

@diptanu
Copy link
Contributor Author

diptanu commented Sep 16, 2016

@schmichael Addressed all your comments.

@diptanu
Copy link
Contributor Author

diptanu commented Sep 16, 2016

@dadgar I would recommend merging this PR since the basic features have been implemented. We can add support for downloading support of rootfs and MacVLAN in subsequent PRs.

Can you please review this?

@diptanu diptanu force-pushed the f-lxc-driver branch 2 times, most recently from 6b22537 to e512245 Compare September 16, 2016 05:52
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

The multiple SetConfigItem calls are my only concern. If you've verified it works as expected :shipit:

if err := c.SetConfigItem("lxc.mount.entry", allocDirMount); err != nil {
return nil, fmt.Errorf("error setting alloc dir bind mounts configuration: %v", err)
}
if err := c.SetConfigItem("lxc.mount.entry", taskDirMount); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Calling SetConfigItem("lxc.mount.entry",... twice will append the second entry, not overwrite the first?

I couldn't tell from looking at the lxc docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it won't, if you look at lxc configuration there can be multiple lxc.mount.entry - as many fstab entries you want.

func (h *lxcDriverHandle) Kill() error {
h.logger.Printf("[INFO] driver.lxc: shutting down container %q", h.container.Name())
if err := h.container.Shutdown(h.killTimeout); err != nil {
h.logger.Printf("[INFO] driver.lxc: shutting down container %q failed: %v", h.container.Name(), err)
Copy link
Member

Choose a reason for hiding this comment

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

INFO -> WARN perhaps?

)

const (

Copy link
Contributor

@dadgar dadgar Oct 3, 2016

Choose a reason for hiding this comment

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

Extra line?

)

// Add the lxc driver to the list of builtin drivers
func init() {
Copy link
Contributor

@dadgar dadgar Oct 3, 2016

Choose a reason for hiding this comment

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

Why is this here, do it like the other drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dadgar So that we don't have to build the driver on other platforms. This driver needs the lxc-dev package which is unavailable on other platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah missed the build tag

if err := mapstructure.WeakDecode(task.Config, &driverConfig); err != nil {
return nil, err
}
lxcPath := lxc.DefaultConfigPath()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set the path to be the task_dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not very sure if we can have multiple Config Paths on a machine, usually all the containers live in /var/lib/lxc. Having it in the task dir is desirable since then we can make use of disk space accounting.

Copy link
Contributor

Choose a reason for hiding this comment

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

LXC allows unprovileged containers, i.e. I can run nomad as nomad user & without any root privileges to spin LXC containers owned by nomad user. In such setup, generally the containers will stay at /home/nomad/.local/share/lxc (or $HOME/.local/share/lxc).. but this is configurable. I am not aware of any case where the nomad client would be running as different user /or same user with different lxc path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the lxc path can be configurable by the operator. I think @dadgar is suggesting using a different path per container.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I was suggesting specific path per container. I was looking at lxc docs and it seems like you can

@ranjib
Copy link
Contributor

ranjib commented Oct 3, 2016

@diptanu i took this for a spin during the weekend. Its working on my cluster. Could not get to went through all the code. I think you should enable wantdaemonize + close file descriptor by default.


const (
// The option that enables this driver in the Config.Options map.
lxcConfigOption = "driver.lxc.enable"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this documented on the website?

Copy link
Contributor

Choose a reason for hiding this comment

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

comments should start with the variable/constant/method/struct name

Copy link
Member

Choose a reason for hiding this comment

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

Fixed the comment.

@diptanu did you get a chance to write any docs? If not I don't mind doing it.

return nil, err
}
lxcPath := lxc.DefaultConfigPath()
if path := d.config.Read("lxc.path"); path != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be driver.lxc.path and document it

taskLocalDir, ok := ctx.AllocDir.TaskDirs[task.Name]
if !ok {
return nil, fmt.Errorf("failed to find task local directory: %v", task.Name)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, see #1830

}

// Set the resource limits
if err := c.SetMemoryLimit(lxc.ByteSize(task.Resources.MemoryMB) * lxc.MB); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Set resources before starting?

@diptanu diptanu merged commit 855fe5f into master Oct 25, 2016
@diptanu diptanu deleted the f-lxc-driver branch October 25, 2016 22:17
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants