1243421 - Institute CheckAmi through runner for AWS 2008

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: Puppet, task)

Comment 1

[Mark Cornmesser [:markco]]

https://dxr.mozilla.org/build-central/source/puppet/modules/runner/files/check_ami.py

Comment 2

[Jake Watkins [:dividehex]]

Attachment: bug1243421-1.patch

Untested.  Allows check_ami.py to shutdown windows properly

Comment 3

[Jake Watkins [:dividehex]]

Comment on attachment 8712778 [details] [diff] [review]
bug1243421-1.patch

remote:   https://hg.mozilla.org/build/puppet/rev/4f9e09b29020
remote:   https://hg.mozilla.org/build/puppet/rev/3a2a0d4be2f0

Comment 4

[Mark Cornmesser [:markco]]

In additional to setting up runner to run the task, pyyaml will need to be installed. 

In testing with pyyaml installed it is failing on this portion: 

def main():
    my_ami = get_aws_metadata("ami-id")
    az = get_aws_metadata("placement/availability-zone")
    user_data = get_aws_userdata()
    if not user_data:
        log.error("cannot operate without userdata")
        exit(1)

However the ami id can be found at http://169.254.169.254/latest/meta-data/ami-id.

Comment 5

[Mark Cornmesser [:markco]]

The current instance is i-12270da1 10.134.66.82.

Comment 6

[Jake Watkins [:dividehex]]

The problem here is that on linux ec2 instances we use the userdata to set parameters in a yaml format. check_ami.py reads objects like hostname and moz_instance_type but on windows instances, userdata is being used by ec2config to exec a powershell script. I can't seem to find what format ec2config is expecting in userdata.  It looks like a simple <tag></tag> format but I have no other insight other than that.  AFAIK, all we are told is to use <powershell> or <script> and <persist> also seems be a recognized tag.  We might be able to add the yaml data to the end of the userdata string and slap it between some custom tag like <yamldata> for check_ami to extract it before parsing it.

Comment 7

[Jake Watkins [:dividehex]]

:markco will test whether ec2config can handle extra data in the userdata blob and still exec the powershell.  If it can, it's a simple fix to check_ami.py.

Last, we will need to make sure windows instances are including the same yaml metadata in userdata as the linux instances.

Comment 8

[Jake Watkins [:dividehex]]

@grenade: do you know if ec2config would handle extra data packed on the end of userdata?

Comment 9

[Jake Watkins [:dividehex]]

Attachment: bug1243421-2.patch

Attempts to extract yaml data placed in between <mozuserdata> and </mozuserdata> before parsing it. That is, if ec2config doesn't freakout with the extra bits in userdata.

Also logs exception if thrown.

Comment 10

[Rob Thijssen [:grenade (EET/UTC+0300)]]

there's no problem adding tags as you've described, however I'm interested in why it's needed. Looking at what's included in a linux userdata yaml file, we get:

#cloud-config
fqdn: {fqdn}
hostname: {fqdn}
package_update: false
resize_rootfs: true
manage_etc_hosts: true
disable_root: false
ssh_pwauth: true
moz_instance_type: {moz_instance_type}
mounts: false
rsyslog:
 - filename: log_aggregator_client.conf
   content: "*.* @@log-aggregator.srv.releng.{region_dns_atom}.mozilla.com:1514"

So, the only interesting tokens in this data are: fqdn, moz_instance_type, region_dns_atom. I'd like to understand why check-ami needs any of this. The hostname is available as an environment variable (%COMPUTERNAME%), moz_instance_type is just the first 6 chars of the hostname (%COMPUTERNAME:~0,6%)

In summary, sure, no problem adding a section to userdata but in practice, I think it just adds unnecessary complexity to obtaining values that are easily accessible from the cmd/bash/sh environment.

Comment 11

[Rob Thijssen [:grenade (EET/UTC+0300)]]

Comment on attachment 8713394 [details] [diff] [review]
bug1243421-2.patch

no problem with this code, but I question the need for it, as per my comment above.

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

I'd like to keep the two OS's determining that info in the same way, at least.  If that means switching linux to parse this info out of its hostname, that's cool.

Comment 13

[Jake Watkins [:dividehex]]

(In reply to Dustin J. Mitchell [:dustin] from comment #12)
> I'd like to keep the two OS's determining that info in the same way, at
> least.  If that means switching linux to parse this info out of its
> hostname, that's cool.

Thanks Rob.  I agree with Dustin here.  The source of the data used by check_ami needs to be consistent across platforms.  And I believe the yaml data is for cloud-init, so it might be very much needed on linux.

Comment 14

[Jake Watkins [:dividehex]]

Attachment: https://github.com/mozilla/build-cloud-tools/pull/172

Comment 15

[Jake Watkins [:dividehex]]

Comment on attachment 8713394 [details] [diff] [review]
bug1243421-2.patch

remote:   https://hg.mozilla.org/build/puppet/rev/e14a26915f4c
remote:   https://hg.mozilla.org/build/puppet/rev/295dccb23614

Comment 16

[Rob Thijssen [:grenade (EET/UTC+0300)]]

Comment on attachment 8713863 [details] [review]
https://github.com/mozilla/build-cloud-tools/pull/172

https://github.com/mozilla/build-cloud-tools/commit/6a3e196ec757c1c873ea39c1ef1130f74685c23f

Comment 17

[Mark Cornmesser [:markco]]

Attachment: BUG1243421.patch

Comment 18

[Justin Wood (:Callek)]

Comment on attachment 8714427 [details] [diff] [review]
BUG1243421.patch

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

::: modules/buildslave/manifests/startup/runner.pp
@@ +37,4 @@
>      include runner::tasks::halt
>      if ($::operatingsystem == Windows) {
>          include runner::tasks::clobber
> +        include runner::tasks::check_ami

perhaps we should remove ::clobber from this line and move:

https://dxr.mozilla.org/build-central/source/relabs-puppet/modules/toplevel/manifests/slave/releng/build.pp#56

Out of the != Windows block, and put check_ami on the actual build.pp manifest since its not technically a requirement of runner itself?

::: modules/runner/manifests/tasks/check_ami.pp
@@ +15,5 @@
> +            }
> +            file {
> +                "C:/opt/runner/check_ami.py" :
> +                    source => 'puppet:///modules/runner/check_ami.py';
> +            }

we probably want a puppet dep on the file{} prior to the runner::task, for sanity :-)

Comment 19

[Mark Cornmesser [:markco]]

(In reply to Justin Wood (:Callek) from comment #18)
> Comment on attachment 8714427 [details] [diff] [review]
> BUG1243421.patch
> 
> Review of attachment 8714427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/buildslave/manifests/startup/runner.pp
> @@ +37,4 @@
> >      include runner::tasks::halt
> >      if ($::operatingsystem == Windows) {
> >          include runner::tasks::clobber
> > +        include runner::tasks::check_ami
> 
> perhaps we should remove ::clobber from this line and move:
> 
> https://dxr.mozilla.org/build-central/source/relabs-puppet/modules/toplevel/
> manifests/slave/releng/build.pp#56
> 
> Out of the != Windows block, and put check_ami on the actual build.pp
> manifest since its not technically a requirement of runner itself?
Were is check_ami called at for POSIX? I am not seeing it anywhere.

Comment 20

[Mark Cornmesser [:markco]]

Attachment: BUG1243421.patch

Made the suggested change on where to call clobber and check_ami manifest. Plus a few other small manifest corrections.

Comment 21

[Mark Cornmesser [:markco]]

Comment on attachment 8714579 [details] [diff] [review]
BUG1243421.patch

 https://hg.mozilla.org/build/puppet/rev/c1a78398dc06