schema/app: add annotation to set support of sd_notify() #626

alepuccetti · 2016-06-15T14:26:30Z

supportNotify exposes if or not an application supports notifications
using sd_notify(). This information may be used to signal the init
process when a service is ready.

jonboulle · 2016-06-17T10:21:14Z

spec/aci.md

@@ -239,6 +240,7 @@ JSON Schema for the Image Manifest (app image manifest, ACI manifest), conformin
        * **port** (integer, required) port number that will be used; see also **count**. Must be >=1 and <=65535.
        * **count** (integer, optional, defaults to 1 if unset) specifies a range of ports, starting with "port" and ending with "port" + "count" - 1. Must be >=1.
        * **socketActivated** (boolean, optional, defaults to "false" if unsupplied) if set to true, the application expects to be [socket activated](http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html) on these ports. To perform socket activation, the ACE MUST pass file descriptors using the [socket activation protocol](http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html) that are listening on these ports when starting this app. If multiple apps in the same pod are using socket activation then the ACE must match the sockets to the correct apps using getsockopt() and getsockname().
+    * **supportNotify** (boolean, optional, defaults to "false" if unset) if set to true, the application should use [sd_notify()](https://www.freedesktop.org/software/systemd/man/sd_notify.html) to signal when it is ready. Setting this parameter to true for an application that does not send a ready signal with `sd_notify()` may result in misbehave.


"may result in misbehave [sic]" is too vague - let's define a policy here.
What MUST executors do if an application sets this parameter to true but does not send a ready signal?
Also, what SHOULD executors do if an application sets this parameter to true but the executor does not support sd_notify?

Nit, prefer "supportsNotify" to "supportNotify"

What MUST executors do if an application sets this parameter to true but does not send a ready signal?
Also, what SHOULD executors do if an application sets this parameter to true but the executor does not support sd_notify?

In both case ma answer is that this should not happen, because setting to true supportNotify means that the app will be responsible for the notification. For example, in systemd, if the init system do not get the ready message after a timeout will fill the app.

I could write something like: "Setting this parameter to true for an application that does not send a ready signal with sd_notify() may result in starvation or termination of the app. When setting to true the app will be responsible to provide the minimum notifications to the init system"

perhaps it should be rephrased as "if set to true the application MUST use sd_notify" to more clearly express that the alternate behaviour is not well defined.

I think the second case can happen more easily. For example, with stage1-fly there is not always a systemd to notify. Should stage1-fly refuse to run an application with supportsNotify=true, or should stage1-fly ignore it and the application be able to detect that it's not running under systemd and run normally?

On Friday, June 17, 2016, Euan Kemp notifications@github.com wrote:

In spec/aci.md
#626 (comment):

@@ -239,6 +240,7 @@ JSON Schema for the Image Manifest (app image manifest, ACI manifest), conformin
* port (integer, required) port number that will be used; see also count. Must be >=1 and <=65535.
* count (integer, optional, defaults to 1 if unset) specifies a range of ports, starting with "port" and ending with "port" + "count" - 1. Must be >=1.
* socketActivated (boolean, optional, defaults to "false" if unsupplied) if set to true, the application expects to be socket activated on these ports. To perform socket activation, the ACE MUST pass file descriptors using the socket activation protocol that are listening on these ports when starting this app. If multiple apps in the same pod are using socket activation then the ACE must match the sockets to the correct apps using getsockopt() and getsockname().

* supportNotify (boolean, optional, defaults to "false" if unset) if set to true, the application should use sd_notify() to signal when it is ready. Setting this parameter to true for an application that does not send a ready signal with sd_notify() may result in misbehave.

perhaps it should be rephrased as "if set to true the application MUST use
sd_notify" to more clearly express that the alternate behaviour is not well
defined.

This sound good.

I think the second case can happen more easily. For example, with
stage1-fly there is not always a systemd to notify. Should stage1-fly
refuse to run an application with supportsNotify=true, or should
stage1-fly ignore it and the application be able to detect that it's not
running under systemd and run normally?

I don't k ow how stage1-fly works, but for the moment the manly reason to
support notification is to have a a clear signal when the service in
the pod is ready. This allow to sort out dependencies between pods not
easily.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/appc/spec/pull/626/files/79ff3e69f02267582f5253175ac2cb01264a3bfa#r67568121,
or mute the thread
https://github.com/notifications/unsubscribe/AGRc7zXAzEcr9FYT2I2E6ydhFbN1RJzVks5qMv7UgaJpZM4I2aiJ
.

stage1-fly, unlike stage1-coreos, does not use systemd (nor any init process at all) but instead naively execs the entrypoint of the application. It is already not a compliant ACE iirc since it doesn't support pods with multiple applications, so it might not be the best example.

I think the options for an ACE that doesn't use systemd upon seeing this flag are:

error out that it will not be able to support running this app, or

proceed to run the app as normal, letting notify calls fail (which practically all programs using sd_notify behave well under).

sd_notify() does not fail and just return zero when the executor has not set up the notification socket (i.e. $NOTIFY_SOCKET is undefined).

So the stage1-fly executor can just ignore that field and the app must deal with it.

What about the following wording:

supportsNotify (boolean, optional, defaults to "false" if unset) if set to true, the application MUST use the sd_notify mechanism to signal when it is ready. Executors MAY ignore supportNotify. An application with supportsNotify=true MUST be able to detect if the executor had not set up the sd_notify mechanism and skip the notification without error (sd_notify() from libsystemd does that automatically).

euank · 2016-06-17T21:34:57Z

Since this option is somewhat tied to implementation (systemd), I'm not sure if this should be part of the spec or part of specific ACEs (e.g. rkt). The spec also doesn't clearly define that an ACE must provide a means to do things like show the status of an application nor talk about state transitions, which I think would also be helpful to have if this is to be specified.

The alternative is to support it in rkt, and any other ACE that desires, as metadata that lives outside of the ACI (e.g. as an additional flag or via interpreting some well-known annotation).

Other opinions would be quite welcome :)

