Puppetize a dedicated VM for AWS related management tools

RESOLVED FIXED

Status

Infrastructure & Operations
RelOps: Puppet
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rail, Assigned: rail)

Tracking

Details

Attachments

(8 attachments, 5 obsolete attachments)

22.28 KB, patch
dustin
: review+
Details | Diff | Splinter Review
990 bytes, patch
catlee
: review+
Details | Diff | Splinter Review
3.57 KB, patch
catlee
: review+
Details | Diff | Splinter Review
2.85 KB, patch
dustin
: review+
Details | Diff | Splinter Review
1.05 KB, patch
dustin
: review+
Details | Diff | Splinter Review
1.73 KB, patch
dustin
: review+
Details | Diff | Splinter Review
8.60 KB, patch
dustin
: review+
Details | Diff | Splinter Review
1.73 KB, patch
catlee
: review+
Details | Diff | Splinter Review
We should move everything related to AWS from cruncher and host it on a separate VM, probably in AWS.
Created attachment 8340495 [details] [diff] [review]
initial skeleton
Attachment #8340495 - Flags: feedback?(dustin)
Comment on attachment 8340495 [details] [diff] [review]
initial skeleton

Review of attachment 8340495 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good start!

Do you want to create a new users::buildduty instead of using users::builder?

::: manifests/moco-nodes.pp
@@ +682,5 @@
>  }
> +
> +node "aws-manager.b.m.o" {
> +    include toplevel::server
> +    include aws_manager

When I initially suggested just toplevel::server, it was with the thought that the rest would be configured by hand.  It's definitely better to do that config with puppet, but in that case this should be via a new toplevel class (toplevel::server::aws_manager maybe?)

::: modules/aws_manager/manifests/cron.pp
@@ +6,5 @@
> +    include users::builder
> +    aws_manager::crontask {
> +        "aws_watch_pending.py":
> +            ensure         => present,
> +            $minute        => '*/5',

no $ here :)

::: modules/config/manifests/base.pp
@@ +241,5 @@
>  
>      # "yes" to install 'em, "no" to not do so.  See bug 913011
>      $install_avds = "no"
>  
> +    # AWS management

extra newline after this
Attachment #8340495 - Flags: feedback?(dustin) → feedback+
Depends on: 945713
Depends on: 946292
Blocks: 935533
No longer blocks: 939543
Created attachment 8387780 [details] [diff] [review]
aws_cruncher-puppet.diff

This worked fine. I tested only one script and it worked. I'll still need to add 1 cronjob as a followup for this.
Attachment #8340495 - Attachment is obsolete: true
Depends on: 981065
Log rotation would be great to have, eg aws_watch_pending.log goes back to Dec 20 last year, 520MB in size. Keeping a month or so should be plenty.
Created attachment 8388912 [details] [diff] [review]
aws_cruncher-puppet-7.diff

Adding logrotate would be great, yeah. TODO :)
Attachment #8387780 - Attachment is obsolete: true
Attachment #8388912 - Flags: review?(dustin)
Comment on attachment 8388912 [details] [diff] [review]
aws_cruncher-puppet-7.diff

Review of attachment 8388912 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  Don't forget docs for the new module and its secrets.
Attachment #8388912 - Flags: review?(dustin) → review+
Created attachment 8395943 [details] [diff] [review]
aws_cruncher-puppet-1.diff

refresh + instance2ami.py cron job

I would like to start the migration tomorrow.
Attachment #8388912 - Attachment is obsolete: true
Attachment #8395943 - Flags: review?(dustin)
Attachment #8395943 - Flags: review?(dustin) → review+
Attachment #8396396 - Flags: review?(dustin) → review+
Created attachment 8396413 [details] [diff] [review]
watch_pending.diff
Attachment #8396413 - Flags: review?(catlee)
Created attachment 8396425 [details] [diff] [review]
watch_pending.diff

s/us-west-1//
Attachment #8396413 - Attachment is obsolete: true
Attachment #8396413 - Flags: review?(catlee)
Attachment #8396425 - Flags: review?(catlee)
Attachment #8396425 - Flags: review?(catlee) → review+
Created attachment 8396463 [details] [diff] [review]
tuneup.diff

