Skip to content

Commit be92518

Browse files
authored
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
1 parent 920d0d9 commit be92518

File tree

8 files changed

+320
-98
lines changed

8 files changed

+320
-98
lines changed

docs/api.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,9 @@ When instantiated, it should be passed a charm instance and a relation name.
3131

3232
* `is_available` Whether the provider is available to take requests
3333
* `is_changed` Whether there are any new or changed responses which have not been acknowledged
34-
* `all_responses` A list of all received responses, even if they have not changed
35-
* `new_responses` A list of all responses which are new or have changed and not been acknowledged
34+
* `all_responses` A list of all received responses
35+
* `complete_responses` A list of all up to date received responses, even if they have not changed
36+
* `new_responses` A list of all complete responses which are new or have changed and not been acknowledged
3637

3738
### Flags
3839

loadbalancer_interface/base.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
from . import schemas
1111

1212

13-
class LBBase(Object):
13+
class VersionedInterface(Object):
1414
def __init__(self, charm, relation_name):
1515
super().__init__(charm, relation_name)
1616
self.charm = weakref.proxy(charm)
@@ -40,7 +40,9 @@ def relations(self):
4040
for relation in sorted(relations, key=attrgetter('id'))
4141
if self._schema(relation)]
4242

43-
def _schema(self, relation):
43+
def _schema(self, relation=None):
44+
if relation is None:
45+
return schemas.versions[schemas.max_version]
4446
if relation.app not in relation.data:
4547
return None
4648
data = relation.data[relation.app]

loadbalancer_interface/consumer.py

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,13 @@
33
from cached_property import cached_property
44

55
from ops.framework import (
6+
StoredState,
67
EventBase,
78
EventSource,
89
ObjectEvents,
910
)
1011

11-
from .base import LBBase
12+
from .base import VersionedInterface
1213

1314

1415
class LBRequestsChanged(EventBase):
@@ -19,14 +20,16 @@ class LBConsumersEvents(ObjectEvents):
1920
requests_changed = EventSource(LBRequestsChanged)
2021

2122

22-
class LBConsumers(LBBase):
23+
class LBConsumers(VersionedInterface):
2324
""" API used to interact with consumers of a loadbalancer provider.
2425
"""
26+
state = StoredState()
2527
on = LBConsumersEvents()
2628

2729
def __init__(self, charm, relation_name):
2830
super().__init__(charm, relation_name)
2931
self.relation_name = relation_name
32+
self.state.set_default(known_requests={})
3033

3134
for event in (charm.on[relation_name].relation_created,
3235
charm.on[relation_name].relation_joined,
@@ -50,7 +53,7 @@ def all_requests(self):
5053
schema = self._schema(relation)
5154
local_data = relation.data[self.app]
5255
remote_data = relation.data[relation.app]
53-
for key, request_sdata in remote_data.items():
56+
for key, request_sdata in sorted(remote_data.items()):
5457
if not key.startswith('request_'):
5558
continue
5659
name = key[len('request_'):]
@@ -65,21 +68,34 @@ def all_requests(self):
6568
if addr:
6669
request.backends.append(addr)
6770
requests.append(request)
71+
self.state.known_requests.setdefault(request.id, None)
6872
return requests
6973

7074
@property
7175
def new_requests(self):
72-
"""A list of requests with changes or no response.
76+
""" A list of requests with changes or no response.
7377
"""
74-
return [req for req in self.all_requests
75-
if not req.response or req.response.request_hash != req.hash]
78+
return [request for request in self.all_requests
79+
if request.hash != self.state.known_requests[request.id]]
80+
81+
@property
82+
def removed_requests(self):
83+
""" A list of requests which have been removed, either explicitly or
84+
because the relation was removed.
85+
"""
86+
current_ids = {request.id for request in self.all_requests}
87+
unknown_ids = self.state.known_requests.keys() - current_ids
88+
schema = self._schema()
89+
return [schema.Request._from_id(req_id, self.relations)
90+
for req_id in sorted(unknown_ids)]
7691

7792
def send_response(self, request):
7893
""" Send a specific request's response.
7994
"""
80-
request.response.request_hash = request.hash
95+
request.response.received_hash = request.sent_hash
8196
key = 'response_' + request.name
8297
request.relation.data[self.app][key] = request.response.dumps()
98+
self.state.known_requests[request.id] = request.hash
8399
if not self.new_requests:
84100
try:
85101
from charms.reactive import clear_flag
@@ -88,9 +104,18 @@ def send_response(self, request):
88104
except ImportError:
89105
pass # not being used in a reactive charm
90106

107+
def revoke_response(self, request):
108+
""" Revoke / remove the response for a given request.
109+
"""
110+
if request.id:
111+
self.state.known_requests.pop(request.id, None)
112+
if request.relation:
113+
key = 'response_' + request.name
114+
request.relation.data.get(self.app, {}).pop(key, None)
115+
91116
@property
92117
def is_changed(self):
93-
return bool(self.new_requests)
118+
return bool(self.new_requests or self.removed_requests)
94119

95120
def manage_flags(self):
96121
""" Used to interact with charms.reactive-base charms.

loadbalancer_interface/provider.py

Lines changed: 90 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
)
99
from ops.model import ModelError
1010

