(MODULES-8431) Update windows installation to use powershell #362

mcdonaldseanp · 2019-01-11T22:53:52Z

This commit updates the installation method on windows to use a powershell
script rather than a batch file

branan · 2019-01-11T23:06:48Z

I think you forgot to git rm the batch file

mcdonaldseanp · 2019-01-11T23:07:34Z

whoops

puppetcla · 2019-01-12T06:00:18Z

CLA signed by all contributors.

manifests/windows/install.pp

spec/classes/puppet_agent_windows_install_spec.rb

templates/install_puppet.ps1.erb

glennsarti · 2019-01-12T13:09:43Z

templates/install_puppet.ps1.erb

+  [String] $InstallDir='<%= @install_dir %>',
+  [String] $PuppetMaster='<%= @_puppet_master %>',
+  [String] $PuppetStartType='<%= @_agent_startup_mode %>',
+  [String] $InstallArgs='<%= @_install_options.map do |option|


Personally I'd prefer this "logic" be in the manifest and the $InstallArgs be a dumb string, but I'm not too fussed

I agree with Glenn

glennsarti · 2019-01-12T13:10:24Z

templates/install_puppet.ps1.erb

+  Fetch the location of the puppet installation from the registry
+#>
+function Script:fetch_install_dir {
+  param()


Without cmdlet binding, this is not needed.

glennsarti · 2019-01-12T13:10:46Z

templates/install_puppet.ps1.erb

+  Move/rename puppetres.dll to a temporary location
+#>
+function Script:move_puppetres_dll {
+  param()


Without cmdlet binding, this is not needed.

glennsarti · 2019-01-12T13:11:59Z

templates/install_puppet.ps1.erb

+    # for the already installed package
+    $InstallDir = fetch_install_dir
+    if (Test-Path "$InstallDir\puppet\bin\puppetres.dll") {
+      Out-File -InputObject "Moving puppetres.dll to $temp_puppetres" -FilePath $Logfile -Append


The canonical way of doing this is normally;

"Moving puppetres.dll to $temp_puppetres" | Out-File -FilePath $Logfile -Append"

May be easier to abstract this into a method? Log <error_message> is more readable.

+1 to refactor to it's own function

glennsarti · 2019-01-12T13:13:16Z

templates/install_puppet.ps1.erb

+    $InstallDir = fetch_install_dir
+    if (Test-Path $temp_puppetres) {
+      Out-File -InputObject "Restoring puppetres.dll" -FilePath $Logfile -Append
+      Move-Item -Path $temp_puppetres -Destination "$InstallDir\puppet\bin\puppetres.dll"


This should fail gracefully. Moving the locked DLL will fail on rollback

I can also test for the existence of the normal puppetres.dll too.

templates/install_puppet.ps1.erb

glennsarti · 2019-01-12T13:16:20Z

templates/install_puppet.ps1.erb

+  # but using WaitForExit works fine.
+  $msiexec_proc.WaitForExit()
+  Out-File -InputObject "****************************** End msiexec.exe output ******************************" -FilePath $Logfile -Append
+  if ($msiexec_proc.ExitCode -ne 0){


Not quite true.

Exit code could be 0, 3010 or 1641 ~~(? I think it's 1604 - Soft reboot required)~~

Since we force /norestart in the MSI wouldn't the restart exit codes technically be failures as far as we're concerned?

It's my understanding that the agent should absolutely never cause a restart on upgrade.

Those are both successful installation methods. 1641 simply means "The requested operation completed successfully. The system will be restarted so the changes can take effect" and 3010 means "The requested operation is successful. Changes will not be effective until the system is rebooted."

Thus one is initiating the reboot automatically, and the other is waiting for the user to reboot. The difference is in the switches or properties you've set when calling the MSI.

3010 is a succesfull install but it's pending a reboot. i.e. the agent package WANTED to reboot but it couldn't due to /norestart

glennsarti

As per comments.

Also strictly speaking the function names in the PS script aren't Verb-Noun
Are we looking at running Pester tests over this?
This script will need manual verification (i.e. functional review) for it's ability to upgrade, and will need to prove the puppetres.dll workaround works.

ekinanp · 2019-01-12T21:31:19Z

@glennsarti I was looking into https://tickets.puppetlabs.com/browse/PA-663 to understand the puppetres.dll issue and it looks like your comment on 2017/03/01 seems to imply that the workaround is only necessary until https://tickets.puppetlabs.com/browse/PA-768 is resolved?

ekinanp · 2019-01-12T22:43:14Z

templates/install_puppet.ps1.erb

+  # Wait for the puppet run to finish
+  Out-File -InputObject "Waiting for puppet to stop, PID:$PuppetPID" -FilePath $Logfile -Append
+  try {
+    Wait-Process -Id $PuppetPID -Timeout 120


You could also do

$proc = Get-Process -Id $PuppetPID -ErrorAction SilentlyContinue if ($proc) { if (-Not $proc.WaitForExit(120000)) { // milliseconds Log "Timeout" } } else { Log "Puppet was already finished" }

when waiting for the Puppet run to end.

Or more simply;

try { Get-Process -ID $PuppetPID -ErrorAction SilentlyContinue | Wait-Process -Timeout 120` } catch [System.TimeoutException] { # TIMEOUT OCCURED } catch { # unhandled error }

ekinanp · 2019-01-12T22:48:38Z

templates/install_puppet.ps1.erb

+      throw $_
+    }
+  }
+  foreach($service in $service_names) {


The previous install_puppet.bat file had a long comment about why stopping the services was important. Maybe that comment should be here too?

The long comment was about waiting for the puppet PID, rather than waiting for the services:

https://github.com/puppetlabs/puppetlabs-puppet_agent/blob/master/templates/install_puppet.bat.erb#L35L40

but I will add that back. And I'll add a comment as to why we need to shut down the services

glennsarti · 2019-01-15T04:46:03Z

Pending manual Functional Review of the DLL locking repro case and description of manual testing performed by @mcdonaldseanp

ekinanp · 2019-01-15T22:37:34Z

templates/install_puppet.ps1.erb

+  Lock-Installation $install_pid_lock
+  # Wait for the puppet run to finish
+  #
+  # We *must* wait for Puppet Agent to finished applying its prior catalog


Nit: Typo here -- "to finished applying".

Also, this comment should be expanded on a bit more. Probably something like

We must wait for the puppet agent to finish applying its catalog before we stop all of our services. Otherwise if the catalog has additional resources that start our services (e.g. such as those from the PE module), then the install will fail to proceed.

i.e. it isn't something specific to pxp-agent, it applies to all services.

ekinanp · 2019-01-15T22:42:06Z

templates/install_puppet.ps1.erb

+    [String] $message
+  )
+  begin {
+    $message | Out-File -FilePath $Logfile -Append


May be worth including a timestamp.

mcdonaldseanp · 2019-01-15T22:45:17Z

Manual testing description: https://tickets.puppetlabs.com/browse/MODULES-8431?focusedCommentId=619991&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-619991

This commit updates the installation method on windows to use a powershell script rather than a batch file

glennsarti · 2019-01-16T12:27:22Z

templates/install_puppet.ps1.erb

+    $InstallDir = Find-InstallDir
+    if (Test-Path "$InstallDir\puppet\bin\puppetres.dll") {
+      Write-Log "Moving puppetres.dll to $temp_puppetres"
+      Move-Item -Path "$InstallDir\puppet\bin\puppetres.dll" -Destination $temp_puppetres


+1 I wasn't sure if this would work the same in PowerShell. Guess it does :-)

glennsarti

Ship it!

glennsarti · 2019-01-16T12:36:52Z

Iristyle · 2019-01-18T23:16:17Z

templates/install_puppet.ps1.erb

+  # correctly from the module.
+  [parameter(Position=0)]
+  [String] $PuppetPID,
+  [String] $Source='<%= @_msi_location %>',


I'm wondering if we should be passing params via the invocation, rather than injecting values into a template. I think the only reason we used a template on the batch side was because arg passing with batch can be a real pain.

Iristyle · 2019-01-18T23:19:13Z

templates/install_puppet.ps1.erb

+  # PuppetPID _must_ come first!, this script needs PuppetPID to be a positional parameter to execute
+  # correctly from the module.
+  [parameter(Position=0)]
+  [String] $PuppetPID,


This should be a numeric value, shouldn't it?

Iristyle · 2019-01-18T23:24:17Z

templates/install_puppet.ps1.erb

+)
+# Find-InstallDir, Move-PuppetresDLL and Reset-PuppetresDLL serve as a workaround for older
+# installations of puppet: we used to need to move puppetres.dll out of the way during puppet
+# upgrades because the file would lock and cause network stack restarts.


That's one possibility, but the problem was a little more subtle.

Any application connected to the event log - like a local event log collector application or the PowerShell cmdlets, loads / locks all the resource DLLs registered to the event log. This means the event log service holds a lock on puppetres.dll.

Because Windows will share svchost.exe instances amongst multiple services, any services co-located to the same process as the event log will get bounced, even when just the event log service needs to be restarted. Furthermore, in the event that one of the other hosted services was a dependency of yet other services, it would cause a domino effect. So in the worst case situation, I believe we saw DHCP being shared with Event Log. Higher level services like Exchange Server depended on DHCP, which caused cascading service restarts.

It may be sufficient to mention network stack restarts, but that's just one of the ways this manifested depending on version of OS, which services Windows decided to share with event log, etc, etc.

Iristyle · 2019-01-18T23:40:41Z

templates/install_puppet.ps1.erb

+  Fetch the location of the puppet installation from the registry
+#>
+function Script:Find-InstallDir {
+  begin {


Since this function doesn't accept pipeline input, begin, process, end are superfluous.

Iristyle · 2019-01-19T00:47:35Z

templates/install_puppet.ps1.erb

@@ -0,0 +1,262 @@
+# install_puppet.ps1
+<#


On a similar note about this being a template... these inline docs won't inform the PowerShell runtime during test / debugging since it's not a PS1 until rendered.

Iristyle · 2019-01-24T23:48:07Z

templates/install_puppet.ps1.erb

+    Reset-PuppetresDLL $temp_puppetres
+  }
+} finally {
+  Unlock-Installation $install_pid_lock


I think you also want to call Unregister-Event to perform cleanup... though exiting PowerShell altogether may do this for you

mcdonaldseanp force-pushed the powershell branch 2 times, most recently from 778a293 to 75c9fbb Compare January 11, 2019 23:03

mcdonaldseanp requested review from caseywilliams, ekinanp and branan and removed request for caseywilliams January 11, 2019 23:04

mcdonaldseanp force-pushed the powershell branch from 75c9fbb to f7a82db Compare January 11, 2019 23:08

mcdonaldseanp requested a review from Iristyle January 11, 2019 23:09

mcdonaldseanp force-pushed the powershell branch 5 times, most recently from fdba31a to e9c94c5 Compare January 12, 2019 00:20

glennsarti reviewed Jan 12, 2019

View reviewed changes

manifests/windows/install.pp Outdated Show resolved Hide resolved

glennsarti reviewed Jan 12, 2019

View reviewed changes

spec/classes/puppet_agent_windows_install_spec.rb Outdated Show resolved Hide resolved

glennsarti reviewed Jan 12, 2019

View reviewed changes

templates/install_puppet.ps1.erb Show resolved Hide resolved

glennsarti reviewed Jan 12, 2019

View reviewed changes

templates/install_puppet.ps1.erb Outdated Show resolved Hide resolved

glennsarti reviewed Jan 12, 2019

View reviewed changes

glennsarti suggested changes Jan 12, 2019

View reviewed changes

ekinanp reviewed Jan 12, 2019

View reviewed changes

mcdonaldseanp force-pushed the powershell branch 9 times, most recently from 9e3164f to 6aba3a6 Compare January 15, 2019 00:53

glennsarti approved these changes Jan 15, 2019

View reviewed changes

mcdonaldseanp force-pushed the powershell branch 4 times, most recently from 7acc4ba to 122896c Compare January 15, 2019 21:56

ekinanp approved these changes Jan 15, 2019

View reviewed changes

mcdonaldseanp force-pushed the powershell branch from 122896c to abd4c6a Compare January 15, 2019 22:50

(MODULES-8431) Update windows installation to use powershell

dd14ad0

This commit updates the installation method on windows to use a powershell script rather than a batch file

mcdonaldseanp force-pushed the powershell branch from abd4c6a to dd14ad0 Compare January 16, 2019 00:03

glennsarti reviewed Jan 16, 2019

View reviewed changes

glennsarti approved these changes Jan 16, 2019

View reviewed changes

ekinanp merged commit 4867935 into puppetlabs:master Jan 16, 2019

Iristyle reviewed Jan 18, 2019

View reviewed changes

Iristyle reviewed Jan 19, 2019

View reviewed changes

Iristyle reviewed Jan 24, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(MODULES-8431) Update windows installation to use powershell #362

(MODULES-8431) Update windows installation to use powershell #362

mcdonaldseanp commented Jan 11, 2019

branan commented Jan 11, 2019

mcdonaldseanp commented Jan 11, 2019

puppetcla commented Jan 12, 2019

glennsarti Jan 12, 2019 •

edited

ekinanp Jan 12, 2019

glennsarti Jan 12, 2019

glennsarti Jan 12, 2019

glennsarti Jan 12, 2019

ekinanp Jan 12, 2019

glennsarti Jan 14, 2019

glennsarti Jan 12, 2019

mcdonaldseanp Jan 14, 2019

glennsarti Jan 12, 2019 •

edited

mcdonaldseanp Jan 14, 2019

glennsarti Jan 15, 2019

glennsarti left a comment

ekinanp commented Jan 12, 2019

ekinanp Jan 12, 2019 •

edited

glennsarti Jan 14, 2019

ekinanp Jan 12, 2019

mcdonaldseanp Jan 14, 2019

glennsarti commented Jan 15, 2019

ekinanp Jan 15, 2019 •

edited

ekinanp Jan 15, 2019

mcdonaldseanp commented Jan 15, 2019

glennsarti Jan 16, 2019

glennsarti left a comment

glennsarti commented Jan 16, 2019

Iristyle Jan 18, 2019

Iristyle Jan 18, 2019

Iristyle Jan 18, 2019

Iristyle Jan 18, 2019

Iristyle Jan 19, 2019

Iristyle Jan 24, 2019

(MODULES-8431) Update windows installation to use powershell #362

(MODULES-8431) Update windows installation to use powershell #362

Conversation

mcdonaldseanp commented Jan 11, 2019

branan commented Jan 11, 2019

mcdonaldseanp commented Jan 11, 2019

puppetcla commented Jan 12, 2019

glennsarti Jan 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti Jan 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti left a comment

Choose a reason for hiding this comment

ekinanp commented Jan 12, 2019

ekinanp Jan 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti commented Jan 15, 2019

ekinanp Jan 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdonaldseanp commented Jan 15, 2019

Choose a reason for hiding this comment

glennsarti left a comment

Choose a reason for hiding this comment

glennsarti commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti Jan 12, 2019 •

edited

glennsarti Jan 12, 2019 •

edited

ekinanp Jan 12, 2019 •

edited

ekinanp Jan 15, 2019 •

edited