pkg/util/log: route panics through network sinks for SQL pods #101369
Assignees
Labels
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Comments
abarganier
added
C-enhancement
craig bot
pushed a commit
that referenced
this issue
May 15, 2023
102772: pkg/util/log: unify explicit flush of network and file sinks r=knz a=abarganier Ref: #101562 both for files *and* network sinks, such as fluent-servers. I received some feedback that we shouldn't divide these flush operations based on sink type, and instead we should unify the flush operation to flush both (as the crash reporter already does). The existing function to flush just the file sinks is quite widely used. Introducing flushing of network sinks begs the question, "What if this adds to the runtime of code using this explicit flush mechanism?" Well, the existing FlushFileSinks calls fsync() [1] with F_FULLFSYNC [2]. IIUC, this means that it already is a blocking operation as the fsync() call will wait for the buffered data to be written to permanent storage (not just the disk cache). Given that, I think any caller of this function already assumes it's a blocking operation and therefore should be tolerant of that. [1]: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/fsync.2.html [2]: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/fcntl.2.html#//apple_ref/doc/man/2/fcntl Nonetheless, we implement a timeout mechanism for the flushing of the buffered network sinks as an added protection. Fixes: #101369 102785: codeowners, roachtest: merge sql-schema, sql-sessions to sql-foundations r=celiala a=celiala Reflecting merge sql-schema, sql-sessions to sql-foundations in: - [x] github CODEOWNERS - [x] pkg/cmd/roachtest owners - [x] TEAMS.yaml Tests should pass once new team is created via https://github.com/cockroachlabs/crl-infrastructure/pull/775 This is a task for Part 2, below. ____ ### Tasks to update: GitHub Team name + GitHub Projects + Blathers Part 1 - [x] Create GitHub T- label: https://github.com/cockroachdb/cockroach/labels/T-sql-foundations - [ ] Create new sql-foundations GitHub team name: https://github.com/cockroachlabs/crl-infrastructure/pull/775 Part 2 - [ ] Update Blathers: https://github.com/cockroachlabs/blathers-bot/pull/123 - [ ] Update CODEOWNERS, roachtest, TEAMS.yaml: - [ ] master: #102785 - [ ] release-23.1: #102924 - [ ] release-22.2: #102925 - [ ] [Manual Step] To be done at same time as merging Part 2 PRs: - [ ] Manually rename "SQL Schema" Project to "SQL Foundations": https://github.com/cockroachdb/cockroach/projects/47 Part 3 - [ ] Remove old GitHub team names: https://github.com/cockroachlabs/crl-infrastructure/pull/776 ____ Partial work for DEVINFHD-867 Release note: None Epic: DEVINFHD-867 103118: jwtauthccl: allow cluster SSO to pick principals from custom claims r=kpatron-cockroachlabs a=kpatron-cockroachlabs Previously, cluster SSO (JWT authentication) required user principals to be in the subject field of the JWT, which by the JWT specification required them to be a single string. A customer has a desire have which SQL users a human is allowed to login as be specified by their IDP via a "groups" field in the JWT. To accomadate this, a new cluster setting has been introduced (server.jwt_authentication.claim), which when populated specifies which claim of the JWT to read as containing the principal. This value can be either a string or an array of strings. The resulting principal(s) are then fed through the identity map as before to produce the set of valid SQL users that this token can login as. As before, humans can specify which SQL user they wish to login with via the username field as long as it matches one of the values the token is a valid authentication method for. Fixes: CC-24595 Release note (enterprise change): Cluster SSO (JWT authentication) can now read SQL usernames from any JWT claims instead of requiring the subject claim to be used. 103240: ui: no need to refresh page after error r=maryliag a=maryliag Previously, if one page crashes on DB Console, all other pages would show the same error message and the user had to force a refresh on the browser to be able to see the other pages. Now only the broken page shows the error and all the other pages load as expected. The user still needs to force a refresh on the broken page if they want to retry. Fixes #97533 https://www.loom.com/share/56a6d811d9604b7abe673c1430ee605e Release note (ui change): If a page crashed, a force refresh is no longer required to be able to see the other pages on DB Console. 103301: backupccl: disable restore data processor memory monitoring by default r=rhu713 a=rhu713 Temporarily disable memory monitoring for the restore data processor due to current logic not handling deletes correctly. Epic: none Co-authored-by: Alex Barganier <[email protected]> Co-authored-by: Celia La <[email protected]> Co-authored-by: Kyle Patron <[email protected]> Co-authored-by: maryliag <[email protected]> Co-authored-by: Rui Hu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
It's been brought to our attention that panics are logged to stderr, but in a serverless environment, those panics don't make it to downstream systems like Splunk from SQL pods.
It seems that our crash reporter does indeed log the panic-associated error through the ops channel and stderr, so one possibility here is that the OPS channel is not routed through the network sinks (fluent sinks) used to send logs to splunk.
Describe the solution you'd like
Identify if panics do indeed get routed through the logging pipeline and its associated logging sinks. If it doesn't, make it happen. If it does, and we're not collecting the OPS channel, consider updating the SQL pod log config to send that channel along to targets like Splunk. If OPS is too noisy to collect, find an alternative solution.
Additional context
Relevant slack thread
Jira issue: CRDB-26914
The text was updated successfully, but these errors were encountered: