1338151 - PuppetAgain puppetization support for Ubuntu 16.04

Status: RESOLVED FIXED

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

Comment 1

[Dragos Crisan [:dragrom]]

Attachment: puppetit.patch

Comment 2

[Dragos Crisan [:dragrom]]

Attachment: puppetagain.patch

Comment 3

[Jake Watkins [:dividehex]]

Comment on attachment 8840849 [details] [diff] [review]
puppetit.patch

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

Good start! You've got the right idea.  Just simplify it and use the proper variables.

::: modules/pxe/templates/kickstart/profiles/puppetagain.erb
@@ +26,4 @@
>  text
>  #Install OS instead of upgrade
>  install
> +<% if @os_release == '16.04' -%>

I don't see where $os_release is being set in the manifests. I would suggest using @os_codename == 'xenial" instead.

@@ +26,5 @@
>  text
>  #Install OS instead of upgrade
>  install
> +<% if @os_release == '16.04' -%>
> +url --url http://repos/repos/apt/<%= @os_type %>-<%= @os_release %>

since the above conditional would only apply to 'xenial', it is safe (and easier to read) to not use variables in this line.  It can simply read 'url --url http://repos/repos/apt/Ubuntu-16.04'

@@ +149,4 @@
>  
>  <% if @os_type == 'ubuntu' -%>
>  cat > /etc/apt/sources.list <<EOF
> +<% if @os_release == '16.04' -%>

Same as above,  I don't see $os_release being defined anywhere.  You can simply use @os_codename == 'xenial'

@@ +150,5 @@
>  <% if @os_type == 'ubuntu' -%>
>  cat > /etc/apt/sources.list <<EOF
> +<% if @os_release == '16.04' -%>
> +deb http://repos/repos/apt/<%= @os_type %>-<%= @os_release %>/ <%= @os_codename %> main restricted universe
> +deb http://repos/repos/apt/<%= @os_type %>-<%= @os_release %>/ <%= @os_codename %>-security main restricted universe

Same as above for these 2 lines.  There is no variable $os_release, AFAIK.  And you should just hardcode the urls since it will only ever apply to 'xenial'

Comment 4

[Jake Watkins [:dividehex]]

:dragrom, take a look at modules/pxe/manifests/menuitem/kickstart/puppetagain.pp to see what variables are being defined before modules/pxe/kickstart/profiles/puppetagain.erb is being generated.

Comment 5

[Jake Watkins [:dividehex]]

Comment on attachment 8840850 [details] [diff] [review]
puppetagain.patch

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

You are on the right path.  Just make changes noted and this should work.

