Support cordon only

[mazzy89]

Would be possible to support cordon only instead of also drain? I'm keen to wire PR and make a contribution. I'm just asking here before doing that if the feature would be well accepted.

My use case is that I have CI jobs running in Spot instances and cordon and drain the node would result in a poor experience for the devs because their jobs would be killed beforehand.

Instead of only cordon, the node would make any new jobs non-schedulable in the spot instances that are going to be killed.

[ulisse31]

For Spot Instances, the node would be killed in 2 minutes in any case so the end result would be the same as draining the node, without the additional value of a "clean" pod shutdown, when possible?

It would however have a good use when normal instances are scheduled for termination following AWS hardware issues...

[mazzy89]

Imagine a scenario of a high number of CI jobs scheduled. If the node is not cordoned within those 2 minutes still other jobs can be scheduled in that node. A cordon would at least save those new jobs to be scheduled and killed.

[ulisse31]

mhhh I'm not expert at all, so probably I'm misunderstanding what you mean, sorry.
The NTH already cordons + drain so basically achieving already what you need? Unless your CI jobs takes less than 100~120 secs to complete (the time within the node and all its pods will be killed in any case by AWS).

Having the option to only cordon however, doesn't remove or limit any existing feature, so I'd say it would be a nice to have. :)

[mazzy89]

Thank you @ulisse31 for the fast reaction here.

NTH already cordons the nodes but also drain them which means killing the jobs that are still executing. Having the feature of only cordon them would make possible to those jobs to finish and if they are able to complete within 100~120 secs give a better experience to devs.

Sure if those jobs don't finish within 100~120secs from the signal creation then as you correctly said the experience is the same as cordon + drain.

I will give a try then and wire this feature. Thank you 🙏

[ulisse31]

Thank to you too!
Disclaimer: I'm not involved in the development of this project, only an user so you may want to check with the project maintainer :)
cheers

[mattrandallbecker]

Sounds like a good idea to me (one of the maintainers).

Even if your jobs take longer than 120s it may be possible there is less than 120s of work left before they finish and if you don't have any mechanisms to checkpoint your work and pick up where you left off your best option may be to let them run their course but prevent scheduling new pods.

We tried to allow some additional flexibility on how much longer you could use the instance via https://github.com/aws/aws-node-termination-handler/blob/master/config/helm/aws-node-termination-handler/values.yaml#L52 but I think this suggestion allows for some more granular control of what happens when instances receive an ITN.

[mazzy89]

Hi @mattrandallbecker thank you to jumping on this.

if you don't have any mechanisms to checkpoint your work and pick up where you left off your best option may be to let them run their course but prevent scheduling new pods.

exactly. you got perfectly the point. unfortunately, CI jobs don't have the possibility to implement checkpoints and it would be very useful to have the drain-only feature.

I would like to give it a shot if it is ok for you.

[mattrandallbecker]

Definitely ok with me, look forward to the contribution.

[mazzy89]

I see that we use a channel named drainChan to push there drain events after an interruption event is found.

It would be better to create here a new channel and use something like cordonChan and use it for cordon only events or rename the existing channel to cover both the use cases cordon-only and cordon&drain? I would vote for the second because at the end of the day a cordon comes before a drain and by introducing this feature we just say that we want the only cordon and not drain.

This choice would lead to renaming functions and variables because I saw that a lot of things are named like drain

[bwagner5]

@mazzy89 agreed on just renaming drainChan to accommodate cordon+drain and just cordon, maybe interruptionChan?. And yep, renaming a bunch of stuff, since we included cordon as an integral part of what we're calling drain.

[mazzy89]

yeah the naming is perfect interruptionChan agreed. thank you @bwagner5