jonboulle · 2016-06-21T14:30:47Z

@euank my thinking was it would be reasonable to have this in the spec because we already have a precedent with socketActivated, but I admit that's arguably a lazy rationale, and I do see the argument it should be per implementation (shame on me for not considering annotations for this).

I'm still noodling on the best way to move forward..

alepuccetti · 2016-06-21T14:41:59Z

@jonboulle and @euank Honestly I do not know other init system and if they implement notification also if stage1_fly do not use any init system this option do no have any effect on it. Again I am not familiar with stage1_fly or other init system. I am working on a patch for rkt to allow services inside a pod to notify when they are ready to better handle inter-pod dependencies. I do not have a strong opinion where expose this features in appc/spec.

… to the 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 sets `--service-type=notify`. Also appc/spec in godeps is patched to expose `SupportNotify`. Fixes: rkt#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.

jonboulle · 2016-06-23T11:55:17Z

any other opinions - @vbatts ?

vbatts · 2016-06-23T19:53:34Z

notifying the init is an interesting concept, but to my knowledge is only something implemented by systemd.
While we did already add the socket activation as a first class field, perhaps this would be a good place to start deferring more to be an annotation. Especially if the executor can ignore it.

alepuccetti · 2016-06-23T21:36:02Z

Ok then, if we put it in annotations we can describe it a little better. We can define a systemd "class" of annotations and so supportsNotify become systemdNotifySupport. The convention will be to prefix with the string "systemd" all the systemd related parameters. This could be extended to other init systems or other tool in general. I think it is a good convention to allow easy extensibility.

@vbatts what do you think about it? Should we also push socketActivated in annotations? In a different PR of course.

jonboulle · 2016-06-24T13:02:54Z

For backwards compatibility we can't really remove socketActivated, but we could add it as an annotation as well (and just fall back to checking it if the standard one isn't set).

Perhaps something like appc.io/executor/supportsSystemdNotify and appc.io/executor/socketActivated - IMO the "class" (namespace) is executor features, rather than systemd per se

vbatts · 2016-06-24T13:13:14Z

there may need to be a one-off for appc.io/executor/socketActivated of ensuring if set then both the annotation and the field are set and matching. As a backward-compat thing.

alepuccetti · 2016-06-24T13:14:38Z

@jonboulle

For backwards compatibility we can't really remove socketActivated, but we could add it as an annotation as well (and just fall back to checking it if the standard one isn't set).

We could have both and give priority to the annotation if set and then remove the first class field in the future.

alepuccetti · 2016-06-24T13:19:53Z

@jonboulle

Perhaps something like appc.io/executor/supportsSystemdNotify and appc.io/executor/socketActivated - IMO the "class" (namespace) is executor features, rather than systemd per se

