[WIP] Adding middleware support in factory #195

almahmoud · 2019-02-21T23:43:39Z

This adds support for middleware in factory, which then gets inherited by any provider object created by this factory.

Three options to add middleware from factory:

Direct middleware add which will keep the same object across calls from any provider created by this factory after adding (in this example, the count will continue for all calls)


        class SomeDummyClass(object):
            count = start_count

            @intercept(event_pattern="*", priority=2499)
            def return_incremented(self, event_args, *args, **kwargs):
                self.count += 1
                return self.count

        some_obj = SomeDummyClass()
        factory.middleware.add(some_obj)

Middleware constructor add which will create the middleware with the class and arguments when making a new middleware manager for the provider object (in this example, count will only keep track of calls within each provider):

        class SomeDummyClassWithArgs(object):
            def __init__(self, start, increment):
                self.count = start
                self.increment = increment

            @intercept(event_pattern="*", priority=2499)
            def return_incremented(self, event_args, *args, **kwargs):
                self.count += self.increment
                return self.count

        factory = CloudProviderFactory()
        factory.middleware.add_constructor(SomeDummyClassWithArgs,
                                           start_count, increment)

and 3) adding events directly which will recreate a handler with the same callback function (bound methods will remain bound to their object)

        def return_hello(event_args, *args, **kwargs):
            return "hello"

        factory = CloudProviderFactory()
        factory.middleware.events.intercept("*", 2490, return_hello)

For all these scenarios, provider objects will inherit the events from the factory:

aws = factory.create_provider(ProviderList.AWS, {})
az = factory.create_provider(ProviderList.AZURE, {})
# Both providers will inherit all middleware/handlers added to the factory

coveralls · 2019-02-22T01:28:49Z

Pull Request Test Coverage Report for Build 1500

361 of 377 (95.76%) changed or added relevant lines in 12 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+10.1%) to 88.855%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cloudbridge/cloud/factory.py	112	128	87.5%

Totals
Change from base Build 1470:	10.1%
Covered Lines:	7292
Relevant Lines:	7902

💛 - Coveralls

nuwang

Let's discuss the details later on today. I think the idea of maintaining similarity to a local event manager when subscribing to factory level events is a good one, we just need to make it behave in a way that's unsurprising. Perhaps the one that's least surprising is to simply share the middleware object. Fundamentally, I guess we have to answer these questions:
a. is factory level middleware different from individual provider level middleware, or should they be interchangeable? That is, is can provider.middleware.add(middleware) behave the same as factory.middleware.add(middleware)?
b. How do we avoid middleware internal to providers accidentally subscribing to "global" events. Should we?
c. If the answer is no, how do provider internal events, relate to global external events? How would the chain of handlers work? Would a namespace work?

nuwang · 2019-02-22T08:13:34Z

cloudbridge/cloud/base/provider.py

@@ -83,11 +83,11 @@ def debug_mode(self):


 class BaseCloudProvider(CloudProvider):
-    def __init__(self, config):
+    def __init__(self, config, middleware_manager=None):


This is great, better than sending in middleware as a list.

nuwang · 2019-02-22T08:14:46Z

cloudbridge/cloud/factory.py

+        super(ParentMiddlewareManager, self).__init__(event_manager)
+        self.middleware_constructors = []
+
+    def add_constructor(self, middleware_class, *args):


This could possibly be renamed to add_middleware_class, accepting the class in question, and both the constructor args and kwargs.

nuwang · 2019-02-22T08:22:58Z

cloudbridge/cloud/factory.py

+            new_handler = handler.__class__(handler.event_pattern,
+                                            handler.priority,
+                                            handler.callback)
+            new_manager.events.subscribe(new_handler)


So this means that events added to the factory later won't be subscribed to previously created providers? This is ok, but I think we need to clarify how exactly factory event subscriptions are meant to work and what the intended use cases are, so we know for sure that this is the correct behaviour.

That was the original question that I had about this: whether we should make the provider object independent after it was created, or always treat it as a "child" of the factory. If the latter, it's definitely doable, but I would then suggest making the factory accessible through the provider as well.

nuwang · 2019-02-22T08:23:59Z

cloudbridge/cloud/factory.py

+    def get_subscribed_handlers(self):
+        handlers = []
+        # Todo: Expose this better in pyeventsystem library
+        event_dict = self.events._SimpleEventDispatcher__events


Not sure why we can't just access self.events?

self.events is the SimpleEventDispatcher, which currently doesn't have a property exposing the events dict. We definitely should add the property, but I was avoiding changing the other library for the time being, before we discuss this further.

nuwang · 2019-02-22T08:28:58Z

cloudbridge/cloud/factory.py

