-
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 17 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,28 +283,28 @@ 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) | ||
hosts_to_delete.append(host) | ||
except sqlalchemy.orm.exc.ObjectDeletedError: | ||
logger.exception( | ||
"Encountered sqlalchemy.orm.exc.ObjectDeletedError exception during delete_by_id operation. Host was " | ||
"already deleted." | ||
) | ||
|
||
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: | ||
for deleted_host_id in 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. I'm being a bit picky here, but we should likely change deleted_host_id to deleted_host or something similar. |
||
emit_event(events.delete(deleted_host_id)) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -3,6 +3,10 @@ | |||||||
|
||||||||
from marshmallow import fields | ||||||||
from marshmallow import Schema | ||||||||
from marshmallow import validate | ||||||||
|
||||||||
from app.models import CanonicalFacts | ||||||||
from app.validators import verify_uuid_format | ||||||||
|
||||||||
logger = logging.getLogger(__name__) | ||||||||
|
||||||||
|
@@ -11,7 +15,28 @@ class HostEvent(Schema): | |||||||
id = fields.UUID() | ||||||||
timestamp = fields.DateTime(format="iso8601") | ||||||||
type = fields.Str() | ||||||||
account = fields.Str(validate=validate.Length(min=1, max=255)) | ||||||||
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’m thinking whether it wouldn’t be possible to DRY this to some extent with the existing HostSchema using inheritance. I don’t insist on doing it in this pull request though. 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 not repeating the field definitions, but it might take some work to remove the duplication. I think we'd have to break the HostSchema down into smaller chunks (CanonicalFactsSchema, for example, which only contains the canonical facts) and then we'd have to construct the larger schemas (HostSchema, HostEventSchema, etc) out of the smaller schemas using inheritance/composition (if that's possible using marshmallow...i've not looked into marshmallow in that much detail). 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 proposed solution sounds great! I created an issue for that, so it’s not forgotten. #398 |
||||||||
insights_id = fields.Str(validate=verify_uuid_format) | ||||||||
rhel_machine_id = fields.Str(validate=verify_uuid_format) | ||||||||
subscription_manager_id = fields.Str(validate=verify_uuid_format) | ||||||||
satellite_id = fields.Str(validate=verify_uuid_format) | ||||||||
fqdn = fields.Str(validate=validate.Length(min=1, max=255)) | ||||||||
bios_uuid = fields.Str(validate=verify_uuid_format) | ||||||||
ip_addresses = fields.List(fields.Str(validate=validate.Length(min=1, max=255))) | ||||||||
mac_addresses = fields.List(fields.Str(validate=validate.Length(min=1, max=255))) | ||||||||
external_id = fields.Str(validate=validate.Length(min=1, max=500)) | ||||||||
|
||||||||
|
||||||||
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(), | ||||||||
**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
|
||||||||
"type": "delete", | ||||||||
} | ||||||||
) | ||||||||
.data | ||||||||
) |
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.