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

Turn save-fuel-price-data into a task we can run in AWS #1923

Merged
merged 89 commits into from May 8, 2019

Conversation

3 participants
@chrisgilmerproj
Copy link
Contributor

commented Mar 27, 2019

Description

Requires we first set up AWS in transcom/ppp-infra#487

This PR containerizes bin/save-fuel-price-data into something we can deploy to AWS ECS Fargate as a scheduled task.

Reviewer Notes

I've made a few updates to the script so we can target the correct database parameters.

Setup

You can immediately try the containerized version using:

make tasks_save_fuel_price_data

This will build a container locally and then run the binary, which is the only thing in that container other than the DB definition. All environment vars are passed through to the container but we could instead use flags if needed.

The rest of the deploy process is a bit more hairy and follows this order:

  1. Build the binary
  2. Build the Dockerfile with the new binary to make a new image
  3. Push the new image to our ECR repository in AWS
  4. Run a script to deploy that image to ECS Fargate using a Task Definition

The script needs to be rewritten and tested as its only a copy of bin/ecs-deploy-service-container that has a couple changes to target scheduled tasks. Generally the script will do this:

  1. Render a new Task Definition which is how ECS Fargate uses to update/deploy the container
  2. Register the new Task Definition
  3. Update the Scheduled Task service (using green/blue deploy)

Download the container to check it out:

exec $(aws ecr get-login --no-include-email)
docker pull <image>

Open Questions

  • Do we deploy tasks concurrently with the app or do we do it only on successful app deployment?
  • What does rollback mean for this task?
  • Are we going to have issues making a secure connection between the binary and the RDS instance?

Code Review Verification Steps

  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

chrisgilmerproj added some commits Mar 27, 2019

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@kimallen @reggieriser @pjdufour-truss @rdhariwal - This is ready for review!

I'd love for a closer look at the two scripts I've created/modified. The circleci config may need scrutiny too. And finally I am hoping to get in #2049 before this one so I can eliminate a lot of the changes I made to the Makefile in this PR.

COPY bin/rds-combined-ca-bundle.pem /bin/rds-combined-ca-bundle.pem
COPY bin/chamber /bin/chamber
COPY bin/save-fuel-price-data /bin/save-fuel-price-data
COPY config/database.yml /config/database.yml

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Author Contributor

Remove this.

FROM gcr.io/distroless/base

COPY bin_linux/save-fuel-price-data /bin/save-fuel-price-data
COPY config/database.yml /config/database.yml

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Author Contributor

Remove this.

COPY bin/chamber /bin/chamber
COPY bin/milmove /bin/milmove

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal May 6, 2019

Contributor

does the order matter?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Author Contributor

Not really. I was tidying things up and probably did a vimdiff and made it look like another Dockerfile.

- run: make bin/ecs-deploy-task-container
- deploy:
name: Deploy task service
command: scripts/do-exclusively --job-name ${CIRCLE_JOB} bin/ecs-deploy-task-container --aws-account-id ${AWS_ACCOUNT_ID} --aws-region ${AWS_DEFAULT_REGION} --service app --environment experimental --repository-name app-tasks --image-tag git-${CIRCLE_SHA1} --rule save-fuel-price-data

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Author Contributor

change based on environment

quit(logger, nil, errors.Wrap(err, "Unable to parse current task definition arn"))
}

// TODO: This isn't clean and could probably use a regex

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Author Contributor

fix todo

}

// Get the database host using the instance identifier
// TODO: Allow passing in from DB_HOST

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 6, 2019

Author Contributor

fix todo

// Don't sort flags
flag.SortFlags = false
}

// Command: go run cmd/save_fuel_price_data/main.go

This comment has been minimized.

Copy link
@kimallen

kimallen May 6, 2019

Contributor

Now that you figured out the proper command to run it, will you change this comment accordingly?

@kimallen
Copy link
Contributor

left a comment

save_fuel_price_data looks good to me- ran fine in development. Just a comment to edit the comment for the actual command to use on CLI.

@codecov

This comment has been minimized.

Copy link

commented May 6, 2019

Codecov Report

Merging #1923 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1923      +/-   ##
==========================================
+ Coverage   60.15%   60.16%   +0.01%     
==========================================
  Files         222      222              
  Lines       13248    13240       -8     
==========================================
- Hits         7969     7965       -4     
+ Misses       4310     4306       -4     
  Partials      969      969

chrisgilmerproj added some commits May 6, 2019

Separate the command being run from the CloudWatch rule name because …
…the rule name needs the environment in it
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

save_fuel_price_data looks good to me- ran fine in development. Just a comment to edit the comment for the actual command to use on CLI.

@kimallen - I made the changes. Can you approve? I'll still wait for one approval from Infra too, but I'd like at least your sign off.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@rdhariwal - if you have no more comments can you approve?

chrisgilmerproj added some commits May 7, 2019

@rdhariwal
Copy link
Contributor

left a comment

👍

@chrisgilmerproj chrisgilmerproj merged commit 6d584fa into master May 8, 2019

20 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 64f0338...1eef3c0
Details
codecov/project/go 59.98% remains the same compared to 64f0338
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_163909133_ecs_scheduled_tasks branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.