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

release-23.1: cli/interactive_tests: re-enable sql_mem_monitor test #106656

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

yuzefovich
Copy link
Member

Backport 1/1 commits from #106566.

/cc @cockroachdb/release


This commit re-enables sql_mem_monitor CLI interactive test that has been skipped for some time. The test verifies that SQL memory accounting system prevents the server crashing when a memory intensive query is executed. In particular, it runs the query twice:

  • the first run has memory monitoring disabled, so the crash is expected
  • the second run sets low root SQL monitor limit and monitoring enabled, so the query errors out without crashing the server.

I ran this test quite a few times and got a couple of failures, and each of them was due to the server not crashing on the first run. I believe this was because the test has rotted over time: the query is performing a cross join that can now spill to disk (it couldn't when the test was written), so it seems feasible that the server wouldn't crash as the test expects. This commit goes around this by effectively disabling the memory accounting system (it sets very high root SQL memory limit) as well as disk spilling (by setting very high distsql_workmem limit).

This commit also adds a couple of other errors I saw into the allow-list of how the server can crash:

  • _Cfunc_calloc - seems reasonable enough
  • fatal error: unexpected signal during runtime execution might seem like it doesn't belong here, but it's an artifact of how we're limiting the memory usage of the CRDB process. In particular, ulimit is a "user limit" which is enforced not at the OS kernel level (i.e. not via oomkiller) but at the process execution level. As a result, memory allocation error doesn't kill the process, so it keeps on running and can hit this "fatal error" later on.

Fixes: #106462.

Release note: None

Release justification: test-only change.

This commit re-enables `sql_mem_monitor` CLI interactive test that has
been skipped for some time. The test verifies that SQL memory accounting
system prevents the server crashing when a memory intensive query is
executed. In particular, it runs the query twice:
- the first run has memory monitoring disabled, so the crash is expected
- the second run sets low root SQL monitor limit and monitoring enabled,
so the query errors out without crashing the server.

I ran this test quite a few times and got a couple of failures, and each
of them was due to the server not crashing on the first run. I believe
this was because the test has rotted over time: the query is performing
a cross join that can now spill to disk (it couldn't when the test was
written), so it seems feasible that the server wouldn't crash as the
test expects. This commit goes around this by effectively disabling the
memory accounting system (it sets very high root SQL memory limit) as
well as disk spilling (by setting very high `distsql_workmem` limit).

This commit also adds a couple of other errors I saw into the allow-list
of how the server can crash:
- `_Cfunc_calloc` - seems reasonable enough
- `fatal error: unexpected signal during runtime execution` might seem
like it doesn't belong here, but it's an artifact of how we're limiting
the memory usage of the CRDB process. In particular, ulimit is a "user
limit" which is enforced not at the OS kernel level (i.e. not via
oomkiller) but at the process execution level. As a result, memory
allocation error doesn't kill the process, so it keeps on running and
can hit this "fatal error" later on.

Release note: None
@yuzefovich yuzefovich requested a review from rafiss July 12, 2023 03:49
@yuzefovich yuzefovich requested a review from a team as a code owner July 12, 2023 03:49
@blathers-crl
Copy link

blathers-crl bot commented Jul 12, 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

@yuzefovich yuzefovich merged commit 314b73f into cockroachdb:release-23.1 Jul 12, 2023
@yuzefovich yuzefovich deleted the backport23.1-106566 branch July 12, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants