Track request / response removal (#2)

* Add UID to requests on provider and refactor functional test

* Add ability to revoke requests or responses

Allow for requests or responses to be revoked and for each side to
handle those revocations and clean up any associated resources.

* Fix handling of request defaults change detection

The providing side needs to be notified if anything in the request
changes, even if they were defaults filled in on our side (e.g., the
backend addresses from ingress-address).

Also added some more sorting to ensure more stable ordering of items.

* Rename nonce field and remove hard-coded hashes from unit tests

Author: johnsca

docs/api.md

@@ -31,8 +31,9 @@ When instantiated, it should be passed a charm instance and a relation name.
 
   * `is_available` Whether the provider is available to take requests
   * `is_changed` Whether there are any new or changed responses which have not been acknowledged
-  * `all_responses` A list of all received responses, even if they have not changed
-  * `new_responses` A list of all responses which are new or have changed and not been acknowledged
+  * `all_responses` A list of all received responses
+  * `complete_responses` A list of all up to date received responses, even if they have not changed
+  * `new_responses` A list of all complete responses which are new or have changed and not been acknowledged
 
 ### Flags
 

loadbalancer_interface/base.py

@@ -10,7 +10,7 @@
 from . import schemas
 
 
-class LBBase(Object):
+class VersionedInterface(Object):
     def __init__(self, charm, relation_name):
         super().__init__(charm, relation_name)
         self.charm = weakref.proxy(charm)
@@ -40,7 +40,9 @@ def relations(self):
                 for relation in sorted(relations, key=attrgetter('id'))
                 if self._schema(relation)]
 
-    def _schema(self, relation):
+    def _schema(self, relation=None):
+        if relation is None:
+            return schemas.versions[schemas.max_version]
         if relation.app not in relation.data:
             return None
         data = relation.data[relation.app]

loadbalancer_interface/consumer.py

@@ -3,12 +3,13 @@
 from cached_property import cached_property
 
 from ops.framework import (
+    StoredState,
     EventBase,
     EventSource,
     ObjectEvents,
 )
 
-from .base import LBBase
+from .base import VersionedInterface
 
 
 class LBRequestsChanged(EventBase):
@@ -19,14 +20,16 @@ class LBConsumersEvents(ObjectEvents):
     requests_changed = EventSource(LBRequestsChanged)
 
 
-class LBConsumers(LBBase):
+class LBConsumers(VersionedInterface):
     """ API used to interact with consumers of a loadbalancer provider.
     """
+    state = StoredState()
     on = LBConsumersEvents()
 
     def __init__(self, charm, relation_name):
         super().__init__(charm, relation_name)
         self.relation_name = relation_name
+        self.state.set_default(known_requests={})
 
         for event in (charm.on[relation_name].relation_created,
                       charm.on[relation_name].relation_joined,
@@ -50,7 +53,7 @@ def all_requests(self):
             schema = self._schema(relation)
             local_data = relation.data[self.app]
             remote_data = relation.data[relation.app]
-            for key, request_sdata in remote_data.items():
+            for key, request_sdata in sorted(remote_data.items()):
                 if not key.startswith('request_'):
                     continue
                 name = key[len('request_'):]
@@ -65,21 +68,34 @@ def all_requests(self):
                         if addr:
                             request.backends.append(addr)
                 requests.append(request)
+                self.state.known_requests.setdefault(request.id, None)
         return requests
 
     @property
     def new_requests(self):
-        """A list of requests with changes or no response.
+        """ A list of requests with changes or no response.
         """
-        return [req for req in self.all_requests
-                if not req.response or req.response.request_hash != req.hash]
+        return [request for request in self.all_requests
+                if request.hash != self.state.known_requests[request.id]]
+
+    @property
+    def removed_requests(self):
+        """ A list of requests which have been removed, either explicitly or
+        because the relation was removed.
+        """
+        current_ids = {request.id for request in self.all_requests}
+        unknown_ids = self.state.known_requests.keys() - current_ids
+        schema = self._schema()
+        return [schema.Request._from_id(req_id, self.relations)
+                for req_id in sorted(unknown_ids)]
 
     def send_response(self, request):
         """ Send a specific request's response.
         """
-        request.response.request_hash = request.hash
+        request.response.received_hash = request.sent_hash
         key = 'response_' + request.name
         request.relation.data[self.app][key] = request.response.dumps()
+        self.state.known_requests[request.id] = request.hash
         if not self.new_requests:
             try:
                 from charms.reactive import clear_flag
@@ -88,9 +104,18 @@ def send_response(self, request):
             except ImportError:
                 pass  # not being used in a reactive charm
 
+    def revoke_response(self, request):
+        """ Revoke / remove the response for a given request.
+        """
+        if request.id:
+            self.state.known_requests.pop(request.id, None)
+        if request.relation:
+            key = 'response_' + request.name
+            request.relation.data.get(self.app, {}).pop(key, None)
+
     @property
     def is_changed(self):
-        return bool(self.new_requests)
+        return bool(self.new_requests or self.removed_requests)
 
     def manage_flags(self):
         """ Used to interact with charms.reactive-base charms.

loadbalancer_interface/provider.py

@@ -8,7 +8,7 @@
 )
 from ops.model import ModelError
 
