fix(aws-ecs): export instance drain hook#1392
Conversation
|
Yes. It's not clear to me why it should be exported. @cohalz, what problems is this causing for you? |
|
Even if I create your own without using the ECS construct library I want to use instance drain hook. |
|
That brings up my next question: what use cases of yours is the rest of the ECS library not satsifying? |
|
Oh I see, spot instances |
|
yes, even when I make ASG or clusters myrself, the functions provided by InstanceDrainHook are essential, so there are cases I want to use. |
|
I don't have any experience with spot instances myself, but are you sure the termination hook as coded will even work properly? Seems like spot instances cannot be kept in TERMINATING state for an arbitrary amount of time (as this script will try to do) and the termination notification actually works differently (by polling the instance metadata service). I've gleaned this from code found at the bottom of this post. Are you sure the SNS notification hook will be fired? |
|
I don't confirm whether hooks can be used in spot instances, I think that there is no loss by being exported when using ondemand. |
|
I'm sorry, I'm going to close this PR. For now the drain hook is an implementation detail that I'd rather not expose, and I don't think it will work for your use case. If I'm wrong, or a definite use case for it pops up, we can reconsider this. |
fix #1391
Pull Request Checklist
Please check all boxes, including N/A items:
Testing
Documentation
Title and description
fix(module): <title>bug fix (patch)feat(module): <title>feature/capability (minor)chore(module): <title>won't appear in changelogbuild(module): <title>won't appear in changelogBREAKING CHANGE: <describe exactly what changed and how to achieve similar behavior + link to documentation/gist/issue if more details are required>Fixes #xxxorCloses #xxxBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.