@@ -27,9 +79,14 @@ class CloudProviderFactory(object):
    """

    def __init__(self):
+        self._middleware = ParentMiddlewareManager()


I think we need a different name because ParentMiddlewareManager doesn't seem to explain what it's doing. Also, there is the issue of namespacing. And finally, I guess there's the issue of being able to intercept behaviour of a particular cloud (say openstack), and change behaviour for all classes of that cloud. One way would be to prefix the cloudname to the event. Another way would be is to check the instancetype of the sender: if isinstance(OpenStackProvider, event_args['sender'].provider) or some such thing. I think namespacing would be good as long as isolation is maintained.

nuwang · 2019-02-22T08:34:19Z

cloudbridge/cloud/factory.py

+    # Removing install step from add. Since this manager is meant to create
+    # other managers rather than run. This will also simplify separating
+    # handlers added through middleware from those subscribed directly
+    def add(self, middleware):


I don't think we should provide dual semantics here - one for shared middleware objects, and another for independent middleware, just to keep it simple. I'm currently leaning in favour of the factory namespacing child event dispatchers, and forwarding them as necessary, so that we still maintain event isolation within a given provider. I think we probably just need to avoid a scenario where providers are unwittingly subscribing to events outside of the provider.

codecov · 2019-02-22T15:24:44Z

Codecov Report

Merging #195 into master will decrease coverage by 0.07%.
The diff coverage is 94.42%.

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage   88.03%   87.96%   -0.08%     
==========================================
  Files          36       36              
  Lines        7775     7902     +127     
  Branches      854      868      +14     
==========================================
+ Hits         6845     6951     +106     
- Misses        594      610      +16     
- Partials      336      341       +5

Impacted Files	Coverage Δ
cloudbridge/cloud/providers/azure/services.py	`88.2% <100%> (ø)`	⬆️
cloudbridge/cloud/providers/aws/services.py	`90.09% <100%> (ø)`	⬆️
cloudbridge/cloud/providers/gcp/provider.py	`91.95% <100%> (ø)`	⬆️
cloudbridge/cloud/providers/aws/provider.py	`96.22% <100%> (ø)`	⬆️
cloudbridge/cloud/providers/mock/provider.py	`100% <100%> (ø)`	⬆️
cloudbridge/cloud/base/provider.py	`88.23% <100%> (ø)`	⬆️
cloudbridge/cloud/providers/openstack/provider.py	`88.46% <100%> (ø)`	⬆️
cloudbridge/cloud/base/services.py	`88.23% <100%> (ø)`	⬆️
cloudbridge/cloud/providers/openstack/services.py	`82.08% <100%> (ø)`	⬆️
cloudbridge/cloud/providers/gcp/services.py	`83.31% <100%> (ø)`	⬆️
... and 2 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 ccafd0b...aa50481. Read the comment docs.

almahmoud added 6 commits February 21, 2019 14:39

Middleware inheritance from factory

1c10169

Testing middleware inheritance from factory

e827905

Middleware list in factory

0ecadbe

Middleware support in factory with constructor option

614ca04

Direct event subscription in factory

901b4b6

Move generator to create_provider

21f817a

afgane added the in progress label Feb 21, 2019

almahmoud requested a review from nuwang February 21, 2019 23:44

Add empty manager to provider constructor

25390d4

almahmoud force-pushed the factory branch from e17f04e to 25390d4 Compare February 21, 2019 23:46

nuwang reviewed Feb 22, 2019

View reviewed changes

almahmoud added 2 commits February 22, 2019 11:33

Changing generic provider to provider ID in event names

eea4356

Introducing mamespace logic

aa50481

almahmoud force-pushed the factory branch from c31bdec to aa50481 Compare February 23, 2019 02:24

nuwang marked this pull request as draft October 30, 2022 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Adding middleware support in factory #195

[WIP] Adding middleware support in factory #195

almahmoud commented Feb 21, 2019

coveralls commented Feb 22, 2019 •

edited

Loading

nuwang left a comment

nuwang Feb 22, 2019

nuwang Feb 22, 2019

nuwang Feb 22, 2019

almahmoud Feb 22, 2019

nuwang Feb 22, 2019

almahmoud Feb 22, 2019

nuwang Feb 22, 2019

nuwang Feb 22, 2019 •

edited

Loading

codecov bot commented Feb 22, 2019 •

edited

Loading

[WIP] Adding middleware support in factory #195

Are you sure you want to change the base?

[WIP] Adding middleware support in factory #195

Conversation

almahmoud commented Feb 21, 2019

coveralls commented Feb 22, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1500

💛 - Coveralls

nuwang left a comment

Choose a reason for hiding this comment

nuwang Feb 22, 2019

Choose a reason for hiding this comment

nuwang Feb 22, 2019

Choose a reason for hiding this comment

nuwang Feb 22, 2019

Choose a reason for hiding this comment

almahmoud Feb 22, 2019

Choose a reason for hiding this comment

nuwang Feb 22, 2019

Choose a reason for hiding this comment

almahmoud Feb 22, 2019

Choose a reason for hiding this comment

nuwang Feb 22, 2019

Choose a reason for hiding this comment

nuwang Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2019 • edited Loading

Codecov Report

coveralls commented Feb 22, 2019 •

edited

Loading

nuwang Feb 22, 2019 •

edited

Loading

codecov bot commented Feb 22, 2019 •

edited

Loading