-from .base import LBBase
+from .base import VersionedInterface
 
 
 class LBProviderAvailable(EventBase):
@@ -24,7 +24,7 @@ class LBProviderEvents(ObjectEvents):
     responses_changed = EventSource(LBResponsesChanged)
 
 
-class LBProvider(LBBase):
+class LBProvider(VersionedInterface):
     """ API used to interact with the provider of loadbalancers.
     """
     state = StoredState()
@@ -52,15 +52,20 @@ def _check_provider(self, event):
                 self.on.available.emit()
             if self.is_changed:
                 self.on.responses_changed.emit()
-        else:
+        elif self.state.was_available:
             self.state.was_available = False
+            if self.state.response_hashes:
+                self.state.response_hashes = {}
+                self.on.responses_changed.emit()
 
     @property
     def relation(self):
         return self.relations[0] if self.relations else None
 
     def get_request(self, name):
         """ Get or create a Load Balancer Request object.
+
+        May raise a ModelError if unable to create a request.
         """
         if not self.charm.unit.is_leader():
             raise ModelError('Unit is not leader')
@@ -82,42 +87,105 @@ def get_request(self, name):
     def get_response(self, name):
         """ Get a specific Load Balancer Response by name.
 
-        This is equivalent to `get_request(name).response`.
+        This is equivalent to `get_request(name).response`, except that it
+        will return `None` if the response is not available.
         """
-        return self.get_request(name).response
+        if not self.is_available:
+            return None
+        request = self.get_request(name)
+        if not request.response:
+            return None
+        return request.response
 
     def send_request(self, request):
         """ Send a specific request.
+
+        May raise a ModelError if unable to send the request.
         """
+        if not self.charm.unit.is_leader():
+            raise ModelError('Unit is not leader')
+        if not self.relation:
+            raise ModelError('Relation not available')
+        # The sent_hash is used to tell when the provider's response has been
+        # updated to match our request. We can't used the request hash computed
+        # on the providing side because it may not match due to default values
+        # being filled in on that side (e.g., the backend addresses).
+        request.sent_hash = request.hash
         key = 'request_' + request.name
         self.relation.data[self.app][key] = request.dumps()
-        self.state.response_hashes[request.name] = None
+
+    def remove_request(self, name):
+        """ Remove a specific request.
+
+        May raise a ModelError if unable to remove the request.
+        """
+        if not self.charm.unit.is_leader():
+            raise ModelError('Unit is not leader')
+        if not self.relation:
+            return
+        key = 'request_' + name
+        self.relation.data[self.app].pop(key, None)
+        self.state.response_hashes.pop(name, None)
+
+    @property
+    def all_requests(self):
+        """ A list of all requests which have been made.
+        """
+        requests = []
+        if self.relation:
+            for key in sorted(self.relation.data[self.app].keys()):
+                if not key.startswith('request_'):
+                    continue
+                requests.append(self.get_request(key[len('request_'):]))
+        return requests
+
+    @property
+    def revoked_responses(self):
+        """ A list of responses which are no longer available.
+        """
+        return [request.response
+                for request in self.all_requests
+                if not request.response
+                and request.name in self.state.response_hashes]
 
     @cached_property
     def all_responses(self):
         """ A list of all responses which are available.
         """