Do you mean to have a first class field named "executor"?

jonboulle · 2016-06-27T11:53:13Z

@alepuccetti No, I'm saying having well-known annotations with those names; since annotations are free-form/arbitrary, we're just using a DNS-like name to provide some kind of namespacing

jonboulle · 2016-06-27T11:54:04Z

e.g. {"annotations": [ "name": "appc.io/executor/socketActivated", "value": "true" ] }

alepuccetti · 2016-06-27T12:00:38Z

No, I'm saying having well-known annotations with those names; since annotations are free-form/arbitrary, we're just using a DNS-like name to provide some kind of namespacing

@jonboulle Gotcha, I like that. So for the notification: {"annotations": [ {"name": "appc.io/executor/supportsSystemdNotify"}]}

I will make the change for supportsSystemdNotify and then submit an other PR for socketActivated

alepuccetti · 2016-06-28T14:24:35Z

@jonboulle I will use "appc.io/executor/systemd/support-notify" because when I use uppercases I get this error panic: ACIdentifier must contain only lower case alphanumeric characters plus "-._~/"

alepuccetti · 2016-06-28T16:07:55Z

I just noticed that annotations may be applied to all the apps, but this is a per app feature.

alepuccetti · 2016-07-08T08:45:34Z

@jonboulle

e.g. {"annotations": [ "name": "appc.io/executor/support-notify", "value": "true" ] }

Annotations refer to the pod, but this feature is per app. So If we have in the same pod an app supporting notifications and a second one that it does not the pod will be never marked as active due to the fact the pod's systemd is waiting for all the apps to send the ready notification.

One solution it would be to document that you should set this annotation to true ONLY IF all the apps in the pod supports notification. If you agree I will update the PR so we can get this thing done.

alban · 2016-07-11T12:23:59Z

@alepuccetti

Annotations refer to the pod, but this feature is per app.

Annotations can be added both in the pod manifest (as defined here) and in the app manifest (as defined here).

As you said, the sd_notify feature is per app. Your patch defines "supportsSystemdNotify" in the app manifest and not in the pod manifest, so this looks correct to me.

alepuccetti · 2016-07-12T09:45:52Z

@jonboulle If there is nothing more about this, can we merge this?

jonboulle · 2016-07-12T12:21:00Z

examples/image.json

@@ -128,6 +128,10 @@
        {
            "name": "homepage",
            "value": "https://example.com"
+        },
+        {
+            "name": "appc.io/executor/systemd/support-notify",


The name is not correct here

This made me think that it could be a good choice to use appc.io/executor/systemd/ as prefix for all the systemd related annotations. I will change the docs accordingly.

alepuccetti · 2016-07-12T13:01:40Z

Patch updated

euank · 2016-07-12T20:44:29Z

spec/aci.md

@@ -169,7 +169,7 @@ JSON Schema for the Image Manifest (app image manifest, ACI manifest), conformin
                "count": 1000,
                "protocol": "tcp"
            }
-        ]
+        ],


the comma makes this invalid json

jonboulle · 2016-07-13T12:08:44Z

Two nits:
i) can you fix the name in the commit message - it's different from the actual ones now
ii) since this is an annotation I'm a little wary of the MUST, can it be SHOULD instead?

After that, this LGTM.

Note since this is not changing the spec itself, it can be implemented in rkt without being blocked by this.

The annotation "appc.io/executor/supports-systemd-notify" exposes if or not an application supports notifications using sd_notify(). This information may be used to signal the init process when a service is ready.

jonboulle · 2016-07-13T12:59:17Z

LGTM. Thanks!

alban mentioned this pull request Jun 15, 2016

stage1: use sd_notify to notify when pod is ready rkt/rkt#1464

Closed

4 tasks

alepuccetti force-pushed the alessandro/support-notify branch 3 times, most recently from e69d565 to 79ff3e6 Compare June 16, 2016 09:26

jonboulle reviewed Jun 17, 2016
View reviewed changes

alepuccetti force-pushed the alessandro/support-notify branch 2 times, most recently from d49b057 to 6b4cef9 Compare June 22, 2016 14:36

alepuccetti force-pushed the alessandro/support-notify branch 2 times, most recently from c9a6592 to 57a1299 Compare June 23, 2016 13:00

alepuccetti mentioned this pull request Jun 23, 2016

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

