roachtests: fix 5x upreplication with timeseries zonecfg range

[nvanbenschoten]

Fixes #127496.
Fixes #127497.
Fixes #127500.

This commit fixes the following roachtests, which were all attempting to configure 5x replication on all ranges, and broke due to 59f5cb9:

• decommission/slow
• drain-and-decommission/nodes=9
• slow-drain/duration=1m0s

The fact that tests broke due to that commit deserve some discussion, independent of this fix. Was that an expected result of that change? Do users now need to configure zone configurations differently with the presence of the default timeseries zone? Will that breaking change be documented as such? The commit does not have release notes; should it?

Release note: None

[cockroach-teamcity]

This change is 

[andrewbaptist]

@nvanbenschoten Thanks for fixing these test breakages.

@rafiss and @fqazi - What are your thoughts on Nathan's questions. I think the important questions are

1. Process: How do we prevent breaking roachtests as a change of default values. The three listed here for sure but is there an easier way to gain confidence that we can make a change that impacts "default" behavior and have it do the right thing.

2. This seems like it should have a release note at a minimum. Should we create a "dummy PR" with just a release note to capture the change in behavior.

3. Its not clear if this only applies to new systems or will impact existing systems? I think only new systems, but is that desired? Did we do it this way because its easy or because we don't want to modify existing systems. This would be more clear if there was a release note attached.

4. Do we think the new behavior is "better" than the old behavior. It is definitely more explicit, but is it what users expect. I don't have a strong opinion whether this helps, but partly this is due to the way zone configs apply and work.

[rafiss]

Process: How do we prevent breaking roachtests as a change of default values.

I believe the only way to be sure of this is to run every roachtest whenever any default value is changed. I don't believe this is necessary. It seems like the process is working as intended, as we've made a decision to only run these tests nightly.

---

This seems like it should have a release note at a minimum. Should we create a "dummy PR" with just a release note to capture the change in behavior.

That sounds like a good idea to me.

---

Its not clear if this only applies to new systems or will impact existing systems? I think only new systems, but is that desired? Did we do it this way because its easy or because we don't want to modify existing systems. This would be more clear if there was a release note attached.

It only applies to new clusters that are bootstrapped.

It does not seem desirable to modify existing clusters, since that could result in an unexpected, or perhaps even incompatible, change. I am thinking of the case where a user has already modified the default range zoneconfig. Then adding this new timeseries range zone config would result in a behavior change in the cluster, and there would be no way to opt-out of that during the cluster upgrade process.

---

Do we think the new behavior is "better" than the old behavior. It is definitely more explicit, but is it what users expect. I don't have a strong opinion whether this helps, but partly this is due to the way zone configs apply and work.

My understanding was that #123762 was filed because you were making the case that this new behavior would be better. I'll quote the reasoning from that issue for ease of discussion:

On a system creation, we should create an explicit timeseries zone config that exactly matches the RANGE default zone config. This would prevent confusion and allow users to make the right decisions on what RF they want. Otherwise users who don't carefully read the documentation are not likely to consider this range.

Since I only created #127034 to address this request, if the feeling is now that there is no strong desire to address that issue, then we can revert #127034 and keep #123762 closed. (Also, see the related issue #125144 in case you weren't aware.)

[rafiss]

regardless of the outcome of the above discussion, this PR is safe to merge and won't cause a problem no matter what we decide next. lgtm, and thanks for tracking down the fix!

[nvanbenschoten]

I'll merge this PR for now to deflake the tests. We can continue the discussion of next steps for this change next week.

bors r+

[craig]

Build succeeded:

• acceptance
• macos_amd64_build
• linux_amd64_fips_build
• local_roachtest_fips
• docker_image_amd64
• unit_tests
• linux_amd64_build
• macos_arm64_build
• check_generated_code
• local_roachtest
• windows_build
• examples_orms
• lint
• linux_arm64_build