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

Improvements for declarative nixos containers #3021

Closed
wants to merge 9 commits into from

Conversation

chrisfarms
Copy link
Contributor

  • Use unique service units for each container rather than using
    templates (containers@xxx) to work around reloading issues.
  • Fix triggering of switching to new container configuration when
    the container's config has changed.
  • Add new options: macvlanInterface, macvlanAddress and
    macvlanPrefixLength to enable a macvlan interface within the
    container (eth1).
  • Do not force a veth pair to be created when privateNetwork is
    true (Sometimes you just want to isolate the container).
  • veth pairs are now created only when localAddress or hostAddress
    are set.
  • Add a wantedBy option so that you can control when the container
    unit should be started (defaults to multi-user.target).

* Use unique service units for each container rather than using
templates (containers@xxx) to work around reloading issues.

* Fix triggering of switching to new container configuration when
the container's config has changed.

* Add new options: `macvlanInterface`, `macvlanAddress` and
`macvlanPrefixLength` to enable a macvlan interface within the
container (eth1).

* Do not force a veth pair to be created when `privateNetwork` is
true (Sometimes you just want to isolate the container).

* veth pairs are now created only when `localAddress` or `hostAddress`
are set.

* Add a `wantedBy` option so that you can control when the container
unit should be started (defaults to `multi-user.target`).
@thoughtpolice
Copy link
Member

👍 to all these changes, in particular making declarative containers start on boot.

@edolstra
Copy link
Member

Doesn't this break imperative containers? I used unique per-container units originally, but replaced it with a template to support imperative containers (ba88db3).

What were the reloading issues exactly?

@chrisfarms
Copy link
Contributor Author

Oh really dammit, I tried searching for what it might affect and couldn't spot anything.

Can you point me towards where imperative containers might use the unit, and I'll investigate?

The reloading issues were that it simply didn't reload anything. Changes to the container's config would not trigger a switch in the container at all for anything I tried. It had a "FIXME" label next to the reload, so I'm guessing it never worked?

Separate units were also required so that each unit could be tailored individually. I've just added a wantedByso far, but when there are more complex dependencies between containers, it is going to be useful to allow more customisation of the container service.

@chrisfarms
Copy link
Contributor Author

Ok ... yep you're right.

gets on it

…late instance' during switch if it does not have a unit file.
@chrisfarms
Copy link
Contributor Author

I've discovered the root cause of the reloading issue ... switch-to-configuration treats all units in the form template@instance.service as if they must be instantiated from a template. However systemd allows you to create units in the form template@instance.service specifically for the cases where you want to override the template.

It looks like this appeared around aa6fd9f

@edolstra can you remember the reason for treating template instances that way?
In the latest commit I've reverted to using the template style to keep nixos-container happy, but to resolve the reloading issue I have changed the logic for detecting template instances during switch to check for the existence of unit files in /etc/systemd/system before deciding it is ephemeral. Does that make sense?

…er's journal is linked with the host. Setting this to 'guest' keeps the journal data in the container's filesystem while still being able to montior the container via 'journalctl -M <containername>' on the host. While setting to 'host' can give you a single collection of journals in /var/log/journal on the host.
…ties' option. This enables you to run more priviledges containers for example those that must access other /dev items
@the-kenny
Copy link
Contributor

I think we should stay conservative with the 'start on boot' option. Adding an option is nice, but a default value which starts all containers on boot might be dangerous.

@chrisfarms
Copy link
Contributor Author

I think intuitively declarative containers should default to auto-starting. I was certainly surprised when they didn't, Everything else that you define (like services) start after you enable them. If you don't want to be running persistent containers then I would expect most people to have reached for imperative containers not defining them in configuration.nix.

Maybe a better solution would be to add an enable option to each container so that they are more inline with service definitions. This might be nice to have anyway to make it easy to do things like enable/disable containers easily (say) by hostname and if the default was enable=false then anyone updating to this new style won't be bombarded with lots of unexpected running containers (but would have to go edit their config to enable their containers and set them up correctly). What do you think?

@wmertens
Copy link
Contributor

👍 on what @chrisfarms says

When a container is enabled a seperate service unit is created
to manage auto-starting of the container via the `wantedBy`
option. When disabled the container is still built but will not
be started by systemd, it can then be started/stopped manually
via `systemctl` or `nixos-container`.

Containers were not getting cleanly shutdown by systemd as
machinectl did not wait for container to shutdown and the
resulting RTMIN+4 signal would upset the service and trigger
systemd to start killing off the processes (I believe this needs
some work from upstream tbh). Now the container is shutdown via
the command socket, and machinectl is used to wait resulting in
a clean shutdown/exit.
@alexanderkjeldaas
Copy link
Contributor

+1 to auto-start by default.

@thoughtpolice
Copy link
Member

@edolstra What's the status on this? I'm strongly in favor of having this functionality, and this PR is going nowhere. I was about to implement it myself, but I remembered this. Can we please merge it? Containers are now optionally autostart with the changes @chrisfarms made. Unless this was obsoleted by something, I'm inclined to merge it soon.

@lucabrunox
Copy link
Contributor

Anybody to explain what are the consequences of this PR? What are the usages?

@Fuuzetsu
Copy link
Member

Another ping, I think unless there are objections raised then you can just merge it in @thoughtpolice , it's pointless to have this sit around for nothing with no discussion and it's not like 3 weeks is not a fair notice.

@chrisfarms
Copy link
Contributor Author

2337a85 appears to have added auto start capability but it's not clear how to set the AUTO_START flag for declarative containers.

I'm happy to update this patch to work with the AUTO_START flag if there is still interest in the rest of these changes?

@aristidb
Copy link
Contributor

aristidb commented Nov 9, 2014

What's the status of this?

@fpletz
Copy link
Member

fpletz commented Nov 9, 2014

@chrisfarms 👍 on getting this updated and merged, especially the macvlan and privateNetwork changes.

@fpletz
Copy link
Member

fpletz commented Mar 20, 2016

This will conflict with the changes in #14018.

@kampfschlaefer Do you maybe have time to integrate some of the changes here? Otherwise I'll try fo find some time in the next few weeks.

@kampfschlaefer
Copy link
Contributor

@fpletz Thanks for showing me this PR. I might take a look at it but I would like to get my PR with its less changes into master sooner than later if possible.

@kampfschlaefer
Copy link
Contributor

@fpletz

Do you maybe have time to integrate some of the changes here? Otherwise I'll try fo find some time in the next few weeks.

I think I like the explicit service definitions per container. I noticed that with hostbridge and/or extra network interfaces, there should be a dependency from containers to start only after all network devices or (better) after the network devices used by the container. And this is better solved when there is one explicit service per declarative container.

Also this would allow to define extra dependencies and extra before/after scripts per container… Gotta think about this.

kampfschlaefer added a commit to kampfschlaefer/nixpkgs that referenced this pull request Apr 2, 2016
Without the templating (which is still present for imperative containers), it
will be possible to set individual dependencies. Like depending on the network
only if the hostbridge or hardware interfaces are used.

Ported from NixOS#3021
fpletz pushed a commit to mayflower/nixpkgs that referenced this pull request Apr 20, 2016
Without the templating (which is still present for imperative containers), it
will be possible to set individual dependencies. Like depending on the network
only if the hostbridge or hardware interfaces are used.

Ported from NixOS#3021
@bjornfor
Copy link
Contributor

Ping all.

@joachifm
Copy link
Contributor

Ping. Can we resolve this before release?

@danbst
Copy link
Contributor

danbst commented Oct 4, 2016

to be clear:

Use unique service units for each container rather than using templates (containers@xxx) to work around reloading issues.

