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

Have docker register its machine with systemd #13526

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@rhatdan
Contributor

rhatdan commented May 28, 2015

This patch will register the container with machinectl. This will allow tools
on the container like machinectl and eventually journalctl to interact with the
container.

Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan
Contributor

rhatdan commented Jun 10, 2015

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jun 26, 2015

Contributor

@rhatdan What kind of interactions will this enable? I'm assuming list is one thing, but what about the rest? Is the goal to expand support for more machinectl commands afterwards?

Contributor

icecrime commented Jun 26, 2015

@rhatdan What kind of interactions will this enable? I'm assuming list is one thing, but what about the rest? Is the goal to expand support for more machinectl commands afterwards?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jun 27, 2015

Contributor

Yes I would like to see as much machinectl support, as possible. We would probably need to work with systemd guys on clarifying which commands will not work with docker containers. machinectl status, show, list, terminate, kill

Might be interesting getting copy-to and copy-from to work.

machinectl login, poweroff, reboot might work if the --init=systemd patch was accepted.

Contributor

rhatdan commented Jun 27, 2015

Yes I would like to see as much machinectl support, as possible. We would probably need to work with systemd guys on clarifying which commands will not work with docker containers. machinectl status, show, list, terminate, kill

Might be interesting getting copy-to and copy-from to work.

machinectl login, poweroff, reboot might work if the --init=systemd patch was accepted.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jun 27, 2015

Contributor

My biggest goal with this is just to get

journalctl -M CONTAINERID

To work in the situation where journald is running within the container.

Contributor

rhatdan commented Jun 27, 2015

My biggest goal with this is just to get

journalctl -M CONTAINERID

To work in the situation where journald is running within the container.

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jul 13, 2015

Contributor

Just making sure here: the rest of machinectl/Docker integration will be done in the machinectl codebase? It just needs the initial information that the container exists, right?

Contributor

icecrime commented Jul 13, 2015

Just making sure here: the rest of machinectl/Docker integration will be done in the machinectl codebase? It just needs the initial information that the container exists, right?

return err
}
obj := conn.Object("org.freedesktop.machine1", "/org/freedesktop/machine1")

This comment has been minimized.

@jessfraz

jessfraz Jul 13, 2015

Contributor

should machine1 be hardcoded like that

@jessfraz

jessfraz Jul 13, 2015

Contributor

should machine1 be hardcoded like that

This comment has been minimized.

@rhatdan

rhatdan Jul 13, 2015

Contributor

That is the API. There is no machine2...

@rhatdan

rhatdan Jul 13, 2015

Contributor

That is the API. There is no machine2...

This comment has been minimized.

@rhatdan

rhatdan Jul 13, 2015

Contributor

@cgwalters Do you agree?

@rhatdan

rhatdan Jul 13, 2015

Contributor

@cgwalters Do you agree?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jul 13, 2015

Contributor

Right now we have a race condition with this. If a container exits before RegisterMachine gets called, it will log a message. I can add a patch to search for the Container PID on error and just ignore the error. In a perfect world we could prevent the container PID from disappearing until after the RegisterMachine completes.

Also should we move this code to runc rather then having it in the docker daemon at all.

Contributor

rhatdan commented Jul 13, 2015

Right now we have a race condition with this. If a container exits before RegisterMachine gets called, it will log a message. I can add a patch to search for the Container PID on error and just ignore the error. In a perfect world we could prevent the container PID from disappearing until after the RegisterMachine completes.

Also should we move this code to runc rather then having it in the docker daemon at all.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 17, 2015

Contributor

I'd like to see registerMachine and terminateMachine behind an interface.
I could see wanting to be able to register containers with more than just systemd.

Contributor

cpuguy83 commented Jul 17, 2015

I'd like to see registerMachine and terminateMachine behind an interface.
I could see wanting to be able to register containers with more than just systemd.

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jul 17, 2015

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jul 17, 2015

Contributor

@cpuguy83 Added a machine interface, with linux and non linux. If someone adds another machine/container registration, then we could look at adding a buildtag for selinux

Contributor

rhatdan commented Jul 17, 2015

@cpuguy83 Added a machine interface, with linux and non linux. If someone adds another machine/container registration, then we could look at adding a buildtag for selinux

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jul 23, 2015

Contributor

@rhatdan I was pretty upset when machinectl terminate NAME killed my X server. So, I dunno what to say about design :)

Contributor

LK4D4 commented Jul 23, 2015

@rhatdan I was pretty upset when machinectl terminate NAME killed my X server. So, I dunno what to say about design :)

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Jul 23, 2015

Contributor

@LK4D4 It is a feature. You needed to get a way from the keyboard for a few minutes.

Contributor

rhatdan commented Jul 23, 2015

@LK4D4 It is a feature. You needed to get a way from the keyboard for a few minutes.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jul 23, 2015

Contributor

hahahahahaha

On Thu, Jul 23, 2015 at 1:55 PM, Daniel J Walsh notifications@github.com
wrote:

@LK4D4 https://github.com/LK4D4 It is a feature. You needed to get a
way from the keyboard for a few minutes.


Reply to this email directly or view it on GitHub
#13526 (comment).

Contributor

jessfraz commented Jul 23, 2015

hahahahahaha

On Thu, Jul 23, 2015 at 1:55 PM, Daniel J Walsh notifications@github.com
wrote:

@LK4D4 https://github.com/LK4D4 It is a feature. You needed to get a
way from the keyboard for a few minutes.


Reply to this email directly or view it on GitHub
#13526 (comment).

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Jul 23, 2015

Contributor

Contributor

vbatts commented Jul 23, 2015

RegisterMachine truncate container names to first 64 chars
Signed-off-by: Sally O'Malley <somalley@redhat.com>

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Aug 14, 2015

Contributor

So, this stuff is working somehow. But I think this is malicious code because of systemd bugs(killing not what it supposing to kill). So if include it at all in current design, then only in experimental branch.
ping @docker/core-maintainers

Contributor

LK4D4 commented Aug 14, 2015

So, this stuff is working somehow. But I think this is malicious code because of systemd bugs(killing not what it supposing to kill). So if include it at all in current design, then only in experimental branch.
ping @docker/core-maintainers

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Aug 14, 2015

Contributor

We are looking at moving this code into runc pre/post scripts rather then docker.
That way we could eliminate the race condition, where the container exits before it is registered.

Contributor

rhatdan commented Aug 14, 2015

We are looking at moving this code into runc pre/post scripts rather then docker.
That way we could eliminate the race condition, where the container exits before it is registered.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Aug 14, 2015

Contributor

@rhatdan As I see current PR makes no sense, because it only extends go systemd API. Maybe you should propose this changes to coreos/go-systemd first and then when we'll have pre/post scripts you will open PR with real implementation. Maybe systemd will be more generous to my X servers that time.

Contributor

LK4D4 commented Aug 14, 2015

@rhatdan As I see current PR makes no sense, because it only extends go systemd API. Maybe you should propose this changes to coreos/go-systemd first and then when we'll have pre/post scripts you will open PR with real implementation. Maybe systemd will be more generous to my X servers that time.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Aug 14, 2015

Contributor

@LK4D4 Actually the calls have been lost from the patch. I sent patches for RegisterMachine/TerminateMachine to coreos a while ago, The current patch does not need some of the hire level stuff coreos version of systemd did.

I am going to close this and rework it for runc hooks, if and when this gets merged.

Contributor

rhatdan commented Aug 14, 2015

@LK4D4 Actually the calls have been lost from the patch. I sent patches for RegisterMachine/TerminateMachine to coreos a while ago, The current patch does not need some of the hire level stuff coreos version of systemd did.

I am going to close this and rework it for runc hooks, if and when this gets merged.

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