(MODULES-3657) Fix waiting on Windows during upgrade #137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The previous checks for PID running uses a combination of tasklist,
findstr and some more cryptic batch-isms to detect if a puppet
agent is currently executing (by PID). This check also relies on
the English string "No tasks are running", because tasklist.exe
doesn't set exitcodes as desired when the query is not a match.
C:\Users\Administrator>tasklist.exe /FI "PID eq 1234" /NH
INFO: No tasks are running which match the specified criteria.
C:\Users\Administrator>echo %ERRORLEVEL%
0
There are a couple of alternative methods for achieving the
desired functionality. The simplest is qprocess.exe:
C:\Users\Administrator>qprocess 1234
No Process exists for 1234
C:\Users\Administrator>echo %ERRORLEVEL%
1
C:\Users\Administrator>qprocess 32936
USERNAME SESSIONNAME ID PID IMAGE
>administrator console 1 32936 mmc.exe
C:\Users\Administrator>echo %ERRORLEVEL%
0
Unfortunately qprocess is no longer available on Nano Server, so
the next best option is wmic.exe, which exists everywhere.
C:\>wmic PATH Win32_Process where handle=1111 get handle /format:textvaluelist |
findstr /I "Handle=1111"
No Instance(s) Available.
C:\>echo %ERRORLEVEL%
1
The string 'Handle' appears everywhere, so no locale specific
string parsing is required on non-English versions of Windows, such
as German:
C:\Users\moses>tasklist /FI "PID eq 1234" /NH
INFORMATION: Es werden keine Aufgaben mit den angegebenen Kriterien ausgeführt.
C:\Users\moses>echo %ERRORLEVEL%
0
- Previously, the batch file would wait for an infinite amount of time for the calling Puppet process to exit, before it would proceed with an upgrade. Prior to rewriting the PID check with wmic, this could cause an infinite loop when trying to match an unlocalized string. Since infinite loops are never a good idea, cap the wait limit to 120 seconds, and log an event log message under Puppet / ID 101 that the agent upgrade failed. This should improve the end user experience under failing circumstances.
Previously the script was using the timeout.exe application however this attempts to redirect STDIN and during a headless operation, for example during an actual puppet run, this throws an error and does not wait the proper time. This commit changes the wait method to use ping. Ping waits 1 second between attempts therefore a simple `ping 127.0.0.1 -n 6` will pause for 5 seconds. This commit also changes the location of the PID file slightly. Previously it used `%TEMP%` however this variable can be removed or be in an unexpected place. Instead the PID file is always placed in the same directory as the script (`%~dp0`). To assist in debugging upgrade failures the install batch file also outputs all environment variables prior to running.
| set AGENT_PID=%1 | ||
| set windowTitle=Puppet Agent Upgrade | ||
| title %windowTitle% | ||
|
|
||
| set pid= | ||
| for /f "tokens=2" %%a in ('tasklist /v ^| findstr /c:"%windowTitle%"') do set pid=%%a | ||
| set pid_path=%TEMP%\puppet_agent_upgrade.pid | ||
| set pid_path=%~dp0puppet_agent_upgrade.pid |
There was a problem hiding this comment.
Hmm... and we're certain this is always writable? And that there are no side effects of writing into this Puppet module template dir (consider PMT install vs pluginsync)?
I think it would be preferable to just set %TEMP% to %windir%\Temp if it's not already set:
IF "%TEMP%"=="" (SET TEMP=%windir%\Temp)There was a problem hiding this comment.
Comment withdrawn. I was mistakenly thinking that ~dp0 is the template directory. Its the location of the install_puppet.bat created from the ERB template, which is known to be the temp directory.
👍 to the change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the script was using the timeout.exe application however this
attempts to redirect STDIN and during a headless operation, for example during
an actual puppet run, this throws an error and does not wait the proper time.
This commit changes the wait method to use ping. Ping waits 1 second between
attempts therefore a simple
ping 127.0.0.1 -n 6will pause for 5 seconds.This commit also changes the location of the PID file slightly. Previously it
used
%TEMP%however this variable can be removed or be in an unexpected place.Instead the PID file is always placed in the same directory as the
script (
%~dp0).To assist in debugging upgrade failures the install batch file also outputs
all environment variables prior to running.
This PR builds on #136 and #135