Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Get-ChocoInstalledPackage is very slow, causing long Test, Set, and Upgrade times for multiple packages #90

Closed
jrdnr opened this issue Jul 18, 2017 · 20 comments
Closed

Get-ChocoInstalledPackage is very slow, causing long Test, Set, and Upgrade times for multiple packages #90

jrdnr opened this issue Jul 18, 2017 · 20 comments
Labels
Enhancement

Comments

@jrdnr
Copy link
Contributor

jrdnr commented Jul 18, 2017

Because cChocoPackageInstallerSet, just provides a foreach overlay for cChocoPackageInstaller it is very slow 1-2 seconds per package due to the time it takes to run choco list -lo

Is there a fundamental reason that cChocoPackageInstaller could not be refactored to allow an array of package names, and then change cChocoPackageInstallerSet to simply be an alias?

This would also address issue #80.
@ferventcoder
Copy link
Member

No reason - was just discussing this with a colleague recently.

@ferventcoder
Copy link
Member

I think refactoring would be a great idea

@ferventcoder
Copy link
Member

As long as it is done in a compatible way that is.

@jrdnr
Copy link
Contributor Author

jrdnr commented Jul 18, 2017

Is this something you want to add as a 2.x milestone possibly replacing #80, or is this just a nice idea?

If no one else is going to be working on it, I'd be glad to try to take a first whack, I've never contributed to a project or worked with AppVeyor before so I'll have a little bit of a learning curve to get over to get used to the tools before I can make any real contribution on the project itself.

@ferventcoder
Copy link
Member

2.x is current, 3.x is next major.

@ferventcoder
Copy link
Member

@jrdnr For contributions, I'd suggest reading Chocolatey's CONTRIBUTING at https://github.com/chocolatey/choco/blob/master/CONTRIBUTING.md#contributing-1. It's very comprehensive and helpful for first time folks to regular contributors alike.

@ferventcoder
Copy link
Member

A couple of things to keep in mind:

  • CLA signature does not apply to cChoco.
  • Setting up your environment is different as well

@jrdnr
Copy link
Contributor Author

jrdnr commented Jul 19, 2017

Ok I'll review the contribution articles

@jrdnr
Copy link
Contributor Author

jrdnr commented Aug 24, 2017

I finally got a chance to sit down and finish getting my head around how everything ties together, and see that a few of my original assumptions were a little off base.

It looks like there are probably a few good reasons (ie exposing a limited set of parameters to cChocoPackageInstallerSet for 1) that there should be both cChocoPackageInstaller vs ..InstallerSet. Also due to the way DSC processes the module the only way to only pull the choco list -lo once per run appears to be to glob all packages into a single instance in the mof file, which does not appear to be an appropriate work around.

The only ways I have been able to come up with to improve the speed of the dsc run would be to directly work on choco list -lo (something that is most likely over my head) or for the dsc resource to cache the output of choco list -lo to a file the first time it runs, and just read it back in from the file when it needs the list.

@ferventcoder would it make sense to update the title on this issue, or close it and open the issue somewhere else?

@ferventcoder
Copy link
Member

Update the title.

@jrdnr jrdnr changed the title cChocoPackageInstallerSet Is very slow Get-ChocoInstalledPackage is very slow, causing long Test, Set, and Upgrade times for multiple packages Aug 25, 2017
@jrdnr
Copy link
Contributor Author

jrdnr commented Aug 25, 2017

Is this an issue we can address in the DSC resource by caching to disk or something along those lines, or should the fix for this really be made in the core of choco list itself?

@ferventcoder
Copy link
Member

@jrdnr choco list -lo is subsecond return for me (150+ packages). But sometimes there are factors on systems that cause it to take up to 2 seconds or more (and not really explainable reasons).

@ferventcoder
Copy link
Member

I think we should work on making it better here.

@ferventcoder
Copy link
Member

More explanation - Puppet and Chef run this query one time before all packages during a run and cache the results for all. I'm actually surprised (well not really) that Microsoft decided to ignore this pattern.

@jrdnr
Copy link
Contributor Author

jrdnr commented Aug 28, 2017

Doing some testing the simplest work around I've found is to add an Export-Clixml to the Get-ChocoInstalledPackage function, then when setting $installedPackages I just check if the cache file exists, and if its less then a minute old I import from the file instead of running the full Get-ChocoInstalledPackage again. On my system this takes 30 reps of setting the $installedPackages variable from 67.44 seconds, down to 2.27 seconds. This is not elegant, and I'm not really sure that storing a list of installed programs in C:\Choco\ is the best way to go, however it appears to do the job.

@ferventcoder
Copy link
Member

Storing to a file could have security implications, would you be able to hold a lock on the file or store it somewhere secure during the run?

@jrdnr
Copy link
Contributor Author

jrdnr commented Aug 29, 2017

Unfortunately coming back around to the initial problem with DSC being stateless, I don't think file locking is an option.

However because DSC runs as System we can easily write to a secure location. My current version tests for and if needed creates a cChoco directory in $env:ProgramData and then exports the choco list -lo object as Clixml.

In the current implementation I'm using last write time to ensure the cached package list was updated less than 60 seconds ago, if its stale or does not exist I re-run choco list -lo and Export-Clixml again to ensure the package list stays up to date.