11-
from .base import LBBase
11+
from .base import VersionedInterface
1212

1313

1414
class LBProviderAvailable(EventBase):
@@ -24,7 +24,7 @@ class LBProviderEvents(ObjectEvents):
2424
responses_changed = EventSource(LBResponsesChanged)
2525

2626

27-
class LBProvider(LBBase):
27+
class LBProvider(VersionedInterface):
2828
""" API used to interact with the provider of loadbalancers.
2929
"""
3030
state = StoredState()
@@ -52,15 +52,20 @@ def _check_provider(self, event):
5252
self.on.available.emit()
5353
if self.is_changed:
5454
self.on.responses_changed.emit()
55-
else:
55+
elif self.state.was_available:
5656
self.state.was_available = False
57+
if self.state.response_hashes:
58+
self.state.response_hashes = {}
59+
self.on.responses_changed.emit()
5760

5861
@property
5962
def relation(self):
6063
return self.relations[0] if self.relations else None
6164

6265
def get_request(self, name):
6366
""" Get or create a Load Balancer Request object.
67+
68+
May raise a ModelError if unable to create a request.
6469
"""
6570
if not self.charm.unit.is_leader():
6671
raise ModelError('Unit is not leader')
@@ -82,42 +87,105 @@ def get_request(self, name):
8287
def get_response(self, name):
8388
""" Get a specific Load Balancer Response by name.
8489
85-
This is equivalent to `get_request(name).response`.
90+
This is equivalent to `get_request(name).response`, except that it
91+
will return `None` if the response is not available.
8692
"""
87-
return self.get_request(name).response
93+
if not self.is_available:
94+
return None
95+
request = self.get_request(name)
96+
if not request.response:
97+
return None
98+
return request.response
8899

89100
def send_request(self, request):
90101
""" Send a specific request.
102+
103+
May raise a ModelError if unable to send the request.
91104
"""
105+
if not self.charm.unit.is_leader():
106+
raise ModelError('Unit is not leader')
107+
if not self.relation:
108+
raise ModelError('Relation not available')
109+
# The sent_hash is used to tell when the provider's response has been
110+
# updated to match our request. We can't used the request hash computed
111+
# on the providing side because it may not match due to default values
112+
# being filled in on that side (e.g., the backend addresses).
113+
request.sent_hash = request.hash
92114
key = 'request_' + request.name
93115
self.relation.data[self.app][key] = request.dumps()
94-
self.state.response_hashes[request.name] = None
116+
117+
def remove_request(self, name):
118+
""" Remove a specific request.
119+
120+
May raise a ModelError if unable to remove the request.
121+
"""
122+
if not self.charm.unit.is_leader():
123+
raise ModelError('Unit is not leader')
124+
if not self.relation:
125+
return
126+
key = 'request_' + name
127+
self.relation.data[self.app].pop(key, None)
128+
self.state.response_hashes.pop(name, None)
129+
130+
@property
131+
def all_requests(self):
132+
""" A list of all requests which have been made.
133+
"""
134+
requests = []
135+
if self.relation:
136+
for key in sorted(self.relation.data[self.app].keys()):
137+
if not key.startswith('request_'):
138+
continue
139+
requests.append(self.get_request(key[len('request_'):]))
140+
return requests
141+
142+
@property
143+
def revoked_responses(self):
144+
""" A list of responses which are no longer available.
145+
"""
146+
return [request.response
147+
for request in self.all_requests
148+
if not request.response
149+
and request.name in self.state.response_hashes]
95150