* Better logging is on the list of the things to improve. Let's mimic cruncher's approach for now.
* missing \n
Attachment #8396463 - Flags: review?(dustin)
Attachment #8396463 - Flags: review?(dustin) → review+
Created attachment 8396531 [details] [diff] [review]
combine_stderr_stdout.diff

We want moar spam!
Attachment #8396531 - Flags: review?(dustin)
Attachment #8396531 - Flags: review?(dustin) → review+
Created attachment 8396548 [details] [diff] [review]
combine_stderr_stdout2.diff

another followup
Comment on attachment 8396548 [details] [diff] [review]
combine_stderr_stdout2.diff

standing review from me for this sort of thing :)
Attachment #8396548 - Flags: review+
Created attachment 8396649 [details] [diff] [review]
servo-puppet.diff

More or less everything went smoothly. The only thing I forgot to address is servo related tasks.

We have only 2 cron jobs for servo, they use the same scripts but use different parameters and files. I added $script argument to allow reuse the same script name.

The only part that makes me uncomfortable to request a review is having some hard coded values.

* passwords-servo.json.erb uses "servobld". This can be moved to the config, but I'm not quite sure what this indirection would help us with.
* Similar with $servo_passwords

As possible work around I can add something like ::config::servo_is_cool (of course!) and make the manifest entries conditional on that.
Attachment #8396649 - Flags: feedback?(dustin)
Comment on attachment 8396649 [details] [diff] [review]
servo-puppet.diff

I think this is OK as-is, since it's an odd case of moco stuff managing servo infra.  It would be more natural, I think, for this whole module to be instantiated separately in a dedicated servo instance, and do the right thing there -- but that's a lot more abstraction, plus an additional always-on instance.
Attachment #8396649 - Flags: feedback?(dustin) → feedback+
Depends on: 987989
Created attachment 8397071 [details] [diff] [review]
servo-puppet-1.diff

The same patch + /etc/invtool.conf
A --noop puppet output:

Notice: /Stage[main]/Aws_manager::Cron/Aws_manager::Crontask[aws_watch_pending_servo]/File[/etc/cron.d/aws_manager-aws_watch_pending_servo.cron]/ensure: current_value absent, should be file (noop)
Notice: /Stage[main]/Aws_manager::Install/File[/etc/invtool.conf]/ensure: current_value absent, should be file (noop)
Notice: /Stage[main]/Aws_manager::Cron/Aws_manager::Crontask[aws_stop_idle_servo]/File[/etc/cron.d/aws_manager-aws_stop_idle_servo.cron]/ensure: current_value absent, should be file (noop)
Notice: /Stage[main]/Aws_manager::Secrets/File[/builds/aws_manager/secrets/passwords-servo.json]/ensure: current_value absent, should be file (noop)
Notice: /Stage[main]/Aws_manager::Secrets/File[/builds/aws_manager/secrets/aws-secrets-servo.json]/ensure: current_value absent, should be file (noop)
Notice: /Stage[main]/Aws_manager::Cron/Aws_manager::Crontask[aws_stop_idle_servo]/File[/builds/aws_manager/bin/aws_manager-aws_stop_idle_servo.sh]/ensure: current_value absent, should be file (noop)
Notice: Class[Aws_manager::Install]: Would have triggered 'refresh' from 1 events
Notice: Aws_manager::Crontask[aws_stop_idle_servo]: Would have triggered 'refresh' from 2 events
Notice: /Stage[main]/Aws_manager::Cron/Aws_manager::Crontask[aws_watch_pending_servo]/File[/builds/aws_manager/bin/aws_manager-aws_watch_pending_servo.sh]/ensure: current_value absent, should be file (noop)
Notice: Aws_manager::Crontask[aws_watch_pending_servo]: Would have triggered 'refresh' from 2 events
Attachment #8396649 - Attachment is obsolete: true
Attachment #8397071 - Flags: review?(dustin)
Depends on: 988324
Comment on attachment 8397071 [details] [diff] [review]
servo-puppet-1.diff

*STAMP*
Attachment #8397071 - Flags: review?(dustin) → review+
The docs are live: https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/Modules/aws_manager
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Depends on: 990136
Attachment #8401836 - Flags: review?(catlee) → review+
You need to log in before you can comment on or make changes to this bug.