Adding delete #236

jhjaggars · 2019-04-25T15:46:25Z

No description provided.

Glutexo

Unfortunately this pull request can’t go through. To summarize my inline comments, I see mainly these issues there:

Emitting events to the message queue doesn’t work. There aren’t tests for that so it was not caught.
Result of the operation is not reported, 200 OK is always returned instead of 207 Multi-Status. Even the 404 Not Found described in the API specification is never returned.
The message queue producer is instantiated on import, causing side-effects and hinders testability.
The tests use real message queue.

Also there are now conflicts with master that need to be resolved before merging.

Just as a matter of personal taste, it might be even possible to have separate pull requests for the Kafka producer and for the delete operation. But this pull request is quite small, so no need to forcibly split it. Just saying.

README.md

Glutexo · 2019-05-10T10:09:07Z

api/host.py

 from app.models import Host, HostSchema, PatchHostSchema
 from app.auth import current_identity
-from app.exceptions import InventoryException, InputFormatException
+from app.exceptions import InventoryException


This change is unrelated to the DELETE operation.

Glutexo · 2019-05-10T10:16:57Z

api/host.py

@@ -259,11 +274,16 @@ def get_host_by_id(host_id_list, page=1, per_page=100):
    )


-def _get_host_list_by_id_list(account_number, host_id_list):
-    return Host.query.filter(
+def _get_host_list_by_id_list(account_number, host_id_list, order=True):


Thinking whether we couldn’t just remove the order_by from this function entirely and use it in the respective places. E.g. the patch operation doesn’t need the ordering at all. The oder operations (get_host_by_id and get_host_system_profile_by_id) use paginate, which is the reason for the ordering. So although repeating the oder_by wouldn’t be dry, it would fit nicely together. But this is not necessary to edit in this PR.

I think this is fine for now.

Glutexo · 2019-05-10T10:19:26Z

app/events.py

@@ -0,0 +1,17 @@
+import logging


Although I like putting the event schema into a separate module instead of the ever growing app.models one, it’s currently unsystematic. The app.models module already contains many schemas that aren’t models at all, e.g. the PatchHostSchema. I’d keep everything in that bulky module for now and split it later separately.

The delete method can be for now put inline into the delete_by_id operation. It’s not very nice, but it’s how we currently do it. The HTTP responses, errors and everything are composed there too. I’d rather have the code consistent than to see many first steps to various refactorings.

Glutexo · 2019-05-10T12:26:32Z

tasks/__init__.py

@@ -15,6 +16,13 @@
 TOPIC = os.environ.get("KAFKA_TOPIC", "platform.system-profile")
 KAFKA_GROUP = os.environ.get("KAFKA_GROUP", "inventory")
 BOOTSTRAP_SERVERS = os.environ.get("KAFKA_BOOTSTRAP_SERVERS", "kafka:29092")
+EVENT_TOPIC = os.environ.get("KAFKA_EVENT_TOPIC", "platform.inventory.events")
+
+producer = KafkaProducer(bootstrap_servers=BOOTSTRAP_SERVERS)


Starting a producer on import is not a very good thing. It’s heavy on resources and dependencies and there are a lot of side effects. Let’s initialize it explicitly like start_consumer and put it into the application context. Like that it’ll be even possible to replace the producer with a dummy (a mock) when running in the test suite. Without that it’s impossible to test the message production.

Glutexo · 2019-05-10T13:56:58Z

test_api.py

+        self.assertEqual(response["count"], 0)
+        self.assertEqual(response["total"], 0)
+        self.assertEqual(response["results"], [])
+


Two newlines between classes, please.

Glutexo · 2019-05-10T14:00:20Z

test_api.py

+        response = self.get(HOST_URL + "/" + original_id, 200)
+
+        # Delete the host
+        response = self.delete(HOST_URL + "/" + original_id, 200, return_response_as_json=False)


Why the return_response_as_json argument here? I know that there is not much need of decoding the JSON when the response is not used for anything, but neither it causes any big harm (a few CPU cycles). It adds noise though – it looks like we want to do something with the original string. Aha! It’s there because decoding the empty response fails. Ok.

Moreover, the response variable is not used at all – the response value is not checked. There is however no reason to check, because this request doesn’t return anything. I think it could return a list of deleted hosts, or even better a 207 Multi-Status response with a status and details about every deleted host, just as create_host_list does.

Glutexo · 2019-05-13T14:01:15Z

api/host.py

+        hosts.delete(synchronize_session="fetch")
+    db.session.commit()
+    metrics.delete_host_count.inc(hosts.count())
+    for deleted_host in hosts:


This actually doesn’t work The hosts variable is a BaseQuery, not a list. This query is used just a few lines earlier to delete the hosts. Then here, iterating over the hosts run a SELECT query, which doesn’t find anything, because the hosts are already deleted.

@api_operation @metrics.api_request_time.time() def delete_by_id(host_id_list): query = _get_host_list_by_id_list( current_identity.account_number, host_id_list, order=False ) hosts = query.all() with metrics.delete_host_processing_time.time(): query.delete(synchronize_session="fetch") db.session.commit() metrics.delete_host_count.inc(len(hosts)) for deleted_host in hosts: emit_event(events.delete(deleted_host.id))

This should be fixed.

Glutexo · 2019-05-13T14:11:07Z

api/host.py

+    metrics.delete_host_count.inc(hosts.count())
+    for deleted_host in hosts:
+        emit_event(events.delete(deleted_host.id))
+


There is no return statement. The API specification specifies though, that this operations should return 404 Not Found if the host is not found. This is currently not true: 200 OK is returned automatically, but nothing is deleted.

Because this can operate on more than one host, I suggest using 207 Multi-Status instead, just at add_host_list does. Like that it’d be possible to return more statūs, one for each records. It can happen that some hosts are found (and deleted) and some are not.

At this point, I do not think adding the 207 multi-status is required/needed. We can add it later on if we find that it is needed.

Glutexo · 2019-05-13T14:14:20Z

test_api.py

+        original_id = created_host["id"]
+
+        # Get the host
+        response = self.get(HOST_URL + "/" + original_id, 200)


The response variable is not used. Why the assignment?

What about extracting the URL to a variable. It’s shared between this get call and the delete call a few lines below.

Glutexo · 2019-05-14T07:46:55Z

tasks/__init__.py

@@ -15,6 +16,13 @@
 TOPIC = os.environ.get("KAFKA_TOPIC", "platform.system-profile")
 KAFKA_GROUP = os.environ.get("KAFKA_GROUP", "inventory")
 BOOTSTRAP_SERVERS = os.environ.get("KAFKA_BOOTSTRAP_SERVERS", "kafka:29092")
+EVENT_TOPIC = os.environ.get("KAFKA_EVENT_TOPIC", "platform.inventory.events")


I’d prefer to have the environment variable names prefixed to not clash with other apps in development. But since other ones are not prefixed too, it’s not necessary to do it here and make this pull request inconsistent. Created a separate issue for that instead. #271

Glutexo · 2019-05-14T12:27:35Z

tasks/__init__.py

+
+
+def emit_event(e):
+    producer.send(EVENT_TOPIC, value=e.encode("utf-8"))


According to the documentation, the messages are sent asynchronously. If the application crashes before the messages is actually sent to the queue, it’s lost. This can be solved by calling producer.flush, which with synchronously block, until all messages are sent.

Glutexo · 2019-05-15T11:23:20Z

Created a little demo on how to test the emitted events: see the delete-hosts branch in my fork. It swaps the message producer in the tests, replacing the KafkaProducer with a class that mocks the used part of its interface, storing the messages in the memory. Those can be then inspected by the tests. I added one simple test that checks the message. There is also a fix to the event-emitting code.

(Don’t worry about the recent force-push to the this upstream branch. I just mistook the remote and reverted it then back to its original state.)

Signed-off-by: Jesse Jaggars <[email protected]>

* Adding a couple of metrics for the host delete operation * Commit the deleted hosts * Only emit events for hosts that were actually deleted * Verify the host is deleted in the test case

* Adding delete event info to the README

Signed-off-by: Jesse Jaggars <[email protected]>

Co-Authored-By: Glutexo <[email protected]>

method will initilize / start the event producer and system profile consumer if kafka is enabled.

…tialization Modified the way the tasks module is initialized.

Glutexo · 2019-05-21T04:23:43Z

I added one more commit to my fixes demo branch. It uses Flask’s App Context teardown to flush the message queue instead of doing so right after the emit. This is how it should work. The messages are flushed even if an exception and 500 Internal Server Error occur.

* adding delete api * emitting events for deletes * Adding a couple of metrics for the host delete operation * Verify the host is deleted in the test case * Delete hosts add info to readme (RedHatInsights#251) * Adding delete event info to the README

* adding delete api * emitting events for deletes * Adding a couple of metrics for the host delete operation * Verify the host is deleted in the test case * Delete hosts add info to readme (#251) * Adding delete event info to the README

Glutexo requested changes May 14, 2019

View reviewed changes

Glutexo mentioned this pull request May 14, 2019

Prefix environment variables #271

Open

Glutexo reviewed May 14, 2019

View reviewed changes

Glutexo force-pushed the delete-hosts branch from 65d3d38 to 6b7e201 Compare May 15, 2019 11:14

jhjaggars and others added 10 commits May 15, 2019 13:23

adding delete api

b78511a

Signed-off-by: Jesse Jaggars <[email protected]>

emitting events for deletes

b24a23b

Signed-off-by: Jesse Jaggars <[email protected]>

adding events module

56b6e1f

Signed-off-by: Jesse Jaggars <[email protected]>

removing unused imports

7b90090

Signed-off-by: Jesse Jaggars <[email protected]>

Modified how the event json is built

0015e22

Changing the formatting back to the original (#247)

8c8c155

Adding a couple of metrics for the host delete operation (#248)

91650a6

* Adding a couple of metrics for the host delete operation * Commit the deleted hosts * Only emit events for hosts that were actually deleted * Verify the host is deleted in the test case

Delete hosts add info to readme (#251)

961523e

* Adding delete event info to the README

using a null producer when kafka isn't available

7de7ec5

Signed-off-by: Jesse Jaggars <[email protected]>

fixing up old patch spec

bfdd88a

Signed-off-by: Jesse Jaggars <[email protected]>

jhjaggars force-pushed the delete-hosts branch from 1bb093e to bfdd88a Compare May 15, 2019 18:31

jhjaggars and others added 9 commits May 15, 2019 13:57

testing the event emit function

7dbe785

Signed-off-by: Jesse Jaggars <[email protected]>

Update README.md

ac2a81f

Co-Authored-By: Glutexo <[email protected]>

Return a 404 when no hosts are found on the delete operation

43b9b05

Use the PreCreatedHostsBaseTestCase to implement the delete tests

19c25f7

Add kafka server to the development docker compose file

199b835

Add some more logging around the event producer

47e6316

Modified the way the tasks module is initialized. The init_tasks()

5015d89

method will initilize / start the event producer and system profile consumer if kafka is enabled.

Change the way a global variable is handled

100ac9c

Merge pull request #274 from dehort/delete-hosts_reorganize_tasks_ini…

1f767e0

…tialization Modified the way the tasks module is initialized.

Added a test for deleting with an invalid host id (uuid)

eee9b0a

dehort merged commit af65dea into master May 21, 2019

Glutexo mentioned this pull request May 24, 2019

Remove unittest.mock from API tests #280

Open

gmcculloug deleted the delete-hosts branch November 5, 2020 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding delete #236

Adding delete #236

jhjaggars commented Apr 25, 2019

Glutexo left a comment •

edited

Loading

Glutexo May 10, 2019

Glutexo May 10, 2019

dehort May 15, 2019

Glutexo May 10, 2019

Glutexo May 10, 2019

Glutexo May 10, 2019

dehort May 20, 2019

Glutexo May 10, 2019 •

edited

Loading

Glutexo May 13, 2019

dehort May 15, 2019

Glutexo May 13, 2019

dehort May 20, 2019

Glutexo May 13, 2019

Glutexo May 14, 2019

Glutexo May 14, 2019

Glutexo commented May 15, 2019 •

edited

Loading

Glutexo commented May 21, 2019 •

edited

Loading



		def emit_event(e):
		producer.send(EVENT_TOPIC, value=e.encode("utf-8"))

Adding delete #236

Adding delete #236

Conversation

jhjaggars commented Apr 25, 2019

Glutexo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glutexo May 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glutexo commented May 15, 2019 • edited Loading

Glutexo commented May 21, 2019 • edited Loading

Glutexo left a comment •

edited

Loading

Glutexo May 10, 2019 •

edited

Loading

Glutexo commented May 15, 2019 •

edited

Loading

Glutexo commented May 21, 2019 •

edited

Loading