Azure / platform-chaos

A node sdk for building services capable of injecting chaos into PaaS offerings. βš™οΈ 🌩
MIT License
18 stars 7 forks source link

[Discussion] Multiple resources inconsistency #31

Closed Ethan-Arrowood closed 6 years ago

Ethan-Arrowood commented 6 years ago

preface: this issue could be bs and is just the fault of my own understanding

The platform-chaos sdk defines a parser resourcesToObjects that takes in the req object and then maps over req.body.resources processing each resource.

This assumes req.body.resources looks something like so: [ "abc/123/def", "ghi/456/jkl"]

And the output of resourcesToObjects:

[ { subscriptionId: 'abc',
    resourceGroupName: '123',
    resourceName: 'def' },
  { subscriptionId: 'ghi',
    resourceGroupName: '456',
    resourceName: 'jkl' } ]

When the user uses platform-chaos-cli and passes multiple resources like so:

chaos start StartStopWebApp --resources "abc/123def, ghi/456/jkl"

The cli request-processor takes this string and splits it over the , (comma) and iterates over each resource dispatching independent this._issueAsync requests for each value.

When the CLI is used with an extension implementing the SDK (for example the wiki tutorial and Thomas' extension) I believe there is an inconsistency with how resources are being handled.

Solutions

  1. CLI is responsible for sending out multiple requests; one for each resource passed in. Extensions (implementing the SDK) will always expect to handle a single resource.
  2. CLI will always send a single request with a list of the passed resources (can be 1 or more). Extensions will always expect to handle a list of resources (1 or more)
  3. CLI will have an option that indicates to either dispatch multiple requests or just 1 request with all resources. Extensions should then do one of the following: a. The extension will exclusively handle 1 resource or multiple resources. The developer running the CLI will need to use the appropriate CLI option with the given extension. b. The extension will be written to handle either option or both using the same option passed to the CLI (the option could be forwarded through in the request). This removes the responsibility from the developer running the CLI and will require slightly more complicated extensions.

Next step

What are your thoughts? Questions? I'm happy to clarify anything confusing about this post.

@allydurks has a good take on this situation. We should probably go with option 1 or 2 for now and save option 3 for a later release as it is more complicated and will require more effort to implement correctly.

If this is not enough information to understand the situation I can write out an example referencing exact snippets of code (just LMK).

Ethan-Arrowood commented 6 years ago

Quick follow up. If everyone understands what is going on vote using emojis for how to proceed; i'm happy to work on updating the sdk and cli. Option 1 = πŸ‘ Option 2 = πŸ˜„ Option 3 = πŸŽ‰

bengreenier commented 6 years ago

what is the issue here? you're expecting cli to issue n requests for n resources given and it only issues one? you're expecting it to issue only 1 and it issues n?

I believe there is an inconsistency

please elaborate

Ethan-Arrowood commented 6 years ago

Currently:

https://github.com/Azure/platform-chaos-cli/blob/3e231162e03b2dc83ba451ff9462fe156c50f1e6/lib/request-processor.js#L33-L44

https://github.com/Azure/platform-chaos/blob/069486b722975130541aa5c6604e1d2c1b48e75a/parsers.js#L17-L29

Extension:

https://github.com/trstringer/azure-chaos-fn-webapp-startstop/blob/a6534481f8d9036eb885fe909e20c3e1b9eadc20/start/index.js#L18-L25

bengreenier commented 6 years ago

cli code has two paths (not sure why, historical i guess) - second code path is the supported one, per the originally spec.

ie:

__always_: cli --resources aa/bb/cc dd/ee/ff or cli --resources "aa/bb/cc" "dd/ee/ff" never__: cli --resources "aa/bb/cc,dd/ee/ff"

we should probably remove that first code path or touch it up, i agree. Do you want to revisit this original spec behavior as well? open to changing if it makes sense to do so.

Ethan-Arrowood commented 6 years ago

Lets stick with what you are referring to as the always condition and work from there.

Ethan-Arrowood commented 6 years ago

Regardless of 1 resource or multiple, should the CLI dispatch a single request with all the resources in a list, or multiple requests (one per resource passed to the --resources option?

bengreenier commented 6 years ago

imho/original design: always single

Ethan-Arrowood commented 6 years ago

Okay so finally, extensions implementing the SDK should always expect to handle a list of resources?