Service and task definition setup #16

apurvis · 2016-09-26T19:17:10Z

https://lumoslabs.atlassian.net/browse/DE-11289

this is a proposal based on the script code in https://github.com/lumoslabs/events/pull/103 for how to add the option of setting up services and the initial task_definition from a broadside config that i was playing with a little; figure it might be easier to discuss having it in front of you

@matl33t @rfroetscher @albertrdixon

DE-11289 #time 90m

DE-11289 #time 30m

apurvis · 2016-09-26T19:28:40Z

lib/broadside/deploy/ecs.rb

-      container_def = new_task_def[:container_definitions].select { |c| c[:name] == family }.first
-      container_def[:environment] = @deploy_config.env_vars
-      container_def[:image] = image_tag
-      container_def[:command] = @deploy_config.command


container_def was never being used; not sure if that was a bug or just leftover cruft lying around.

this is actually modifying object data within new_task_def, which is used on L149 to generate a task definition revision. Admittedly not very clearly though.

i see it now... seems like the create_task_definition method stuff could all just live in update_task actually?

yeah, create_task_definition wouldn't be needed - update_task basically grabs the latest task definition through create_new_task_revision, deletes some unneeded fields, injects in configurations into the container relevant to broadside, then makes the api call to generate a new revision from that.

yeah so update_task could just create the first task definition if there wasn't one (and if @task_definition_config it was configured appropriately)

what's the difference between @command and @deploy_config.command?

@command is an instance variable belonging to https://github.com/lumoslabs/broadside/blob/master/lib/broadside/configuration/deploy.rb#L45

