1036999 - Bare Metal Provisioning 2008 Puppet: buildslave module Windows support

Status: RESOLVED FIXED

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

Comment 1

[Mark Cornmesser [:markco] OOO 2024/04/15]

Attachment: 1036999.patch

Comment 2

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

Comment on attachment 8454565 [details] [diff] [review]
1036999.patch

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

I'm still worried about psutil here -- I thought that was getting installed from a ZIP?  It's rare (but not unheard-of) for us to install packages by just copying files out of a module, and in any case those files need to be text files (Python source) and not object code, EXEs, etc.

start-buildbot.bat looks wrong, too - did something funny happen there?

::: modules/buildslave/files/start-buildbot.bat
@@ +6,5 @@
> +set log="c:\tmp\buildbot-startup.log"
> +
> +echo "Mozilla tools directory: %MOZBUILDDIR%"
> +
> + Use the "new" moztools-static

Is ' Use' a batch command?  Should there be some prefix here?

::: modules/buildslave/manifests/startup/windows.pp
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +class buildslave::startup::windows {
> +     include packages::mozilla::mozilla_build
> +        #Bat file to start buildbot

Indentation to fix here..

@@ +21,5 @@
> +        #Importing the XML file using schtasks
> +        #Refrence http://technet.microsoft.com/en-us/library/cc725744.aspx and http://technet.microsoft.com/en-us/library/cc722156.aspx
> +        exec { "startbuildbot":
> +            creates => 'C:\programdata\PuppetLabs\puppet\var\lib\startbuildbot.txt',
> +            command => '"C:\Windows\winsxs\x86_microsoft-windows-sctasks_31bf3856ad364e35_6.1.7601.17514_none_8c46e17f1398738b\schtasks.exe" /Create  /XML "C:\mozilla-build\startbuild.xml" /tn Sta

Is that path for real?  Is that stable, and going to work across installs of Windows? Across different versions?  Wow..

::: modules/buildslave/manifests/winpy.pp
@@ +8,5 @@
> +        source  => "puppet:///modules/python/runslave.py",
> +    }
> +    file { 'C:\mozilla-build\python27\python_psutil':
> +        ensure => directory,
> +        source  => "puppet:///modules/python/python_psutil",

I don't see these files -- is it just a few Python files?

Comment 3

[Mark Cornmesser [:markco] OOO 2024/04/15]

Attachment: 1036999.patch

> I'm still worried about psutil here -- I thought that was getting installed
> from a ZIP?  It's rare (but not unheard-of) for us to install packages by
> just copying files out of a module, and in any case those files need to be
> text files (Python source) and not object code, EXEs, etc.
> 

That manifests should not be here. We are installing psutil from a ZIP. Replaced this with startup.pp which is the correct manifest. 

> start-buildbot.bat looks wrong, too - did something funny happen there?

Yes... REM was removed from in front of use and added at the bottom. Corrected.

> > +            command => '"C:\Windows\winsxs\x86_microsoft-windows-sctasks_31bf3856ad364e35_6.1.7601.17514_none_8c46e17f1398738b\schtasks.exe" /Create  /XML "C:\mozilla-build\startbuild.xml" /tn Sta
> 
> Is that path for real?  Is that stable, and going to work across installs of
> Windows? Across different versions?  Wow..
> 

Yep, it is the real path. I suspect that this may not work across the other OSes. However, I figure as we expand report and in areas like this we could do a conditional statement based on the OS version variable that is created during the MDT install.

Comment 4

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

Comment on attachment 8455594 [details] [diff] [review]
1036999.patch

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

OK, lots of comments on startup.pp, but the rest looks good :)

::: modules/buildslave/manifests/startup.pp
@@ +6,5 @@
> +    if ($::operatingsystem != Windows) {
> +        anchor {
> +            'buildslave::startup::begin': ;
> +            'buildslave::startup::end': ;
> +        }

The Anchor should be in place on Windows too.  See https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/HowTo/Anchor_Classes

@@ +7,5 @@
> +        anchor {
> +            'buildslave::startup::begin': ;
> +            'buildslave::startup::end': ;
> +        }
> +        include ::shared

I actually have no idea why 'include ::shared' is needed here.  It shouldn't cause problems to just include it on Windows.

@@ +8,5 @@
> +            'buildslave::startup::begin': ;
> +            'buildslave::startup::end': ;
> +        }
> +        include ::shared
> +        include buildslave::install

Is installing Buildbot being left for another patch?  If so, a comment to that effect would be good.

@@ +10,5 @@
> +        }
> +        include ::shared
> +        include buildslave::install
> +        include dirs::usr::local::bin
> +        include users::root

