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

Issue 320 fix? #623

Open
wants to merge 5 commits into
base: development
Choose a base branch
from
Open

Issue 320 fix? #623

wants to merge 5 commits into from

Conversation

reitse
Copy link

@reitse reitse commented Jun 7, 2019

A New PR

THANK YOU - We love to get PR's and really appreciate your time and help to improve this module

Accepting a PR

Before we accept the PR - please confirm that you have run the tests locally to avoid our automated build and release process failing. You can see how to do that in our wiki

https://github.com/sqlcollaborative/dbachecks/wiki

Please confirm you have 0 failing Pester Tests

[x] There are 0 failing Pester tests (there were 0 Pester tests so that was easy ;))

Changes this PR brings

Please add below the changes that this PR covers. It doesnt need an essay just the bullet points :-)
This function checks the available disk space and compares it with expected file growth. If the growth is larger than the available space, time to act!

Hopefully this helps with issue 320

begin {
<#
Step one. Get de free diskspace from the disks where Sql Server is parking it's files.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Get the free

Copy link
Collaborator

Choose a reason for hiding this comment

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

Love the comments in the code :-) Thank you :-)

Copy link
Author

Choose a reason for hiding this comment

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

Typo fixed

Copy link
Collaborator

@SQLDBAWithABeard SQLDBAWithABeard left a comment

Choose a reason for hiding this comment

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

I like this as a starter for ten.

Once this function is settled it can probably be submitted to dbatools but for now it can then be called for the Assertion which can then be used in the Pester Check

Question - How will it handle multiple instances ?


begin {
<#
Step one. Get de free diskspace from the disks where Sql Server is parking it's files.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love the comments in the code :-) Thank you :-)

#>

$DiskFreeSpace = Get-DbaDbFile -SqlInstance $SqlInstance | Select-Object @{label='DriveLetter';Expression={$_.PhysicalName.substring(0,3)}}, VolumeFreeSpace |Sort-Object -Property Driveletter -Unique

Copy link
Collaborator

Choose a reason for hiding this comment

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

please can this have a try catch

#>

$FileGrowth = Get-DbaDbFile -SqlInstance $SqlInstance | Select-Object @{label='DriveLetter';Expression={$_.PhysicalName.substring(0,3)}}, NextGrowthEventSize

Copy link
Collaborator

Choose a reason for hiding this comment

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

please can this have a try catch

If the drives are the same, the comparison will take place and the result will be shown.
#>


Copy link
Collaborator

Choose a reason for hiding this comment

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

This is super, can we create a pscustomobject here so that we have something like

ComputerName | SQLInstance | DriveLetter | FreeSpace | Growth | GrowthAchievable
Beard BeardInstance | Q | 100000 | 500000 | true

Made some progress with the helpfull comments of Sean and Rob.

The last change is somewhat iffy. I've created the custom object but do you want it to write to the host or do you want to write it to a file?
Fixed a little bug with a wrong column name in the output.

Excluded the finally from the try catch. It got there every time for no apparent reason.
@reitse
Copy link
Author

reitse commented Aug 5, 2019

Goodmorning @SQLDBAWithABeard and @shaneis,

I'm blocking time this week to get this baby cleaned and babypowdered ;).
Last time we met at DataGrillen there were two things i had to work on. Because my lately memory is best described as 'mildly incontinent', all i could remember was that i had to make sure it could work with the assertion module.
I tried to read that module and understand what's happening there. To my untrained eye it looks like the custom object that i've added to the code is picked up, asserted and the result is returned.
Would you mind giving me a pointer as to where the code should be heading? I've browsed through the tests as well, should i add to, e.g. Database.Tests.ps1?

@shaneis
Copy link
Collaborator

shaneis commented Aug 7, 2019

Reitse!
Have you checked dbachecks/internal/assertions/ for examples?

@reitse
Copy link
Author

reitse commented Aug 7, 2019 via email

@SQLDBAWithABeard
Copy link
Collaborator

You are absolutely correct with your logic for the processing of the check @reitse :-)

Now, where should the check life?

That is indeed a fantastic question and one which I can logicallly answer with Server or Instance or Database!!

I am going to say Instance and have it fail at an instance level as possibly if there is not enough space for one database to grow then you dont want 100 errors for the next databases

Assert-DBGrowthEvent added to the InstanceAssertions.
Removed the commandline output in the GetDatabaseDiskFreeSpace that has been replaced with the custom function.
@reitse
Copy link
Author

reitse commented Aug 18, 2019

I've added the assertion and hope It's somewhere along the line of where it should be.

Eagerly awaiting your constructive comments! :)

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