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

Use a separate orchestrator for each namespace #855

Merged
merged 38 commits into from
Jun 14, 2022

Conversation

awrichar
Copy link
Contributor

@awrichar awrichar commented Jun 9, 2022

Changed in this PR:

  • Namespace manager is now the root manager in the system
  • There is a separate orchestrator for each namespace, along with a separate instance of (almost) every manager
  • Config for including different plugins in each namespace is now honored
  • Each plugin supports multiple listeners, and each listener function (mostly on event manager) will filter out events from just the one namespace to which it's assigned

Broken in this PR:

  • You cannot have multiple namespaces that point at the same blockchain FireFly contract, or the same token pool. They will end up sharing an event stream and therefore only one of them will process events. Fixing this will require splitting a lot of the contract/event stream logic out of the blockchain plugins into a new per-namespace component.
  • Performance of event processing degrades almost linearly with the number of created namespaces, due to the fact that most events are fired to all instances of the event manager and then examined in order to process or ignore them. In the future, we can push namespace knowledge down into lower layers to make this decision earlier and avoid this extra overhead.
  • SPI route for "get all operations" is moved from /operations to /namespaces/{ns}/operations, which represents a breaking change against FFTM.

Also includes the changes from #856.

shorsher and others added 23 commits June 3, 2022 11:15
Update namespace manager to contain an orchestrator per namespace.

Now, namespace manager will validate & get all plugins, validate all
namespaces defined in configuration, and instantiate
an orchestrator for each namespace.

Orchestrator now receives the defined plugins for that orchestrator's
namespace and instantiates the managers.

This is a first pass - some items are still broken and need further
work.

Signed-off-by: Alex Shorsher <alex.shorsher@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Some of these may come back in a different form, but they need to be
removed for the moment because they exist outside of the namespaced
orchestrators.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Temporarily disable many of the tests that are no longer relevant.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Most of the orchestrator and namespace manager tests are disabled, but
the rest of the tests now pass.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Move this out of /status (since it's just a collection query), and put
it under namespaces like most of the other routes.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Avoid setting these twice.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Now under /namespaces.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Remove the "/status" prefix, since this is a global route and no longer
grouped with the other "status" routes.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Returns very basic information about all available namespaces. Note that
there is no "GetNamespaces" query on orchestrator or the database layer now,
as these details may be spread across multiple databases.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Only the "get many" route for operations is broken now.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@awrichar awrichar marked this pull request as draft June 9, 2022 01:37
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #855 (f7559c9) into main (15c8802) will decrease coverage by 0.02%.
The diff coverage is 99.51%.

❗ Current head f7559c9 differs from pull request most recent head f96daef. Consider uploading reports for the commit f96daef to get more accurate results

@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
- Coverage   99.98%   99.96%   -0.03%     
==========================================
  Files         299      300       +1     
  Lines       19523    19510      -13     
==========================================
- Hits        19521    19504      -17     
- Misses          1        5       +4     
  Partials        1        1              
Impacted Files Coverage Δ
internal/apiserver/route_spi_get_ops.go 100.00% <ø> (ø)
internal/database/sqlcommon/namespace_sql.go 100.00% <ø> (ø)
internal/events/subscription_manager.go 100.00% <ø> (ø)
internal/metrics/metrics.go 100.00% <ø> (ø)
internal/identity/tbd/tbd.go 90.00% <50.00%> (-10.00%) ⬇️
internal/sharedstorage/ipfs/ipfs.go 97.50% <50.00%> (-2.50%) ⬇️
internal/tokens/fftokens/fftokens.go 99.52% <90.47%> (-0.48%) ⬇️
cmd/firefly.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_namespace.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_namespaces.go 100.00% <100.00%> (ø)
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15c8802...f96daef. Read the comment docs.

Each listener will ignore events outside its namespace.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Each listener will ignore events outside its namespace.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Each listener will ignore events outside its namespace.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@awrichar awrichar marked this pull request as ready for review June 9, 2022 19:28
metricsEnabled bool
metrics metrics.Manager
adminEvents spievents.Manager
utOrchestrator orchestrator.Orchestrator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this testing workaround, but I was struggling to find a better way.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@awrichar awrichar mentioned this pull request Jun 13, 2022
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@@ -37,6 +37,7 @@ var spiGetOps = &ffapi.Route{
Extensions: &coreExtensions{
FilterFactory: database.OperationQueryFactory,
CoreJSONHandler: func(r *ffapi.APIRequest, cr *coreRequest) (output interface{}, err error) {
// TODO: cr.or will be nil (this must be converted to a namespaced query)
Copy link
Contributor

@peterbroadhurst peterbroadhurst Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not do this in this PR? Having a broken endpoint seems bad, and it should be fine to make it namespaces/{ns}/operations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(We still need the operations/{nsopid} routes for GET and PATCH, where the namespace is embedded into the ID)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that works - I can replace that route. Just wanted to get your feedback on how to spell it in the context of the other SPI routes and the usage by FFTM.

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling out all the considerations for where we end up after this (big!) step forwards. Looks good. I did have a couple of very minor comments along the way to review before merge.

@awrichar awrichar merged commit 362f721 into hyperledger:main Jun 14, 2022
@awrichar awrichar deleted the namespacemgr branch June 14, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants