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

stage1: Implemented notifications from the app in the container to the host #2826

Merged
merged 3 commits into from Sep 15, 2016

Conversation

Projects
None yet
7 participants
@alepuccetti
Contributor

alepuccetti commented Jun 22, 2016

After this patch, systemd on the host marks the container as "active"
when the init systemd in the container sends the ready message
instead of doing it after the container is created.
After this patch rkt sets --notify-ready=yes on systemd-nspawn.

Also appc/spec in godeps is patched to expose SupportsNotify.

Fixes: #1464

Warning:
If the application launched by rkt does not notify (with sd_notify())
that it is ready, then the application may be killed after a timeout.

Note: Requires systemd v231 in stage1, so this commit must not be merged before.

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Jun 23, 2016

Contributor

do we need an issue about bumping to systemd 231?

Contributor

jonboulle commented Jun 23, 2016

do we need an issue about bumping to systemd 231?

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 23, 2016

Contributor

Dependency appc/spec#626

Contributor

alepuccetti commented Jun 23, 2016

Dependency appc/spec#626

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Jun 28, 2016

Contributor

Dependency #2843

Contributor

alepuccetti commented Jun 28, 2016

Dependency #2843

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Aug 31, 2016

Member

Both dependencies are now cleared and I think we can proceed on this. Tentatively milestoning for 1.15.0.

Member

lucab commented Aug 31, 2016

Both dependencies are now cleared and I think we can proceed on this. Tentatively milestoning for 1.15.0.

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Aug 31, 2016

Member

@alepuccetti let us know if the ETA is ok for you, @sur can shepherd assist on this.

Member

lucab commented Aug 31, 2016

@alepuccetti let us know if the ETA is ok for you, @sur can shepherd assist on this.

@alepuccetti

This comment has been minimized.

Show comment
Hide comment
@alepuccetti

alepuccetti Aug 31, 2016

Contributor

@lucab I think that the ETA is ok, I will start work on it this afternoon or tomorrow and give you a better feedback about it.

Contributor

alepuccetti commented Aug 31, 2016

@lucab I think that the ETA is ok, I will start work on it this afternoon or tomorrow and give you a better feedback about it.

@alban alban referenced this pull request Sep 1, 2016

Merged

rkt: 1.13.0 -> 1.14.0 #2160

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Sep 14, 2016

Contributor

@alepuccetti somehow master commits were mangled into this PR, can you correct this?

Contributor

s-urbaniak commented Sep 14, 2016

@alepuccetti somehow master commits were mangled into this PR, can you correct this?

@alepuccetti alepuccetti changed the title from [WIP] stage1: Implemented notifications from the app in the container to the host to stage1: Implemented notifications from the app in the container to the host Sep 14, 2016

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Sep 15, 2016

Contributor

@alepuccetti your commits are mangled

Contributor

s-urbaniak commented Sep 15, 2016

@alepuccetti your commits are mangled

@squeed

This comment has been minimized.

Show comment
Hide comment
@squeed

squeed Sep 15, 2016

Contributor

There is a merge conflict with prepare-app.c. You'll have to add an additional , false field at the end of the files_mount_table.

Contributor

squeed commented Sep 15, 2016

There is a merge conflict with prepare-app.c. You'll have to add an additional , false field at the end of the files_mount_table.

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Sep 15, 2016

Contributor

ok, LGTM once green, I create a follow-up issue for functional tests.

Contributor

s-urbaniak commented Sep 15, 2016

ok, LGTM once green, I create a follow-up issue for functional tests.

@squeed

This comment has been minimized.

Show comment
Hide comment
@squeed

squeed Sep 15, 2016

Contributor

Hmm, failed on Fedora, coreos flavor, TestNetDefaultRestrictedConnectivity. Investigating.

Contributor

squeed commented Sep 15, 2016

Hmm, failed on Fedora, coreos flavor, TestNetDefaultRestrictedConnectivity. Investigating.

@iaguis

This comment has been minimized.

Show comment
Hide comment
@iaguis

iaguis Sep 15, 2016

Member

Known flake: #2943

Member

iaguis commented Sep 15, 2016

Known flake: #2943

Alessandro Puccetti added some commits Jun 28, 2016

Alessandro Puccetti
stage1: Implemented notifications from the app in the container to th…
…e host

After this patch, systemd on the host marks the container as "active"
when the init systemd in the container sends the ready message
instead of doing it after the container is created.
After this patch, rkt adds the switch `--notify-ready=yes` when it runs the pod
with systemd-nspawn.

Fixes: #1464

Warning:
If the application launched by rkt does not notify (with sd_notify())
that it is ready, then the application may be killed after a timeout.
Alessandro Puccetti
stage1: Parse pod manifest annotation to enable --notify-ready option
This patch parses the image manifest and if the annotation
"appc.io/executor/supports-systemd-notify" is set to true,
it sets the service type to `notify` in the systemd unit file.
Alessandro Puccetti
docs/using-rkt-with-systemd: notification example
Show how to run a pod with notifications enabled.
@squeed

This comment has been minimized.

Show comment
Hide comment
@squeed

squeed Sep 15, 2016

Contributor

Merging. The build was mostly green (except some known flakes), but there was a small documentation change.

Contributor

squeed commented Sep 15, 2016

Merging. The build was mostly green (except some known flakes), but there was a small documentation change.

@squeed squeed merged commit 0f4d592 into rkt:master Sep 15, 2016

0 of 3 checks passed

Jenkins Build started sha1 is merged.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
semaphoreci The build is pending on Semaphore.
Details

alepuccetti pushed a commit to kinvolk/rkt that referenced this pull request Sep 20, 2016

Alessandro Puccetti
tests/inspect: added Notify option
`Notify` is used to test the notfiy features introduced by
rkt#2826

alepuccetti pushed a commit to kinvolk/rkt that referenced this pull request Sep 20, 2016

Alessandro Puccetti
tests: added test for notification
Test for the feature added by rkt#2826.
Depends on coreos/go-systemd#200.

Fixes rkt#3198

alepuccetti pushed a commit to kinvolk/rkt that referenced this pull request Sep 20, 2016

Alessandro Puccetti
tests/inspect: added Notify option
`Notify` is used to test the notfiy features introduced by
rkt#2826

alepuccetti pushed a commit to kinvolk/rkt that referenced this pull request Sep 20, 2016

Alessandro Puccetti
tests: added test for notification
Test for the feature added by rkt#2826.
Depends on coreos/go-systemd#200.

Fixes rkt#3198

alepuccetti pushed a commit to kinvolk/rkt that referenced this pull request Sep 20, 2016

Alessandro Puccetti
tests: added test for notification
Test for the feature added by rkt#2826.
Depends on coreos/go-systemd#200.

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