plugins/logger: fix default event subscription mask.

[klihub]

Let the stub determine which events the logger plugin subscribes to, to fix a recently introduced startup failure.

Ever since support for the new pod update events has been merged, unless the event subscription mask is overidden with the --events command line option, the sample logger plugin fails to start up with the following error message:

plugin exited with error internal error: unhandled events UpdatePodSandbox,PostUpdatePodSandbox (0x1800)

This patch should fix that.

[mikebrow]

all is still problematic because it filters down to all previously valid/known

maybe add a known option to replace all where that filters out by version / event types we've decided to register for and monitor...

then can make all mean any pod/container ... perhaps later on pod/container/image?

[mikebrow]

should probably add the new pod events

[klihub]

should probably add the new pod events

My plan is to add them once the runtimes start supporting them. Otherwise even with this change in place, we would still fail to register to the runtime, by default, it just would fail on the other end of the ttrpc transport, because those new events are unknown to the runtime yet.

[klihub]

all is still problematic because it filters down to all previously valid/known

maybe add a known option to replace all where that filters out by version / event types we've decided to register for and monitor...

But, if you refer to "all" in

nri/pkg/api/event.go

Line 84 in c0d45aa

case "all":

, that is a helper function in the API/protocol description, and the idea is that it represents the event mask of all known subscribable NRI events.

Sorry, I don't understand what you mean by filtering out by version. The plugin is compiled against the package, so it is talking/using the same version as the version of the NRI package itself. So the notion of "all" should be the same for the plugin and the NRI package. On the stub side, by convention, using a 0 event mask will result in the stub registering with an event mask for all events the plugin has a handler for. That's what should have been used in the logger plugin by default all along, instead of the default being "all". And that's what this PR aims to fix.

[mikebrow]

all is still problematic because it filters down to all previously valid/known
maybe add a known option to replace all where that filters out by version / event types we've decided to register for and monitor...

But, if you refer to "all" in

nri/pkg/api/event.go

Line 84 in c0d45aa

case "all":

, that is a helper function in the API/protocol description, and the idea is that it represents the event mask of all known subscribable NRI events.
Sorry, I don't understand what you mean by filtering out by version. The plugin is compiled against the package, so it is talking/using the same version as the version of the NRI package itself. So the notion of "all" should be the same for the plugin and the NRI package. On the stub side, by convention, using a 0 event mask will result in the stub registering with an event mask for all events the plugin has a handler for. That's what should have been used in the logger plugin by default all along, instead of the default being "all". And that's what this PR aims to fix.

interesting... so nil is actually all need to read through it better... I caught that all was a mask on known and figured that would need to be versioned... do we block/force NRI plugins to the specific version of containerd/crio vendor import for nri?

I was thinking there was value in the all "known" mask and that each version of NRI will have a set of known

[klihub]

all is still problematic because it filters down to all previously valid/known
maybe add a known option to replace all where that filters out by version / event types we've decided to register for and monitor...

But, if you refer to "all" in

nri/pkg/api/event.go

Line 84 in c0d45aa

case "all":

, that is a helper function in the API/protocol description, and the idea is that it represents the event mask of all known subscribable NRI events.
Sorry, I don't understand what you mean by filtering out by version. The plugin is compiled against the package, so it is talking/using the same version as the version of the NRI package itself. So the notion of "all" should be the same for the plugin and the NRI package. On the stub side, by convention, using a 0 event mask will result in the stub registering with an event mask for all events the plugin has a handler for. That's what should have been used in the logger plugin by default all along, instead of the default being "all". And that's what this PR aims to fix.

interesting... so nil is actually all need to read through it better... I caught that all was a mask on known and figured that would need to be versioned... do we block/force NRI plugins to the specific version of containerd/crio vendor import for nri?

No, we don't check or block it at the moment. But we reject the plugin if it tries to subscribe to events the runtime does not know about.

I was thinking there was value in the all "known" mask and that each version of NRI will have a set of known

[mikebrow]

LGTM