Institute CheckAmi through runner for AWS 2008

RESOLVED FIXED

Status

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

People

(Reporter: markco, Assigned: dividehex)

Tracking

Details

Attachments

(4 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Assignee: relops → mcornmesser
(Reporter)

Updated

2 years ago
Assignee: mcornmesser → jwatkins
(Reporter)

Comment 1

2 years ago
https://dxr.mozilla.org/build-central/source/puppet/modules/runner/files/check_ami.py
(Assignee)

Comment 2

2 years ago
Created attachment 8712778 [details] [diff] [review]
bug1243421-1.patch

Untested.  Allows check_ami.py to shutdown windows properly
Attachment #8712778 - Flags: review?(mcornmesser)
(Reporter)

Updated

2 years ago
Attachment #8712778 - Flags: review?(mcornmesser) → review+
(Assignee)

Comment 3

2 years ago
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
Attachment #8712778 - Flags: checked-in+
(Reporter)

Comment 4

2 years ago
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.
(Reporter)

Updated

2 years ago
Component: RelOps → RelOps: Puppet
QA Contact: arich → dustin
(Reporter)

Comment 5

2 years ago
The current instance is i-12270da1 10.134.66.82.
Flags: needinfo?(jwatkins)
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(jwatkins)
(Assignee)

Comment 7

2 years ago
: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.
(Assignee)

Comment 8

2 years ago
@grenade: do you know if ec2config would handle extra data packed on the end of userdata?
Flags: needinfo?(rthijssen)
(Assignee)

Comment 9

2 years ago
Created attachment 8713394 [details] [diff] [review]
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.
Attachment #8713394 - Flags: review?(rthijssen)
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.
Flags: needinfo?(rthijssen)
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.
Attachment #8713394 - Flags: review?(rthijssen) → review+
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.
(Assignee)

Comment 13

2 years ago
(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.
(Assignee)

Comment 14

2 years ago
Created attachment 8713863 [details] [review]
https://github.com/mozilla/build-cloud-tools/pull/172
Attachment #8713863 - Flags: review?(rthijssen)
(Assignee)

Comment 15

2 years ago
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
Attachment #8713394 - Flags: checked-in+
Comment on attachment 8713863 [details] [review]
https://github.com/mozilla/build-cloud-tools/pull/172

https://github.com/mozilla/build-cloud-tools/commit/6a3e196ec757c1c873ea39c1ef1130f74685c23f
Attachment #8713863 - Flags: review?(rthijssen) → review+
(Reporter)

Comment 17

2 years ago
Created attachment 8714427 [details] [diff] [review]
BUG1243421.patch
(Reporter)

Updated

2 years ago
Attachment #8714427 - Flags: review?(jwatkins)
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 :-)
Attachment #8714427 - Flags: feedback+
(Reporter)

Comment 19

2 years ago
(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.
(Reporter)

Comment 20

2 years ago
Created attachment 8714579 [details] [diff] [review]
BUG1243421.patch

Made the suggested change on where to call clobber and check_ami manifest. Plus a few other small manifest corrections.
Attachment #8714427 - Attachment is obsolete: true
Attachment #8714427 - Flags: review?(jwatkins)
Attachment #8714579 - Flags: review?(jwatkins)
(Assignee)

Updated

2 years ago
Attachment #8714579 - Flags: review?(jwatkins) → review+
(Reporter)

Comment 21

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

 https://hg.mozilla.org/build/puppet/rev/c1a78398dc06
Attachment #8714579 - Flags: checked-in+
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.