Skip to content
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

Create Signal Protocol #295

Open
learnitall opened this issue Jul 13, 2021 · 4 comments
Open

Create Signal Protocol #295

learnitall opened this issue Jul 13, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@learnitall
Copy link
Collaborator

learnitall commented Jul 13, 2021

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.
@webbnh
Copy link

webbnh commented Jul 13, 2021

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.)

@learnitall
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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?

@Maxusmusti
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants