Conversation
The sp_ereport_ingester background task runs on a 30-second timer and ingests ereports from simulated SPs into the database. The support bundle collection test asserts that zero ereports are found, but if the ingester fires before the bundle collector runs, it populates the DB with 11 ereports from the simulated SPs (configured in sp_sim_config.test.toml), causing the assertion to fail nondeterministically. Fix this by disabling the sp_ereport_ingester in the test config. The ingester has its own dedicated tests that construct it directly, so no test coverage is lost. Comments are added at both assertion sites explaining the dependency on the ingester being disabled.
Gah, sorry. This one's my fault. 😬 |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
So, the change to lose the ereport ingester task's output in test_omdb_success_cases is a bit of a bummer, I kinda wonder if we could have those tests edit the config file to turn the task back on or something? But if that's too annoying, I'd definitely rather fix the flake than test the actual output...
| total ereports received: <TOTAL_EREPORTS_RECEIVED_REDACTED> | ||
| new ereports ingested: <NEW_EREPORTS_INGESTED_REDACTED> | ||
| total HTTP requests sent: <TOTAL_HTTP_REQUESTS_SENT_REDACTED> | ||
| total collection errors: <TOTAL_COLLECTION_ERRORS_REDACTED> | ||
| reporters with ereports: <REPORTERS_WITH_EREPORTS_REDACTED> | ||
| reporters with collection errors: <REPORTERS_WITH_COLLECTION_ERRORS_REDACTED> |
There was a problem hiding this comment.
Hm, losing this is a bit of a shame, I'd really much rather see the more realistic output in the test output here...
There was a problem hiding this comment.
I can tweak the omdb test to explicitly enable this task - which will preserve this output
I do think "random ereports might be injected to your test" could cause some unexpected downstream effects, so disabling sp_ereport_ingester seems like a reasonable default, but the omdb tests seem totally clear to opt-into it.
| webhook_deliverator.second_retry_backoff_secs = 20 | ||
| read_only_region_replacement_start.period_secs = 999999 | ||
| sp_ereport_ingester.period_secs = 30 | ||
| sp_ereport_ingester.disable = true |
There was a problem hiding this comment.
Would maybe like to see a comment here explaining why this task is disabled for tests.
| // Use a custom ControlPlaneBuilder so we can enable background tasks | ||
| // which might otherwise be disabled. | ||
| // We want it enabled here so the omdb is realistic. | ||
| let cptestctx = | ||
| nexus_test_utils::ControlPlaneBuilder::new("test_omdb_success_cases") | ||
| .with_extra_sled_agents(1) | ||
| .customize_nexus_config(&|config| { | ||
| config.pkg.background_tasks.sp_ereport_ingester.disable = false; | ||
| }) | ||
| .start::<omicron_nexus::Server>() | ||
| .await; |
There was a problem hiding this comment.
Thanks, this looks good to me! I agree with your comment on matrix that in the long term, it would be better to just not have the simulated emit the same ereports i nevery test, but this seems good in the meantime.
2 participants
The sp_ereport_ingester background task runs on a 30-second timer and ingests ereports from simulated SPs into the database. The support bundle collection test asserts that zero ereports are found, but if the ingester fires before the bundle collector runs, it populates the DB with 11 ereports from the simulated SPs (configured in sp_sim_config.test.toml), causing the assertion to fail.
Fix this by disabling the sp_ereport_ingester in the test config. The ingester has its own dedicated tests that construct it directly, so no test coverage is lost. Comments are added at both assertion sites explaining the dependency on the ingester being disabled.
Fixes #10179