xVMSwitch: Adding new capability to enable Iov on the virtual switch

[dcuomo]

Pull Request (PR) description

This PR enables a new IovEnabled capability to resolve Issue #165. IovEnabled is required for advanced scenarios like Guest RDMA. It is also a virtual switch best practice (according to MSFT Networking team) to create the virtual switch with IovEnabled $true e.g. New-VMSwitch -EnableIov $true (note: the property on the VMSwitch is IovEnabled however the parameter to create the VMSwitch is -EnableIov)

This Pull Request (PR) fixes the following issues

• Fixes HyperVDsc: should support IovEnabled property #165

Task list

• Added an entry under the Unreleased section of the change log in the CHANGELOG.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 

[codecov-io]

Codecov Report

Merging #166 into dev will increase coverage by <1%.
The diff coverage is 90%.

@@         Coverage Diff          @@
##            dev   #166    +/-   ##
====================================
+ Coverage    76%    77%   +<1%     
====================================
  Files        11     11            
  Lines      1284   1294    +10     
====================================
+ Hits        988    997     +9     
- Misses      296    297     +1

[bgelens]

Hi @dcuomo, thanks for your contribution!

Please fix the failing Pester tests so we can start a review.

Could give a link to the best practice? Atm, I think I would prefer to have it disabled by default (following the New-VMSwitch cmdlet implementation) so this won't be a breaking change (people who need it, would enable explicitly)

[dcuomo]

Hi @bgelens -

From the output in the failing tests, there's only one that I believe my changes would have affected (Should run without exception while re-creating the VMSwitch). Is there a hint you could give as to why the bandwidth reservation tests are failing?

I'm also having difficulty reviewing why the Mock for VMSwitch is failing A parameter cannot be found that matches parameter name 'EnableIov' at C:\projects\xhyper-v\Tests\Unit\MSFT_xVMSwitch_Id.Tests.ps1: line 265 - I've verified that the resource works on a local system. Is this a test failure that would need to be updated? Appreciate the help...

I'm fine changing the default for Iov to false. It is our best practice albeit not yet documented because it's destructive to fix it later and there's no negative to enabling it. This is a legacy design that has not been updated in the cmdlet.

[bgelens]

When I find some time I'll see if I can help you pinpoint the issues. Hopefully somewhere this week

[bgelens]

I was able to have all test errors resolved:

I found that MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1 should be updated at line 86. I got the test to pass by having the hashtable look like:

$mockedVMSwitch = @{
    Name = $Name
    SwitchType = 'External'
    AllowManagementOS = $AllowManagementOS
    NetAdapterInterfaceDescription = 'Microsoft Network Adapter Multiplexor Driver'
    IovEnabled = $false
}

MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1 should be updated at line 300 to pass the tests. I got the test to pass by having the hashtable look like:

$global:mockedVMSwitch = @{
    Name = "TestSwitch"
    SwitchType = "External"
    AllowManagementOS = $true
    EmbeddedTeamingEnabled = $true
    Id = [Guid]::NewGuid()
    NetAdapterInterfaceDescriptions = @("Microsoft Network Adapter Multiplexor Driver #1", "Microsoft Network Adapter Multiplexor Driver #2")
    IovEnabled = $false
}

MSFT_xVMSwitch_Id.Tests.ps1 should be updated at line 79 and 147 to pass the tests.

At line 79 a hashtable update should look like this:

$mockedVMSwitch = @{
    Name                            = $Name
    SwitchType                      = 'External'
    AllowManagementOS               = $true
    EmbeddedTeamingEnabled          = $true
    LoadBalancingAlgorithm          = 'HyperVPort'
    BandwidthReservationMode        = 'Default'
    NetAdapterInterfaceDescriptions = @("Microsoft Network Adapter Multiplexor Driver #1", "Microsoft Network Adapter Multiplexor Driver #2")
    IovEnabled                      = $false
}

At line 147, an additional parameter should be included:

