Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

Improve speed of Test-IsNanoServer function #154

Merged
merged 10 commits into from
Jun 3, 2019

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented May 24, 2019

Pull Request (PR) description

Currently, the Test-IsNanoServer function calls the Get-ComputerInfo cmdlet to check with the computer is running Nano server or not. Get-ComputerInfo can take 40+ seconds to run each time. This function is called for each user and group resource within a Dsc configuration and therefore can make the Dsc application very slow.

This PR replaces Get-ComputerInfo with checking the registry value at HKLM:\Software\Microsoft\Windows NT\CurrentVersion\Server\ServerLevels\NanoServer, as documented here: https://msdn.microsoft.com/en-us/library/windows/desktop/hh846315(v=vs.85).aspx

This Pull Request (PR) fixes the following issues

None

Task list

  • Added an entry under the Unreleased section of the change log in the README.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (dev@3dcb7f6). Click here to learn what that means.
The diff coverage is 71%.

Impacted file tree graph

@@         Coverage Diff          @@
##             dev   #154   +/-   ##
====================================
  Coverage       ?    83%           
====================================
  Files          ?     19           
  Lines          ?   2760           
  Branches       ?      4           
====================================
  Hits           ?   2303           
  Misses         ?    453           
  Partials       ?      4

@X-Guardian
Copy link
Contributor Author

MSI Integration tests are still failing:

Executing script C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.EndToEnd.Tests.ps1

  Describing MsiPackage End to End Tests

     Context Install Msi package from HTTPS Url
      [-] Should compile and run configuration 2.9s
        Expected no exception to be thrown, but an exception "PowerShell DSC resource MSFT_MsiPackage  failed to execute Set-TargetResource functionality with error message: The variable '$responseStream' cannot be retrieved because it has not been set. " was thrown from C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.EndToEnd.Tests.ps1:565 char:25
            + ...             Start-DscConfiguration -Path $TestDrive -ErrorAction 'Sto ...
            +                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~.
        566:                     } | Should Not Throw
        at <ScriptBlock>, C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.EndToEnd.Tests.ps1: line 562
      [-] Should return true from Test-TargetResource with the same parameters after configuration 145ms
        Expected $true, but got $false.
        579:                 MSFT_MsiPackage\Test-TargetResource @msiPackageParameters | Should Be $true
        at <ScriptBlock>, C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.EndToEnd.Tests.ps1: line 579
      [-] Package should exist on the machine 101ms
        Expected $true, but got $false.
        583:                 Test-PackageInstalledById -ProductId $script:packageId | Should Be $true
        at <ScriptBlock>, C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.EndToEnd.Tests.ps1: line 583

    Context Uninstall Msi package from HTTPS Url
      [-] Should return False from Test-TargetResource with the same parameters before configuration 117ms
        Expected $false, but got $true.
        604:                 $testTargetResourceInitialResult | Should Be $false
        at <ScriptBlock>, C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.EndToEnd.Tests.ps1: line 604
      [-] Package should exist on the machine before configuration is run 106ms
        Expected $true, but got $false.
        618:                 Test-PackageInstalledById -ProductId $script:packageId | Should Be $true
        at <ScriptBlock>, C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.EndToEnd.Tests.ps1: line 618

Executing script C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.Integration.Tests.ps1

  Describing MSFT_MsiPackage Integration Tests

    Context Set-TargetResource
      [-] Should correctly install and remove a package from a HTTPS URL 32.39s
        RuntimeException: The variable '$responseStream' cannot be retrieved because it has not been set.
        at Set-TargetResource, C:\projects\psdscresources\DSCResources\MSFT_MsiPackage\MSFT_MsiPackage.psm1: line 272
        at <ScriptBlock>, C:\projects\psdscresources\Tests\Integration\MSFT_MsiPackage.Integration.Tests.ps1: line 233

@mhendric
Copy link
Contributor

MSI Integration tests are still failing:

Crap... I was worried that would be the case. Sorry for making you wait a month for a fix that didn't actually work (#143). Moving forward though, I think we need to get some better instrumentation into this failing code. Especially given the intermittent nature of this issue, and the fact that it always pops up for certain PR's, even if they don't appear to touch that code, but never pops up for others.

I have a couple ideas on how to proceed here, but wanted to get your opinion:

  1. I can submit a separate PR that adds some instrumentation, and then you can rebase and we'll see if it solves the issue. If not, keep repeating until it does.

  2. Since this particular code change seems to always trigger this issue, I can take over this code change (probably via a new PR), add instrumentation, and keep working on it myself until I can get the failures to go away.

Let me know which route you'd prefer. I'm thinking number 2 would probably be the most efficient, but I'm fine going either way.

@mhendric
Copy link
Contributor

Forgot option 3: YOU add some instrumentation and troubleshoot this yourself.

I'm totally fine if you want to volunteer to take that on, but I'm definitely not asking you to.

@X-Guardian
Copy link
Contributor Author

Happy for you to take over this PR @mhendric. Good luck!

@mhendric
Copy link
Contributor

Alright, sounds good. I've opened Issue #155 to track this. I'll probably work on the PR offline before attempting to submit, but will submit as soon as tests appear to be passing.

Copy link
Contributor

@mhendric mhendric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mhendric mhendric merged commit ca2572f into PowerShell:dev Jun 3, 2019
@mhendric
Copy link
Contributor

mhendric commented Jun 3, 2019

Hey @X-Guardian , I was finally able to get tests passing, and your change looks good. Thanks for your contribution and your patience on this one.

@X-Guardian X-Guardian deleted the Test-IsNanoServer branch June 21, 2019 22:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants