1347129 - On Ubuntu 16, upstart was deprecated. Modify service templates or configure Ubuntu 16 to use initd (upstart)

Status: RESOLVED FIXED

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

Description

[Dragos Crisan [:dragrom]]

Could not evaluate: Execution of '/sbin/status network-manager' returned 1: status: Unable to connect to Upstart: Failed to connect to socket /com/ubuntu/upstart: Connection refused

Comment 1

[Dragos Crisan [:dragrom]]

After some researches I found the following:

/sbin/start and /sbin/status, booth are symlinks from /sbin/initctl. /sbin/initctl is deprecated in Ubuntu 16.04, and for this reason, for the services I received the error: 

"Could not evaluate: Execution of '/sbin/status network-manager' returned 1: status: Unable to connect to Upstart: Failed to connect to socket /com/ubuntu/upstart: Connection refused"

To fix this, I created the following scripts:

/sbin/status

#!/bin/bash
/usr/sbin/service $1 status
echo $?

/sbin/start

#!/bin/bash
/usr/sbin/service $1 start
echo $?

Remain to implement these using puppet, with different approach for Ubuntu 12.04, 14.04 and 16.04

Comment 2

[Dragos Crisan [:dragrom]]

also need to change the modules/gui/manifests/init.pp class to add Ubuntu 16.04 capabilities

Comment 3

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_services.patch

Comment 4

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

(In reply to Dragos Crisan [:dragrom] from comment #0)
> Could not evaluate: Execution of '/sbin/status network-manager' returned 1:
> status: Unable to connect to Upstart: Failed to connect to socket
> /com/ubuntu/upstart: Connection refused

So it sounds like the root issue is, our copy of puppet is from before systemd was a thing (September 2014), so it doesn't know to use the right methods to talk to systemd.  So, this hack is a nice way to get things working, but IMHO should increase the urgency of upgrading puppet.  /cc dividehex for that.

Comment 5

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

Comment on attachment 8847642 [details] [diff] [review]
puppetagain_services.patch

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

::: modules/disableservices/manifests/common.pp
@@ +38,5 @@
> +                    # initctl is deprecated in Ubuntu 16.04 and was replaced with systemd
> +                    file {
> +                        "/sbin/status":
> +                            ensure => present,
> +                            content => "/usr/sbin/service $1 status";

These will need an executable `mode` property too.  Also, they should probably have `#! /bin/sh\n`.

::: modules/gui/manifests/init.pp
@@ +157,5 @@
> +                            # we do not ensure this is running; the system will start
> +                            # it after puppet is done
> +                            ensure => running;
> +                    }
> +                }