Merged

alepuccetti force-pushed the alessandro/support-notify branch 2 times, most recently from 8a9d130 to b571ab8 Compare June 28, 2016 14:42

alepuccetti force-pushed the alessandro/support-notify branch from b571ab8 to 699e725 Compare July 8, 2016 12:11

jonboulle reviewed Jul 12, 2016
View reviewed changes

alepuccetti force-pushed the alessandro/support-notify branch 2 times, most recently from c8ed0f1 to 72ee0f3 Compare July 12, 2016 13:01

euank reviewed Jul 12, 2016
View reviewed changes

alepuccetti force-pushed the alessandro/support-notify branch from 72ee0f3 to 4869656 Compare July 12, 2016 20:50

schema/app: add annotation to expose sd_notify() support

8ac5ec7

The annotation "appc.io/executor/supports-systemd-notify" exposes if or not an application supports notifications using sd_notify(). This information may be used to signal the init process when a service is ready.

alepuccetti force-pushed the alessandro/support-notify branch from 4869656 to 8ac5ec7 Compare July 13, 2016 12:31

jonboulle merged commit 2a954f7 into appc:master Jul 13, 2016

jonboulle changed the title ~~schema/app: add parameter to set support of sd_notify()~~ schema/app: add annotation to set support of sd_notify() Jul 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema/app: add annotation to set support of sd_notify() #626

schema/app: add annotation to set support of sd_notify() #626

alepuccetti commented Jun 15, 2016

jonboulle Jun 17, 2016

jonboulle Jun 17, 2016

alepuccetti Jun 17, 2016

euank Jun 17, 2016

alepuccetti Jun 17, 2016

euank Jun 17, 2016 •

edited

Loading

alban Jun 21, 2016

euank commented Jun 17, 2016

jonboulle commented Jun 21, 2016

alepuccetti commented Jun 21, 2016

jonboulle commented Jun 23, 2016

vbatts commented Jun 23, 2016

alepuccetti commented Jun 23, 2016

jonboulle commented Jun 24, 2016

vbatts commented Jun 24, 2016

alepuccetti commented Jun 24, 2016 •

edited

Loading

alepuccetti commented Jun 24, 2016 •

edited

Loading

jonboulle commented Jun 27, 2016

jonboulle commented Jun 27, 2016

alepuccetti commented Jun 27, 2016

alepuccetti commented Jun 28, 2016 •

edited

Loading

alepuccetti commented Jun 28, 2016

alepuccetti commented Jul 8, 2016

alban commented Jul 11, 2016

alepuccetti commented Jul 12, 2016

jonboulle Jul 12, 2016

alepuccetti Jul 12, 2016 •

edited

Loading

alepuccetti commented Jul 12, 2016

euank Jul 12, 2016

jonboulle commented Jul 13, 2016

jonboulle commented Jul 13, 2016

schema/app: add annotation to set support of sd_notify() #626

schema/app: add annotation to set support of sd_notify() #626

Conversation

alepuccetti commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euank Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euank commented Jun 17, 2016

jonboulle commented Jun 21, 2016

alepuccetti commented Jun 21, 2016

jonboulle commented Jun 23, 2016

vbatts commented Jun 23, 2016

alepuccetti commented Jun 23, 2016

jonboulle commented Jun 24, 2016

vbatts commented Jun 24, 2016

alepuccetti commented Jun 24, 2016 • edited Loading

alepuccetti commented Jun 24, 2016 • edited Loading

jonboulle commented Jun 27, 2016

jonboulle commented Jun 27, 2016

alepuccetti commented Jun 27, 2016

alepuccetti commented Jun 28, 2016 • edited Loading

alepuccetti commented Jun 28, 2016

alepuccetti commented Jul 8, 2016

alban commented Jul 11, 2016

alepuccetti commented Jul 12, 2016

Choose a reason for hiding this comment

alepuccetti Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

alepuccetti commented Jul 12, 2016

Choose a reason for hiding this comment

jonboulle commented Jul 13, 2016

jonboulle commented Jul 13, 2016

euank Jun 17, 2016 •

edited

Loading

alepuccetti commented Jun 24, 2016 •

edited

Loading

alepuccetti commented Jun 24, 2016 •

edited

Loading

alepuccetti commented Jun 28, 2016 •

edited

Loading

alepuccetti Jul 12, 2016 •

edited

Loading