3

I am writing a script for work that will query all of our AD domains and then return the computers that have admin privileges. The below script works and returns the expected results but it is very slow on our larger domains.

When working with 23k objects and less it runs in only a few minutes (6min or so), but when its has to handle 90k+ it gets into hours.

I am new to PowerShell and have no idea what operation here would have an exponential runtime increase so I have been unable to narrow it down. My hunch is that it has to deal with that way PowerShell is expanding the arrays to continuously add more objects? I was also thinking about making better use of the pipeline... but being new and coming from bash I am unfamiliar with this concept and how to utilize it in this code

Is there a way I can speed this up to run faster than a few hours? Any help would be greatly appreciated.

$date = Get-Date -uFormat %d-%m-%y

ForEach($domain in $domains)
{
        $all_computers = Get-ADComputer -Server $domain -filter * -Properties enabled | select-object name,enabled,@{name="distinguishedname";expression={$_.DistinguishedName -replace "(CN[^,]+,)"}}
        #Gets all of the objects that are in a group and pulls their names this is to get the admin flag that is appended to names in this group
        $group_name = (Get-ADGroup -Server $domain -Filter{name -like "*admin*"}).Name

        #Counts the devices pulled from the computer query
        $DevNum = $all_computers.count
        echo "Number of devices: " $DevNum > "$domain LARGE $date.txt"

        #Remove servers from the list
        $all_computers = $all_computers | ?{ $_ -NotMatch "Servers" }

        #Counts the number of servers we removed
        $NumSkipped = $DevNum - $all_computers.count

        Switch($all_computers){
            #Finding all of the accounts where both types of admins exist and removing them from the master list
            {$group_name -contains $($_.name + "Admins") -and $group_name -contains $($_.name + "UPEPAdmins")} {$_ | Add-Member "admintype"  "both";Continue}
            #Finding all of the accounts with only exception admins and removing them from the master list
            {$group_name -contains $($_.name + "Admins")} {$_ | Add-Member "admintype" "old";Continue}
            #Finding all of the accounts with only upep admins and removing them from the master list
            {$group_name -contains $($_.name + "UPEPAdmins")} {$_ | Add-Member "admintype" "UPEP";Continue}
            #These accounts have no admin
            default {$_ | Add-Member "admintype" "No"}
        }

        echo "Number of servers skipped: " $NumSkipped >> "$domain LARGE $date.txt"

        echo "Number of workstations: " $all_computers.count >> "$domain LARGE $date.txt"

        echo "Number of Exception admins found: " $($all_computers|?{$_.admintype -match "old|both"}).count >> "$domain LARGE $date.txt"

        echo "Number of UPEP admins found: " $($all_computers|?{$_.admintype -match "upep|both"}).count >> "$domain LARGE $date.txt"

        echo "Number of both UPEP and Exception admins found: " $($all_computers|?{$_.admintype -eq "both"}).count >> "$domain LARGE $date.txt" 

        #output
        $all_computers | Export-Csv "$domain LARGE $date.csv"
}

Edit 1:

Updated code to reflect suggestions from SodaWillow, TheMadTechnician, and removing trim and replacing it with -replace decreased runtime by a little bit.

Edit 2:

Updated the code to the correct solution, ontop of TheMadTechnician's suggestions I filtered the groups to decrease their number and also inserted group names into an array rather than a table. The use of an array sped up operations significantly when combined with the decreased group numbers.

Current bugs: The "Both" admin types are being exported to CSV correctly but not being reported in the text file at all, I think it has to do with the logic in the if statement. I am looking at that currently

Edit 3:

Fixed Both admin type logic bug, this is the final solution for this problem. the $domains var is declared out of view of this code block due to private information.

Thank you everyone for your time!