From limited testing it does not appear as though DSC runs a Test-* after running a Set-* and the default LCM config will run another test for 30 min, The only time I see the 60 second limit potentially being a problem would be if someone were to run a Start-DscConfiguration and immediately follow with a Test-DscConfiguration. If necessary this could be remedied by having InstallPackage, UninstallPackage, and Upgrade-Package each test for and remove the cache file or just setting time to stale to a shorter time period.

jrdnr added a commit to jrdnr/cChoco that referenced this issue Aug 29, 2017
@jrdnr
(chocolateyGH-90) Update Get-ChocoInstalledPackage to Cache to Progra…
ef84fb7 
…mData

Get-ChocoInstalledPackage must run and parse choco list -lo for every package managed by DSC

This commit ensures a cChoco folder in ProgramData and exports the custom object to Clixml, and will simply read it from file if last write time is less that 60 Seconds ago.
@jrdnr
Copy link
Contributor Author

jrdnr commented Sep 1, 2017

@ferventcoder, sorry I'm new to the open source collaboration (and I know you have a life and a day job). I'm just not sure about next steps.

Should I go ahead and submit a pull request now, or do we continue discussing the proposed solution until any potential issues have been addressed and then I submit the PR?

@ferventcoder
Copy link
Member

Go for the PR, we have the basics down and are agreeing on them. Specifics are easily changed.

jrdnr added a commit to jrdnr/cChoco that referenced this issue Sep 1, 2017
@jrdnr
Merge branch 'development' into (chocolateyGH-90)_Cache_InstalledPack…
624a8fb 
…ages_for_faster_run

* development:
  (chocolateyGH-87) Moved to using reduced output
  (maint) formatting
  (chocolateyGH-70) Apply Apache v2 Licensing
  (chocolateyGH-85) Pass the source to cChocoPackageInstallerSet correctly
  (chocolateyGH-44) Pass the Source to choco correctly
  Added version to example
  (chocolateyGH-77) Push to gallery on new tags
  (maint) formatting
  (chocolateyGH-72) PowerShell scripts in format for signing

# Conflicts:
#	DSCResources/cChocoPackageInstall/cChocoPackageInstall.psm1
jrdnr added a commit to jrdnr/cChoco that referenced this issue Sep 1, 2017
@jrdnr
(chocolateyGH-90) Implemented Caching Package list to File
560eb2b 
The Get-ChocoInstalledPackage function pulls a new list from
'choco list -lo -r' costing measurable system impact when choco is
installing several packages.

This commit will allow the Installed package list to be cached locally
in the $env:ChocolateyInstall folder for 60 seconds before pulling a
new list.
jrdnr added a commit to jrdnr/cChoco that referenced this issue Sep 1, 2017
@jrdnr
Merge branch '(chocolateyGH-90)_Cache_InstalledPackages_for_faster_ru…
13a93c5 
…n' into development

* (chocolateyGH-90)_Cache_InstalledPackages_for_faster_run:
  (chocolateyGH-90) Implemented Caching Package list to File
  Resource uses its own static file location
  Fix $path hard coded to C:\
  (chocolateyGH-90) Update Get-ChocoInstalledPackage to Cache to ProgramData
jrdnr added a commit to jrdnr/cChoco that referenced this issue Dec 1, 2017
@jrdnr
(chocolateyGH-90) Update Get-ChocoInstalledPackage to Cache to Progra…
c7c212b 
…mData

Get-ChocoInstalledPackage must run and parse choco list -lo for every package managed by DSC

This commit ensures a cChoco folder in ProgramData and exports the custom object to Clixml, and will simply read it from file if last write time is less that 60 Seconds ago.
jrdnr added a commit to jrdnr/cChoco that referenced this issue Dec 1, 2017
@jrdnr
(chocolateyGH-90) Implemented Caching Package list to File
249faf4 
The Get-ChocoInstalledPackage function pulls a new list from
'choco list -lo -r' costing measurable system impact when choco is
installing several packages.

This commit will allow the Installed package list to be cached locally
in the $env:ChocolateyInstall folder for 60 seconds before pulling a
new list.

# Conflicts:
#	DSCResources/cChocoPackageInstall/cChocoPackageInstall.psm1
jrdnr added a commit to jrdnr/cChoco that referenced this issue Dec 1, 2017
@jrdnr
(chocolateyGH-90) Cache choco list output
8542f18 
Previously Get-ChocoInstalledPackage function pulled a new list from
'choco list -lo -r' costing measurable system impact when choco was
installing several packages.

This allows the installed package list to be cached locally
in the $env:ChocolateyInstall\cache folder for 60 seconds before pulling a
new list. Also enabled purging the Cache when a package is changed.
ferventcoder added a commit that referenced this issue Dec 1, 2017
@ferventcoder
Merge pull request #92 from jrdnr/(GH-90)_Cache_InstalledPackages_for…
b20d1e4 
…_faster_run

(GH-90) Implemented Caching Package list to File
@jrdnr
Copy link
Contributor Author

jrdnr commented Dec 3, 2017

@ferventcoder This issue can be closed now correct?

@jrdnr jrdnr closed this as completed Dec 12, 2017
pauby added a commit that referenced this issue Nov 13, 2018
@pauby
Merge branch 'development'
7113f4c 
* development:
  (build) Updated module version
  (GH-114) Fix typo
  (GH-114) Skip verifying Pester publisher check
  [#105] Chocolatey params passed in when upgrading a package.
  Choco Run Errors out because of Null Source
  (GH-90) Cache choco list output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ferventcoder @jrdnr @pauby