1046926 - runner needs to run after instance_metadata

Status: RESOLVED FIXED

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

Description

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

The runner job we're adding in bug 1022763 requires /etc/instance_metadata.json to be up to date, but

lrwxrwxrwx 1 root root 16 Jul  2 16:10 S98runner -> ../init.d/runner
lrwxrwxrwx 1 root root 27 Jul 28 14:59 S99instance_metadata -> ../init.d/instance_metadata

so it's from the *last* boot, which is going to mean it has stale information on the first boot

Comment 1

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

Ian, this is blocking work on bug 1022763.  What do you think the best way forward will be?  Is changing the startup order pretty easy, or should we look to putting instance_metadata into runner?

Comment 2

[Ian Connolly [:ianconnolly]]

Changing startup order shouldn't be difficult, just make Runner depend upon instance_metadata. We'll also need to `chkconfig instance_metadata resetpriorities` somehow.

Comment 3

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

Attachment: bug1046926.patch

How's this?  And, is this safe to deploy?

Comment 4

[Ian Connolly [:ianconnolly]]

Comment on attachment 8470991 [details] [diff] [review]
bug1046926.patch

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

I don't think so. If instance_metadata is already at a runlevel that is later than runner's, then it won't be updated.

We need to exec a `chkconfig instance_metadata resetpriorities` somehow. It may just need to be a bit added to the runner init.d refresh Exec block. We can remove after it gets deployed across all of the machines (as it'll be unnecessary for new machines).

Comment 5

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

Attachment: bug1046926-p1.patch

How's this (untested)?

Comment 6

[Ian Connolly [:ianconnolly]]

Comment on attachment 8471054 [details] [diff] [review]
bug1046926-p1.patch

>commit c5bbaeda85bef35c44765cc06b5c8be24c752dbf
>Author: Dustin J. Mitchell <[e-mail]>
>Date:   Mon Aug 11 14:37:52 2014 -0400
>
>    Bug 1046926: make runner depend on instance_metadata
>    
>    This will alter the runner initd script, triggering a resetpriorities
>    run.
>
>diff --git a/modules/instance_metadata/manifests/init.pp b/modules/instance_metadata/manifests/init.pp
>index 871dfb8..89e9fac 100644
>--- a/modules/instance_metadata/manifests/init.pp
>+++ b/modules/instance_metadata/manifests/init.pp
>@@ -29,27 +29,46 @@ class instance_metadata {
>             case $::operatingsystem {
>                 Ubuntu, CentOS: {
>                     file {
>                         "/etc/init.d/instance_metadata":
>                             require => File["/usr/local/bin/instance_metadata.py"],
>                             content => template("instance_metadata/instance_metadata.initd.erb"),
>                             mode    => 0755,
>                             owner   => "root",
>-                            notify  => Service["instance_metadata"];
>+                            notify  => $::operatingsystem ? {
>+                                Ubuntu => Service["instance_metadata"],
>+                                # on CentOS, also reset priorities; not sure
>+                                # how to do this on Ubuntu
>+                                CentOS => [
>+                                    Service["instance_metadata"],
>+                                    Exec["instance_metadata-resetpriorities"],
>+                                ],
>+                            };
>                     }
>                     service {
>                         "instance_metadata":
>                             require => [
>                                 File["/etc/init.d/instance_metadata"],
>                                 File["/usr/local/bin/instance_metadata.py"],
>                             ],
>                             hasstatus => false,
>                             enable    => true;
>                     }
>+
>+                    # note: CentOS only
>+                    exec {
>+                        'instance_metadata-resetpriorities':
>+                            # resetpriorities tells chkconfig to update the
>+                            # symlinks in rcX.d with the values from the service's
>+                            # init.d script
>+                            command => '/sbin/chkconfig runner resetpriorities',

Shouldn't this be `chkconfig instance_metadata resetpriorities`?

>+                            require => Class['instance_metadata'],
>+                            refreshonly => true;
>+                    }
>                 }
>                 default: {
>                     fail("instance_metadata is not supported on $::operatingsystem")
>                 }
>             }
>         }
>         default: {
>             # Non-AWS machines should have empty metadata
>diff --git a/modules/instance_metadata/templates/instance_metadata.initd.erb b/modules/instance_metadata/templates/instance_metadata.initd.erb
>index 4357e3e..8c0deaf 100644
>--- a/modules/instance_metadata/templates/instance_metadata.initd.erb
>+++ b/modules/instance_metadata/templates/instance_metadata.initd.erb
>@@ -3,17 +3,17 @@
> #
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> # chkconfig: 2345 49 08
> ### BEGIN INIT INFO
> # Provides:          instance_metadata
>-# Required-Start:    $local_fs $network buildbot
>+# Required-Start:    $local_fs $network
> # Should-Start:      $remote_fs
> # Should-Stop:       $remote_fs
> # Default-Start:     2 3 4 5
> # Default-Stop:      K 0 1 6
> # Short-Description: instance_metadata
> # Description:       Fetches instance metadata for VMs and stores it in a local file
> ### END INIT INFO
> 
>diff --git a/modules/runner/manifests/service.pp b/modules/runner/manifests/service.pp
>index b4554c7..113ec1d 100644
>--- a/modules/runner/manifests/service.pp
>+++ b/modules/runner/manifests/service.pp
>@@ -1,28 +1,33 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> # Make sure runner runs at boot
> class runner::service {
>     include runner::settings
>+
>+    # TODO: this really should just just run within runner
>+    include instance_metadata

Don't think we need this I don't think, it's included already on all AWS machines. Including it here will mean it's included on Mac and Windows in the future.

>+
>     case $::operatingsystem {
>         'CentOS': {
>             file {
>                 '/etc/init.d/runner':
>                     content => template('runner/runner.initd.erb'),
>                     mode    => '0755',
>-                    notify  => Exec['initd-r-refresh'];
>+                    notify  => Exec['runner-resetpriorities'];
>             }
>             exec {
>-                'initd-r-refresh':
>+                'runner-resetpriorities':
>                     # resetpriorities tells chkconfig to update the
>                     # symlinks in rcX.d with the values from the service's
>                     # init.d script
>                     command => '/sbin/chkconfig runner resetpriorities',
>+                    require => Class['instance_metadata'],

Unfamiliar with this, why's it necessary? Don't we want to refresh runner regardless of platform (as instance_metadata is AWS only)?

>                     refreshonly => true;
>             }
>             service {
>                 'runner':
>                     require   => [
>                         Python::Virtualenv[$runner::settings::root],
>                     ],
>                     hasstatus => false,
>diff --git a/modules/runner/templates/runner.initd.erb b/modules/runner/templates/runner.initd.erb
>index 60940a9..60ecda0 100644
>--- a/modules/runner/templates/runner.initd.erb
>+++ b/modules/runner/templates/runner.initd.erb
>@@ -3,17 +3,17 @@
> #
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> # chkconfig: 2345 98 08
> ### BEGIN INIT INFO
> # Provides:          runner
>-# Required-Start:    $local_fs $network puppet
>+# Required-Start:    $local_fs $network puppet instance_metadata
> # Should-Start:      $remote_fs
> # Should-Stop:       $remote_fs
> # Default-Start:     2 3 4 5
> # Default-Stop:      K 0 1 6
> # Short-Description: runner
> # Description:       runs tasks
> ### END INIT INFO
> 


I think as a whole you're fine without any changes to the runner puppet stuff, except for the addition of the dependency in runner/templates/runner.initd.erb.

(untested)

Comment 7

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

isntance_metadata is included in toplevel::base -- it's just empty on non-AWS hosts.

The requires is necessary because runner.initd refers to instance_metadata.  But, that might be a problem on non-AWS hosts where that initscript doesn't exist.

Ugh, this feels like it's tying these two things together too closely.  Would it be better to just refactor instance_metadata into a runner script?

Comment 8

[Ian Connolly [:ianconnolly]]

I think so - I don't think there's anything in there that needs to be edited actually? The initscript looks logic-less.

Comment 9

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

Attachment: bug1046926-p2.patch

How's this look?  In particular, how do you feel about me using runner::task directly, instead of including a runner::tasks::instance_metadata class?  I think once everything's using runner, this could probably get moved entirely within the runner module, to be honest.

Comment 10

[Ian Connolly [:ianconnolly]]

Comment on attachment 8471767 [details] [diff] [review]
bug1046926-p2.patch

If this is working and will unblock you, then I say go with it.

r+ on the condition that we move this into the runner module once ubuntu support lands.

Comment 11

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

Comment on attachment 8471767 [details] [diff] [review]
bug1046926-p2.patch

Shipped.  I'll leave this open for that eventual fix.

Comment 12

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

Huh, instance_metadata runs on the masters, too (it's in toplevel::base, actually!).

Since runner only runs on buildslaves, that will make running instance_metadata from runner difficult.

I'll move instance_metadata to toplevel::slave::releng for the moment, then.

Comment 13

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

Attachment: bug1046926-p3.patch