xCluster: add Nodes parameter (enables cluster creation with several nodes)

[woodp]

Pull Request (PR) description
Adds a "Nodes" string array parameter, specifying node names that will be added to the cluster along with the local computer at creation time (i.e passed to the New-Cluster command).

This Pull Request (PR) fixes the following issues:
#148, #157
The motivation behind this PR is that I was unable to join a secondary node to the cluster, tried both from the secondary node and from the primary node. The only way I had to achieve a cluster with more than one node was to specify all the nodes in the creation (using New-Cluster command and passing all the nodes into the -Node parameter).

Task list:
Will complete the task list if this PR makes sense to the owners.

---

This change is 

[codecov-io]

Codecov Report

Merging #177 into dev will decrease coverage by 3%.
The diff coverage is 51%.

@@         Coverage Diff         @@
##            dev   #177   +/-   ##
===================================
- Coverage   100%    96%   -4%     
===================================
  Files         8      8           
  Lines       486    500   +14     
===================================
- Hits        486    482    -4     
- Misses        0     18   +18

[johlju]

Great work on this one! This looks good. It doesn't break the previous behavior. I'm good with this change. Please continue with the task list. 😄
I made a few comments, please see if they need to be addressed as well.

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 46 at r1 (raw file):

        [Parameter()]
        [System.String[]]
        $Nodes,

This parameter is not needed here since it is non-mandatory and not used in the Get-TargetResource function.
Please see https://github.com/PowerShell/DscResources/blob/master/BestPractices.md#get-targetresource-should-not-contain-unused-non-mandatory-parameters

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 202 at r1 (raw file):

            if($Nodes -ne $null)
            {
                $AllNodes += $Nodes

If $Nodes already contains $env:COMPUTERNAME this add a duplicate? 🤔
Throughout.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 249 at r1 (raw file):

$list

Would you mind renaming this variable to $existingNodes or something similar if there is a better descriptive name for this instead of `list'? This is to align with the comment for the same thing in the other function.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 258 at r1 (raw file):

                        Write-Verbose -Message ($script:localizedData.RemoveOfflineNodeFromCluster -f $targetNodeName, $Name)

                        Remove-ClusterNode -Name $targetNodeName -Cluster $Name -Force

In Test-TargetResource break was used to exit the foreach-loop. Is that appropriate here too?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 379 at r1 (raw file):

$list

Would you mind renaming this variable to $existingNodes or something similar if there is a better descriptive name for this instead of `list'.?

---

Comments from Reviewable

[woodp]

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 46 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This parameter is not needed here since it is non-mandatory and not used in the Get-TargetResource function.
Please see https://github.com/PowerShell/DscResources/blob/master/BestPractices.md#get-targetresource-should-not-contain-unused-non-mandatory-parameters

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 202 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If $Nodes already contains $env:COMPUTERNAME this add a duplicate? 🤔
Throughout.

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 249 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$list

Would you mind renaming this variable to $existingNodes or something similar if there is a better descriptive name for this instead of `list'? This is to align with the comment for the same thing in the other function.

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 258 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

In Test-TargetResource break was used to exit the foreach-loop. Is that appropriate here too?

I don't think so, because we need to go through all of the nodes in the Set

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 379 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$list

Would you mind renaming this variable to $existingNodes or something similar if there is a better descriptive name for this instead of `list'.?

Done.

---

Comments from Reviewable

[woodp]

I addressed your points, updated the mof file, fixed some styling errors and added a unit test.
Nevertheless I get several failing tests, including the one I added, and I'm unable to figure out why they are failing, I need some guidance on this.
Also appveyor complaints about some invalid characters used, I'm using "@()" to initialize an array. Should I be using a different way to create an empty array ?

[woodp]

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 258 at r1 (raw file):

Previously, woodp (Pedro Wood) wrote…

I don't think so, because we need to go through all of the nodes in the Set

You are right, now is done

---

Comments from Reviewable

[woodp]

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 258 at r1 (raw file):

Previously, woodp (Pedro Wood) wrote…

You are right, now is done

Done.

---

Comments from Reviewable

[johlju]

Sorry for being away so long for this PR. It has been a lot of work at the day job and maintaining in other resources modules. But I will focus on getting this through now. 😄

Found a few more review comments for this one.

---

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
There are failing tests for the Set-TargetResource. Can you look them over and see why they are failing with this code change?

https://ci.appveyor.com/project/PowerShell/xfailovercluster/build/1.6.342.0#L474

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 222 at r3 (raw file):

Get-All-Nodes($Nodes)

Please change this to named arguments Get-AllNodes -Nodes $Nodes. Throughout.

Also see other comment for renaming this function.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 222 at r3 (raw file):

$AllNodes

Please rename this to use camelCase syntax ($allNodes lower case 'a'). Throughout.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 233 at r3 (raw file):

if(

Please add a space between the if-statement and the parenthesis

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 365 at r3 (raw file):

foreach(

Please add a space between the foreach-statement and the parenthesis

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 376 at r3 (raw file):

                        {
                            Write-Verbose -Message ($script:localizedData.ClusterNodeIsDown -f $targetNodeName, $Name)
                            $returnValue = $false

Should this be $found = $false? For the evaluation on row 383 to be true?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 383 at r3 (raw file):

if(

Please add a space between the if-statement and the parenthesis

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 530 at r3 (raw file):

Get-All-Nodes

Could we use the Verb-Noun syntax for this? So for this function name it would be Get-AllNodes instead?

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 543 at r3 (raw file):

if(

Please add a space between the if-statement and the parenthesis

---

Comments from Reviewable

[johlju]

I did a typo that could have been seen as strange, what I meant to say in previous review comment was that I will focus on getting this through now. Let me know if you need any help 😄

[woodp]

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 376 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should this be $found = $false? For the evaluation on row 383 to be true?

not sure about this one

---

Comments from Reviewable

[woodp]

Review status: 1 of 3 files reviewed at latest revision, 9 unresolved discussions.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 222 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Get-All-Nodes($Nodes)

Please change this to named arguments Get-AllNodes -Nodes $Nodes. Throughout.

Also see other comment for renaming this function.

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 222 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$AllNodes

Please rename this to use camelCase syntax ($allNodes lower case 'a'). Throughout.

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 233 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if(

Please add a space between the if-statement and the parenthesis

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 365 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

foreach(

Please add a space between the foreach-statement and the parenthesis

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 383 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if(

Please add a space between the if-statement and the parenthesis

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 530 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Get-All-Nodes

Could we use the Verb-Noun syntax for this? So for this function name it would be Get-AllNodes instead?

Done.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 543 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if(

Please add a space between the if-statement and the parenthesis

Done.

---

Comments from Reviewable

[johlju]

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 376 at r3 (raw file):

Previously, woodp (Pedro Wood) wrote…

not sure about this one

LGTM. This is good as-is. Was me looking at it wrongly. Thanks for correcting me.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 322 at r2 (raw file):

Assert-MockCalled -CommandName New-Cluster -Exactly -Times 1 -Scope It -ParameterFilter {
$Nodes -eq 'foo' -and
$Nodes -eq 'bar'
}

I liked this ParameterFilter to assert that the call to New-Cluster cmdlet was the correct one. But maybe this can work for the parameter filter?

Assert-MockCalled -CommandName New-Cluster -Exactly -Times 1 -Scope It -ParameterFilter {
    $Node.Contains($env:COMPUTERNAME) -eq $true `
    -and $Node.Contains('foo') -eq $true `
    -and $Node.Contains('bar) -eq $true
}

---

Comments from Reviewable

[woodp]

Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 376 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

LGTM. This is good as-is. Was me looking at it wrongly. Thanks for correcting me.

Done.

---

Comments from Reviewable

[woodp]

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

Tests/Unit/MSFT_xCluster.Tests.ps1, line 322 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Assert-MockCalled -CommandName New-Cluster -Exactly -Times 1 -Scope It -ParameterFilter {
$Nodes -eq 'foo' -and
$Nodes -eq 'bar'
}

I liked this ParameterFilter to assert that the call to New-Cluster cmdlet was the correct one. But maybe this can work for the parameter filter?

Assert-MockCalled -CommandName New-Cluster -Exactly -Times 1 -Scope It -ParameterFilter {
    $Node.Contains($env:COMPUTERNAME) -eq $true `
    -and $Node.Contains('foo') -eq $true `
    -and $Node.Contains('bar) -eq $true
}

Done.

---

Comments from Reviewable

[woodp]

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

There are failing tests for the Set-TargetResource. Can you look them over and see why they are failing with this code change?

https://ci.appveyor.com/project/PowerShell/xfailovercluster/build/1.6.342.0#L474

done

---

Comments from Reviewable

[woodp]

Tests/Unit/MSFT_xCluster.Tests.ps1, line 322 at r2 (raw file):

Previously, woodp (Pedro Wood) wrote…

Done.

Done.

---

Comments from Reviewable

[johlju]

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

a discussion (no related file):
Please add the Nodeparameter to the resource in the README.md.

See other comment of renaming Nodes to Node.

---

a discussion (no related file):
Please update the Unreleased section in the CHANGELOG.md with an entry explaining this change.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 83 at r5 (raw file):

    }

    @{

We should return the existing node names in the property Node in this hash table.

See other comment of renaming Nodes to Node.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 98 at r5 (raw file):

Nodes

I suggest we rename this to Node singular, because this can be set to one or more. It aligns with the other parameters. Throughout.

---

DSCResources/MSFT_xCluster/MSFT_xCluster.psm1, line 539 at r5 (raw file):

    $allNodes = @()
    $allNodes += $env:COMPUTERNAME

A question. It is not really clear in the code that $Nodes will always default to $env:COMPUTERNAME, so I thought of the following.

Would it work to add $env:COMPUTERNAME as the default value for $Nodes parameter, and remove the entry from here? Wouldn't the user specify the current target node and any other nodes in this parameter, and if it is not assigned it will be set to the default value$env:COMPUTERNAME? If so, is this helper function necessary?

If wer add the value as default value to the $Nodes parameter, we also need to update the parameter description in README.md, comment-based help and schema.mof.

---

Comments from Reviewable

[johlju]

Labeling this 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 the PR is taken up again.

[woodp]

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

The tests are failing, can you look and see if you can resolve them?

I'm not being able to figure out why these 2 tests fail. For the first one I always get an exception, if I add the filter for the GetClusterNode mock, I get "StubNotImplemented" otherwise I get an exception without any info at all. Is very difficult with only this info to know why it fails. Also don't seem to get how the mocks/stubs work.

---

[johlju]

The Stubs are used to have something to attach the mock to. A unit test should not need the actual module with the cmdlets installed, instead those are replaced by stubs (empty functions except for the throw you are seeing) so that when using Mock the mock can find the function and get the parameters from the stub.
If the stub is not mocked, or mocked with the wrong parameter filter, the stub throws a not implemented error so that we can know the mock was not called but the actual function (the stub or the cmdlet in the Failover Cluster module).

So in your case, when you get StubNotImplemented the mock is not called.

Did this help you? Let me know if you need more information.

[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]

Closing this PR due to inactivity. If there are someone who wants to work on this issue or PR please open a new PR.