oxidecomputer / oxidecomputer/omicron

Another possible race in `oximeter` integration tests

Open
#1,006 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Test Flake
Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

@davepacheco saw this test failure. The relevant bit is here:



4703 | 2022-05-03T18:56:15.474Z | failures:
-- | -- | --
4704 | 2022-05-03T18:56:15.480Z |  
4705 | 2022-05-03T18:56:15.487Z | ---- integration_tests::oximeter::test_oximeter_reregistration stdout ----
4706 | 2022-05-03T18:56:15.493Z | log file: "/var/tmp/omicron_tmp/test_all-1557dfd33414e1a3-test_oximeter_reregistration.5150.46.log"
4707 | 2022-05-03T18:56:15.499Z | note: configured to log to "/var/tmp/omicron_tmp/test_all-1557dfd33414e1a3-test_oximeter_reregistration.5150.46.log"
4708 | 2022-05-03T18:56:15.504Z | thread 'integration_tests::oximeter::test_oximeter_reregistration' panicked at 'assertion failed: `(left == right)`
4709 | 2022-05-03T18:56:15.510Z | left: `Target { name: "integration_target", fields: [Field { name: "name", value: String("integration-test-target") }, Field { name: "name", value: String("integration-test-target") }] }`,
4710 | 2022-05-03T18:56:15.516Z | right: `Target { name: "integration_target", fields: [Field { name: "name", value: String("integration-test-metric") }, Field { name: "name", value: String("integration-test-metric") }] }`', nexus/tests/integration_tests/oximeter.rs:308:5
4711 | 2022-05-03T18:56:15.521Z | note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
4712 | 2022-05-03T18:56:15.527Z | WARN: dropped CockroachInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
4713 | 2022-05-03T18:56:15.534Z | WARN: temporary directory leaked: /var/tmp/omicron_tmp/.tmpKT5h8q
4714 | 2022-05-03T18:56:15.539Z | WARN: dropped ClickHouseInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
4715 | 2022-05-03T18:56:15.545Z |  
4716 | 2022-05-03T18:56:15.551Z |  
4717 | 2022-05-03T18:56:15.557Z | failures:
4718 | 2022-05-03T18:56:15.565Z | integration_tests::oximeter::test_oximeter_reregistration

That assertion should never be hit. We store the field names for a timeseries target and metric in the order they're declared in the Rust struct that defines them. Those are stored in a ClickHouse table called oximeter.timeseries_schema, which includes the fields as nested table. When selecting a row with a nested table, we get back an array for each of the nested columns, guaranteed to be the same length.

Looking at the exact error message, this is quite confusing. This is expected to show that the names of the fields are the same in both cases, since they're not changing during the test. That is we'd, expect two arrays ["foo", "bar"] in both cases. When I first read this, I thought it showed the arrays reversed in the two cases, e.g., ["foo", "bar"] in one and ["bar", "foo"] in the other. What it actually shows is ["foo", "foo"] and ["bar", "bar"]. I'm really not sure how this can even happen, as there is no row in the database with a nested field that has either of those names together. This appears to be consistent with the idea that query returns rows while they're being sorted. (I'm not saying that's what's going on.)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with nexus/tests/integration_tests/oximeter.rs at the failing assertion around line 308 and run integration_tests::oximeter::test_oximeter_reregistration to reproduce the failure. Read oximeter/db/src/db-init.sql around line 152 and investigate how the nested timeseries_schema fields are queried. Done means the suspected race is understood and the test no longer produces inconsistent field arrays.

Written by the indexing model from the issue text.

Assessment

Tech stack
clickhouse, rust
Domain
databases, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.