Windows: pkg.install returns failed for msiexec/instmsi exit code 3010 (ERROR_SUCCESS_REBOOT_REQUIRED)

[lorengordon]

Description of Issue/Question

When using pkg.install on Windows with a msiexec or instmsi installer, an exit code of 3010 (ERROR_SUCCESS_REBOOT_REQUIRED) is technically a successful install, but pkg.install returns it as a failure.

[ERROR   ] Command ['c:\\salt\\var\\cache\\salt\\minion\\extrn_files\\base\\xxxx\\4.6.01055\\NDP461-KB3102436-x86-x64-AllOS-ENU.exe', '/q', '/norestart'] failed with return code: 3010

Versions Report

PS C:\Users\Administrator> C:\salt\salt-call.bat --local --versions-report
Salt Version:
           Salt: 2015.8.5

Dependency Versions:
         Jinja2: 2.7.3
       M2Crypto: Not Installed
           Mako: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.7.0
         Python: 2.7.11 (v2.7.11:6d1b6a68f775, Dec  5 2015, 20:40:30) [MSC v.1500 64 bit (AMD64)]
           RAET: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.2
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.4.2
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
        libgit2: Not Installed
        libnacl: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
   python-gnupg: 0.3.7
          smmap: Not Installed
        timelib: Not Installed

System Versions:
           dist:
        machine: AMD64
        release: 2012ServerR2
         system: 2012ServerR2 6.3.9600  Multiprocessor Free

[lorengordon]

Tracing through the code, and I don't understand the reasoning for the error logic. The package is installed using result = __salt__['cmd.run_stdout'](...), which returns only the stdout. But then if result has any non-False equivalent value, the result is logged as an error? Won't that log an error and return the failed dictionary if there is anything at all on stdout? Can someone please help me understand this?

It seems to me that pkg.install should maybe be calling cmd.run_all and parsing the return dictionary?

[lorengordon]

Looks like it was changed from cmd.run to cmd.run_stdout in this commit. Pinging @twangboy...

[damon-atkins]

.

[TheBigBear]

this is also referred to here saltstack/salt-winrepo-ng#141 by @rterbush

[Ch3LL]

@lorengordon thanks for submitting a PR. Its really appreciated! Once it gets merged we will go ahead and close the issue.

[ghost]

Hi @lorengordon the PR looks great, but I wonder if more should be done. If using state.apply, to know if we one of the installs returned the 3010 error code, it looks like we have to search the output, but we shouldn't have to do that.

There may be a much better way to do this using pillars or something (I'm not experienced enough with salt to know), but something we could do would be to use mod_init (the same way it's being used for refresh, so we only have to refresh once) to set some custom windows registry entry specifically for this case. After we'eve applied state are last state formula can run some module that checks for our registry entry, resets the entry and schedules a reboot in x seconds (using the windows task scheduler) if the custom registry entry is set.

The new module should check

1. our custom registry entry and
2. HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Updates\UpdateExeVolatile