96151
@cached_property
97152
def all_responses(self):
98153
""" A list of all responses which are available.
99154
"""
100-
local_data = self.relation.data[self.app]
101-
names = [key[len('request_'):]
102-
for key in local_data.keys()
103-
if key.startswith('request_')]
104-
requests = [self.get_request(name) for name in names]
105-
return [request.response for request in requests if request.response]
155+
return [request.response
156+
for request in self.all_requests
157+
if request.response]
158+
159+
@cached_property
160+
def complete_responses(self):
161+
""" A list of all responses which are up to date with their associated
162+
request.
163+
"""
164+
return [request.response
165+
for request in self.all_requests
166+
if request.response.received_hash == request.sent_hash]
106167

107168
@property
108169
def new_responses(self):
109-
""" A list of all responses which have not yet been acknowledged as
170+
""" A list of complete responses which have not yet been acknowledged as
110171
handled or which have changed.
111172
"""
173+
acked_responses = self.state.response_hashes
112174
return [response
113-
for response in self.all_responses
114-
if self.state.response_hashes[response.name] != response.hash]
175+
for response in self.complete_responses
176+
if response.hash != acked_responses.get(response.name)]
115177

116178
def ack_response(self, response):
117179
""" Acknowledge that a given response has been handled.
180+
181+
Can be called on a revoked response as well to remove it
182+
from the revoked_responses list.
118183
"""
119-
self.state.response_hashes[response.name] = response.hash
120-
if not self.new_responses:
184+
if response:
185+
self.state.response_hashes[response.name] = response.hash
186+
else:
187+
self.state.response_hashes.pop(response.name, None)
188+
if not self.is_changed:
121189
try:
122190
from charms.reactive import clear_flag
123191
prefix = 'endpoint.' + self.relation_name
@@ -127,12 +195,16 @@ def ack_response(self, response):
127195

128196
@property
129197
def is_changed(self):
130-
return bool(self.new_responses)
198+
return self.new_responses or self.revoked_responses
131199

132200
@property
133201
def is_available(self):
134202
return bool(self.relation)
135203

204+
@property
205+
def can_request(self):
206+
return self.is_available and self.unit.is_leader()
207+
136208
def manage_flags(self):
137209
""" Used to interact with charms.reactive-base charms.
138210
"""

loadbalancer_interface/schemas/v1.py

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ class _Schema(Schema):
1818
success = fields.Bool(required=True)
1919
message = fields.Str(missing=None)
2020
address = fields.Str(missing=None)
21-
request_hash = fields.Str(missing=None)
21+
received_hash = fields.Str(missing=None)
2222

2323
@validates_schema
2424
def _validate(self, data, **kwargs):
@@ -72,10 +72,27 @@ class _Schema(Schema):
7272
tls_key = fields.Str(missing=None)
7373
ingress_address = fields.Str(missing=None)
7474
ingress_ports = fields.List(fields.Int(), missing=list)
75+
sent_hash = fields.Str(missing=None)
76+
77+
@classmethod
78+
def _from_id(cls, req_id, relations):
79+
""" Return an empty Request with the given ID.
80+
81+
This represents an unknown or removed request.
82+
"""
83+
name, rel_id = req_id.split(':')
84+
request = cls(name)
85+
request._id = req_id
86+
for relation in relations:
87+
if relation.id == rel_id:
88+
request.relation = relation
89+
break
90+
return request
7591

7692
def __init__(self, name):
7793
super().__init__()
7894
self._name = name
95+
self._id = None
7996
# On the provider side, requests need to track which relation they
8097
# came from to know where to send the response.
8198
self.relation = None
@@ -85,6 +102,14 @@ def __init__(self, name):
85102
def name(self):
86103
return self._name
87104

105+
@property
106+
def id(self):
107+
if self._id is None:
108+
if self.relation is None:
109+
return None
110+
self._id = '{}:{}'.format(self.relation.id, self.name)
111+
return self._id
112+
88113
@classmethod
89114
def loads(cls, name, request_sdata, response_sdata=None):
90115
self = cls(name)

setup.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
SETUP = {
55
"name": "loadbalancer_interface",
6-
"version": "1.0.1",
6+
"version": "1.0.2",
77
"author": "Cory Johns",
88
"author_email": "[email protected]",
99
"url": "https://github.com/juju-solutions/loadbalancer-interface",

0 commit comments

Comments
 (0)