Update MSSQL test Docker image #2120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous version was incompatible with the updated kernel now used by Github Actions runners. See: microsoft/mssql-docker#868 The fix has been backported as far as the 2019 image, but not to the 2017 image. Seeing that the 2017 image didn't match what was being run in production in any case we might as well upgrade. This requires a few small changes: * The `sqlcmd` tool has moved from `/opt/mssql-tools/bin` to `/opt/mssql-tools18/bin`. This is due to the newer image using the `mssql-tools18` apt package. Note that references to the old path are still present in the codebase because _our_ Docker image still installs the `mssql-tools` package. * The `sqlcmd` invocation now requires the `-C` option to automatically trust the server certificate.
Deploying databuilder-docs with
|
| Latest commit: |
3995a65
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f43b915.databuilder.pages.dev |
| Branch Preview URL: | https://evansd-fix-mssql-in-ci.databuilder.pages.dev |
The previous code used a single big regex and relied on a precise sequence of IO statistics being returned in a fixed order. In SQL Server 2019 new items were added and the order slightly changed. We rewrite the parsing code to be robust to such changes.
rebkwok
approved these changes
Sep 25, 2024
| ( | ||
| b"Table '#tmp_1______________________________________________________________________________________________________________0000000014D9'." | ||
| b" Scan count 1, logical reads 4, physical reads 0, read-ahead reads 0," | ||
| b" lob logical reads 0, lob physical reads 0, lob read-ahead reads 0." | ||
| b" Scan count 1, logical reads 4, physical reads 0, page server reads 0," |
There was a problem hiding this comment.
Shouldn't the new page server items also show up in the #tmp_1 entry in the table_io assert?
There was a problem hiding this comment.
The way I've got it working at the moment is that it discards any stats it isn't expecting, because it only iterates the keys already in cumulative_stats:
ehrql/ehrql/utils/mssql_log_utils.py
Lines 92 to 101 in 3995a65
ehrql/ehrql/utils/mssql_log_utils.py
Lines 122 to 123 in 3995a65
I wanted to try to keep the output consistent between dev and prod in this respect.
I agree the tests here aren't as comprehensible as they could be, but they do cover everything and I didn't think it was worth the time making them better for a fairly small feature.
rebkwok
added a commit
to opensafely-core/sqlrunner
that referenced
this pull request
Oct 8, 2024
Copying the commit message from the equivalent PR on ehrql: opensafely-core/ehrql#2120 The previous version was incompatible with the updated kernel now used by Github Actions runners. See: microsoft/mssql-docker#868 The fix has been backported as far as the 2019 image, but not to the 2017 image. Seeing that the 2017 image didn't match what was being run in production in any case we might as well upgrade.
rebkwok
added a commit
to opensafely-core/sqlrunner
that referenced
this pull request
Oct 8, 2024
Copying the commit message from the equivalent PR on ehrql: opensafely-core/ehrql#2120 The previous version was incompatible with the updated kernel now used by Github Actions runners. See: microsoft/mssql-docker#868 The fix has been backported as far as the 2019 image, but not to the 2017 image. Seeing that the 2017 image didn't match what was being run in production in any case we might as well upgrade. This requires a few small changes: * The `sqlcmd` tool has moved from `/opt/mssql-tools/bin` to `/opt/mssql-tools18/bin`. This is due to the newer image using the `mssql-tools18` apt package. * The `sqlcmd` invocation now requires the `-C` option to automatically trust the server certificate.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous version was incompatible with the updated kernel now used by Github Actions runners. See:
microsoft/mssql-docker#868
The fix has been backported as far as the 2019 image, but not to the 2017 image. Seeing that the 2017 image didn't match what was being run in production in any case we might as well upgrade.
This requires a few small changes:
The
sqlcmdtool has moved from/opt/mssql-tools/binto/opt/mssql-tools18/bin. This is due to the newer image using themssql-tools18apt package. Note that references to the old path are still present in the codebase because our Docker image still installs themssql-toolspackage.The
sqlcmdinvocation now requires the-Coption to automatically trust the server certificate.The precise sequence in which IO statistics are returned has changed and so we update the code which parses these to be insensitive to changes in order.