10
  • You may want to try LDAP filters and System.DirectoryServices.DirectorySearcher (examples here : powershelladmin.com/wiki/…). Measure the time consumed by each part of your code to find the longest queries, and then begin by optimizing those ones (if you haven't done this already) Commented May 23, 2016 at 17:00
  • I have tried to run through and use Measure-Command on chunks of code to see where I may be adding unnecessary time. Nothing seems to be excessive but when put together take a really long time Commented May 23, 2016 at 17:04
  • I would have imagined L2 and L4 lasting much longer than the rest. L4 could be rewritten with Get-ADGroup maybe ? Commented May 23, 2016 at 17:07
  • Yea, those take a hot second. a few minutes a piece. but nothing unreasonable. It looks like the trim (L28 & L37) are the heavy hitters, would replace work in place of trim with that sub expression? The reason I did not just use get-adgroup is because the information I need for metrics is split between adcomputer and ad group due to poor AD organization. Commented May 23, 2016 at 17:10
  • There is a LOT of processing afterwards, that needs to be completely broken down and understood to optimize the script. I'll try to dive in :). Commented May 23, 2016 at 17:12

2 Answers 2

1

Well, it looks like at the least you could benefit from combining some lines. There's a few places that you define a variable, and then immediately redefine it by filtering itself. Such as:

$servs = $all_computers | Select-Object Name,DistinguishedName
$servs = $servs -Match "Servers" 

Can be reduced to:

$servs = $all_computers | Select-Object Name,DistinguishedName | Where {$_ -match "Servers"}

Now do that same thing for $admin_exist_cnt and $upep_admin_exist_cnt

$admin_exist_cnt = Compare-Object -DifferenceObject $group_name -ReferenceObject $com_name_admin -ExcludeDifferent -IncludeEqual -Property name | select-object @{name="name";expression={$($_.name).toString().TrimEnd("A","d","m","i","n","s")}}

and

$upep_admin_exist_cnt = Compare-Object -DifferenceObject $group_name -ReferenceObject $com_name_upep -ExcludeDifferent -IncludeEqual -Property name | Select-Object @{name="name";expression={$($_.Name).ToString().TrimEnd("U","P","E","P","A","d","m","i","n","s")}}

Then near the end you run through all the computers looking for "both" admintypes, and remove those from the $all_computers variable, then run through the whole thing again, and do that like 4 times. No, don't do that, use the Switch command instead. It would look like this:

Switch($all_computers){
    #Finding all of the accounts where both types of admins exist and removing them from the master list
    {$both_exist_cnt.name -contains $_.name} {$_ | Add-Member "admintype"  "both";Continue}
    #Finding all of the accounts with only exception admins and removing them from the master list
    {$admin_exist_cnt.name -contains $_.name} {$_ | Add-Member "admintype" "old";Continue}
    #Finding all of the accounts with only upep admins and removing them from the master list
    {$upep_admin_exist_cnt.name -contains $_.name} {$_ | Add-Member "admintype" "UPEP";Continue}
    #These accounts have no admin
    default {$_ | Add-Member "admintype" "No"}
}

Then you can reduce the output section to just:

#output
$all_computers | Export-Csv test.csv

Edit: Ok, went back over things and saw that you're redefining things a whole lot. Instead I propose just running it through a Switch once, then counting up your results afterwards. This will reduce memory consumption considerably, which may not matter on small test runs but should make a considerable difference when running against large quantities. Try this revised script, and let me know if you have any specific questions:

#TODO Fix the server thing and put it in a for loop
$all_computers = Get-ADComputer -Server NW -filter * -Properties enabled | select name,enabled,distinguishedname
#Gets all of the objects that are in a group and pulls their names this is to get the admin flag that is appended to names in this group
$group_name = Get-ADObject -Server NW -Filter{objectCategory -eq "group"} | select name

#Counts the devices pulled from the computer query
#TODO Replace "test.txt" with a descriptive file name
$DevNum = $all_computers.count
echo "Number of devices: " $DevNum > test.txt

#Remove servers from the list
$all_computers = $all_computers | ?{ $_ -NotMatch "Servers" }

#Counts the number of servers we removed
$NumSkipped = $DevNum - $all_computers.count

Switch($all_computers){
    #Finding all of the accounts where both types of admins exist and removing them from the master list
    {$group_name.name -contains $($_.name + "Admins") -and $group_name.name -contains $($_.name + "UPEPAdmins")} {$_ | Add-Member "admintype"  "both";Continue}
    #Finding all of the accounts with only exception admins and removing them from the master list
    {$group_name.name -contains $($_.name + "Admins")} {$_ | Add-Member "admintype" "old";Continue}
    #Finding all of the accounts with only upep admins and removing them from the master list
    {$group_name.name -contains $($_.name + "UPEPAdmins")} {$_ | Add-Member "admintype" "UPEP";Continue}
    #These accounts have no admin
    default {$_ | Add-Member "admintype" "No"}
}

#TODO Replace "test.txt" with a descriptive file name
echo "Number of servers skipped: " $NumSkipped >> test.txt

#Calculating the number of workstations 
echo "Number of workstations: " $all_computers.count >> test.txt

#TODO Replace "test.txt" with a descriptive file name
echo "Number of Exception admins found: " $($all_computers|?{$_.admintype -match "old|both"}).count >> test.txt

#TODO Replace "test.txt" with a descriptive file name
echo "Number of UPEP admins found: " $($all_computers|?{$_.admintype -match "upep|both"}).count >> test.txt

#Find both exception and upep admin names
if($($all_computers|?{$_.admintype -ne "No"}))
{
        echo "Number of both UPEP and Exception Admins: 0" >> test.txt
}else
{
        echo "Number of both UPEP and Exception Admins: " $($all_computers|?{$_.admintype -match "both"}).count >> test.txt
}

#output
$all_computers | Export-Csv test.csv
Sign up to request clarification or add additional context in comments.

11 Comments

I made your changes on a branch of the script and have ran it on a test bed server several times, on average it has increased running time by 6 seconds (to 36 seconds). Where as the previous iteration on the test bed server was able to complete in 30 seconds. Is there something in the switch statement that would scale better than what I was doing because you lose the overhead of redefining the variables? Also, thank you for your time and help I really appreciate it! :)
I re-read your script and I'm making some serious changes and will get back to you. I think there's definitely some improvements that can be done to speed it up and help manage memory for larger iterations.
Okay, thanks again. Just a heads up, I removed the trim command and replaced it with -replace $upep_admin_exist_cnt = Compare-Object -DifferenceObject $group_name -ReferenceObject $com_name_upep -ExcludeDifferent -IncludeEqual -Property name | Select-Object @{name="name";expression={$($_.Name).ToString() -replace "UPEPAdmins"}} Is now what it looks like and I will update my question to reflect your current suggestions and my changes!
I wish I could upvote this answer more than once, really nice effort. Maybe you could give it a try on codereview.stackexchange.com since the script is already working pretty well, though it seems to me a script running for hours is kind of an issue to me :) I'm really curious about the results of this on a very large scale.
I've never used codereview.stackexchange.com, I'm not familiar with it, but I'll have to check it out. Yeah, last script I had that ran for hours was dealing with almost 700k server objects, every group they touched (nested, and nested in), and all of the other members of those groups.
|
0

Depending on the powershell version you have available (iirc you need PSv4), you might use the .where method on your lists, it is much faster:

$servs = $all_computers.where{$_ -match "Servers"} | Select-Object Name,DistinguishedName

I suspect using select afterwards is also a tiny bit faster, but this is just guesswork.

1 Comment

actually using {$_ -eq "Servers"} might be even faster ;)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.