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.

Sign up for GitHub

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

Jump to bottom

release-23.1: codeowners, roachtest: merge sql-schema, sql-sessions to sql-foundations #102924

Merged
merged 1 commit into from
May 12, 2023
Merged

release-23.1: codeowners, roachtest: merge sql-schema, sql-sessions to sql-foundations #102924

merged 1 commit into from
May 12, 2023

Conversation

celiala
Copy link
Collaborator

@celiala celiala commented May 9, 2023 •
edited
Loading

Backport 1/1 commits from #102785.

/cc @cockroachdb/release

Reflecting merge sql-schema, sql-sessions to sql-foundations in:

  • github CODEOWNERS
  • pkg/cmd/roachtest owners
  • 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

Part 2

Part 3

Partial work for DEVINFHD-867
Release note: None
Epic: DEVINFHD-867
Release Justification: non-production code change.

@blathers-crl
Copy link

blathers-crl bot commented May 9, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

This was referenced May 9, 2023
Merged
Merged
@celiala celiala marked this pull request as ready for review May 9, 2023 16:36
@celiala celiala requested review from a team as code owners May 9, 2023 16:36
@celiala celiala requested review from herkolategan and renatolabs and removed request for a team May 9, 2023 16:36
@celiala
Copy link
Collaborator Author

celiala commented May 9, 2023

Hi! This is RFAL:

Changes made to TEAMS.yaml for the renamed cockroachdb/sql-foundations team:

  • aliases have been kept/mapped to the renamed cockroachdb/sql-foundations team
  • label for new T-sql-foundations team has been mapped to the renamed cockroachdb/sql-foundations team

These changes have been applied to master + both backports:

@celiala celiala force-pushed the backport23.1-102785 branch 2 times, most recently from 361643b to 18ff36c Compare May 10, 2023 21:57
celiala
celiala commented May 10, 2023
View reviewed changes
Copy link
Collaborator Author

@celiala celiala left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @renatolabs)

TEAMS.yaml line 29 at r1 (raw file):

    cockroachdb/sqlproxy-prs: other
    cockroachdb/sql-api-prs: other
  triage_column_id: 19467489

Updating triage_column_id to 19467489, as per feedback from

#102785 (review)

@celiala
codeowners, roachtest: merge SQL schema + sessions to SQL foundations
d3b6364 
The SQL Schema and Sessions teams are merging to form SQL
Foundations. As described in DEVINFHD-867 we want to:

- reroute old teams to new sql-foundations team
- rename "SQL Schema" project to "SQL Foundations"
- deprecate "SQL Sessions" project

Release note: None
@celiala celiala merged commit a8be935 into cockroachdb:release-23.1 May 12, 2023
craig bot pushed a commit that referenced this pull request May 15, 2023
@abarganier @celiala
@kpatron-cockroachlabs @maryliag
Merge #102772 #102785 #103118 #103240 #103301
58fbcfb 
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 <abarganier@cockroachlabs.com>
Co-authored-by: Celia La <celia@cockroachlabs.com>
Co-authored-by: Kyle Patron <kyle@cockroachlabs.com>
Co-authored-by: maryliag <marylia@cockroachlabs.com>
Co-authored-by: Rui Hu <rui@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlinder jlinder jlinder approved these changes

@herkolategan herkolategan Awaiting requested review from herkolategan herkolategan is a code owner automatically assigned from cockroachdb/test-eng

@renatolabs renatolabs Awaiting requested review from renatolabs renatolabs is a code owner automatically assigned from cockroachdb/test-eng

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@celiala @cockroach-teamcity @jlinder