::: modules/packages/manifests/setup.pp
@@ +211,5 @@
>              }
>              packages::aptrepo {
> +                case $::operatingsystemrelease {
> +                    "16.04" : {
> +                        "${lsbdistcodename}":

Since any code in this case only applies to '16.04', there is no need to use the variable.  Simply use 'xential' instead of ${lsbdistcodename}.  This makes it easier to read.

@@ +212,5 @@
>              packages::aptrepo {
> +                case $::operatingsystemrelease {
> +                    "16.04" : {
> +                        "${lsbdistcodename}":
> +                            url_path     => "repos/apt/$::operatingsystem-$::operatingsystemrelease",

Same here.  There is no need to use facts in this case, just write the actually string.

@@ +220,5 @@
> +                            url_path     => "repos/apt/$::operatingsystem-$::operatingsystemrelease",
> +                            distribution => "${lsbdistcodename}-security",
> +                            components   => ["main", "restricted", "universe"];
> +                    }
> +                    default: {

This should not be 'default' but actually '12.04','14.04'.  We want to be explicit as to which ubuntu version this case applies to.  There should also be a default case with a fail() with a warning msg.  eg.  fail("Ubuntu $operatingsystemrelease is not supported").

::: setup/common/apt-setup-repos.erb
@@ +3,4 @@
>    # file, You can obtain one at http://mozilla.org/MPL/2.0/. %>
>  cat > /etc/apt/sources.list <<EOF
>  # blow away any existing repositories and add our own
> +<% if @os_release == '16.04' -%>

Theese erb templates (apt-setup-repos.erb and ubuntu-kickstart.cfg.erb) attempt to mimic the kickstart file used in IT puppet.  So there is no need to apply conditional logic here especially with variable like @os_release, @os_type and $os_codename, which are unique to the puppet manifests in IT puppet.

For both of these files, just replace '/ubuntu/ with '/XXXXX/' and put a comment # noting the repo urls are unique to the the version of ubuntu.

::: setup/ubuntu-kickstart.cfg.erb
@@ +24,4 @@
>  text
>  #Install OS instead of upgrade
>  install
> +<% if @os_release == '16.04' -%>

Same as above.  Change '/ubuntu' with '/XXXXXX' and comment that it needs to be set based on the ubuntu version

@@ +53,4 @@
>  # https://bugzilla.mozilla.org/show_bug.cgi?id=843847
>  preseed apt-setup/services-select multiselect security
>  preseed apt-setup/security_host string repos
> +<% if @os_release == '16.04' -%>

Same as above.  'XXXX' and comment

Comment 6

[Dragos Crisan [:dragrom]]

Attachment: puppetit.patch

Comment 7

[Dragos Crisan [:dragrom]]

Attachment: puppetagain.patch

Comment 8

[:dhouse]

Comment on attachment 8841549 [details] [diff] [review]
puppetagain.patch

Is an extra tab causing indent in setup.pp?
-                "releng-updates":
+               	"releng-updates":

Comment 9

[Dragos Crisan [:dragrom]]

(In reply to Dave House [:dhouse] from comment #8)
> Comment on attachment 8841549 [details] [diff] [review]
> puppetagain.patch
> 
> Is an extra tab causing indent in setup.pp?
> -                "releng-updates":
> +               	"releng-updates":

Was a extra tab, recreated the patch

Comment 10

[Dragos Crisan [:dragrom]]

Attachment: puppetagain.patch

Comment 11

[Jake Watkins [:dividehex]]

Comment on attachment 8841511 [details] [diff] [review]
puppetit.patch

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

Perfect, I'll land this today.

Comment 12

[Jake Watkins [:dividehex]]

Comment on attachment 8841562 [details] [diff] [review]
puppetagain.patch

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

Good! Just remove the erroneous comment and the trailing white spaces before landing.

::: manifests/moco-nodes.pp
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  ## testers
> +node "t-ubuntu16-ix-022.test.releng.scl3.mozilla.com" {
> +    # hosts starting with t and ending in -digit.test.releng.scl3.mozilla.com

Nit: this comment doesn't apply here.  It refers to the use of regex in the other test host definitions

::: modules/packages/manifests/setup.pp
@@ +226,5 @@
> +                            url_path     => "repos/apt/Ubuntu-16.04",
> +                            distribution => "xenial-security",
> +                            components   => ["main", "restricted", "universe"];
> +                    }
> +                    "14.04" : {

This is acceptable although, you could have reused the current code block and combined '12.04' and '14.04' into one case since they share the same 'url_path'.  But like I said this is acceptable also.

eg. "12.04", "14.04": {

@@ +244,5 @@
> +                            components   => ["main", "restricted", "universe"];
> +                        "precise-security":
> +                            url_path     => "repos/apt/ubuntu",
> +                            distribution => "precise-security",
> +                            components   => ["main", "restricted", "universe"];                        

nit: make sure there are no trailing white spaces.

Comment 13

[Jake Watkins [:dividehex]]

dragrom,  make sure indentation is 4 white spaces and not actual tab characters.
Also, take a look at the puppet again hacking wiki page for tips and guidelines.

https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/HowTo/Hack_on_PuppetAgain

Comment 14

[Jake Watkins [:dividehex]]

Comment on attachment 8841511 [details] [diff] [review]
puppetit.patch

commit e29a7449015f7aa45d07096dfbde06cddebd9876
Author: Dragos Crisan <[e-mail]>
Date:   Mon Feb 27 11:48:58 2017 -0800

    Bug 1338151: Update puppetagain kickstart template; r=jwatkins

        This patch updates the releng puppetagain pxe kickstart template
        for the use of xential which as a different repo url path
        then previous ubuntu version

Comment 15

[Dragos Crisan [:dragrom]]

Attachment: puppetagain.patch

Corrected the trialing white spaces and put 12.04 and 14.04 into one case

Comment 16

[Jake Watkins [:dividehex]]

Comment on attachment 8841974 [details] [diff] [review]
puppetagain.patch

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

Looks good. Just fix the trailing white space and land it.  No need to resubmit for review.

::: manifests/moco-nodes.pp
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  ## testers
> +node "t-ubuntu16-ix-022.test.releng.scl3.mozilla.com" {
> +    # host to test the upgrade to Ubuntu 16.04 

nit: trailing white space

Comment 17

[Dragos Crisan [:dragrom]]

Attachment: puppetagain.patch

During the build I received the following error: "Error: Could not parse for environment production: Syntax error at '::operatingsystemrelease'; expected '}' at /home/travis/build/mozilla/build-puppet/modules/packages/manifests/setup.pp:225"

packages::aptrepo is a define type, so I needed to move the case statement outside it and include the packages::aptrepo inside case

Comment 18

[Dragos Crisan [:dragrom]]

(In reply to Dragos Crisan [:dragrom] from comment #17)
> Created attachment 8842412 [details] [diff] [review]
> puppetagain.patch
> 
> During the build I received the following error: "Error: Could not parse for
> environment production: Syntax error at '::operatingsystemrelease'; expected
> '}' at
> /home/travis/build/mozilla/build-puppet/modules/packages/manifests/setup.pp:
> 225"
> 
> packages::aptrepo is a define type, so I needed to move the case statement
> outside it and include the packages::aptrepo inside case

[root@t-ubuntu16-ix-022 ~]# puppet agent -t --environment=dcrisan --server=releng-puppet2.srv.releng.scl3.mozilla.com
Tested with success on my environment: 

The results:
Notice: Finished catalog run in 19.38 seconds
[root@t-ubuntu16-ix-022 ~]#

Comment 19

[Jake Watkins [:dividehex]]

Comment on attachment 8842412 [details] [diff] [review]
puppetagain.patch

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

::: modules/packages/manifests/setup.pp
@@ +222,5 @@
>                      source => "puppet:///modules/packages/apt.conf.mozilla";
>              }
> +            case $::operatingsystemrelease {
> +                "16.04" : {
> +                    packages::aptrepo {

You are correct, the packages::aptrepo block does need to be included inside the case.  Sorry I missed that.

@@ +250,5 @@
> +                    fail("Ubuntu $operatingsystemrelease is not supported")
> +                }
> +            }
> +
> +            @packages::aptrepo {

Moving the @packages::aptrepo to here causes the 'releng', 'releng-updates' and 'puppetlabs' repos to become virtual resources which aren't realized anywhere.  If we were going to use these repos with 16.04, I would recommend just keeping these in a (non-virtualize) packages::aptrepo {} block outside the case switch.  But since releng and releng-updates are obsolete repos only used by 12.04 and 14.04, they should be moved inside the 14.04,12.04 case.  The puppetlabs repo is a different story but same outcome, the mirror doesn't offer the version we need for xenial so are probably going to use the puppet packages that come in the 'universe' component until we upgrade to puppet 4.x.  At that point, we can mirror the puppet collections repo just for xenial.

In summary, move the 'releng', 'releng-updates' and 'puppetlabs' repo into the 14.04, 12.04 case block.  I've already done the same in the kickstart config.

Comment 20

[Dragos Crisan [:dragrom]]

Attachment: puppetagain.patch

Comment 21

[Jake Watkins [:dividehex]]

Comment on attachment 8842783 [details] [diff] [review]
puppetagain.patch

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

Looks good.  Shipit!

Comment 22

[Jake Watkins [:dividehex]]

https://hg.mozilla.org/build/puppet/rev/d733479af90a1f6e4913106496150a2b9433bb9c
Bug 1338151: Add support for Ubuntu 16.04 repos; r=jwatkins

Comment 23

[Jake Watkins [:dividehex]]

Comment on attachment 8842783 [details] [diff] [review]
puppetagain.patch

https://hg.mozilla.org/build/puppet/rev/d733479af90a1f6e4913106496150a2b9433bb9c
https://hg.mozilla.org/build/puppet/rev/bdd6ccd4d10f7192256bcdce68ffbb8031cab9c5