Find dba orphaned file recurse #6846
Conversation
Add a new parameter, -Recurse, that will enable recursive search of
directories. This is useful in situations where an instance has a
number of databases with a common root directory e.g.:
D:\DataFiles\MyFirstDb\data.mdf
D:\DataFiles\MySecondDb\data.mdf
The user can then call the function with:
...-Path 'D:\Datafiles' -Recurse...
rather than having to explicitly derive the directory tree
under 'D:\Datafiles'.
- Move file type filtering into sql query:
reduce the size of the returned data set;
eliminate the $matching variable
- Move system file filtering into sql query:
reduce the size of the returned data set;
simplify later logic
- Rename variables for clarity:
$paths > $sqlpaths;
$filestructure > $sqlfiles;
.Comparison > .ComparisonPath
- Add ComparisonPath property to results from Get-SqlFileStructure
to hold the validated path:
eliminate the $valid variable
- Use dotted notation in the final foreach loop:
eliminate the $dirtreematcher variable
- Pipe SQL query results straight into $dirtreefiles:
eliminate the $datatable variable
- Make more use of the pipeline
- Put LocalOnly and RemoteOnly into different parameter sets
to discourage both being used at the same time (order of processing
means only LocalOnly will be recognized)
- Add comments for clarity
Update tests to exercise -Path and -Recursive parameter functionality
Update logic to work with all paths in UNC format so that local and UNC references to the same file will match successfully. Filename and RemoteFilename values in output are unaffected. This commit includes a new internal function, Split-AdminUnc, which splits a UNC file path into server name and local path; i.e. the inverse of the Join-AdminUnc function.
And add explicit references to the FullName property. Hopefully this will resolve the build failure. Also replace the alias Detach-DbaDatabase with Dismount-DbaDatabase, which is easier to find on https://dbatools.io/commands/.
|
Thanks so much for the PR, @nmbell ! Unfortunately, the test failed and I couldn't immediately identify the cause :/ |
|
Hi Chrissy
Thanks for reaching out. No problem, I’m still working on it. I think I have a fix; I’ll commit this evening after work and we’ll see what happens 😊.
Nathan
|
Assign a UNC path based on the machine name that the SQL instance is running on rather than the build machine. Also, for consistency, remove the orphaned files using the remote name.
In order to test that file matching occurs correctly between UNC and local paths for the same file.
|
OK, I'm stumped. If anyone could provide any insight as to why the build is failing, it'd be much appreciated: https://ci.appveyor.com/project/sqlcollaborative/dbatools/builds/35288490/job/nf9eu0kyhl3wi3la https://ci.appveyor.com/project/sqlcollaborative/dbatools/builds/35288714/job/mqux8564ukuiyrm2 |
|
Can the test be run against your test environment? You can create a constants.local.ps1 in the root directory of the repo and set those instances to your environment and then our tests use those instances. Generally if the test is successful locally it will run successful in appveyor...generally. |
|
According to the output in the link the tests database is trying to create the log file over the admin share... |
Add a line to the dbatools.Tests.ps1 file to expose the modules that have been imported into the session. This is to aid in troubleshooting and configuration of local test environments.
Revert integration test to its original form to see if the new logic has caused a regression per the original test.
|
tests are a bit funky but let's go over the changes first .... why should the instance enum the admin shares ? there are shops with servers that have no admin shares available, and this will surely break.
PS: Join-AdminUNC has been created to make life to users easier if they DO have admin shares available, but it's not something our functions use outside of the realm of the PS session: in this case MSSQL can't be assured to be able to enumerate it and, sincerely, I don't find a reason why you changed the defaults from the fullpath to the admin-share-representation-of-it pretty much everywhere. EDIT: also, using the CTE breaks usability with MSSQL 2000 |
|
@nmbell : give me a shout if you can't figure it out, I streamlined your code and it all seems to work as it should, tests too. |
|
In SSMS, if I run:
then:
I get:
Then running the original version of Find-DbaOrphanedFiles:
Returns the following:
which is incorrect. Why is this? In the function, the calls to Get-SqlDefaultPaths returns only a local path. The comparison is only matching the paths as text strings rather than e.g. using Get-Item to compare the file objects (which I imagine would create a huge performance penalty). The file paths need to be translated to some common format (which I believe was also the point of the Comparison (now ComparisonPath) property). I suppose I could have gone the other way and converted everything to local paths, but UNC's are, well, universal. When you say 'admin shares', are you thinking specifically of folders that have been setup as a 'network share'? I don't know about older servers, but on every machine I've worked on (Windows Server 2008 R2 I think is the oldest version for me), you can refer to any file or folder with its UNC path, regardless of whether it's available via an explicit 'shared folder' or not. Once the function has gathered the file paths (in whatever form they are returned), they are thereafter only treated as strings. There is no need to hope the shares are available, because it's irrelevant. The user has the choice of working with either Filename or RemoteFilename in the output as suits their needs. It's a fair point if the function is no longer compatible with SQL 2000, however my goal is only to present working code for consideration. If the project administrators decide that backward compatibility is more valuable than the new functionality, then fair enough. I could only hope that at some point in the future the scales would tip the other way. |
|
I fixed the issue with the build failing due to formatting. I needed to downgrade my version of PSScriptAnalyzer from 1.19.1 to 1.18.2. I tried running the tests locally with Pester 5.0.4 and PSScriptAnalyzer 1.19.1 (latest versions at time of writing) and got the following formatting errors (edited for clarity):
It seems like the newer version of PSScriptAnalyzer would break the build if it was upgraded on the build server. @potatoqualitee: just thinking, rather than failing the build because of incorrect formatting, what about applying formatting after a successful build? Could you trigger it off commits to the main development branch? That way contributors don't have to worry about whether a difference in their local version will break the build off their pull request commits, and the clonable/distributable code base always complies with whichever version is running on the build server. |
Meddling with
Huh oh... They absolutely need to be available (and that was the point of my previous comment) ! I get you wanted an universal representation of a path, but you are making MSSQL use
If you look at the function, yes, we do support SQL2000. The new feature is cool, but doesn't strictly need a cte to work: it's perfectly achievable without. |
|
about formatting: the point is always the same....assure a consistent formatting throughout the module. Everyone has their IDEs and not every IDE is settable to format-on-save on a single style. We just leverage the one from Invoke-ScriptAnalyzer because THAT one is consistent :D Anywayyyyy....I'm going to investigate why scriptanalyzer changed the defaults (maybe it's just a As for "failing fast & providing a button to fix it" or "not failing and making sure who merges does it instead of the user", is just a matter of additional burden on the merger. We chose to fail fast to avoid the burden on mergers. |
…Query. Update logic so that the directories seen by xp_dirtree are always in local format, as UNC paths may not always be viable.
…ier. When gathering file structure, bypass use of a recursive CTE when the -Recurse switch is $false. To avoid regression, for SQL Server versions earlier than 2005 the -Recurse switch is forced to $false i.e. ignored.
|
The function was gathering a list of directory paths in $sqlpaths which is passed in to Get-SQLDirTreeQuery, but if any of those directories, which are passed to xp_dirtree, are in UNC format, the server won't return results if it doesn't support UNC paths. OK, now I see why it wouldn't work. I've updated the code to guarantee only local paths are passed in. Without the Split-AdminUNC function, or some equivalent, I don't think there's any way for this: Regarding recursion not strictly needing a CTE, I'll leave that for someone else to tackle. For now I've updated to keep the original behavior on pre-2005 versions. And finally, a successful build :-). Creating a database with a UNC file path obviously doesn't work on the build server, so this will just have to remain a testing gap. Test of the recursive logic is working good, though :-). |
|
My point about the formatting wasn't about whether it should be the committer or the merger that applies formatting (the issue is just as likely to occur if the merger's version of PSScriptAnalyzer is not the same as the build server, isn't it?). Having the build machine apply the formatting after a successful build side-steps the issue entirely: it will always be correct and never a burden for anyone. It was a non-trival effort to figure out why the build was failing even after I'd applied the formatting locally; OK, not a huge deal, but still a frustration that could have been avoided. The build setup isn't detailed anywhere (or is it?), so any new contributor is likely to experience the same. I included an output in one of the builds to expose the module versions: https://ci.appveyor.com/project/sqlcollaborative/dbatools/builds/35296201/job/r612h2n02ueqy36e. If formatting can't be automated, maybe at least the module versions can be exposed somewhere appropriate as a permanent part of the build output...? |
not really. if SQL is using local paths, you must enum local paths. Your split-adminunc just works for paths that are \servername\diskname$\path (admin shares) and not \servername\sharename\path (normal share).... which from MSSQL 2012 is fully supported, db files on any SMB share.
NP, I'll push a fixed version |
Type of Change
Purpose
Add a new parameter, -Recurse, to Find-DbaOrphanedFile that will enable recursive search of
directories specified in the -Path parameter.
Approach
Update the logic to control the value of the 'depth' parameter passed to xp_dirtree when recursion is required (i.e. 0).
Also update the SQL query to construct the full paths from the results returned.
Commands to test
Find-DbaOrphanedFile