These two can move within the case $::operatingsystem below.

@@ +13,5 @@
> +        include dirs::usr::local::bin
> +        include users::root
> +    }
> +    if ($::operatingsystem == Windows) {
> +        include packages::mozilla::mozilla_build

This should be moved to the case statement below.

@@ +45,5 @@
> +     if ($::operatingsystem != Windows) {
> +        Anchor['buildslave::startup::begin'] ->
> +            class {
> +                "buildslave::startup::$startuptype":
> +             } -> Anchor['buildslave::startup::end']

If you don't run this on Windows, then `modules/buildslave/manifests/startup/windows.pp` is never used!

The Anchor stuff should remain here, as well.

Comment 5

[Mark Cornmesser [:markco] OOO 2024/04/15]

Attachment: 1029666.patch

Comment 6

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

Comment on attachment 8501836 [details] [diff] [review]
1029666.patch

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

So, this is looking good, with a few minor updates, but I need to see a patch against the latest version in the repository to make sure it's OK and landing won't hurt POSIX.

::: modules/buildslave/files/start-buildbot.bat
@@ +3,5 @@
>  SET MOZBUILDDIR=%~dp0
>  SET MOZILLABUILD=%MOZBUILDDIR%
> +REM set Buildbot version
> +cmd /c "C:/mozilla-build/bbpath.bat"
> +

This file (start-buildbot.bat) isn't in the repository now -- is this patch based on another that does include the file?  All I see is start-buildbot-win64.bat (http://hg.mozilla.org/build/puppet/file/8c28d2cece12/modules/buildslave/files/).  I see some other places in this patch that don't apply against the current tip, too.  For example, in version.pp you replace "$tools = .." with "$virtualenv_path = ..", but there's no $tools in http://hg.mozilla.org/build/puppet/file/8c28d2cece12/modules/buildslave/manifests/install/version.pp

So I think this may need to be rebased and re-reviewed on that basis.

::: modules/buildslave/manifests/install/version.pp
@@ +62,5 @@
> +                    Anchor["buildslave::install::version::${version}::begin"] ->
> +                        file {
> +                            "$virtualenv_path":
> +                                ensure => "link",
> +                                target => "$tools/buildbot-$version";

This still uses $tools, which you removed.  The "$tools/buildbot-$version" bit should have been replaced with $virtualenv_path, but not "$tools/buildbot".  Since this is POSIX-only, you can just replace "$tools/buildbot" with "/tools/buildbot".

::: modules/python/manifests/virtualenv.pp
@@ +75,5 @@
> +                                    Class['python::virtualenv::prerequisites'],
> +                                ],
> +                            creates => "$virtualenv/bin/pip",
> +                            cwd => $virtualenv;
> +                    }

These two exec's look the same with the exception of command and the presence or absence of the user parameter.  If you set $ve_user = undef for Windows, that will take care of that parameter.  And $command could be put in a variable or specified with

  command => $operatingsystem ? {
    windows => "....",
    default => "...."
  }

Also note typo "$pyhton"

::: modules/python/manifests/virtualenv/package.pp
@@ +55,5 @@
>          "pip $title":
>              name => "$virtualenv/bin/pip install $pip_options $pkg",
>              logoutput => on_failure,
>              onlyif => "$virtualenv/bin/python $pip_check_py $pkg",
> +            user => $user,

Here, too, the two cases could be combined into one with $user being undef on Windows (which you'd get automatically if $ve_user was undef in virtualenv.pp)

Comment 7

[Mark Cornmesser [:markco] OOO 2024/04/15]

Attachment: 1036999.patch

Comment 8

[Mark Cornmesser [:markco] OOO 2024/04/15]

Comment on attachment 8507139 [details] [diff] [review]
1036999.patch

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

This patch was incomplete.

Comment 9

[Mark Cornmesser [:markco] OOO 2024/04/15]

Attachment: 1036999.patch

Comment 10

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

Comment on attachment 8507976 [details] [diff] [review]
1036999.patch

per irc, this is the wrong patch :(

Comment 11

[Mark Cornmesser [:markco] OOO 2024/04/15]

Attachment: 1029666.patch

This is "pick 77c84db BUG 1036999 BuildSlave support for Windows" from branch BUG1035894-3 in my enviroment.

Comment 12

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

Looking in the git log from that commit, I see

> commit 77c84db53d2c3db9d8b17c272154742b9dcf0a79 (markco/BUG1035894-3)
> Author: Mark Cornmesser <[e-mail]>
> Date:   Fri Oct 17 12:41:21 2014 -0700
> 
>     BUG 1036999 BuildSlave support for Windows
> 
> commit f8340f8adc8b451d6592f78d337d4ff42745a78f
> Author: Mark Cornmesser <[e-mail]>
> Date:   Tue Oct 21 09:57:05 2014 -0700
> 
>     WIP for new brnach
> 
> commit c48d9c1ce9da9f12d57a9b7870b7f287266b485f
> Author: Mark Cornmesser <[e-mail]>
> Date:   Mon Oct 20 14:41:57 2014 -0700
> 
>     TMP branch switch
> 
> commit 8f354c50b88f46f5fc5e7329aa91e55806495394
> Author: Mark Cornmesser <[e-mail]>
> Date:   Mon Oct 20 10:24:44 2014 -0700
> 
>     BUG1085477 windows_firewall module from Puppet Forge
> 
> commit 08ff5fac14575a7d96a65d4362138c1c2307604f
> Author: Mark Cornmesser <[e-mail]>
> Date:   Mon Oct 20 10:14:20 2014 -0700
> 
>     BUG1085468 Remove extra  ' from shared::execonce in Windows  directory
> 
> commit 725f08d9c92c9643c9a2758f098ed2a66670db5c
> Author: Mark Cornmesser <[e-mail]>
> Date:   Thu Oct 16 10:45:55 2014 -0700
> 
>     Buildslave

where 8f354c and 08ff5f were already committed in separate (named) bugs.  However, 725f08 is definitely important to this bug, and f8340f looks similar to bug 1086998.

I ran a rebase -i 725f08d9c92c9643c9a2758f098ed2a66670db5c^ with this TODO:

pick 725f08d Buildslave
fixup 77c84db BUG 1036999 BuildSlave support for Windows

and this worked without conflicts, with a decent-looking result.  I rebased that onto the latest master.  I'll upload the result.

Comment 13

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

Attachment: bug1036999.patch

Does this look like the right patch?

Comment 14

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

Comment on attachment 8509574 [details] [diff] [review]
bug1036999.patch

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

This is definitely the right structure -- just some bits and pieces to fix up.  I'll want to do some testing on Linux and OS X before deploying, just to be sure.

::: modules/buildslave/manifests/install/version.pp
@@ +9,5 @@
>  define buildslave::install::version($active=false, $ensure="present") {
>      $version = $title
> +        $virtualenv_path = $::operatingsystem ? {
> +        windows => "c:\\mozilla-build\\buildbot-$version",
> +            default => "/tools/buildbot-$version",

nit: whitespace

@@ +49,5 @@
>                      packages => $packages;
>              } -> Anchor["buildslave::install::version::${version}::end"]
> +            case $::operatingsystem {
> +                Windows: {
> +                    if ($active) {

The whole case statement could be enclosed in `if ($active)`, saving a few lines of repetition here.

@@ +60,5 @@
> +                default: {
> +                    if $active {
> +                    Anchor["buildslave::install::version::${version}::begin"] ->
> +                        file {
> +                            "$virtualenv_path":

This used to be "/tools/buildbot", but that's not what $virtualenv_path is.

@@ +62,5 @@
> +                    Anchor["buildslave::install::version::${version}::begin"] ->
> +                        file {
> +                            "$virtualenv_path":
> +                                ensure => "link",
> +                                target => "/tools/buildbot-$version";

This, however, *is* $virtualenv_path

::: modules/buildslave/manifests/startup.pp
@@ +25,5 @@
> +                        require => Class['packages::mozilla::mozilla_build'],
> +            }
> +        }
> +        default: {
> +            file {

You can move the dirs::usr::local::bin include here, saving an extra if() above

@@ +38,5 @@
>      # select an implementation class based on operating system
>      $startuptype = $::operatingsystem ? {
>          CentOS      => "runner",
>          Darwin      => "launchd",
> +        Ubuntu      => "desktop",

Ubuntu should be "runner", not "desktop"

::: modules/buildslave/manifests/startup/windows.pp
@@ +13,5 @@
> +    # XML file to set up a schedule task to to launch buildbot on cltbld log in.
> +    # XML file needs to exported from the task scheduler gui. Also note when using the XML import be aware of machine specific values such as name will need to be replaced with a variable.
> +    # Hence the need for the template.
> +    file {
> +        'C:/mozilla-build/startbuild.xml':

startbuild? or startbuildbot?

@@ +19,5 @@
> +            require => Class['packages::mozilla::mozilla_build'],
> +    }
> +    # Importing the XML file using schtasks
> +    # Refrence http://technet.microsoft.com/en-us/library/cc725744.aspx and http://technet.microsoft.com/en-us/library/cc722156.aspx
> +    exec { "startbuildbot":

execonce

::: modules/python/manifests/virtualenv.pp
@@ +10,5 @@
>      $virtualenv = $title
> +    $ve_cmd = $operatingsystem ? {
> +        windows => "$python ${python::virtualenv::settings::misc_python_dir}\\virtualenv.py --python=$python --distribute $virtualenv",
> +        default => "$python -BE ${python::virtualenv::settings::misc_python_dir}/virtualenv.py \
> +                                        --python=$python --distribute --never-download $virtualenv",

I think you *do* want the -BE.  When I was looking at this before I thought those were arguments to virtualenv.py, but they're arguments to python itself.  from `python --help`:

-B     : don't write .py[co] files on import; also PYTHONDONTWRITEBYTECODE=x
-E     : ignore PYTHON* environment variables (such as PYTHONPATH)

You'll also want to keep --never-download

@@ +17,5 @@
>      # Figure out user/group if they haven't been set
> +    # User attributes are not support by Windows when using the exec resource
> +    case $::operatingsystem {
> +        Windows: {
> +            $ve_user = undef

and $ve_group = undef

@@ +71,5 @@
> +                                ],
> +                            creates => "$virtualenv/bin/pip",
> +                            cwd => $virtualenv;
> +                    }
> +

Looks good, just (nit) whitespace is messed up

::: modules/python/manifests/virtualenv/package.pp
@@ +25,5 @@
>  servers = [ @data_server ] + Array(@data_servers)
>  servers.uniq.each do |mirror_server| -%> --find-links=http://<%= mirror_server %>/python/packages <%
>  end
>  -%>")
> +    # User and environment attributes are not supported by Windows when using the exec resource

I see this comment, but no code changes to correspond.. is the comment still relevant?

::: modules/python/manifests/virtualenv/settings.pp
@@ +6,5 @@
>      # the root package directory into which all Python package tarballs are copied
> +    $misc_python_dir = $::operatingsystem ? {
> +        windows => 'c:\mozilla-build',
> +        default => "/tools/misc-python",
> +    }

:thumbsup:

Comment 15

[Mark Cornmesser [:markco] OOO 2024/04/15]

Comment on attachment 8509574 [details] [diff] [review]
bug1036999.patch

With the exception of: 

-        Ubuntu      => "runner"
+        Ubuntu      => "desktop",
+        Windows     => "windows"

I must have missed that when I attempted to put together the new patch. 

I corrected it and added it back into the same commit.

Comment 16

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

Yeah, it's easy to miss stuff like that when rebasing -- I do it all the time.

Comment 17

[Mark Cornmesser [:markco] OOO 2024/04/15]

Attachment: bug1036999.patch

Comment 18

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

Attachment: bug1036999-rebased.patch

A rebased version of the same patch.  I'm getting issues unzipping MozillaBuild:

> Notice: /Stage[main]/Packages::Mozilla::Mozilla_build/Packages::Pkgzip[MozillaBuildSetup-Latest.zip]/Shared::Execonce[MozillaBuildSetup-Latest.zip]/Exec[MozillaBuildSetup-Latest.zip]/returns: Extracting  yasm
> Notice: /Stage[main]/Packages::Mozilla::Mozilla_build/Packages::Pkgzip[MozillaBuildSetup-Latest.zip]/Shared::Execonce[MozillaBuildSetup-Latest.zip]/Exec[MozillaBuildSetup-Latest.zip]/returns: Extracting  yasm\yasm.exe
> Notice: /Stage[main]/Packages::Mozilla::Mozilla_build/Packages::Pkgzip[MozillaBuildSetup-Latest.zip]/Shared::Execonce[MozillaBuildSetup-Latest.zip]/Exec[MozillaBuildSetup-Latest.zip]/returns:
> Notice: /Stage[main]/Packages::Mozilla::Mozilla_build/Packages::Pkgzip[MozillaBuildSetup-Latest.zip]/Shared::Execonce[MozillaBuildSetup-Latest.zip]/Exec[MozillaBuildSetup-Latest.zip]/returns: Sub items Errors: 3
> Error: "C:\Program Files (x86)\7-Zip\7z.exe" x c:\InstallerSource\puppetagain.pub.build.mozilla.org\ZIPs\MozillaBuildSetup-Latest.zip -oC:\mozilla-build -y returned 2 instead of one of [0]
> Error: /Stage[main]/Packages::Mozilla::Mozilla_build/Packages::Pkgzip[MozillaBuildSetup-Latest.zip]/Shared::Execonce[MozillaBuildSetup-Latest.zip]/Exec[MozillaBuildSetup-Latest.zip]/returns: change from notrun to 0 failed: "C:\Program Files (x86)\7-Zip\7z.exe" x c:\InstallerSource\puppetagain.pub.build.mozilla.org\ZIPs\MozillaBuildSetup-Latest.zip -oC:\mozilla-build -y returned 2 instead of one of [0]

the output is voluminous, so I can't tell which 3 sub items errored..

Comment 19

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

Comment on attachment 8515055 [details] [diff] [review]
bug1036999-rebased.patch

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

This looks good!  Once I can get it working, we can land it -- and after bug 1091663

::: modules/python/manifests/virtualenv.pp
@@ +72,5 @@
> +                            ],
> +                            creates => "$virtualenv/bin/pip",
> +                            cwd => $virtualenv;
> +                    }
> +          

The indentation here got a little messed up..

Comment 20

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

Attachment: bug1036999-combined.patch

I had some trouble getting that to run, where it ran 'virtualenv' on every run, and couldn't do the 'pip install's.

Comment 21

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

Attachment: bug1036999-interdiff.patch

Comment 22

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

http://hg.mozilla.org/build/puppet/rev/6feb6731f424