this is already in master (#14018)

Fix triggering of switching to new container configuration when the container's config has changed.

still to be addressed (as explained in #3021 (comment))

Add new options: macvlanInterface, macvlanAddress and macvlanPrefixLength to enable a macvlan nterface within the container (eth1).

Do not force a veth pair to be created when privateNetwork is
true (Sometimes you just want to isolate the container).

veth pairs are now created only when localAddress or hostAddress
are set.

still to be addressed

Add a wantedBy option so that you can control when the container
unit should be started (defaults to multi-user.target).

I think it is possible to set wantedBy now through lib.mkOverride, but haven't checked that.

allow setting of additional container capabilities via 'grantCapabilities' option. This enables you to run more priviledges containers for example those that must access other /dev items

this is done in 0d1e1b1 as additionalCapabilities

@spacekitteh
Copy link
Contributor

@grahamc please tag as security

@danbst
Copy link
Contributor

danbst commented Aug 22, 2017

Fix triggering of switching to new container configuration when the container's config has changed.

#28465

Add new options: macvlanInterface, macvlanAddress and macvlanPrefixLength to enable a macvlan nterface within the container (eth1).

#20935, partially

@disassembler
Copy link
Member

Is there anything in this PR that still would be good to implement or should this be closed?

@danbst
Copy link
Contributor

danbst commented Nov 1, 2017

@disassembler most stuff is implemented

Do not force a veth pair to be created when privateNetwork is
true (Sometimes you just want to isolate the container).

veth pairs are now created only when localAddress or hostAddress
are set.

Add a wantedBy option so that you can control when the container
unit should be started (defaults to multi-user.target).

still to be addressed

@disassembler
Copy link
Member

ok, this sounds good to me. Can you rebase on master and fix any conflicts?

@danbst
Copy link
Contributor

danbst commented Nov 1, 2017

probably autoStart by default is also what this PR makes

ok, this sounds good to me. Can you rebase on master and fix any conflicts?

ouch, I'm not PR author. Also, it may be not easy to perform rebase (3 year-old PR) and I'm not very interested in those featues

@srghma
Copy link
Contributor

srghma commented Apr 22, 2018

Such a shame that this wasn't merged

uvNikita added a commit to uvNikita/nixpkgs that referenced this pull request Oct 29, 2018
Previously, setting "privateNetwork = true" without specifying host and
local addresses would create unconfigured interfaces: ve-$INSTANCE on the host
and eth0 inside the container.

These changes is rebased part of the original PR NixOS#3021.
@arianvp
Copy link
Member

arianvp commented Jan 3, 2019

I think most of the stuff that this PR introduces are now available already. Can we close this?

  • Use unique service units for each container rather than using templates (containers@xxx) to work around reloading issues.

This we don't have yet, but interferes with imperative containers, and I'm not sure what the "Reloading" issues were. Perhaps we can create a new issue for this

  • Fix triggering of switching to new container configuration when
    the container's config has changed.

#28465

  • Add new options: macvlanInterface, macvlanAddress and
    macvlanPrefixLength to enable a macvlan interface within the
    container (eth1).

#20935

  • Do not force a veth pair to be created when privateNetwork is
    true (Sometimes you just want to isolate the container).
  • veth pairs are now created only when localAddress or hostAddress
    are set.

These have both been fixed in: #49392 but still has some stuff to fix in #52417

  • Add a wantedBy option so that you can control when the container
    unit should be started (defaults to multi-user.target)

This seems to have been fixed in #48771

@danbst
Copy link
Contributor

danbst commented Jan 3, 2019

This we don't have yet, but interferes with imperative containers, and I'm not sure what the "Reloading" issues were. Perhaps we can create a new issue for this

It is already done and merged in separate PRs. @Profpatsch can you close this?

@joachifm joachifm closed this Jan 3, 2019
@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/triage-stale-issues-and-prs-automatically/1809/6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.