I don't understand the motivation for this change.  What I see is:

 * drop "enable" (which means that these services won't start on boot -- seems bad)
 * ensure XSession is running, despite a comment saying that we don't ensure it's runing (it's always a good idea to read comments near code you're modifying!!).  I suspect, based on the comment, that we *shouldn't* do this.
 * remove the require and notify's -- why?

At a guess, the issue is that enabling these services doesn't work because they are implemented with systemd instead of upstart.  I suspect you'll need to either find a similar hack to allow puppet to enable services in systemd; or manually enable these services using exec{}; or enable initctl (assuming "deprecated" means there's a compatibility package we could install); or update puppet (!)

Comment 6

[Dragos Crisan [:dragrom]]

Found a way to hack the services. I started to implement the changes. Tomorrow I'll provide a patch

Comment 7

[Jake Watkins [:dividehex]]

(In reply to Dustin J. Mitchell [:dustin] from comment #4)
> (In reply to Dragos Crisan [:dragrom] from comment #0)
> > Could not evaluate: Execution of '/sbin/status network-manager' returned 1:
> > status: Unable to connect to Upstart: Failed to connect to socket
> > /com/ubuntu/upstart: Connection refused
> 
> So it sounds like the root issue is, our copy of puppet is from before
> systemd was a thing (September 2014), so it doesn't know to use the right
> methods to talk to systemd.  So, this hack is a nice way to get things
> working, but IMHO should increase the urgency of upgrading puppet.  /cc
> dividehex for that.


I do believe that our version of puppet (3.7/3.8) does have a systemd provider under the Service resource.  Although, it may be assuming Upstart since 12.04/14.04 were Upstart.  You may need to be explicit in the service resource and state 'provider => systemd'.  Let's try that first before trying to setup a bunch 'hack' workarounds on the system itself.  If the systemd provider doesn't work well, then I'd prefer to replace the Services resource with Exec resource calls to systemctl instead and separate them with case statements so 12.04/14.04 are still handled the same way as before.  That would allow use to go back and replace Exec with Service when we do upgrade puppet.

It should be a last resort to try and emulate initctl on 16.04.

Comment 8

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_services.patch

Comment 9

[Jake Watkins [:dividehex]]

Comment on attachment 8849592 [details] [diff] [review]
puppetagain_services.patch

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

Looks good overall. Although, I have to r- because runner needs to be managed by systemd.  Puppet can't be relied on to start and keep runner going. In fact, this wouldn't work in the aws spot instances since puppet is only used to build the ami and is never actually run on the instances itself.

You might also consider splitting patches per module.  This patch touches disableservices, gui, rsyslog and runner.  All would have been r+ except for runner.

::: modules/gui/manifests/init.pp
@@ +55,5 @@
>                  # this is the EDID data from an Extron EDID adapter configured for 1200x1600
>                  "/etc/X11/edid.bin":
>                      source => "puppet:///modules/${module_name}/edid.bin";
>  
>                  # Bug 859867: prevent nvidia drivers to use sched_yield(),

This comment about the nvidia driver should have also been moved with "/etc/X11/Xsession.d/98nvidia":

@@ +67,4 @@
>  
>                  # make sure the builder user doesn't have any funny business
>                  [ "${users::builder::home}/.xsession",
> +                "${users::builder::home}/.xinitrc",

Looks like the indentation got shifted out of alignment here

::: modules/runner/manifests/service.pp
@@ +62,5 @@
> +                        'runner':
> +                            command => 'runner -v --syslog -n 30 -H -c /opt/runner/runner.cfg /opt/runner/tasks.d >> /var/log/runner.log 2>&1',
> +                            path    => ['/opt/runner/bin','/bin','/usr/bin'],
> +                            unless  => 'ps -aux|grep runner|grep v|wc -l > 0';
> +                    }

Using an exec here does work but runner needs to be managed by systemd.  This will need to install a handcrafted service file, exec systemctl daemon-reload, and then use the Service resource (with provider => systemd) to enable => true

::: modules/runner/manifests/task.pp
@@ +17,5 @@
> +            }
> +        }
> +        16.04: {
> +            file {
> +                "${runner::settings::taskdir}/${title}":

I believe it is still important to specify these files BEFORE the service is handled.  Having a systemd runner service would allow you to include the 'before => $runner::runner_service,'

Comment 10

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_runner.patch

Comment 11

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_services.patch

I deleted the changes for runner and gui from puppet_again.patch. I added a patch for runner module: puppetagain_runner.patch

Comment 12

[Dragos Crisan [:dragrom]]

I deleted the gui module from the patch because I want to replace the exec with the service for x11

Comment 13

[Jake Watkins [:dividehex]]

Comment on attachment 8850056 [details] [diff] [review]
puppetagain_runner.patch

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

I think you're missing the runner.service.erb template.

::: modules/runner/manifests/service.pp
@@ +49,5 @@
> +                16.04: {
> +                    file {
> +                        "/etc/init.d/runner":
> +                            mode    => 0755,
> +                            content => template("runner/runner.initd.erb");

Although, systemd will act backwards compatible with SysV initd scripts and observe the LSB header, we should probably just write a systemd .service for runner and not use this file at all.

@@ +54,5 @@
> +                    }
> +                    file {
> +                        "/lib/systemd/system/runner.service":
> +                            content => template("runner/runner.service.erb"),
> +                            require => File["/etc/init.d/runner"];

I know /lib/systemd/system is an acceptable place to put .service files (in fact, according to the docs there are nine) but I think we should place them in /etc/systemd/system/ just to keep things consistent with where we have put them in the past.

Where is runner.service.erb?  That should be included with this patch.

@@ +66,5 @@
> +                            ],
> +                            hasstatus => false,
> +                            enable    => true,
> +                            ensure    => running;
> +                    }

I believe we purposely do not ensure => running and only enable => true.  Remove the 'ensure => running' to keep it consistent with what is done in 12.04/14.04

Comment 14

[Dragos Crisan [:dragrom]]

(In reply to Jake Watkins [:dividehex] from comment #13)
> Comment on attachment 8850056 [details] [diff] [review]
> puppetagain_runner.patch
> 
> Review of attachment 8850056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you're missing the runner.service.erb template.
> 
The runner.service.erb not appeared in hg diff and I forfota to attach it.
> ::: modules/runner/manifests/service.pp
> @@ +49,5 @@
> > +                16.04: {
> > +                    file {
> > +                        "/etc/init.d/runner":
> > +                            mode    => 0755,
> > +                            content => template("runner/runner.initd.erb");
> 
> Although, systemd will act backwards compatible with SysV initd scripts and
> observe the LSB header, we should probably just write a systemd .service for
> runner and not use this file at all.
> 
> @@ +54,5 @@
> > +                    }
> > +                    file {
> > +                        "/lib/systemd/system/runner.service":
> > +                            content => template("runner/runner.service.erb"),
> > +                            require => File["/etc/init.d/runner"];
> 
> I know /lib/systemd/system is an acceptable place to put .service files (in
> fact, according to the docs there are nine) but I think we should place them
> in /etc/systemd/system/ just to keep things consistent with where we have
> put them in the past.
> 
> Where is runner.service.erb?  That should be included with this patch.
> 
> @@ +66,5 @@
> > +                            ],
> > +                            hasstatus => false,
> > +                            enable    => true,
> > +                            ensure    => running;
> > +                    }
> 
> I believe we purposely do not ensure => running and only enable => true. 
> Remove the 'ensure => running' to keep it consistent with what is done in
> 12.04/14.04

Comment 15

[Dragos Crisan [:dragrom]]

I forgot to attach runner.service.erb

Comment 16

[Jake Watkins [:dividehex]]

Comment on attachment 8850058 [details] [diff] [review]
puppetagain_services.patch

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

r+ ship it

Comment 17

[Jake Watkins [:dividehex]]

Comment on attachment 8850058 [details] [diff] [review]
puppetagain_services.patch

Checked-in on behalf of :dragrom

https://hg.mozilla.org/build/puppet/rev/cea5758aa0bd41e6e4246b98705ab0e1c9751a26
https://hg.mozilla.org/build/puppet/rev/f25c8e128ab542a9b0adef51e917d99cba64fab

Comment 18

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_runner.patch

Comment 19

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_gui.patch

Comment 20

[Jake Watkins [:dividehex]]

Comment on attachment 8850902 [details] [diff] [review]
puppetagain_runner.patch

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

r+ just move the Mozilla Public License to the top of the file and remove the shebang

::: modules/runner/templates/runner.service.erb
@@ +1,4 @@
> +#! /bin/bash
> +#  initscript for runner
> +#
> +# This Source Code Form is subject to the terms of the Mozilla Public

License should always go at the top of the file and any other comments should go below the license. Also, there is no need for a shebang (#! /bin/bash) since this is not a shell script.

Comment 21

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_gui.patch

Comment 22

[Jake Watkins [:dividehex]]

Comment on attachment 8851503 [details] [diff] [review]
puppetagain_gui.patch

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

Overall I have some concerns as to the way these services are being handled by systemd.  Since systemd is designed to make extensive use of parallelism in order to achieve fast boot times, the need for dependencies and ordering to be explicitly defined is essential.  It is highly like to cause problems if these are not established with [Unit] directives such as After=,Before=,Wants=, or Requires=.

In this particular case, we do not want Xsession to start before either x11 or xvfb since it depends on one or the other depending whether there is a gpu or not.  Since we can't require both, this will require the puppet template to indicate which service is required for Xsession to start.  Xsession will also need an ordering directive such as After=x11.service or xvfb.service.  Use erb logic to specify.  I believe you could also refactor the entire logic here (for 16.04 only) to only install one services (either xll or xvfb) based on $on_gpu.

Second issue: both x11 and xvfb require puppet to have completed.  This required logic should also be carried over to systemd.

See additional inline comments

::: modules/gui/manifests/init.pp
@@ +54,5 @@
>  
>              file {
>                  "/etc/init/x11.conf":
>                      content => template("${module_name}/x11.conf.erb"),
>                      notify => Service['x11'];

All of these upstart init files should be moved under the 12.04/14.04 case.

@@ +60,3 @@
>                  "/etc/init/xvfb.conf":
>                      content => template("${module_name}/xvfb.conf.erb"),
>                      notify => Service['xvfb'];

Same here.  Move under 12.04/14.04 case.  There is no need for them in 16.04

@@ +94,1 @@
>                  "/etc/init/Xsession.conf":

Move to 12.04/14.04

@@ +160,5 @@
> +                        'x11':
> +                            ensure => $on_gpu ? { true => undef, default => stopped },
> +                            enable => $on_gpu ? { true => true, default => false },
> +                            require => File['/lib/systemd/system/x11.service'],
> +                            notify => Service['Xsession'];

Since these are systemd services, shouldn't the systemd provider be also stated here since we have see other issues with puppet detecting systemd on Ubuntu 16.04?

::: modules/gui/templates/Xsession.service.erb
@@ +8,5 @@
> +Documentation=man:systemd-sysv-generator(8)
> +
> +[Service]
> +Type=forking
> +Restart=no

Is there a reason not to restart? Same question for the other services.

@@ +13,5 @@
> +TimeoutSec=5min
> +IgnoreSIGPIPE=no
> +KillMode=process
> +GuessMainPID=no
> +RemainAfterExit=yes

Is RemainAfterExit necessary?  This will cause the service to look as though it is running even if it stops and exits. It is usually only used with Type=oneshot.  Same question for the other services?

@@ +14,5 @@
> +IgnoreSIGPIPE=no
> +KillMode=process
> +GuessMainPID=no
> +RemainAfterExit=yes
> +ExecStart=/bin/su - -c "DISPLAY=:0 /etc/X11/Xsession" cltbld

cltbld should never be hardcoded.  Use <%= scope.lookupvar('::config::builder_username') %>

::: modules/gui/templates/xvfb.service.erb
@@ +14,5 @@
> +IgnoreSIGPIPE=no
> +KillMode=process
> +GuessMainPID=no
> +RemainAfterExit=yes
> +ExecStart=/usr/bin/Xvfb :0 -nolisten tcp -screen 0 1600x1200x24

1600x1200x24 should not be hardcoded.  In the upstart templates, the resolution is set with the follow logic:
<%= @screen_width %>x<%= @screen_height %>x<%= if @screen_depth.to_i <= 24 then @screen_depth else 24 end %>

Carry over that same logic.

Also, in the upstart file,  setuid is used to run xvfb as cltbld.  This should also be set with the (I believe) User= directive.
And last, the upstart file exports the variable USER=.  This can be achieved in systemd with Environment=

Comment 23

[Dragos Crisan [:dragrom]]

Attachment: puppetagain_gui.patch

Comment 24

[Jake Watkins [:dividehex]]

Comment on attachment 8852455 [details] [diff] [review]
puppetagain_gui.patch

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

r+ Looks much better.

Comment 25

[Jake Watkins [:dividehex]]

Comment on attachment 8852455 [details] [diff] [review]
puppetagain_gui.patch

https://hg.mozilla.org/build/puppet/rev/9da803c76a074fe075882c39618ed9e16dd673f8
https://hg.mozilla.org/build/puppet/rev/b585da14f3ac0c84faddf8a198ceb885ff5daf04

Comment 26

[Dragos Crisan [:dragrom]]

Comment on attachment 8850902 [details] [diff] [review]
puppetagain_runner.patch

https://hg.mozilla.org/build/puppet/rev/1895fd62aede