997013 - Push an empty .boto on AWS slaves

Status: RESOLVED FIXED

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

Description

[Mike Hommey [:glandium]]

Bug 987537 added a .boto to all slaves instead of just in-house slaves, and that broke sccache on AWS.

The solution is to either remove .boto from AWS slaves, or make it empty. Only the latter might work with https://hg.mozilla.org/build/buildbot-configs/file/bbece32f3105/mozilla/config.py#l242

Comment 1

[Mike Hommey [:glandium]]

How do we get this fixed quickly? It is making all linux and android try builds take between 0 and 20+ minutes more time than usual (with 60% of those builds taking 20+ minutes more).

Comment 2

[Rail Aliiev [:rail]]

We could change class slave_secrets (modules/slave_secrets/manifests/init.pp) so it treats $slave_type as an array and we can assign multiple roles to slaves. We could also rename this argument to $slave_roles or leave as is and add an optional $slave_roles.

Comment 3

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

This isn't a different slave type or slave role, though.  It's the same slave type, just with some location-dependent configuration.  I expect we'll see more of this as we attempt to run jobs in a wider array of contexts.

Maybe a $config::location_map that's keyed by fqdn and used to determine $slave_location?  Then we can add macinthecloud and azure and all those fun things when they come online.

Comment 4

[Michael Shal [:mshal]]

Attachment: slave_location.patch

Is this along the lines of what you're thinking? I ran puppet against my user environment with this, and I get a .boto file with content on an in-house machine, and an empty .boto file on an aws machine.

Note I haven't yet tested that the empty .boto file on aws actually works with sccache properly.

Comment 5

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

Comment on attachment 8407746 [details] [diff] [review]
slave_location.patch

It is, but

1. I think you could just use $fqdn? { ... } instead of sort_servers_by_group, which includes a bunch of unrelated functionality.

2. $config::* variables are generally about system configuration, rather than the specific node puppet is running for, so I'm a bit uncomfortable putting $location into the config.  I'm even more comfortable with $slave_location there -- how is $slave_location a configuration variable for, say, a buildmaster?

3. The particular mapping of location to behavior is also moco-specific -- other organizations may not want the same kinds of boto configs.

4. If it's appropriate for ceph_config to write an empty file, rather than deleting the file, then this is fine but it deserves a comment.  If deleting the file would be better, then that should probably be handled at the slave_secrets level in calculating the value of $ensure.

So, as something of a compromise here:
 * rename the config var to $config::node_location (including using $fqdn ? { ... }, and also adding it to modules/config/manifests/base.pp with a default value)
 * update either slave_secrets or slave_secrets::ceph_config (depending on whether an empty file is best) to determine the content based first on the organization, and second (for moco) on the location.  Something like:

case $config::org {
    moco: {
        case $config::node_location {
            inhouse: { .. }
            ..
        }
    default: {
        ..
    }
}

where the default is probably to have no boto file.

Comment 6

[Mike Hommey [:glandium]]

I'm all for removing the file, if mock_copy is going to be happy with the file not being there.

Comment 7

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #6)
> I'm all for removing the file, if mock_copy is going to be happy with the
> file not being there.

I just tested it, and unfortunately the file is required to be there. Still checking on an empty .boto...

Comment 8

[Michael Shal [:mshal]]

So with glandium's help we determined that an empty .boto should work fine.

Comment 9

[Michael Shal [:mshal]]

(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> 1. I think you could just use $fqdn? { ... } instead of
> sort_servers_by_group, which includes a bunch of unrelated functionality.

Ok, that looks much better.

> 4. If it's appropriate for ceph_config to write an empty file, rather than
> deleting the file, then this is fine but it deserves a comment.  If deleting
> the file would be better, then that should probably be handled at the
> slave_secrets level in calculating the value of $ensure.

As mentioned in #c7 and #c8, we'll need to keep the empty file. I've added a comment.

> 
> So, as something of a compromise here:
>  * rename the config var to $config::node_location (including using $fqdn ?
> { ... }, and also adding it to modules/config/manifests/base.pp with a
> default value)

Done.

>  * update either slave_secrets or slave_secrets::ceph_config (depending on
> whether an empty file is best) to determine the content based first on the
> organization, and second (for moco) on the location.  Something like:
> 
> case $config::org {
>     moco: {
>         case $config::node_location {
>             inhouse: { .. }
>             ..
>         }
>     default: {
>         ..
>     }
> }
> 
> where the default is probably to have no boto file.

It looks to me like the 'case $config::org' with moco/default serves the same purpose as the $config::install_ceph_cfg. Are you suggesting to replace install_ceph_cfg with this construct? Or is there something else here I'm not seeing?

Comment 10

[Michael Shal [:mshal]]

Attachment: node_location.patch

Here's the latest. Please see my question in #c9 regarding your previous review comments.

Comment 11

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

Comment on attachment 8407937 [details] [diff] [review]
node_location.patch

I had forgotten about the $config::install_ceph_cfg.  In general, I think that this should be replaced with a per-org case in slave_secrets, but for the moment this patch works and is good enough -- until some other org wants to install ceph passwords in a different location.  Thanks for the quick turnaround, and for deciphering my review!

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 8407937 [details] [diff] [review]
node_location.patch

https://hg.mozilla.org/build/puppet/rev/0f7dd1b875ce

Also in production