(see https://technet.microsoft.com/en-us/library/cc164360(v=exchg.80).aspx)

There's doubtless much more than this that should be done but this is a start.

BTW I've started to write a trivial module that checks
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Updates\UpdateExeVolatile

and schedules a reboot if necessary but I doubt that it's up to the devels standards.

[ghost]

@lorengordon, your PR doesn't need any changing but once it's merged then I think that some work should be done on salt.modules.state and some other appropriate run module to take advantage. alls i'm sayin'

[ghost]

@lorengarden, One thing that I'm not happy about is that there's no way to check if 3010 is meaningful or not. It's only a windows installer code and could mean anything at all under other contexts. It could even hypothetically mean "no reboot required" for some other installer. We can't just check that msiexec is set to true either, because some exe's wrap an msi so the 3010 code is valid in that case as well. What should probably happen is we need to add a new entry to winrepo-ng formulas so we might see something like:

Reboot Exit Codes: 
                 - 3010
                 - 3020

                                                                          "

This way we can avoid hard coding the 3010 (it can still be hardcoded for when msiexec is True however). I don't think that this consideration should stop your PR from being merged but it's another next step.

[lorengordon]

@hrumph, all good points, probably for one or more new issues. 😁

I was focused in the linked PR just on addressing the bug in the return code logic for a point release. Adding new features would typically go into the develop branch first for later inclusion in a new version.

Also, I did consider that 3010 may not be a unique return code for msiexec and instmsi installers. The way the logic worked previously, though, any installer outputting anything on stdout would have been considered a failed install. I searched issues for other mentions of 3010 and didn't find anything relevant to any other types of installers. So it seemed relatively safe to hard code it for now, to fix the problem in a point release without adding a new feature. Of course, if the saltstack team would prefer not to do this, and favors resolving the issue in a new feature in a new version, that is their prerogative.

Shoot, now that I'm more familiar with win_pkg.py, I may try to tackle that myself. :)

[ghost]

@lorengordon and anyone else interested, now that I think about it I think that there should be a default list of reboot exit codes namely the singleton list containing 3010. This will doubtless work with 99% of cases. But it should be overridable with win repo formulas in the manner I described, so we deal with other installers that really do use different exit codes. I've got more idiotic design suggestions pending so everyone brace yourselves.

[ghost]

ok sorry if in advance if i'm too spammy. some of the ideas of been having are a bit more crystallised now. I'll try to cut back on making suggestions for a while if folks are getting annoyed. First of all no change to what i said about having [3010] as a default but we should still make allowance for other codes. This is where I'm getting more specific now.

• Change highstate in salt/salt/modules/state.py as so that a registry key called called "Reboot required" is initialised to 0 in all cases (for windows minions only).
• In the latest and installed functions in salt/salt/states/pkg.py set the "Reboot required" registry key to 1 whenever a reboot exit code is returned (for windows minions only of course).
• Create a new state in pkg.py (applicable to windows minions only) called something like "reboot_if_required". This state will take a whole number paramater which will be the minutes to wait before rebooting. We'll normally want one minute or so so the apply.state can exit cleanly before the reboot starts. It will schedule a reboot using the Windows task scheduler and then exit (the shutdown command allows for a delay and maybe we should use that in stead of the task scheduler but the restart-system command in powershell has no delay paramater so to be future-oriented we should probably use the task scheduler). Needless to say this state will check our new registry key and the existing ones that also should be checked such as

HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Updates\UpdateExeVolatile

should also be checked

and ther'es another one too. If any of these keys indicate that a reboot should be done then the state will schedule the reboot otherwise the state will exit without a reboot.

Invocation of the state should look something like

reboot if necessary:
pkg.reboot_if_necessary:
- delay: 1
- order: last

There are no requirements in the state invocation because we won't assume that anything succeeded. We just want it to go last.

I understand that other folks have much more sophisticated ideas and that's great but I hope that something simple like this will move us forward for the time being.

[TheBigBear]

@hrumph thanks for all the thinking and effort I like it, I really do, but I also have concerns.

I cannot ever see a windows tools such as salt minion being allowed to to an automatic restart, scheduled or not.

As far asI am concerned any and every logic has to allow for a specific state to be applied that says something like, restart this and that machine IF it requires or is waiting for a restart, but for salt to try to do the logic and setup the schedule on it's own, I can't see a production site that will find that acceptable.

I think salt can (and should) go as far as collecting that exit status at end of installers and work on using the standard windows built-in flags in the registry that allow software to flag the fact that a restart is required.

And admins are then free to decide what their site logic and state runs are to be to deal wiht in actually reading this standard windows reboot required flag and acting on it.
But for salt to try making the acting on the info an automatic and integral part of a salt built-in functionality is not acceptable for an enterprise product in my opinion. Sorry, hope that does not discourage you, but inspire ...

[lorengordon]

I think it would be quite valuable for salt to be able to signal that a package required a reboot and execute on that signal. However, that would be a new feature and I'd really recommend it be added as a new issue so it can continue to be tracked after this one is closed by the patch I submitted.

[ghost]

@TheBigBear

But for salt to try making the acting on the info an automatic and integral part of a salt built-in functionality is not acceptable for an enterprise product in my opinion.

Right now, what is perhaps the biggest (don't know this for a fact) third party windows deployment system has this feature and the instructions don't hesitate to tell you to use it. In fact (IIRC) for some reason they have you scheduling reboots on certain installs whether or not they're even required. This may be because of a lack of flexibility with their system (I don't really know enough to say but I nevertheless suspect). IIRC correctly their online examples shows them scheduling reboots for desktops but not for servers. People perhaps want to be more cautious with servers, but server reboots will have to be done anyway sooner or later. I don't want to mention this other third party closed source system by name but if you google you might identify what I'm talking about pretty quickly.

[ghost]

@lorengordon You are right. I am going to start a new issue. Even though I really shouldn't have I invaded your issue I only did it because I wanted to be sure that everyone had a longer term vision in mind before committing to anything.

[lorengordon]

@hrumph, no problem! I don't mind having the discussion here, I just don't want the feature request to get lost! 😀

[Ch3LL]

going to go ahead and close this up since the PR was merged and another issue was opened for further discussion.