-        local_data = self.relation.data[self.app]
-        names = [key[len('request_'):]
-                 for key in local_data.keys()
-                 if key.startswith('request_')]
-        requests = [self.get_request(name) for name in names]
-        return [request.response for request in requests if request.response]
+        return [request.response
+                for request in self.all_requests
+                if request.response]
+
+    @cached_property
+    def complete_responses(self):
+        """ A list of all responses which are up to date with their associated
+        request.
+        """
+        return [request.response
+                for request in self.all_requests
+                if request.response.received_hash == request.sent_hash]
 
     @property
     def new_responses(self):
-        """ A list of all responses which have not yet been acknowledged as
+        """ A list of complete responses which have not yet been acknowledged as
         handled or which have changed.
         """
+        acked_responses = self.state.response_hashes
         return [response
-                for response in self.all_responses
-                if self.state.response_hashes[response.name] != response.hash]
+                for response in self.complete_responses
+                if response.hash != acked_responses.get(response.name)]
 
     def ack_response(self, response):
         """ Acknowledge that a given response has been handled.
+
+        Can be called on a revoked response as well to remove it
+        from the revoked_responses list.
         """
-        self.state.response_hashes[response.name] = response.hash
-        if not self.new_responses:
+        if response:
+            self.state.response_hashes[response.name] = response.hash
+        else:
+            self.state.response_hashes.pop(response.name, None)
+        if not self.is_changed:
             try:
                 from charms.reactive import clear_flag
                 prefix = 'endpoint.' + self.relation_name
@@ -127,12 +195,16 @@ def ack_response(self, response):
 
     @property
     def is_changed(self):
-        return bool(self.new_responses)
+        return self.new_responses or self.revoked_responses
 
     @property
     def is_available(self):
         return bool(self.relation)
 
+    @property
+    def can_request(self):
+        return self.is_available and self.unit.is_leader()
+
     def manage_flags(self):
         """ Used to interact with charms.reactive-base charms.
         """

loadbalancer_interface/schemas/v1.py

@@ -18,7 +18,7 @@ class _Schema(Schema):
         success = fields.Bool(required=True)
         message = fields.Str(missing=None)
         address = fields.Str(missing=None)
-        request_hash = fields.Str(missing=None)
+        received_hash = fields.Str(missing=None)
 
         @validates_schema
         def _validate(self, data, **kwargs):
@@ -72,10 +72,27 @@ class _Schema(Schema):
         tls_key = fields.Str(missing=None)
         ingress_address = fields.Str(missing=None)
         ingress_ports = fields.List(fields.Int(), missing=list)
+        sent_hash = fields.Str(missing=None)
+
+    @classmethod
+    def _from_id(cls, req_id, relations):
+        """ Return an empty Request with the given ID.
+
+        This represents an unknown or removed request.
+        """
+        name, rel_id = req_id.split(':')
+        request = cls(name)
+        request._id = req_id
+        for relation in relations:
+            if relation.id == rel_id:
+                request.relation = relation
+                break
+        return request
 
     def __init__(self, name):
         super().__init__()
         self._name = name
+        self._id = None
         # On the provider side, requests need to track which relation they
         # came from to know where to send the response.
         self.relation = None
@@ -85,6 +102,14 @@ def __init__(self, name):
     def name(self):
         return self._name
 
+    @property
+    def id(self):
+        if self._id is None:
+            if self.relation is None:
+                return None
+            self._id = '{}:{}'.format(self.relation.id, self.name)
+        return self._id
+
     @classmethod
     def loads(cls, name, request_sdata, response_sdata=None):
         self = cls(name)

setup.py

@@ -3,7 +3,7 @@
 
 SETUP = {
     "name": "loadbalancer_interface",
-    "version": "1.0.1",
+    "version": "1.0.2",
     "author": "Cory Johns",
     "author_email": "cory.johns@canonical.com",
     "url": "https://github.com/juju-solutions/loadbalancer-interface",