Conversation
cuza
changed the base branch from
u/cuza/upgrade-paasta-playground-to-python-3.10
to
master
…w-terminating-pods * master: Released 1.36.3 via make release Moved the scs conf from --conf to env var (#4177) Released 1.36.2 via make release Revert "remove some unused code." Released 1.36.1 via make release Revert "Merge pull request #4151 from Yelp/timma/upgrade_spark_configuration_lib_to_3.3.7" (#4174) Released 1.36.0 via make release Released 1.35.8 via make release remove some mesos-isms from local-run remove some unused code. Changed the expected_scs_conf to json instead of raw string Changed the user to null in scs_conf which fixes the failing tron_tools test Upgraded service configuration lib to 3.3.7 and updated the test accordingly
ilkinmammadzada
left a comment
ilkinmammadzada
left a comment
There was a problem hiding this comment.
LGTM, with a small comment
| # Check if pod is terminating (has deletion timestamp) | ||
| delete_timestamp = getattr(pod, "delete_timestamp", None) |
There was a problem hiding this comment.
i think we can swap this to just pod.delete_timestamp once this is fully rolled out, right?
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/KubernetesPodEvent' | ||
| delete_timestamp: |
There was a problem hiding this comment.
imo, it'd be nice to keep the k8s name here (i.e., s/delete_timestamp/deletion_timestamp)
|
|
||
| A replicaset is considered "actually running" if: | ||
| - It has non-zero desired replicas OR non-zero ready replicas, OR | ||
| - It has pods associated with it (e.g., terminating pods) |
There was a problem hiding this comment.
| - It has pods associated with it (e.g., terminating pods) | |
| - It has ANY pods associated with it (e.g., terminating pods are still "running") |
| and ( | ||
| pod_status_by_replicaset is None | ||
| or not pod_status_by_replicaset.get(rs.metadata.name) |
There was a problem hiding this comment.
hmm, i think that once we're at a newer k8s version, we can replace this with looking at rs.status.terminating_replicas
...but for now, i think this is fine?
(that said, i think this function is either at or getting to the point of being too clever and we might want to refactor this at some point :p)
There was a problem hiding this comment.
(although, maybe just some comments on the new conditions would be good enough for a bit longer? mostly so that folks don't need to figure out why pod_status_by_replicaset being None or a RS not being in there means there's only terminating pods for that rs)
| mock_naturaltime, | ||
| mock_kubernetes_pod, | ||
| ): | ||
| from datetime import datetime as real_datetime |
There was a problem hiding this comment.
we can just use datetime.datetime, right? (since we have a top-level import datetime import)
3 participants
This change ensures that terminating pods are still represented in the status output until they are fully removed.