feat: update pod status logic #214
Conversation
![orca-security-us[bot]](https://avatars.githubusercontent.com/in/276250?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
obs-gh-alexlew
changed the title
|
tested in my sandbox instance and things look reasonable |
| if condition.Type != v1.PodInitialized { | ||
| continue | ||
| } | ||
|
|
||
| return condition.Status == v1.ConditionTrue |
There was a problem hiding this comment.
Maybe change this to
| if condition.Type != v1.PodInitialized { | |
| continue | |
| } | |
| return condition.Status == v1.ConditionTrue | |
| if condition.Type == v1.PodInitialized && condition.Status == v1.ConditionTrue { | |
| return true | |
| } |
to match hasPodReadyCondition
There was a problem hiding this comment.
Bumping this one
There was a problem hiding this comment.
this is actually just helper code I copied from the same kubectl code, if its semantically the same i'd prefer to leave it in case that helper code changes in the future as well it'll be easier to copy/compare
| case isRestartableInitContainer(initContainers[container.Name]) && | ||
| container.Started != nil && *container.Started: | ||
| if container.Ready { | ||
| readyContainers++ | ||
| } | ||
| continue |
There was a problem hiding this comment.
Should this be in the switch statement? It could skip the default case and miss setting the reason there.
| restarts := 0 | ||
| restartableInitContainerRestarts := 0 | ||
| totalContainers := len(pod.Spec.Containers) | ||
| readyContainers := 0 | ||
| lastRestartDate := metav1.NewTime(time.Time{}) | ||
| lastRestartableInitContainerRestartDate := metav1.NewTime(time.Time{}) |
There was a problem hiding this comment.
A lot of these new variables don't seem to be used for anything; is this coming in a follow up?
| }, | ||
| }, | ||
| // { // Tests that we don't override/drop other facets computed in OTTL |
There was a problem hiding this comment.
Can you replace these deleted tests with new ones?
| restarts := 0 | ||
| restartableInitContainerRestarts := 0 | ||
| totalContainers := len(pod.Spec.Containers) | ||
| readyContainers := 0 |
There was a problem hiding this comment.
It still looks like these four variables are unused.
There was a problem hiding this comment.
so here's the logic in datadog agent https://github.com/DataDog/datadog-agent/blob/7d47d25899c025c8b3e5ba91e488fa0f97f7a275/pkg/collector/corechecks/cluster/orchestrator/transformers/k8s/pod.go#L220 they also only use status but kept the rest of the logic so that they can just copy paste it over
There was a problem hiding this comment.
think the argument for taking it out is that its unnecessary computation / more efficient which i agree with but its a slight preference vs. way better imo
There was a problem hiding this comment.
Interesting. I'm happy with where you landed!
If we need to change it again, I think it could be more clear to have a helper that returns all values and is very explicitly full copy/paste. Then we could call that and extra only what we want
| if condition.Type != v1.PodInitialized { | ||
| continue | ||
| } | ||
|
|
||
| return condition.Status == v1.ConditionTrue |
There was a problem hiding this comment.
Bumping this one
| restarts := 0 | ||
| restartableInitContainerRestarts := 0 | ||
| totalContainers := len(pod.Spec.Containers) | ||
| readyContainers := 0 |
There was a problem hiding this comment.
Interesting. I'm happy with where you landed!
If we need to change it again, I think it could be more clear to have a helper that returns all values and is very explicitly full copy/paste. Then we could call that and extra only what we want
Description
OB-XXX Please explain the changes you made here.
Checklist