@deploy_config belongs to this class, basically a copy of the configuration object above (https://github.com/lumoslabs/broadside/blob/master/lib/broadside/deploy.rb#L8). This was done so that an instance of a deploy would not corrupt data in the original configuration object.

i still don't really understand it (and a few other fields) is duplicated between the two objects? AFAICT the @command passed to AWS is always and every time based on the command in the broadside config?

matl33t · 2016-09-26T20:02:56Z

lib/broadside/deploy/ecs.rb

-      resp = ecs_client.describe_services({cluster: config.ecs.cluster, services: [family]})
-      unless resp.failures.empty?
-        exception "Could not find ECS service '#{family}' in cluster '#{config.ecs.cluster}' !"
+    def all_results(method, key, args = {})


matl33t · 2016-09-26T20:42:39Z

lib/broadside/configuration/deploy.rb

@@ -22,14 +22,18 @@ class DeployConfig < ConfigStruct
        :env_vars,


there's actually a spec for this file, mind making some additions there?

i was actually hoping to add a lot of specs with the stub_responses part of the Aws gem but i wanted to get some buy in that this approach basically makes sense before doing that (hence the WIP in the title)

heh, that's a lot of work - i wouldn't worry about speccing ecs.rb for now, let's talk about that separately

it's really hard to jump into a project and change critical code without specs! i would feel like 1000% more confident about this PR if running rspec actually confirmed the right calls were made to AWS.

you know that Aws::Client already provides stubs, right? https://docs.aws.amazon.com/sdkforruby/api/Aws/ClientStubs.html

very true. i'll welcome whatever specs you want to supplement this PR with

rfroetscher · 2016-09-26T20:46:32Z

lib/broadside/deploy/ecs.rb

-      unless resp.failures.empty?
-        exception "Could not find ECS service '#{family}' in cluster '#{config.ecs.cluster}' !"
+    def all_results(method, key, args = {})
+      page = ecs.send(method, args)


public_send, please

one of these days i'll remember that public_send exists before you remind me

rfroetscher · 2016-09-26T20:47:18Z

lib/broadside/deploy/ecs.rb

@@ -326,6 +338,10 @@ def create_new_task_revision
      task_def
    end

+    def service_exists?
+      !ecs_client.describe_services({ cluster: config.ecs.cluster, services: [family] }).failures.empty?


the AWS services don't really return enumerables, they return weird Seahorse data structures with a .failures section.

just saying failures.any? is clearer than !failures.empty?. is that not possible?

ah i see; yeah that makes more sense.

rfroetscher · 2016-09-26T20:51:44Z

lib/broadside/deploy/ecs.rb

+              name: name,
+              command: @command,
+              cpu: 1,
+              essential: true,


i almost want these values to be in a default constant that get merged with the name and command stuff

i thought about that. you can't have a "constant" really because a name, command, family are all dynamic. so i thought about making building the config a separate method but really having a separate method is almost the same as having a method with one line call that builds the aws definition hash.

yeah, you could:

DEFAULT_CONTAINER_DEFINITION = { cpu: 1, essential: true, memory: 1000 } ... ecs.register_task_definition( { container_definitions: [ { name: name, command: @command }.merge(DEFAULT_CONTAINER_DEFINITION) ], family: name }.deep_merge(options) )

but not sure that's much clearer

rfroetscher · 2016-09-26T20:53:34Z

looks pretty solid in general

DE-11289 #time 30m

DE-11289 #time 15m

DE-11289 #time 10m

DE-11289 #time 15m

DE-11289 #time 30m

DE-11289 #time 5m

DE-11289 #time 20m

DE-11289 #time 10m

DE-11289 #time 45m

DE-11289

DE-11289 #time 5m

apurvis · 2016-09-27T23:31:27Z

ok @matl33t:

the new method is called bootstrap. i didn't yet implement the stuff in the actual broadside commands because you said you could help with getting it runnable
there's a couple of specs now that actually stub against AWS stubs. i'm not going to fill out all the specs for all the existing functionality but i do think that would be incredibly valuable.
the big blocker in terms of getting specs to cover the whole deploy is figuring out how to spec the Waiter response.

look it over carefully but i think this should be basically ready.

DE-11289 #time 5m

apurvis · 2016-09-27T23:36:21Z

@matl33t the easiest thing to do might be for you to just make a new branch off this one and add the bootstrap command on a PR merged into this branch; then we can merge it all at once into v1.1

also travis is having issues because there's no configured key set for AWS. not sure what the preferred approach is there but probably setting an env variable would do the trick.

matl33t · 2016-09-27T23:44:53Z

lib/broadside/configuration/deploy_config.rb

      )

      TARGET_ATTRIBUTE_VALIDATIONS = {
        scale: ->(target_attribute) { validate_types([Fixnum], target_attribute) },
        env_file: ->(target_attribute) { validate_types([String], target_attribute) },
        command: ->(target_attribute) { validate_types([Array, NilClass], target_attribute) },
-        predeploy_commands: ->(target_attribute) { validate_predeploy(target_attribute) }
+        predeploy_commands: ->(target_attribute) { validate_predeploy(target_attribute) },
+        service_config: ->(target_attribute) { validate_types([Hash, NilClass], target_attribute) },


service and task_definition are ECS constructs, so maybe best placed into the ecs_config file instead

except that different targets in a file could/will have different service and task configs... if it's in the ECSConfig won't they have to be unified?

to be honest i don't really see much point to the EcsConfig class at all and sort of think all that stuff belongs in the targets.

right now it would be impossible to have a single broadside config file that can deploy to two clusters, as well has having this random extra special config class instead of just a key/value in the targets... and for what?

originally intended to support things outside of ECS, but whatever. let's just go with this and ditch that config file. This will have to be a major version bump since that will be a breaking change.

i don't want to be refactoring at the same time as i'm add features but let's add it to the list of things to be refactored

i'm just going to leave it for now... the breaking change is too much at the moment / i don't feel like repairing configs in a ton of projects.

DE-11289 #time 15m

DE-11289 #time 35m

apurvis · 2016-09-28T03:28:32Z

lib/broadside/deploy.rb

@@ -92,7 +92,7 @@ def image_tag
    end

    def gen_ssh_cmd(ip, options = { tty: false })
-      opts = @deploy_config.ssh
+      opts = @deploy_config.ssh || {}
      cmd = 'ssh -o StrictHostKeyChecking=no'
      cmd << ' -t -t' if options[:tty]
      cmd << " -i #{opts[:keyfile]}" if opts[:keyfile]


bugfix: if opts[:keyfile] fails unless opts is an array

* Debug * debug * debug * debug * Fix bug with container_definition merge * debug * task_definition must come first * Simplify

apurvis · 2016-09-28T04:28:45Z

i added the command and i tested this - deploys still work, so i don't think i broke anything. i was also able to run deploy status.

i fully tested bundle exec broadside bootstrap --target test_target with a config and it seemed to work.

apurvis · 2016-09-28T06:36:47Z

README.md

+      scale: 1,
+      command: ['java', '-cp', '*:.', 'path.to.MyClass'],
+      env_file: '.env.production',
+      service: {


the one thing that kind of sucks here is that if you want to just use the defaults to bootstrap, you still have to specify service: {} and task_definition: {} (without the keys, and without their values being a hash, you won't be able to trigger a bootstrap).

maybe bootstrap should just use the defaults if the user doesn't specify any extra config and not check for the existence of service: and task_definition?

not the end of the world just kind of blah

apurvis · 2016-09-29T01:57:18Z

Just FYI this feature is done and ready to be reviewed. we're about to start churning out a lot more eventS ETLs quickly so i'd like to lineup the release of this feature in the reasonably but not urgently near future.

rfroetscher · 2016-09-29T18:23:03Z

this sounds fine to me but @matl33t has the final word

👍

apurvis · 2016-09-29T22:38:42Z

i was just able to use the original script version of this to deploy a new service in about 2 minutes from scratch, which was awesome compared to the old way.

matl33t · 2016-09-29T22:42:22Z

bin/broadside

@@ -166,6 +171,8 @@ on_error do |exception|
    error exception.message, "\nRun your last command with --help for more information."
    false
  else
+    pp exception.message


adding the env var GLI_DEBUG=true will automatically print out the stacktrace, so don't need this

good to know; i was lost trying to see the stacktrace.

matl33t · 2016-09-29T22:44:15Z

lib/broadside/configuration/deploy_config.rb

        return 'predeploy_commands must be an array' unless target_attribute.is_a?(Array)

-        target_attribute.each do |command|
-          return "predeploy_command '#{command}' must be an array" unless command.is_a?(Array)
+        messages = target_attribute.select { |cmd| !cmd.is_a?(Array) }.map do |command|


how about .reject { |cmd| cmd.is_a?(Array) ...

matl33t · 2016-09-29T22:45:07Z

lib/broadside/deploy/ecs_deploy.rb

@@ -6,33 +7,58 @@

 module Broadside
  class EcsDeploy < Deploy
+
+    DEFAULT_DESIRED_COUNT = 0


thoughts about putting these constants into the deployconfig class?

it's cool with me if it makes sense to have it there.

wait you mean in EcsConfig? the constants are referenced here but that sort of makes sense... not sure it's easier to read if it's in another file

matl33t · 2016-09-29T22:45:55Z

lib/broadside/deploy/ecs_deploy.rb

        end
      end
    end

+    def bootstrap
+      unless get_latest_task_def_id
+        # TODO right now this creates a useless first revision and then update_task_revision will create a 2nd one


i'm fine with this

yeah it's minor; maybe it's not a TODO it's just a comment

matl33t · 2016-09-29T22:48:03Z

lib/broadside/version.rb

@@ -1,3 +1,3 @@
 module Broadside
-  VERSION = '1.0.3'
+  VERSION = '1.1.0'


i'll handle the version bump in a separate PR (need to update changelog and whatnot)

matl33t · 2016-09-29T22:52:55Z

just a couple minor comments and i think we are ready to 🚢 !

apurvis · 2016-09-29T23:49:04Z

i tested all the commands except the beacon deploy... bootstrap works. @matl33t feel free to merge and release if you feel good about the long deploy

apurvis · 2016-09-30T18:54:05Z

i just used this to actually bootstrap and deploy a new eventS ETL; worked great.

[email protected] added 2 commits September 24, 2016 04:13

Add setup of services feature to broadside

b666391

DE-11289 #time 90m

Add service and task setup to broadside

42eb6c1

DE-11289 #time 30m

apurvis commented Sep 26, 2016

View reviewed changes

matl33t reviewed Sep 26, 2016

View reviewed changes

rfroetscher reviewed Sep 26, 2016

View reviewed changes

[email protected] added 13 commits September 27, 2016 16:11

Put back the container definition; address other PR commens

f6f9bc8

DE-11289 #time 30m

Start some specs

252d75c

DE-11289 #time 30m

Rename file

c652a73

DE-11289 #time 15m

Rename file

9a51104

DE-11289 #time 10m

Working construction

7f19df3

DE-11289 #time 15m

Spec for bad config

5e9c004

DE-11289 #time 30m

Rename files

8a59b40

DE-11289 #time 5m

Specs for bootstrapping

fec8d27

DE-11289 #time 20m

Spec for failed deploys

67871a8

DE-11289 #time 10m

Try for a spec of a real deploy

5e0b88f

DE-11289 #time 45m

Need to call update_task

46f56de

DE-11289

Alphabetize

994ff62

Use a default

543c28d

DE-11289 #time 5m

Update task outside the block

2f68827

DE-11289 #time 5m

matl33t reviewed Sep 27, 2016

View reviewed changes

[email protected] added 3 commits September 27, 2016 19:45

Rename file

4c697b7

DE-11289 #time 15m

Use appropriate exception types

2d3cd9a

Closer to a real spec

042828f

DE-11289 #time 35m

apurvis mentioned this pull request Sep 28, 2016

Refactoring/renaming/best practices #18

Closed

spec

5cadea0

apurvis mentioned this pull request Sep 28, 2016

Aws::ECS::Client.new.wait_until throws jmespath error when stubbing responses aws/aws-sdk-ruby#1307

Closed

[email protected] added 5 commits September 27, 2016 23:04

README

57282ee

Revert AWS gem bump for now

718b986

Add command for bootstrapping

c43e23a

print error messages

f78f014

missing ssh bug

8983c97

apurvis commented Sep 28, 2016

View reviewed changes

[email protected] and others added 3 commits September 27, 2016 23:46

Bootstrap task_definition before service

69bd75d

order of operations (#19)

bc8108d

* Debug * debug * debug * debug * Fix bug with container_definition merge * debug * task_definition must come first * Simplify

Fix command parsing

5fe2bbf

version bump

b17e883

apurvis mentioned this pull request Sep 28, 2016

let an app deploy to multiple clusters #5

Closed

apurvis commented Sep 28, 2016

View reviewed changes

matl33t reviewed Sep 29, 2016

View reviewed changes

[email protected] added 2 commits September 29, 2016 19:24

@matl33t comments

63f20ef

README

b400fd2

apurvis merged commit 8665f42 into master Oct 1, 2016

apurvis deleted the service_and_task_definitions branch October 19, 2016 01:25

		@@ -22,14 +22,18 @@ class DeployConfig < ConfigStruct
		:env_vars,

Service and task definition setup #16

Service and task definition setup #16

Conversation

apurvis commented Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfroetscher commented Sep 26, 2016

apurvis commented Sep 27, 2016

apurvis commented Sep 27, 2016

Choose a reason for hiding this comment

apurvis Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

apurvis Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apurvis commented Sep 28, 2016

Choose a reason for hiding this comment

apurvis commented Sep 29, 2016

rfroetscher commented Sep 29, 2016

apurvis commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matl33t commented Sep 29, 2016

apurvis commented Sep 29, 2016

apurvis commented Sep 30, 2016

apurvis commented Sep 26, 2016 •

edited

Loading

apurvis Sep 28, 2016 •

edited

Loading

apurvis Sep 28, 2016 •

edited

Loading