benchmark-wrapper icon indicating copy to clipboard operation
benchmark-wrapper copied to clipboard

Create Signal Protocol

Open learnitall opened this issue 3 years ago • 4 comments

Goal here is to add a new signal protocol to benchmark-wrapper. Benchmark-wrapper is currently missing the ability to export state, making collecting metrics using external tools unnecessarily difficult and inaccurate. In this enhancement, we want to allow for benchmark-wrapper to export the state of the benchmark run to redis.

Additions:

  • Create a new Signal dataclass, which holds the key-value pairs that will be exported to redis
  • Create a new SignalExporter class, which adds functionality for exporting Signal dataclasses to redis
  • Create functionality that allows for benchmarks to understand how many consumers have acknowledged a published signal.
  • Modify run_snafu.py in order to add global iteration structure
  • Add CLI option for specifying redis server export
  • Add CLI option to enable/disable acknowledgements of published signals.

learnitall avatar Jul 13 '21 15:07 learnitall

Create functionality that allows for benchmarks to understand how many consumers have acknowledged a published signal.

I think this is a mis-specification: I think the requirement is for the SignalExporter class to be able to synchronize the signal export with the subscribers. In order to do this, the class will, among other things, need to know how many subscribers there are (so that it can determine when they've all acknowledged the signal), but this is an implementation detail, not an architectural requirement (synchronization could, for instance, be implemented in other ways).

    ---o--O--o---

The first item needed is an architecture and protocol description laying out how benchmark runners like Benchmark-wrapper can publish event notifications to subscribers like Pbench. (There should be no expectation that Benchmark-wrapper will be the only runner to use this architecture, and there should be no expectation that Pbench will be the only subscriber.)

Items which need to be outlined include:

  • The list of events which can be signaled, including but not limited to:
    • benchmark initialization
    • benchmark start
    • iteration start
    • iteration stop
    • benchmark stop
  • Payload details for each event and example responses to each event
  • Capabilities for synchronization between the publisher and the subscribers
  • Capabilities for publishing non-signal information, such as benchmark and/or iteration metadata
  • Capabilities for non-transient information (e.g., a log of events and other state changes, to allow late-coming clients to "catch up")

And then a more functional set of descriptions is needed:

  • How the publisher and subscribers locate the Redis server. (Presumably they acquire the IP address and port via configuration arguments on the command line, etc.)
  • What channels on the Redis server are used, and how they located/named. (With due care given to support for sharing a single Redis server between multiple benchmark instances, which will presumably require a prefix obtained from configuration arguments.)
  • How non-signal information is conveyed and accessed; ditto for non-transient information. (How will these be initialized and finalized, given asynchronous operation of the server, publisher, and subscribers?)
  • How synchronization works between the publisher and subscribers. (How the subscribers acknowledge a signal, and how the publisher waits until all ACKs are received; how the publisher handles NAKs or missing ACKs.)

webbnh avatar Jul 13 '21 19:07 webbnh

I think this is a mis-specification: I think the requirement is for the SignalExporter class to be able to synchronize the signal export with the subscribers.

s/Create functionality that allows for benchmarks to understand how many consumers have acknowledged a published signal./Create functionality within the SignalExporter class which benchmarks can use to understand if consumers are ready for the benchmark to begin./

There should be no expectation that Benchmark-wrapper will be the only runner to use this architecture

Why is this? I understand this:

there should be no expectation that Pbench will be the only subscriber.

But the only use case that the cloud-bulldozer org has for this protocol is for exporting state from benchmark-wrapper (correct me if I'm wrong here @jtaleric @rsevilla87). If there are going to be other tools which use this protocol then shouldn't it live outside of benchmark-wrapper? Otherwise hosting it within benchmark-wrapper seems arbitrary. I don't think it really matters anyways either way, but just curious as to why we have this expectation when we've only talked about the protocol within the scope of benchmark-wrapper.

I think your included architecture requirements and functional description requirements are spot on, I'll get to work on filling those out.

learnitall avatar Jul 14 '21 23:07 learnitall

I've been super caught up in https://github.com/cloud-bulldozer/benchmark-wrapper/pull/299, as well some other changes for migrating benchmark-wrapper over to the more object-orientated structure. @maxusmusti do you want to get started on the design of the protocol and we can touch base on implementation?

learnitall avatar Jul 19 '21 13:07 learnitall

I've been super caught up in #299, as well some other changes for migrating benchmark-wrapper over to the more object-orientated structure. @Maxusmusti do you want to get started on the design of the protocol and we can touch base on implementation?

Working on design doc here

Maxusmusti avatar Jul 19 '21 19:07 Maxusmusti