Improve monitorable server job exception handling #331

evansiroky · 2020-07-24T22:09:34Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This improves the reliability of the MonitorServerStatusJob by doing the following:

Catching exceptions that can occur when making a AWS request to get the instance health and then retrying up to a max number of times (hardcoded as 5).
Catching all possible exceptions that can occur in the MonitorServerStatusJob and then failing the job gracefully in order for the MonitorServerStatusJob to be able to also recover and not leave any hanging instances.
Refactor a helper in MonitorServerStatusJob to include exceptions in some job status failures.
Using fresh ec2 clients to try to avoid some expired request errors (Refactor AWS client creation #332 has code that should avoid all request timeouts related to role session timeouts)
Always checking for exceptions when terminating instances and adding to the job output when instance termination failed.
Corrects the graph build timeout time
Introduces helper class TimeTracker
various otp-runner updates
- Using a nonce to verify otp-runner status files
- Uploads the otp-runner manifest, build-config.json and router-config.json to S3
- Updates the otp-runner version and corresponding code to be compatible with changes to otp-runner that refactor file downloads
- creates and uploads separate otp-runner manifest files for graph building and server running

codecov-commenter · 2020-07-24T22:14:43Z

Codecov Report

Merging #331 into dev will decrease coverage by 0.05%.
The diff coverage is 19.23%.

@@             Coverage Diff              @@
##                dev     #331      +/-   ##
============================================
- Coverage     24.10%   24.04%   -0.06%     
- Complexity      539      542       +3     
============================================
  Files           168      169       +1     
  Lines          8821     8895      +74     
  Branches       1222     1230       +8     
============================================
+ Hits           2126     2139      +13     
- Misses         6486     6543      +57     
- Partials        209      213       +4

Flag	Coverage Δ	Complexity Δ
#unit_tests	`24.04% <19.23%> (-0.06%)`	`542.00 <5.00> (+3.00)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
.../com/conveyal/datatools/common/utils/AWSUtils.java	`7.69% <0.00%> (ø)`	`4.00 <0.00> (ø)`
...datatools/manager/jobs/MonitorServerStatusJob.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...eyal/datatools/manager/jobs/OtpRunnerManifest.java	`100.00% <ø> (ø)`	`1.00 <0.00> (ø)`
...nveyal/datatools/manager/jobs/OtpRunnerStatus.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../datatools/manager/jobs/RecreateBuildImageJob.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
.../conveyal/datatools/manager/utils/TimeTracker.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...com/conveyal/datatools/manager/jobs/DeployJob.java	`19.09% <31.11%> (-0.35%)`	`16.00 <4.00> (+2.00)`	⬇️
...veyal/datatools/manager/persistence/FeedStore.java	`35.23% <100.00%> (+4.76%)`	`15.00 <1.00> (+1.00)`

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 2e85446...6223b9b. Read the comment docs.

…ions

fix #333

Fix tmp gtfs creation

…n to s3

landonreed · 2020-08-06T16:37:06Z

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

-            return null;
-        }
+        // download otp-runner manifest
+        lines.add(String.format("aws s3 cp %s %s", otpRunnerManifestS3FilePath, otpRunnerManifestFileOnInstance));


Oh, yea. This seems much better than echoing the manifest from the user-data. Also, makes things more repeatable. For some reason, in my head I thought we were already uploading the otp-runner file to s3. In any case, nice work.

…job-exception-handling

…ling

…stance fails to terminate

evansiroky · 2020-08-18T00:32:43Z

Ready for review now that #330 has been merged into dev.

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

src/main/java/com/conveyal/datatools/manager/utils/TimeTracker.java

src/main/java/com/conveyal/datatools/manager/jobs/MonitorServerStatusJob.java

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

landonreed · 2020-08-18T17:47:54Z

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

+                            "%s During job cleanup, the graph building instance was not properly terminated!",
+                            status.message
+                        );
+                    }


Rather than surrounding each call to terminateInstances in a try/catch, we should just have the terminateInstances method itself contain the try/catch, check the TerminateInstancesResult and return a success boolean. I don't think the current return type is really used anywhere.

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

landonreed

Left a few comments describing some changes I think would help.

binh-dam-ibigroup

Conditional approval on what Landon said and some minor comments.

src/main/java/com/conveyal/datatools/manager/jobs/MonitorServerStatusJob.java

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

binh-dam-ibigroup · 2020-08-19T16:13:41Z

src/main/java/com/conveyal/datatools/manager/jobs/MonitorServerStatusJob.java

+                );
+            } catch (AmazonEC2Exception e) {
+                status.fail(
+                    String.format("%s. During job cleanup, the instance was not properly terminated!", status.message)


This string appears four times in this file and can become a constant.

see #331

evansiroky · 2020-08-19T23:44:05Z

See various changes that should address the comments.

landonreed · 2020-10-30T17:28:17Z

🎉 This PR is included in version 3.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

fix(deploy): better handle more exceptions in monitor server job

33d6e76

evansiroky added the BLOCKED label Jul 24, 2020

evansiroky assigned landonreed and binh-dam-ibigroup Jul 24, 2020

evansiroky added 7 commits July 25, 2020 22:44

fix(deploy): use fresh ec2 client and catch terminate instance except…

be81ea6

…ions

fix(deploy): use nonce to ensure integrity of otp-runner status file

f08b5e1

test(deploy): make deterministic nonce values

9bfb521

fix(deploy): check for null nonce in status file

660c9c6

fix(deploy): properly check for null otp-runner nonce

7503f0e

fix(deploy): correctly create longer-lasting AWS sessions

004f6de

refactor(deploy): remove code that customized credential session time

579719c

evansiroky mentioned this pull request Jul 28, 2020

Refactor AWS client creation #332

Merged

5 tasks

landonreed and others added 6 commits July 31, 2020 11:28

test(FeedStore): add test to verify file name of temp GTFS file

d8799af

fix(FeedStore): write temp GTFS files with .zip suffix

09fd9e4

fix #333

test(FeedStoreTest): add javadoc, clean up test

f6f020a

Merge pull request #334 from ibi-group/fix-tmp-gtfs-creation

bdc3e6b

Fix tmp gtfs creation

refactor(deploy): correct graph build timeout wait time

cc83994

refactor(deploy): upload otp-runner manifest, build/router-config.jso…

928b610

…n to s3

landonreed reviewed Aug 6, 2020

View reviewed changes

evansiroky added 6 commits August 13, 2020 18:06

Merge branch 'graph-build-image-job' into improve-monitorable-server-…

e20e306

…job-exception-handling

Merge branch 'graph-build-image-job' into improve-monitorable-server-…

56083f7

…job-exception-handling

Merge branch 'graph-build-image-job' into improve-monitorable-server-…

e27b982

…job-exception-handling

Merge branch 'dev' into improve-monitorable-server-job-exception-hand…

35f4f04

…ling

refactor(deploy): make sure to exit deployjob after graph building in…

6fe3bfd

…stance fails to terminate

refactor: introduce TimeTracker class for tracking task timeouts

144defa

evansiroky removed the BLOCKED label Aug 18, 2020