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

WaitForADDomain: Handle timeout better #343

Closed
johlju opened this issue Jun 7, 2019 · 4 comments · Fixed by #455
Closed

WaitForADDomain: Handle timeout better #343

johlju opened this issue Jun 7, 2019 · 4 comments · Fixed by #455
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.

Comments

@johlju
Copy link
Member

johlju commented Jun 7, 2019

The current code uses a for-loop to wait for the domain to be accessible.

https://github.com/PowerShell/xActiveDirectory/blob/44ca75e062beff431cf41e1efb89d71bc4a5d044/DSCResources/MSFT_xWaitForADDomain/MSFT_xWaitForADDomain.psm1#L90-L109

I suggest that we instead use Start-Job -ScriptBlock {] and Wait-Job -Timeout. The code in the - ScriptBlock will continuously try to access the domain before the timeout. If the job haven't returned before the timeout, then the it can be assumed the domain is not yet available and the rest of the logic is run as appropriate.

@johlju johlju added the discussion The issue is a discussion. label Jun 7, 2019
@johlju johlju added this to To do in All issues and PR's Jun 7, 2019
@tmeckel
Copy link
Contributor

tmeckel commented Jun 11, 2019

Neat idea because that would allow the specification of a "real" timeout as a parameter and not those two arguments which only in combination will show the time the function will wait for the Domain to become available. More intuitive to use. But a breaking change if the "old" two parameters will be removed.

@johlju
Copy link
Member Author

johlju commented Jun 12, 2019

Yes, but I think it's better to change the parameters, right? Proposing this schema

[ClassVersion("1.0.0.0"), FriendlyName("WaitForADDomain")]
class MSFT_WaitForADDomain : OMI_BaseResource
{
    [Key, Description("Specifies the fully qualified domain name to wait for.")] String DomainName;
    [Write, Description("Specifies the credentials that are used when accessing the domain, unless the built-in PsDscRunAsCredential is used."), EmbeddedInstance("MSFT_Credential")] String Credential;
    [Write, Description("Specifies the timeout in seconds that the resource will wait for the domain to be accessible.")] uint64 WaitTimeout;
    [Write, Description("Specifies the number of times the node will be reboot in an effort to connect to the domain.")] uint32 RestartCount;
};

@tmeckel
Copy link
Contributor

tmeckel commented Jun 12, 2019

I totally agree, but it's a "major" breaking change, so perhaps a more gradual phase out of the old parameters would be nice. Like we started with the firewall stuff in xDSCWebService dsccommunity/xPSDesiredStateConfiguration#536 (comment)

@johlju
Copy link
Member Author

johlju commented Jun 12, 2019

It’s possible to do that, or we gather a lot of breaking changes and do them at once. Next release would be a breaking change release already so would be good to get more breaking changes in. But not sure if someone have time to resolve this issue until then.

@johlju johlju added breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community. and removed discussion The issue is a discussion. labels Jun 12, 2019
@johlju johlju changed the title xWaitForADDomain: Handle timeout better WaitForADDomain: Handle timeout better Jul 28, 2019
johlju added a commit to johlju/ActiveDirectoryDsc that referenced this issue Jul 30, 2019
- BREAKING CHANGE: Refactored the resource to handle timeout better and
  more correctly wait for a specific amount, and at the same time make
  the resource more intuitive to use. This change has replaced parameters
  in the resource (issue dsccommunity#343).
- Now the resource can use built-in `PsDscRunAsCredential` instead of
  specifying the `Credential` parameter (issue dsccommunity#367).
- New parameter `SiteName` can be used to wait for a domain controller
  in a specific site in the domain.
@johlju johlju added in progress The issue is being actively worked on by someone. and removed help wanted The issue is up for grabs for anyone in the community. labels Jul 30, 2019
johlju added a commit to johlju/ActiveDirectoryDsc that referenced this issue Jul 30, 2019
- BREAKING CHANGE: Refactored the resource to handle timeout better and
  more correctly wait for a specific amount, and at the same time make
  the resource more intuitive to use. This change has replaced parameters
  in the resource (issue dsccommunity#343).
- Now the resource can use built-in `PsDscRunAsCredential` instead of
  specifying the `Credential` parameter (issue dsccommunity#367).
- New parameter `SiteName` can be used to wait for a domain controller
  in a specific site in the domain.
All issues and PR's automation moved this from To do to Done Aug 2, 2019
johlju added a commit that referenced this issue Aug 2, 2019
…imeout better (#455)

- Changes to ActiveDirectoryDsc
  - The helper function `Find-DomainController` is exported in the module
    manifest. When running `Import-Module -Name ActiveDirectoryDsc` the
    module will also import the nested module ActiveDirectoryDsc.Common.
    It is exported so that the resource WaitForADDomain can reuse code
    when running a background job to search for a domain controller.
  - Changes to ActiveDirectoryDsc.Common:
    - Added function `Find-DomainController`.
    - Added function `Get-CurrentUser` (moved from the resource ADKDSKey).
- Changes to WaitForADDomain
  - BREAKING CHANGE: Refactored the resource to handle timeout better and
    more correctly wait for a specific amount of time, and at the same time
    make the resource more intuitive to use. This change has replaced
    parameters in the resource ([issue #343](#343)).
  - Now the resource can use built-in `PsDscRunAsCredential` instead of
    specifying the `Credential` parameter ([issue #367](#367)).
  - New parameter `SiteName` can be used to wait for a domain controller
    in a specific site in the domain.
@johlju johlju removed the in progress The issue is being actively worked on by someone. label Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.
Projects
2 participants