985607 - Merge git build-cloud-tools to official hg cloud-tools

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[John Hopkins (:jhopkins)]

Merge the 'windows' branch of https://github.com/catlee/build-cloud-tools/ to http://hg.mozilla.org/build/cloud-tools to capture the changes we have made to support Windows testing on Amazon.

Comment 1

[John Hopkins (:jhopkins)]

Attachment: initial diff, needs tweaking and testing

Steps to create the attached .diff:

$ git clone https://github.com/catlee/build-cloud-tools/ catlee-cloud-tools
$ cd catlee-cloud-tools
$ git remote add mozmaster https://github.com/mozilla/build-cloud-tools
# merge latest upstream changes to master branch
$ git pull mozmaster master
# merge master -> windows branch
$ git checkout windows
$ git merge master
$ git diff mozmaster/master..windows > ~/cloud-tools.diff

Comment 2

[John Hopkins (:jhopkins)]

Attachment: aws_create_instance.diff

Comment 3

[John Hopkins (:jhopkins)]

Attachment: aws_stop_idle.diff

The attached changes to aws_stop_idle.py can't land as-is.

As discussed, we should look at moving the machine specific logic (eg. getting uptime) to slaveapi.  If we're using the same SSH daemon on tst-w64-ec2-xxx as our other Windows machines then slaveapi's Windows SSH logic should work (may require tweaking the daemon settings).  Also need an 'is slave busy' check.

Comment 4

[John Hopkins (:jhopkins)]

Attachment: new-cloud-tools.diff

These are all new files and only affect tst-w64-ec2-xxx instances (for now).

Comment 5

[Rail Aliiev [:rail]]

Comment on attachment 8394125 [details] [diff] [review]
new-cloud-tools.diff

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

In overall it looks great, just needs to be updated to reflect recent refactoring changes. Also, I'm not an expert to review the powershell files, just rubber stamping.

::: aws/aws_create_win_ami.py
@@ +16,5 @@
> +from boto.ec2 import connect_to_region
> +from boto.ec2.blockdevicemapping import BlockDeviceMapping, BlockDeviceType
> +from boto.ec2.networkinterface import NetworkInterfaceSpecification, \
> +     NetworkInterfaceCollection
> +log = logging.getLogger()

log = logging.getLogger(__name__) maybe? I always get confused which one to use. :)

@@ +18,5 @@
> +from boto.ec2.networkinterface import NetworkInterfaceSpecification, \
> +     NetworkInterfaceCollection
> +log = logging.getLogger()
> +
> +AM_CONFIGS_DIR = "ami_configs"

Can you import this variable from cloudtools.aws? Like in http://hg.mozilla.org/build/cloud-tools/file/6bdf0cf1e8bf/scripts/aws_create_ami.py#l12

@@ +21,5 @@
> +
> +AM_CONFIGS_DIR = "ami_configs"
> +
> +
> +def wait_for_status(instance, status):

Can you reuse the same function from cloudtools.aws?

@@ +33,5 @@
> +            raise
> +        time.sleep(10)
> +
> +
> +def create_connection(secrets_file, region):

We don't use aws_access_key_id and aws_secret_access_key in the secrets. Can you use cloudtools.aws.get_aws_connection() instead?

@@ +125,5 @@
> +    return ami
> +
> +
> +if __name__ == '__main__':
> +    from docopt import docopt

Don't forget to update requirements.txt and add docopt.

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 8394121 [details] [diff] [review]
aws_create_instance.diff

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

::: scripts/aws_create_instance.py
@@ +67,5 @@
> +    log.info("waiting for instance to shut down")
> +    while True:
> +        try:
> +            instance.update()
> +            if instance.state == 'stopped':

Can you use wait_for_status() here? And where do you stop the instance? Missing instance.stop()?

@@ +82,5 @@
> +    # Wait for the instance to come up
> +    while True:
> +        try:
> +            instance.update()
> +            if instance.state == 'running':

the same here

@@ +255,5 @@
> +                    hostname=instance_data['name'],
> +                    domain=instance_data['domain'],
> +                    dns_search_domain=config.get('dns_search_domain'),
> +                    # TODO: Use a real password!
> +                    password="password123!",

You mean deploypass?

@@ +268,4 @@
>                  block_device_map=bdm,
>                  client_token=token,
>                  disable_api_termination=bool(config.get('disable_api_termination')),
> +                security_group_ids=config.get('security_group_ids', []),

I don't think this would work. Security groups should be assigned to the network interface specification.

@@ +274,2 @@
>                  network_interfaces=interfaces,
>                  instance_profile_name=config.get("instance_profile_name"),

multiple instance_profile_name args

Comment 7

[John Hopkins (:jhopkins)]

Attachment: new-cloud-tools-2.diff - address Rail's review feedback

Comment 8

[John Hopkins (:jhopkins)]

Comment on attachment 8394355 [details] [diff] [review]
new-cloud-tools-2.diff  - address Rail's review feedback

https://hg.mozilla.org/build/cloud-tools/rev/fa2fdb157e1a

Comment 9

[John Hopkins (:jhopkins)]

Attachment: aws_create_instance.diff

Comment 10

[Rail Aliiev [:rail]]

Comment on attachment 8394772 [details] [diff] [review]
aws_create_instance.diff

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

::: scripts/aws_create_instance.py
@@ +241,5 @@
> +                    hostname=instance_data['name'],
> +                    domain=instance_data['domain'],
> +                    dns_search_domain=config.get('dns_search_domain'),
> +                    # TODO: Use a real password!
> +                    password="password123!",

[10:36] <rail> jhopkins: what do you mean by password="password123!", in the patch? :)
[10:36] <jhopkins> rail: that's what we're using for now
[10:37] <rail> shouldn't that be moved somewhere else? secrets?
[10:37] <jhopkins> maybe... nobody seems bothered by it, though
[10:38] <jhopkins> suggest we wait until this is out of development to worry too much about it, since changing passwords in windows is a real bother
[10:39] <jhopkins> but i'm open to suggestions
[10:39] <rail> move it to user_data_file and avoid interpolation?
[10:39] <rail> or to secrets
[10:39] <rail> probably secrets would be better
[10:39] <jhopkins> ok, i can do that

Do eet! :)

Comment 11

[John Hopkins (:jhopkins)]

Attachment: [cloud-tools] follow-up to fetch windows password from secrets file

Comment 12

[John Hopkins (:jhopkins)]

Attachment: [cloud-tools] follow-up patch to use deploypass

Comment 13

[John Hopkins (:jhopkins)]

Comment on attachment 8394772 [details] [diff] [review]
aws_create_instance.diff

https://hg.mozilla.org/build/cloud-tools/rev/11bc531b7f15

Comment 14

[John Hopkins (:jhopkins)]

Comment on attachment 8394967 [details] [diff] [review]
[cloud-tools] follow-up patch to use deploypass

https://hg.mozilla.org/build/cloud-tools/rev/11bc531b7f15

Comment 15

[John Hopkins (:jhopkins)]

Tracking the aws_stop_idle.py work in bug 987158.