Maybe we should create a func eventTimestamp(swarmapi.Meta, swarmapi.WatchActionKind) time.Time function that handles extracting the timestamp. Then we can call it in generateClusterEvent and pass in the timestamp instead of needing to handle it in all the different switch statements.

Would it be useful for Watch to support something like Kind: "*"?

In general I want people to filter for the specific events of interest, but the Docker events API is a bit of an unusual case.

We don't want to get task change notifications. They are too many and are implementation details that we don't want user rely on that.

logrus.Warn on default?

Can we use make these string literals const-ants?

Ideally they should be defined in swarmkit. But I don't find them there. I don't think they should be added here.

Comment doesn't match the field name.

Maybe this can just be Events? I like to avoid putting type information in the field names.

Change to WatchStream.

I can't see any code path where it is possible for n.cluster to be nil. I'd suggest removing this - it is a little misleading to suggest that n.cluster can be nil, when other code would blow up in this case.

logrus.WithError(err).Error("failed to receive changes from store watch API")

Should this log anything?

Select on this channel write and <-ctx.Done().

When <-ctx.Done(), shouldn't watch.Recv() return with error? If it doesn't return with error, it may cause a thread leak. Adding <-ctx.Done() along with the channel write wouldn't help because it may not reach here.

Empty case

Empty case

Don't we need something to check if oldNode is nil? The old value is not guaranteed to be available.

Why is that? The watch API asks for old object. In an update case, the old object should always be available.

Providing the old object is best-effort. We're not able to do it if you're looking at past events. I don't think the events code is using this right now, but the point is that the API doesn't guarantee it can provide the old object. Even if it did, this code should still check. It's node good for the client side of a client/server system to crash if the server omits expected values.

Don't we need something to check if oldService is nil? The old value is not guaranteed to be available.

"and publishes it"

Don't we need to check if OldObject is nil before calling GetNode, etc.?

The GetNode function handles it directly.

   func (m *Object) GetNode() *Node {
       if x, ok := m.GetObject().(*Object_Node); ok {
         return x.Node
       }
       return nil
   }

   func (m *Object) GetObject() isObject_Object {
       if m != nil {
         return m.Object
       }
       return nil
   }

Maybe this should be swarm. Not sure though.

@dongluochen @aaronlehmann ?

I'll update it to swarm.

I think we should output node.Role (attributes["role..."]) rather than node.Spec.DesiredRole

Changed to role.new.

The code is still using node.Spec.DesiredRole. I believe @aluzzardi wanted this to look at node.Role instead.

Thanks @aaronlehmann. Updated.

There shouldn't be a need to close this. I think there is a possibility that cluster can still attempt to write to this chan after Cleanup has been called(although very unlikely as shutdownDaemon is not fast).

Thanks. Let me remove it.

If it is not closed, the ProcessClusterNotifications goroutine will never exit.

@aaronlehmann It should take a context then. Or the cluster.Cleanup() should close it after it knows there are no writes coming.

Add a context to ProcessClusterNotifications.

No need to change but wondering why you chose to pass in a channel and handle this in dockerd instead of defining a LogEvent interface in cluster, that would be called to process events.

It was decided to add a watch API to expose cluster change. But do not add a separate LogEvent interface for simplicity.
moby/swarmkit#491 (comment)

I meant interface in docker's cluster pkg like cluster.Backend cluster.NetworkSubnetsProvider that would be implemented by daemon.Daemon

I'll look at it in a separate change.

Does this need to also select on the Done method of the context passed to ProcessClusterNotifications, so that this won't block forever if ProcessClusterNotifications exits due to context cancellation?

I don't think it necessary to do that. c.Cleanup() at https://github.com/moby/moby/blob/master/cmd/dockerd/daemon.go#L278 would trigger return on this goroutine. The call path is Cluster.Cleanup() -> nodeRunner.Stop() -> set nodeRunner.closed -> nodeRunner.handleNodeExit() -> cancel nodeRunner.handleControlSocketChange context -> store watch API got cancelled.

store watch API got cancelled

This will never happen if this function is blocked on the channel write instead of calling Recv. I don't see what prevents ProcessClusterNotifications from exiting before this function.

As an aside, I think it's bad for ProcessClusterNotifications to exit on either a channel close or a context cancellation. There should be only one way to shut it down.

Let's use the context from nodeRunner.handleControlSocketChange to make sure it exits.

We don't display labels changes, by design ?

The attributes are extra information for an event. For example, a node update event 2017-04-06T18:07:01.620569322Z node update 0urgktyobae3etk5e4n4331es (availability.new=pause, availability.old=active, name=ip-172-19-147-51) contains a fix part timestamp node update nodeID and the attributes part availability.new, availability.old, name. We can add labels change to the attributes if they are useful.

Attributes are added to events in an ad-hoc way. Since there are a lot of information in swarm objects, revealing all changes may reduce readability. For example, a node goes thru a list of changes when joining a cluster, which is internal procedure and users shouldn't put much effort to inspect them.

I'm not very sure how this will be used. We are starting with basic events moby/swarmkit#491 (comment). I think it'll be extended based on user feedbacks.

I've seen many tools using labels to add configuration that's used when listening for events - labels would be a likely candidate to add