Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drain container instance on termination #1692

Closed
wants to merge 4 commits into from

Conversation

MiguelMoll
Copy link
Contributor

@MiguelMoll MiguelMoll commented Feb 7, 2017

  • Test cron/one-off jobs
  • Needs time out? Lambda has a 5 minute max

@codecov
Copy link

codecov bot commented Feb 7, 2017

Codecov Report

Merging #1692 into aws-update will increase coverage by <.01%.

@@              Coverage Diff              @@
##           aws-update   #1692      +/-   ##
=============================================
+ Coverage        30.9%   30.9%   +<.01%     
=============================================
  Files             140     140              
  Lines           13608   13608              
=============================================
+ Hits             4205    4206       +1     
+ Misses           8961    8960       -1     
  Partials          442     442
Impacted Files Coverage Δ
api/models/cronjob.go 86.2% <ø> (+3.44%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67132c4...9f5b74c. Read the comment docs.

@@ -114,6 +129,32 @@ func handle(r Record) error {
return nil
}

func waitForInstanceDrain(cluster, ci string) error {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use one of the Waiters in the SDK for this?

Copy link
Contributor Author

@MiguelMoll MiguelMoll Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can and worth a shot. I went this route first to avoid any state changes that happen when calling the Waiter with tasks grabbed before or after instance change to DRAINING.

Either way, a call to ListTasks() (w/ next pages in mind for a waiter) is done. Then pass those task into the waiter. Figured we have the data already we could check it directly.

@MiguelMoll
Copy link
Contributor Author

SDK states:
Any PENDING or RUNNING tasks that do not belong to a service are not affected; you must wait for them to finish or stop them manually.

Have to handle this case for any scheduled jobs or one off processes that happen to be running. We might have to stop them, as unfortunate as that is. Maybe wait for the lifecycle/lambda (10mins/5mins) timeout?

@MiguelMoll
Copy link
Contributor Author

@ddollar ddollar self-requested a review February 7, 2017 23:46
@MiguelMoll MiguelMoll added this to the 20170208 milestone Feb 8, 2017
MiguelMoll added a commit that referenced this pull request Feb 8, 2017
@MiguelMoll MiguelMoll mentioned this pull request Feb 8, 2017
18 tasks
@MiguelMoll MiguelMoll deleted the instance-draining branch February 11, 2017 16:44
SonOfBytes pushed a commit to SonOfBytes/rack that referenced this pull request Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants