Skip to content
This repository was archived by the owner on Aug 23, 2024. It is now read-only.

Failover cluster support and code refactoring#3

Open
gigi81 wants to merge 13 commits into
SNikalaichyk:devfrom
gigi81:cluster
Open

Failover cluster support and code refactoring#3
gigi81 wants to merge 13 commits into
SNikalaichyk:devfrom
gigi81:cluster

Conversation

@gigi81
Copy link
Copy Markdown

@gigi81 gigi81 commented Sep 9, 2016

Hi @SNikalaichyk
This PR fixes issue #2.
I know it's a lot of changes and probably too much to take in for a single PR.
Anyway I had to change a few things to add support for the failover cluster and as long as I was at it I felt like also having some code improvements.
Have a look and let me know your thoughts.

@SNikalaichyk
Copy link
Copy Markdown
Owner

Hi @gigi81!
Wow, thank you for doing this! I am currently quite busy at work, but I will try to review this PR as soon as possible. We need to be sure there are no regressions. I am also planning to add Unit and Integration tests.

@gigi81
Copy link
Copy Markdown
Author

gigi81 commented Sep 9, 2016

Yes I agree. We need to make sure there are no regression. Anyway now that the functions are separate from the DSC resources, it should be easier to test them.
I'm also currently deploying the module to our test environment so I will test the cluster part and update the PR if any issue.

@SNikalaichyk
Copy link
Copy Markdown
Owner

I don't really like the idea of putting Invoke-Command inside of each helper function.
Let's think of the following approach:

if ($Cluster)
{
    $Session = New-cMsmqSession -Cluster $Cluster
}

if ($Session)
{
    Invoke-Command -ScriptBlock ${Function:Get-cMsmqQueue} -ArgumentList $Name -Session $Session
}
else
{
    Get-cMsmqQueue -Name $Name
}

@gigi81
Copy link
Copy Markdown
Author

gigi81 commented Sep 9, 2016

I liked the idea of having the option of calling Get-cMsmqQueue (and others) with the -Cluster parameter from the same PS session and don't worry about how it works behind the scenes.
If we removed the Invoke-Command from the functions, than the user of the function will need to create the session. It can definitely work for the DSC resources point of view, but, as I said before, it would be a little uncomfortable to use directly.

@SNikalaichyk
Copy link
Copy Markdown
Owner

@gigi81, emailed you.

@SNikalaichyk
Copy link
Copy Markdown
Owner

SNikalaichyk commented Sep 9, 2016

What happens if we just set the following environmental variables?

$Cluster = 'TestMsmqCluster'

if ($Cluster)
{
    $env:COMPUTERNAME = $ClusterName
    $env:_Cluster_Network_Hostname_ = $ClusterName
    $env:_Cluster_Network_Name_ = $ClusterName
}

Is creation of a new PSSession required at all?

From my perspective, a new PSSession should be optional and only be created for clustered MSMQ instances.

I'm looking at this from the DSC standpoint and want to avoid overcomplications.
I'd also want the next PS Gallery release of this module to be as close as possible to the HQRM guidelines.

@SNikalaichyk
Copy link
Copy Markdown
Owner

Hi, I've been quite busy at work recently. I will review this request as soon as I have some spare time.

@gigi81
Copy link
Copy Markdown
Author

gigi81 commented Oct 13, 2016

Hi @SNikalaichyk,
I found an issue with the PR when using the code (locally) on a machine where ps remoting is not enabled. As long as all the command use Invoke-Command now, they will not work if remoting is not enabled on the local machine.
I plan to push some further changes to address this issue but it will take me some time. I will let you know as soon as I have some code.

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.

2 participants