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

clusterversion: bump min-supported version to 22.1 #80663

Closed
1 of 2 tasks
celiala opened this issue Apr 27, 2022 · 2 comments · Fixed by #86642
Closed
1 of 2 tasks

clusterversion: bump min-supported version to 22.1 #80663

celiala opened this issue Apr 27, 2022 · 2 comments · Fixed by #86642
Assignees
Labels
branch-master Failures and bugs on the master branch. branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.

Comments

@celiala
Copy link
Collaborator

celiala commented Apr 27, 2022

Describe the problem

This issue tracks the work necessary to bump the min supported binary version on master:

TODOs:

Jira issue: CRDB-18426
Jira issue: CRDB-15560

@blathers-crl
Copy link

blathers-crl bot commented Apr 27, 2022

Hi @celiala, please add a C-ategory label to your issue. Check out the label system docs.

While you're here, please consider adding an A- label to help keep our repository tidy.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@celiala celiala added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) branch-master Failures and bugs on the master branch. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Apr 27, 2022
@celiala celiala removed the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label May 2, 2022
@jlinder jlinder added sync-me and removed sync-me labels May 20, 2022
@celiala celiala self-assigned this Aug 2, 2022
craig bot pushed a commit that referenced this issue Aug 12, 2022
85850: importer: support {} format for arrays in CSV r=ecwall a=rafiss

fixes #84631

Release note (sql change): Arrays can now be imported in a CSV file
using the {} format, similar to COPY FROM. Importing array expressions
(e.g. ARRAY[1, 2, 3]) is still supported as well.

85854: clusterversion,storage: remove 22.1 PebbleFormat version gates r=jbowens a=celiala

This commit removes the following 22.1 version gates:

- PebbleFormatBlockPropertyCollector
- PebbleFormatSplitUserKeysMarked

Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

> However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release note: none

85909: kvserver: instrument RaftTransport workers with pprof labels r=tbg,erikgrinaker a=pavelkalinnikov

The unused arguments in the method signature were used to identify goroutines
in traces. This no longer works after Go 1.17 started passing arguments via
registers.

This commit adds pprof labels when starting these goroutines, to have a cleaner
code, more readable traces, and to work around the new Go convention.

Release note: None

85977: grunning: add library for precise on-CPU time measurement r=irfansharif a=irfansharif

Package grunning is a library that's able to retrieve on-CPU running
time for individual goroutines. It relies on using a patched Go and
provides a primitive for fine-grained CPU attribution and control
through a single API:

    package grunning

    // Time returns the time spent by the current goroutine in the
    // running state.
    func Time() time.Duration

The motivating RFC is over at #82356. Informs #82625.
We build CRDB using use the patched Go runtime for all officially
supported platforms when built using Bazel (#84867). Engineers commonly
building CRDB also use happen to use two platforms we don't use a
patched Go for:
- FreeBSD (we don't have cross-compilers setup), and
- M1/M2 Macs (we don't have a code-signing pipeline, yet).
We use '(darwin && arm64) || freebsd || !bazel' as the build tag to
exclude such platforms. See #84867 for more details.

This package tests various properties we should expect over the running time
value. It does not make assertions given the CI environments we run these
under (CPU-starved, lot of OS thread pre-emption, dissimilar to healthy
CRDB deployments). This is also why they're skipped under stress. Still,
these tests are useful to understand the properties we expect running
time to have:

    === RUN   TestEquivalentGoroutines
        thread=03 expected≈10.00% got= 9.98% of on-cpu time
        thread=06 expected≈10.00% got=10.00% of on-cpu time
        thread=02 expected≈10.00% got=10.01% of on-cpu time
        thread=10 expected≈10.00% got=10.01% of on-cpu time
        thread=07 expected≈10.00% got= 9.99% of on-cpu time
        thread=04 expected≈10.00% got= 9.99% of on-cpu time
        thread=09 expected≈10.00% got=10.00% of on-cpu time
        thread=01 expected≈10.00% got= 9.99% of on-cpu time
        thread=08 expected≈10.00% got=10.02% of on-cpu time
        thread=05 expected≈10.00% got=10.02% of on-cpu time
    --- PASS: TestEquivalentGoroutines (0.56s)

    === RUN   TestProportionalGoroutines
        thread=01 got  1.82% of on-cpu time: expected≈ 1.00x got=1.00x
        thread=02 got  3.64% of on-cpu time: expected≈ 2.00x got=2.00x
        thread=03 got  5.47% of on-cpu time: expected≈ 3.00x got=3.00x
        thread=04 got  7.28% of on-cpu time: expected≈ 4.00x got=4.00x
        thread=05 got  9.09% of on-cpu time: expected≈ 5.00x got=4.99x
        thread=06 got 10.91% of on-cpu time: expected≈ 6.00x got=5.99x
        thread=07 got 12.73% of on-cpu time: expected≈ 7.00x got=6.99x
        thread=08 got 14.54% of on-cpu time: expected≈ 8.00x got=7.99x
        thread=09 got 16.36% of on-cpu time: expected≈ 9.00x got=8.99x
        thread=10 got 18.16% of on-cpu time: expected≈10.00x got=9.97x
    --- PASS: TestProportionalGoroutines (1.72s)

    === RUN   TestPingPongHog
        pinger/ponger expected≈1.00x got=0.96x
    --- PASS: TestPingPongHog (0.91s)

Release note: None

85987: sql: fix aggregation of statistics r=maryliag a=maryliag

Previously, because we were using a join, we were double
counting statistics when we had the same fingerprint in
memory and persisted.
This commit adds a `DISTINCT` so we only count them once.

Fixes #85958

Release note: None

86008: sql: logic test to inform maximum builtin function oid change r=chengxiong-ruan a=chengxiong-ruan

This commit adds a logic test to let engineers who added a
new builtin function know that the new builtin function is
constructed earlier than some existing builtin functions at
init time.

Release note: None

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Celia La <celia@cockroachlabs.com>
Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Co-authored-by: Marylia Gutierrez <marylia@cockroachlabs.com>
Co-authored-by: Chengxiong Ruan <chengxiongruan@gmail.com>
craig bot pushed a commit that referenced this issue Aug 12, 2022
85095: ui: reintroduce end-to-end UI tests with cypress r=laurenbarker,nathanstilwell,rickystewart,srosenberg a=sjbarag

A recent commit [1] removed the stale, unused end-to-end tests. In their
place, add newly-written tests that can run headlessly in TeamCity (via
Docker) as a quick-failing health check and as a more exhaustive suite.

[1] 3d171b2 (ui: remove unused end-to-end UI tests, 2022-07-25)

Release note: None

85777: clusterversion,kvserver,server,security: remove 22.1 gates r=celiala a=celiala

This commit removes the following 22.1 version gates:

_(✅ denotes explicit LGTM)_

**8/8: initial PR version gates** 
- ✅  kvserver: ProbeRequest, 
- ✅ security,pgwire: SCRAMAuthentication,
- ✅ server: UnsafeLossOfQuorumRecoveryRangeLog,
- ✅ kvserver: AlterSystemProtectedTimestampAddColumn

**8/10 Update: Additionally, these version gates to added to this commit**

- ✅ backupccl: EnableProtectedTimestampsForTenant
- ✅ DeleteCommentsWithDroppedIndexes
- sql/catalog: RemoveIncompatibleDatabasePrivileges
- ✅ kvserver: AddRaftAppliedIndexTermMigration
- ✅ clusterversion: PostAddRaftAppliedIndexTermMigration
- kvserver: DontProposeWriteTimestampForLeaseTransfers
- ~✅ storage: EnablePebbleFormatVersionBlockProperties~ 8/12: TODO in future PR.
- ✅ sql: MVCCIndexBackfiller
- ✅ kvserver: LooselyCoupledRaftLogTruncation
- ✅ sql: EnableDeclarativeSchemaChanger
- ~✅ sql: RowLevelTTL~ 8/12: TODO in future PR.
- kvserver: EnableNewStoreRebalancer
- ~✅ sql: SuperRegions~ 8/12: TODO in future PR.
- ~changefeedccl: EnableNewChangefeedOptions~ 8/12: TODO in future PR.
- ✅ SpanCountTable
- ✅ spanconfig: PreSeedSpanCountTable, SeedSpanCountTable

The cleanup for these version gates was fairly straight-forward, using the guidance from [21.2 cleanup](#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

22.1 version gates requiring more nuanced removal will be done in separate PRs.

Partially addresses #80663


Co-authored-by: Sean Barag <barag@cockroachlabs.com>
Co-authored-by: Celia La <celia@cockroachlabs.com>
@celiala
Copy link
Collaborator Author

celiala commented Aug 15, 2022

Removed

#85854

  • PebbleFormatBlockPropertyCollector
  • PebbleFormatSplitUserKeysMarked

Addressed in #85777:

  • kvserver: ProbeRequest,
  • security,pgwire: SCRAMAuthentication,
  • server: UnsafeLossOfQuorumRecoveryRangeLog,
  • kvserver: AlterSystemProtectedTimestampAddColumn
  • backupccl: EnableProtectedTimestampsForTenant
  • DeleteCommentsWithDroppedIndexes
  • sql/catalog: RemoveIncompatibleDatabasePrivileges
  • kvserver: AddRaftAppliedIndexTermMigration
  • clusterversion: PostAddRaftAppliedIndexTermMigration
  • kvserver: DontProposeWriteTimestampForLeaseTransfers
  • sql: MVCCIndexBackfiller
  • kvserver: LooselyCoupledRaftLogTruncation
  • sql: EnableDeclarativeSchemaChanger
  • kvserver: EnableNewStoreRebalancer
  • SpanCountTable
  • spanconfig: PreSeedSpanCountTable, SeedSpanCountTable

@celiala celiala added release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 labels Aug 17, 2022
craig bot pushed a commit that referenced this issue Aug 19, 2022
85864: clusterversion,kvserver: remove EnableLeaseHolderRemoval r=shralex a=celiala

This commit removes the 22.1 `EnableLeaseHolderRemoval` version gates.

Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

> However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release justification: remove version gate

Release note: none

86153: sql: step the transaction before auto-commit r=ajwerner a=ajwerner

We want the pre-commit validation to see the side-effects of all previous
statements. If we don't step the transaction, we won't see writes which
occurred in the last statement. The shocking thing is that this all worked
before this change. The reason it worked was that the internal executor usage
that happened as a part of logging every schema change resulted in the txn
being stepped. That is a separate, and more complex bug to fix. It will be
described and addressed separately.

Fixes #86132

Release justification: Important bug fix 

Release note: None

86419: ci: disable `verify-archive` CI job r=rail,jlinder a=rickystewart

Release justification: Non-production code changes
Release note: None

86430: logictest: add sort order to logic test r=ajwerner a=adityamaru

Fixes a flakey test that lacked a sort order.

Release note: None

Release justification: test only change to fix a flake

Co-authored-by: Celia La <celia@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
Co-authored-by: Aditya Maru <adityamaru@gmail.com>
craig bot pushed a commit that referenced this issue Aug 19, 2022
85938: clusterversion,sql: remove ClusterLocksVirtualTable r=AlexTalks a=celiala

This commit removes the 22.1 `ClusterLocksVirtualTable` version gate.

Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

> However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release note: none
Release justification: remove old version gates.

Co-authored-by: Celia La <celia@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Aug 19, 2022
85419: tracing: fix lazytag export for Jaegar/otel r=aadityasondhi a=aadityasondhi

Previously, lazytags would not render in the otel export to
Jaeger. This was because otel doesn't support an on-demand
nature of the lazytags in its interface.

This change manually adds lazytags as otel tags before an otel
span is finished, allowing them to render in Jaeger after they
are exported.

Resolves: #85166

Release note: None

Release justification: bug fixes

85689: lint: add a linter to prohibit map[...]bool in favor of map[...]struct{} r=yuzefovich a=yuzefovich

The linter is currently limited to `sql/opt/norm` package. This commit
was prompted by an article mentioned in the Golang Weekly newsletter.
The rationale is that `map[...]struct{}` is more efficient, but in some
cases the bool map value is actually desired, so this commit introduces
an opt-out `//nolint:maptobool` comment.

Release justification: low-risk performance improvement.

Release note: None

85939: clusterversion,kvserver,sql: remove AutoStatsTableSettings r=msirek a=celiala

This commit removes the 22.1 `AutoStatsTableSettings` version gate.

Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

> However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release note: None
Release justification: remove old version gates.

86065: admission: carve out some new files r=irfansharif a=irfansharif

Pure code movement. Trying to see what components in this package could be
reasoned about independently to readers less familiar with this package.
Some of these may benefit from being pulled into sub-packages, but for now
separate files are sufficient. We're hurting some git history tracking
with this PR, but hopefully they lead to more understandability with future
work. It's also better to do it now before the release branch is cut to
make backports easier. If we pull them into sub packages next, git tracking
will then come for free.

```
admission: move ioLoadListener into its own file
admission: move GrantCoordinator into its own file
admission: move kvSlotAdjuster into its own file
admission: move sqlNodeCPUOverloadIndicator into its own file
admission: move tokensLinearModel into its own file
admission: add top-level admission.go
admission: move store{AdmissionStats,RequestEstimates}..
admission: segment test files by central type
```

I'm not breaking things up into subpackages in this PR, but this paves the
way for that. The structure I'm imagining is roughly:

    pkg/util/admission/
    ├── admission.go
    ├── admissionpb/
    ├── diskbandwidthlimiter/
    ├── diskloadwatcher/
    ├── grantcoordinator/
    ├── granter/
    ├── ioloadlistener/
    ├── kvslotadjuster/
    ├── sqlcpuoverloadindicator/
    ├── storeworkqueue/
    └── tokenlinearmodel/

Where the sub-packages house concrete implementations of interfaces
defined in admission.go, corresponding tests, and constructors for those
types. They depend on the base level pkg/util/admission package and talk
to the other subpackages through interfaces. The existing interfaces are
already written in a manner to make this happen. For other examples of
this pattern, look at pkg/{upgrade,spanconfig}.

Release justification: low-risk / high-benefit (pure code movement, will make backports easier)


Co-authored-by: Aaditya Sondhi <aadityas@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Celia La <celia@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
craig bot pushed a commit that referenced this issue Aug 20, 2022
85158: ccl/backupccl: do not backup spans for views r=rhu713 a=rhu713

Backups currently backup spans corresponding to views. These spans should not
be backed up as they do not contain data.

Additionally, because we do not split ranges at view boundaries, it is possible
that the range the view span belongs to is shared by another object in the
cluster (that we may or may not be backing up) that might have its own bespoke
zone configurations, namely one with a short GC TTL. This could lead to a
situation where the short GC TTL on the range we are not backing up causes our
protectedts verification to fail when attempting to backup the view span.

Release note (bug fix): Fix a bug in backup where spans for views were being
backed up. Because ranges are not split at view boundaries, this can cause the
backup to send export requests to ranges that do not belong to any backup
target.

Release justification: bug fixes and low-risk updates to new functionality

85937: clusterversion,changefeedccl: remove Changefeed version gates r=samiskin a=celiala

This commit removes the 22.1 `ChangefeedIdleness` and `EnableNewChangefeedOptions` version gates.

Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

> However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release note: none
Release justification: remove old version gates.

Co-authored-by: Rui Hu <rui@cockroachlabs.com>
Co-authored-by: Celia La <celia@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Aug 23, 2022
85825: clusterversion,sql,backupccl: remove PublicSchemasWithDescriptors r=celiala a=celiala

This commit removes the 22.1 version gate `PublicSchemasWithDescriptors`, which was referenced in:

- pkg/ccl/backupccl/
- pkg/ccl/changefeedccl/
- pkg/config/zonepb/zone.go
- pkg/sql/catalog/descs/
- pkg/sql/descriptor.go
- pkg/sql/importer/
- pkg/sql/rename_table.go
- pkg/sql/reparent_database.go

Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)):

> For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

> However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release note: none
Release justification: remove old version gates

86618: distsql: finish the tracing span later in an error case r=yuzefovich a=yuzefovich

Previously, it was possible to run the finished tracing span when
setting up the remote DistSQL flow if that setup encounters an error. In
such a scenario, we need to clean up the resources explicitly, and the
order of that cleanup was incorrect - we would finish the span before
possibly using the context that is still referencing the finished span.
This is now fixed.

Fixes: #86369.

Release justification: bug fix.

Release note: None

Co-authored-by: Celia La <celia@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
arulajmani added a commit to arulajmani/cockroach that referenced this issue Aug 23, 2022
Remove EnsureSPanConfigReconciliation, EnsureSpanConfigSubscription,
and EnableSpanConfigStore.

References cockroachdb#80663
Subsumes cockroachdb#85848

Release justification: cleanup
Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Aug 23, 2022
Remove EnsureSPanConfigReconciliation, EnsureSpanConfigSubscription,
and EnableSpanConfigStore.

References cockroachdb#80663
Subsumes cockroachdb#85848

Release justification: cleanup
Release note: None
craig bot pushed a commit that referenced this issue Aug 25, 2022
86436: kvserver: incorporate remote tracing spans from snapshots r=AlexTalks a=AlexTalks

This adds collected tracing spans into a `SnapshotResponse` object in
order to incorporate remote traces from the receiver side of a snapshot
into the client's (i.e. the sender's) context.

Release justification: Low-risk observability change.
Release note: None

86676: clusterversion, kvserver: remove SpanConfig related version gates r=celiala a=arulajmani

Remove EnsureSPanConfigReconciliation, EnsureSpanConfigSubscription,
and EnableSpanConfigStore.

References #80663
Subsumes #85848

Release justification: cleanup
Release note: None

Co-authored-by: Alex Sarkesian <sarkesian@cockroachlabs.com>
Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
@craig craig bot closed this as completed in fde7aaf Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-master Failures and bugs on the master branch. branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
Projects
None yet
3 participants