-
Notifications
You must be signed in to change notification settings - Fork 82
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
Added new elements to delete event #392
Changes from all commits
1095e6e
79c48c5
610227f
5038067
b0d8808
4c95079
c046158
08a275c
addc78e
40f6db1
e6de4f2
a789023
bc5b827
d6f8476
8937269
37ae521
5595299
8a76897
4612bf4
db47368
b2299f0
a698d7f
1bbf69c
d1b46a2
44b99d3
1af93ce
8e1892e
2dc997b
05367ff
355ba89
fd38720
60025fb
a2b0c4d
c4c01ef
b586b70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -283,29 +283,29 @@ def find_hosts_by_hostname_or_id(account_number, hostname): | |||||
def delete_by_id(host_id_list): | ||||||
query = _get_host_list_by_id_list(current_identity.account_number, host_id_list) | ||||||
|
||||||
host_ids_to_delete = [] | ||||||
hosts_to_delete = [] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better! 😀 |
||||||
for host in query.all(): | ||||||
try: | ||||||
host_ids_to_delete.append(host.id) | ||||||
except sqlalchemy.orm.exc.ObjectDeletedError: | ||||||
logger.exception( | ||||||
"Encountered sqlalchemy.orm.exc.ObjectDeletedError exception during delete_by_id operation. Host was " | ||||||
"already deleted." | ||||||
) | ||||||
hosts_to_delete.append(host) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess we need to figure out how to eagerly load the host here, essentially doing manually what happens on the attribute access. I would not eagerly load all of them at once, just do something like object identity check here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we’re not going to eagerly load in this PR, please abbreviate this to
Suggested change
|
||||||
|
||||||
if not host_ids_to_delete: | ||||||
if not hosts_to_delete: | ||||||
return flask.abort(status.HTTP_404_NOT_FOUND) | ||||||
|
||||||
with metrics.delete_host_processing_time.time(): | ||||||
query.delete(synchronize_session="fetch") | ||||||
db.session.commit() | ||||||
|
||||||
metrics.delete_host_count.inc(len(host_ids_to_delete)) | ||||||
metrics.delete_host_count.inc(len(hosts_to_delete)) | ||||||
|
||||||
logger.debug("Deleted hosts: %s", host_ids_to_delete) | ||||||
logger.debug("Deleted hosts: %s", hosts_to_delete) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works only thanks to the fact that logger has some own exception handling mechanism. This logging call actually calls _Host._repr for every host, even for those that are already deleted. This function internally calls Host.id that triggers the lazy loading and bang, an uncaught exception occurs. Hopefully, this is internally caught by the logger output to stderr and the execution continues normally. This is probably for proper logging calls not to cause unexpected termination. But if logger’s interpolation weren’t used ( So, sorry, @dehort. I understand that you don’t want to have this pending any longer, but we’re really on a thin ice here. The logging doesn’t crash just because it behaves unexpectedly wise, and catching the error there when emitting an event, that’s just randomly putting a try-expect on a first side-effect that triggers the lazy loading. If you really need to have this merged in the current state. Please, at least, let @aleccohan to fix this in a subsequent PR, having that properly in a Sprint. This is not something we want in our codebase. It can go well with writing automatic test for this. |
||||||
|
||||||
for deleted_host_id in host_ids_to_delete: | ||||||
emit_event(events.delete(deleted_host_id)) | ||||||
for deleted_host in hosts_to_delete: | ||||||
try: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change actually modifies the behavior. Before the change, if a host has been already deleted, it is not added to the hosts_to_delete list. That caused aborting with 404 if no hosts were actually deleted, and also those not actually deleted hosts not to be logged. Now, all hosts to delete are logged even though they have been already deleted and raise the ObjectDeletedError here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might change the behavior a bit but i'm not sure how big of a change it really is. With this change, the only difference I see is that we might not send back a 404 in the case where all the hosts that were requested to be deleted, were already deleted. (This situation is likely pretty rare anyhow.) I suspect with the try/catch added around the emit_event(), then we shouldn't produce a delete event in the case where a host was deleted already (that would be the correct behavior in my opinion). In either case (200 return code vs 404 return code), the host would be deleted and on event would be logged. I'm not sure that's a big difference. |
||||||
emit_event(events.delete(deleted_host)) | ||||||
except sqlalchemy.orm.exc.ObjectDeletedError: | ||||||
logger.exception( | ||||||
"Encountered sqlalchemy.orm.exc.ObjectDeletedError exception during delete_by_id operation. Host was " | ||||||
"already deleted." | ||||||
) | ||||||
|
||||||
|
||||||
@api_operation | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -4,14 +4,34 @@ | |||||||
from marshmallow import fields | ||||||||
from marshmallow import Schema | ||||||||
|
||||||||
from app.logging import threadctx | ||||||||
from app.models import CanonicalFacts | ||||||||
from app.validators import verify_uuid_format | ||||||||
|
||||||||
logger = logging.getLogger(__name__) | ||||||||
|
||||||||
|
||||||||
class HostEvent(Schema): | ||||||||
id = fields.UUID() | ||||||||
timestamp = fields.DateTime(format="iso8601") | ||||||||
type = fields.Str() | ||||||||
account = fields.Str(required=True) | ||||||||
insights_id = fields.Str(validate=verify_uuid_format) | ||||||||
request_id = fields.Str() | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the request ID has a fixed format? Even if so, we probably don’t want to specify it here as we’re only blindly forwarding it. |
||||||||
|
||||||||
|
||||||||
def delete(id): | ||||||||
return HostEvent(strict=True).dumps({"id": id, "timestamp": datetime.utcnow(), "type": "delete"}).data | ||||||||
def delete(host): | ||||||||
return ( | ||||||||
HostEvent() | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we actually need to use the Schema to format the message? It’s our code that composes it, so it is inherently trusted. The format can (and should) be tested by our tests. Using Schema for validation makes sense for data from the outside. Host serialization currently doesn’t use Schema and it results in a discrepancy in how DateTime is formatted. In the HTTP output, the created and updated are formatted manually as 2019-08-15T11:36:28.105816Z. Marshmallow formats it as 2019-08-15T11:36:28.105816+00:00. Both are valid ISO 8601 formats, but they are different. But maybe a good solution would be to use a Schema for the Host output too instead of removing it from here. This looks like a new issue. #399 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of using the Schemas for defining what our input and output should look like. I think it makes it easy to point someone to the schema so that they can see what is expected for input/output. Otherwise, it can be kind of difficult to determine what the expected input/output is. I did just think of something that we need to watch out for though. I think we should remove the validation checks from this schema. As @Glutexo mentioned, we are the ones producing the data...so there shouldn't be a need to validate it on the way out. Plus if someone tries to delete an "old" host that has data that doesn't match our current rules, then it would fail to produce a delete event for that object. |
||||||||
.dumps( | ||||||||
{ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This appears to be missing the id field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really! It probably wasn’t in my proposed code snippet and nobody realized. Good catch! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good thing is that the current tests catch this. 🙂 |
||||||||
"timestamp": datetime.utcnow(), | ||||||||
"id": host.id, | ||||||||
**CanonicalFacts.to_json(host.canonical_facts), | ||||||||
"account": host.account, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
"request_id": threadctx.request_id, | ||||||||
"type": "delete", | ||||||||
} | ||||||||
) | ||||||||
.data | ||||||||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1107,10 +1107,18 @@ def create_hosts(self): | |
|
||
for host in hosts_to_create: | ||
host_wrapper = HostWrapper() | ||
host_wrapper.id = generate_uuid() | ||
host_wrapper.account = ACCOUNT | ||
host_wrapper.display_name = host[0] | ||
host_wrapper.insights_id = host[1] | ||
host_wrapper.insights_id = generate_uuid() | ||
host_wrapper.rhel_machine_id = generate_uuid() | ||
host_wrapper.subscription_manager_id = generate_uuid() | ||
host_wrapper.satellite_id = generate_uuid() | ||
host_wrapper.bios_uuid = generate_uuid() | ||
host_wrapper.ip_addresses = ["10.0.0.2"] | ||
host_wrapper.fqdn = host[2] | ||
host_wrapper.mac_addresses = ["aa:bb:cc:dd:ee:ff"] | ||
host_wrapper.external_id = generate_uuid() | ||
host_wrapper.facts = [{"namespace": "ns1", "facts": {"key1": "value1"}}] | ||
|
||
response_data = self.post(HOST_URL, [host_wrapper.data()], 207) | ||
|
@@ -1208,10 +1216,10 @@ def test_invalid_host_id(self): | |
|
||
|
||
class DeleteHostsTestCase(PreCreatedHostsBaseTestCase): | ||
def test_create_then_delete(self): | ||
original_id = self.added_hosts[0].id | ||
@unittest.mock.patch("app.events.datetime", **{"utcnow.return_value": datetime.utcnow()}) | ||
def test_create_then_delete(self, datetime_mock): | ||
|
||
url = HOST_URL + "/" + original_id | ||
url = HOST_URL + "/" + self.added_hosts[0].id | ||
|
||
# Get the host | ||
self.get(url, 200) | ||
|
@@ -1226,7 +1234,18 @@ def __call__(self, e): | |
# Delete the host | ||
with unittest.mock.patch("api.host.emit_event", new=MockEmitEvent()) as m: | ||
self.delete(url, 200, return_response_as_json=False) | ||
assert original_id in m.events[0] | ||
event = json.loads(m.events[0]) | ||
timestamp_iso = datetime_mock.utcnow.return_value.isoformat() | ||
|
||
self.assertIsInstance(event, dict) | ||
expected_keys = {"timestamp", "type", "id", "account", "insights_id", "request_id"} | ||
self.assertEqual(set(event.keys()), expected_keys) | ||
|
||
self.assertEqual(f"{timestamp_iso}+00:00", event["timestamp"]) | ||
self.assertEqual("delete", event["type"]) | ||
self.assertEqual(self.added_hosts[0].id, event["id"]) | ||
self.assertEqual(self.added_hosts[0].insights_id, event["insights_id"]) | ||
self.assertEqual("-1", event["request_id"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we test a real forwarded request ID too along with this default unknown one? |
||
|
||
# Try to get the host again | ||
response = self.get(url, 200) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change shouldn’t be here, but let it be.