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

[#1509] SSRS 2019 initialization fix. #1698

Closed
wants to merge 1 commit into from

Conversation

bozho
Copy link
Contributor

@bozho bozho commented Mar 29, 2021

Pull Request (PR) description

SqlRS: SSRS 2019 initialization fix.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

@bozho
Copy link
Contributor Author

bozho commented Mar 29, 2021

@johlju I've done some testing on SSRS 2016, 2017 and 2019. It seems that executing InitializeReportServer is not needed on SSRS 2016 and 2017 if we simply restart SSRS service (which flips the IsInitialized to $true on every SSRS version).

That means we can probably remove the code that calls it (and update relevant unit tests). If we decide to leave the code as it is now, it is unclear if there's ever a scenario where executing InitializeReportServer would be needed after restarting SSRS service.

Furthermore, regarding unit tests, the unit test for uninitialised SSRS will have to have two scenarios, one with UseSSL set to $true and one with UseSSL set to $false, as the number of calls to Restart-ReportingServicesService depends on its value.

@johlju johlju added the needs review The pull request needs a code review. label Mar 30, 2021
@bozho
Copy link
Contributor Author

bozho commented Mar 30, 2021

Something I've noticed in my tests for SSRS 2019...

In my configuration, I have a custom DSC resource which connects to SSRS server to set some stuff up. It runs immediately after SqlRs. The custom resource uses a small helper function to connect to SSRS proxy URL (.../ReportService2010.asmx?WSDL).

The function performs a small retry loop, sending a GET request to the proxy URL using [System.Net.WebRequest], to "prime" the server, since SSRS may return a 503 for a bit after a restart. I've noticed that SSRS 2019 pretty consistently returns 401 for a few seconds after this initialisation restart and retrying once or twice makes the 401 go away.

I was wondering if this is something we should add to SqlRs resource or simply document the behaviour? If we choose to implement "priming" the server after a restart, I'm not sure if there's a safe/proper way to decide which reserved URL to use...

@johlju
Copy link
Member

johlju commented May 4, 2021

@bozho sorry I haven't been available but there is a lot of stuff going on with the day job so have not as much time as I want.

I was wondering if this is something we should add to SqlRs

This sounds like a WaitForSqlRs resource? 🤔 Essentially you are waiting for the Reporting Services to start and then "warming" it up (or "prime" as you said).

That means we can probably remove the code that calls it

We could have different paths for different versions on Reporting Services. If it needed for older SQL Reporting Services then we can keep it but not use it for newer. 🤔

I think we need to get the integration tests working again for Reporting Services as they were disabled when moving to the new pipeline.

@bozho bozho force-pushed the issue-1509-ssrs-2019-init branch from 30dab1c to cb371eb Compare May 6, 2021 07:23
@bozho
Copy link
Contributor Author

bozho commented May 6, 2021

This sounds like a WaitForSqlRs resource? 🤔 Essentially you are waiting for the Reporting Services to start and then "warming" it up (or "prime" as you said).

Maybe... Although, I think any custom resource dealing with SSRS would have to be able to handle SSRS warming up. For example, DSC might've rebooted the machine and carried on with running the configuration when it hits our custom SSRS resource... You could put the proposed WaitForSqlRs resource before any resources dealing with SSRS, but that seems a bit "meh" :-)

We could have different paths for different versions on Reporting Services. If it needed for older SQL Reporting Services then we can keep it but not use it for newer.

I've tested on SQL 2016-2019 Standard. All of them were initialised when restarted immediately after executing SetDatabaseConnection and there was no need to execute InitializeReportServer (IIRC, the call always fails if SSRS is initialised). We can be a bit defensive and leave the code as it is now (it will execute InitializeReportServer if SSRS is not initialised after the restart).

@bozho
Copy link
Contributor Author

bozho commented May 6, 2021

Ok, I've spent a better day trying to update unit tests for Set-TargetResource... Currently, they fail for uninitialised instances on the number of Restart-ReportingServicesService calls.

The actual code will now restart SSRS right after executing SetDatabaseConnection CIM function. It will then call Get-ReportingServicesData to get fresh SSRS settings after SSRS restart, which return $true for IsInitiaized.

However, in SqlRS unit tests, we do not change the IsInitialized value, so the unit test will always go inside the if which executes InitializeReportServer CMI function and sets $restartReportingService to $true.

How do we simulate real behaviour here? Can we mock Restart-ReportingServicesService to set an "is initialised" flag when executed?

@bozho bozho force-pushed the issue-1509-ssrs-2019-init branch from cb371eb to d54f475 Compare July 1, 2021 09:12
@bozho
Copy link
Contributor Author

bozho commented Aug 9, 2021

Hi @johlju,

Any chance you have a look at this, help me figure out the problems above?

Thank you!

@johlju
Copy link
Member

johlju commented Aug 9, 2021

I will have a look. I been off for a few weeks due to vacation. But will be focusing on the SqlServerDsc for a while now. 😊

@johlju
Copy link
Member

johlju commented Aug 28, 2021

Sorry it took so long. This is a case of the unit tests being written to complex (trying to reuse code too much) so it is hard to add a simple unit tests as in this case. The entire unit test should really be refactored, but no need for this PR. 🙂

The variable $mockDynamicIsInitialized is used to set the value for IsInitialized property in the mock. Suggest adding a new context block that test this new code path. Meaning that the mock of function Get-ReportingServicesData should return IsInitialized as true and false in two different test. Though, the added Restart-ReportingServicesService should probably not be called if IsInitialized -eq $true, so that might make the new test path a bit simpler?

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Aug 28, 2021
johlju added a commit to johlju/SqlServerDsc that referenced this pull request Aug 29, 2021
johlju added a commit to johlju/SqlServerDsc that referenced this pull request Aug 30, 2021
@johlju
Copy link
Member

johlju commented Aug 30, 2021

I'm closin this PR since I added this change to PR #1719 including unit tests an integration test changes. PR #1719 is also dependent on PR #1718.
This change did make the RS 2019 correctly initialize. Great job! Sorry it took so long to get to this.

@johlju johlju closed this Aug 30, 2021
@johlju
Copy link
Member

johlju commented Aug 30, 2021

Btw, the WaitForSqlRs resource would try to hit the RS until it gets http status code 200. I had to add a timer of 30 seconds in the integration test after running SqlRs and before running an Invoke-WebRequest to be able for it to return http status code 200 (otherwise it returned 503). So maybe a WaitForSqlRs could help with that. 🤔 I add a issue for that so we can discuss it.

@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Aug 30, 2021
johlju added a commit to johlju/SqlServerDsc that referenced this pull request Aug 31, 2021
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.

SqlRs: Reporting Services 2019 Configuration SQLRS Error
2 participants