feat: [NPM] Support graceful shutdown in pod #1083
Conversation
JungukCho
added
bug
work-in-progress
npm
JungukCho
changed the title
JungukCho
requested review from
huntergregory,
matmerr,
neaggarwMS and
vakalapa
and removed request for
huntergregory,
matmerr,
neaggarwMS and
vakalapa
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
JungukCho
force-pushed
the
support-pod-grace-period
branch
from
db49d8c to
f0ea128
Compare
| rv string | ||
| podIP string | ||
| labels map[string]string | ||
| isHostNewtwork bool |
There was a problem hiding this comment.
nitpick typo: extra "w" in name
| var defaultGracePeriod int64 = 30 | ||
|
|
||
| type podState struct { | ||
| Phase corev1.PodPhase |
There was a problem hiding this comment.
nitpick: doesn't matter, but is it Go convention to lowercase local struct fields?
There was a problem hiding this comment.
I am not sure, but I think it is good to have lowercase.
There was a problem hiding this comment.
it's not "convention", the casing of struct fields has the same syntactic meaning as public/private keywords in other languages.
this is a field in a private type, it should be private (lowercase).
There was a problem hiding this comment.
So, I addressed it in a right way!
| nPod.Name == podObj.ObjectMeta.Name && | ||
| nPod.Phase == podObj.Status.Phase && | ||
| nPod.PodIP == podObj.Status.PodIP && | ||
| reflect.DeepEqual(nPod.Labels, podObj.ObjectMeta.Labels) && |
There was a problem hiding this comment.
Please don't import reflect outside of tests unless you're doing meta-programming. It's expensive to use, and the result of DeepEqual specifically is sometimes (at the worst times) not what you would expect.
NPM can't spare this performance 🥲
There was a problem hiding this comment.
@rbtr Good catch. We also recently found performance issues using reflect. I think this is good staring point to replace reflect from this PR. I will revise them.
|
Will replace |
* Support graceful shutdown in pod * Update detailed comments and cleaning up codes * Add Unit tests * Address lint errors * Address comments * Addressed comment and add UTs
4 participants
Reason for Change:
PodController in NPM removes ipset information as soon as Pod's status becomes “Terminating”.
So, even though terminated Pod has some activities (e.g., sending traffic to other pods to notify something) during "Terminating" status, NPM does not allow this connection from the Pod.
Thus, NPM needs to delay cleanup process until Pod completes graceful shutdown.
Plus : started trying parallel UTs.
Issue Fixed:
Fixes #1066
Requirements:
Notes:
Only updated podController in v1 now.