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.

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

Duplicate of Abandoned PR #101: Handle FaultException race condition during domain creation & reboots. (Fixes #73) #137

Merged
merged 4 commits into from
Jan 11, 2017

Conversation

kwirkykat
Copy link
Contributor

@kwirkykat kwirkykat commented Jan 11, 2017

This is the same code that was submitted by @slapointe in #101 with the merge conflicts in the README fixed.
Azure is asking for these fixes for one of their templates that the DSC extension uses.
@TravisEz13 What tests are needed for this PR to go through?


This change is Reviewable

@kwirkykat kwirkykat requested a review from TravisEz13 January 11, 2017 21:23
@kwirkykat kwirkykat changed the title Duplicate of Abandonned PR #101: Handle FaultException race condition during domain creation & reboots. (Fixes #73) Duplicate of Abandoned PR #101: Handle FaultException race condition during domain creation & reboots. (Fixes #73) Jan 11, 2017
[Parameter(Mandatory)]
[String] $DomainName
)
Join-Path $Env:TEMP ('{0}.xADDomain.completed' -f $DomainName)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be in temp or the resource cache? Resource cache is automatically cleared when a configuration is updated. Temp of course is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have left it as temp because if the resource is called again and a domain promotion has already occurred, the resource should not attempt to promote the machine to a domain controller again. The second attempt to promote the domain controller would throw the same error this code is trying to fix.
I have added a comment to the code explaining this and providing the resource cache location if it is needed in the future.

if($domainShouldExist) {
$retries++
Write-Verbose ($localizedData.RetryingGetADDomain -f $retries, $maxRetries, $retryIntervalInSeconds)
Sleep -Seconds ($retries * $retryIntervalInSeconds)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use an alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -245,6 +245,8 @@ Setting an ODJ Request file path for a configuration that creates a computer acc
### Unreleased

* xAdDomainController: Update to complete fix for SiteName being required field.
* xADDomain: Added retry logic to prevent FaultException to crash in Get-TargetResource on subsequent reboots after a domain is created because the service is not yet running. This error is mostly occur when the resource is used with the DSCExtension on Azure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Either add tests or add an issue to follow up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since multiple contributors in #73 have confirmed that this fix works, I will skip adding tests for now and open an issue to add tests for this fix later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants