SS 1091 Fix handling of deleted pods in a release #4
Conversation
…to invalid image back to valid image.
…d status objects to app status codes.
|
Just a general thought about logging. We have correlation id enabled in serve. We could pass it here as well for when we would be looking for logs, we would be able to trace the whole chain of events even here. |
| self.assertEqual( | ||
| self.status_data.status_data[release].get("status"), | ||
| "Running", | ||
| f"Release should be Running after delete of first pod, \ | ||
| ts pod deleted={self.pod.metadata.deletion_timestamp} vs \ | ||
| ts invalid_pod deleted={self.invalid_pod.metadata.deletion_timestamp} vs \ | ||
| ts valid_pod created={self.valid_pod.metadata.creation_timestamp}, {msg}", | ||
| ) |
There was a problem hiding this comment.
Just curious, why did you switch to unittests style asserts? assert has a third argument as well, if that was the reason
There was a problem hiding this comment.
This was the pattern already used by Viktor which I chose to continue
| @@ -74,19 +75,222 @@ def test_replica_scenario(self): | |||
| self.new_pod.create(release) | |||
| self.status_data.update({"object": self.new_pod}) | |||
|
|
|||
| time.sleep(0.01) | |||
There was a problem hiding this comment.
Does it actually create objects on the cluster, you've added these sleep calls here?)
There was a problem hiding this comment.
No objects are created on the cluster, just in memory k8s client objects. Without these short pauses the object timestamps were exactly equal which would be exceptionally rare in the real world and were problematic for the implementation.
| def test_waiting_container_reason_pending(self): | ||
| """ | ||
| This scenario tests a k8s pod status object with a container with the following status attributes: | ||
| state=waiting, reason=PodInitializing | ||
| """ | ||
| podstatus = PodStatus() | ||
| podstatus.add_container_status("waiting", "PodInitializing") | ||
| expected = ("Pending", "", "") | ||
| actual = StatusData.determine_status_from_k8s(podstatus) | ||
| self.assertEqual(actual, expected) |
There was a problem hiding this comment.
This and the tests bellow are perfect case for a parameterized test cases. It would reduce test file size quite a bit I think
But if you want to stick to unittest, then there is a subTest context manager that could be used for this
There was a problem hiding this comment.
I see your point but the tests are not so similar as they might seem at first. There are different methods for init containers and non-init containers, and one test adds both objects. So parameterized tests could combine at most 2 or 3 tests into one.
Co-authored-by: Nikita Churikov <[email protected]>
This PR fixes a bug that incorrectly sets the app status to Deleted. This can occur in several scenarios for example if the app image is changed to an invalid image and then back to a valid image.
A function to fetch the current status directly from k8s via the k8s API client is introduced. This is used in special situations when the k8s stream seems to indicate that a pod has been deleted to make sure this is the case.
Also introduces a CI action and extended unit test coverage of the app status logic.