Param(
    [Parameter()]
    [String]
    $Name,

    [Parameter()]
    [String[]]
    $NetAdapterName,

    [Parameter()]
    [String]
    $MinimumBandwidthMode,

    [Parameter()]
    [bool]
    $AllowManagementOS,

    [Parameter()]
    [String]
    $SwitchType,

    [Parameter()]
    [bool]
    $EnableEmbeddedTeaming,

    [Parameter()]
    [Guid]
    $Id,

    [Parameter()]
    [Boolean]
    $EnableIov
)

Hope you have enough info to continue working on your PR :) I will expect some additional tests to be included with the PR as well to validate the newly implemented logic (code paths, expected results).

On a side note: I found that at line 289 of MSFT_xVMSwitch.psm1 some style issues are present which should be fixed as well.

Current:

If ($switch.IovEnabled -ne $IovEnabled) {
    Write-Verbose -Message ($LocalizedData.IovEnabledIncorrect -f $Name)
    $removeReaddSwitch = $true
}

Should look like:

if ($switch.IovEnabled -ne $IovEnabled)
{
    Write-Verbose -Message ($LocalizedData.IovEnabledIncorrect -f $Name)
    $removeReaddSwitch = $true
}

Please note that I did not validate your actual contribution to be correct yet. I need a clean test result to start doing that.

If you need more help, please reach out!

[bgelens]

I'm not sure, is this ready for review now? If so, could you add some tests for your new functionality and update the changelog please

[dcuomo]

I implemented your recommendations and it passed all checks - Please review. I'll update the changelog.

[bgelens]

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @dcuomo)

---

CHANGELOG.md, line 13 at r4 (raw file):

    * [PR #67](https://github.com/PowerShell/xHyper-V/pull/67), Fixes [Issue #145](https://github.com/PowerShell/xHyper-V/issues/145).
  * Fixed Get throws error when NetworkAdapters are not attached or missing properties.
  * Added new option to enable Iov support on the VMSwitch

This should be moved to a MSFT_xVMSwitch item. E.g.

• MSFT_xVMSwitch
  • Added Iov support

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 289 at r4 (raw file):

            }

            If ($switch.IovEnabled -ne $IovEnabled)

This will also disable IOV when not explicitly defined which is a big change compared to previous behavior. In this case, an existing VMSwitch which was manually enabled could end up being recreated with the setting disabled. Maybe it's better to have $PSBoundParameters.ContainsKey('IovEnabled') included in the evaluation

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 291 at r4 (raw file):

            If ($switch.IovEnabled -ne $IovEnabled)
            {
                Write-Verbose -Message ($LocalizedData.IovEnabledIncorrect -f $Name)

what is incorrect, it is now disabled and should be enabled? Please include this in the message

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 302 at r4 (raw file):

                $parameters["Name"] = $Name
                $parameters["NetAdapterName"] = $NetAdapterName
                $parameters["EnableIov"] = $IovEnabled

Same as previous comment. It's probably best to have $PSBoundParameters.ContainsKey('IovEnabled')evaluation surrounding the assignment. E.g.

if ($PSBoundParameters.ContainsKey('IovEnabled'))
{
    $parameters["EnableIov"] = $IovEnabled
}

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 353 at r4 (raw file):

            $parameters = @{}
            $parameters["Name"] = $Name
            $parameters["EnableIov"] = $IovEnabled

Here it is fine as it only applies to new switches

---

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 628 at r4 (raw file):

                    }

                    Write-Verbose -Message ($LocalizedData.CheckIovEnabled -f $Name)

Probably best to only evaluate when set by the user by checking psboundparameters

---

DSCResources/MSFT_xVMSwitch/en-us/MSFT_xVMSwitch.strings.psd1, line 12 at r4 (raw file):

    CheckIovEnabled = Checking switch '{0}' IovEnabled ...
    IovEnabledCorrect = Switch '{0}' IovEnabled is correctly set
    IovEnabledIncorrect = Switch '{0}' IovEnabled property is not correct

Switch '{0}' IovEnabled is set to {1} but should be {2}

[bgelens]

Thanks for all the hard work! Please see the comments I made.

Could you also include some tests to verify the logic you introduced is working as expected?
Also, please update the README with the new option for xVMSwitch

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[johlju]

We have renamed the resource, removing 'x', so please rebase this PR.