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.

Sign up for GitHub

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

Jump to bottom

feat(torture): add rollback #706

Merged
merged 1 commit into from
Jan 23, 2025
Merged

feat(torture): add rollback #706

merged 1 commit into from
Jan 23, 2025

Conversation

gabriele-0201
Copy link
Contributor

No description provided.

@gabriele-0201 gabriele-0201 mentioned this pull request Jan 20, 2025
Merged
@gabriele-0201 gabriele-0201 changed the title WIP feat(torture): add rollbacks WIP feat(torture): add rollback Jan 20, 2025
This was referenced Jan 20, 2025
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@gabriele-0201 Graphite App
Copy link
Contributor Author

gabriele-0201 commented Jan 20, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@gabriele-0201 gabriele-0201 force-pushed the gm_torture_add_rollbacks branch 2 times, most recently from 42a45c2 to 76830d5 Compare January 20, 2025 13:54
@gabriele-0201 gabriele-0201 mentioned this pull request Jan 20, 2025
Merged
@gabriele-0201 gabriele-0201 changed the title WIP feat(torture): add rollback feat(torture): add rollback Jan 20, 2025
@pepyakin pepyakin force-pushed the gm_torture_add_cli_snapshot_changeset_checks_options branch from 0e1eb30 to 6f652f9 Compare January 20, 2025 15:30
@pepyakin pepyakin force-pushed the gm_torture_add_rollbacks branch from 76830d5 to 1f3d3dd Compare January 20, 2025 15:31
@pepyakin pepyakin force-pushed the gm_torture_add_cli_snapshot_changeset_checks_options branch from 6f652f9 to c6e9776 Compare January 20, 2025 15:43
@pepyakin pepyakin force-pushed the gm_torture_add_rollbacks branch from 1f3d3dd to 3d2e541 Compare January 20, 2025 15:43
@gabriele-0201 gabriele-0201 force-pushed the gm_torture_add_cli_snapshot_changeset_checks_options branch from c6e9776 to 6531ee1 Compare January 20, 2025 16:20
@gabriele-0201 gabriele-0201 force-pushed the gm_torture_add_rollbacks branch from 3d2e541 to 2ab0811 Compare January 20, 2025 16:21
pepyakin
pepyakin reviewed Jan 20, 2025
View reviewed changes
torture/src/supervisor/cli.rs
#[clap(default_value = "5")]
#[clap(value_parser=clap::value_parser!(u8).range(0..=100))]
#[arg(long = "workload-rollback")]
pub rollback: u8,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this comment when I was reviewing the crash PR, but leaving it here

I don't find this interface intuitive.

When I have the operations: commit, crash, rollback, then setting the rollback 20 and the crash 30 as a user I expect that for each 10 iterations I will have 5 normal commits, 3 crashing commits and 2 rollbacks.

However, the way you choose whether or not to execute is not independent. IOW,

p(crash) ≠ biases.crash,

but rather

p(crash) = (1 - biases.rollback) * biases.crash

As you can see this also depends on the order of the checks in code. So if we add another check it will gain the dependence on the previous checks.

I think this is fine for now and I don't have an idea how to fix this immediatelly. Also biases are kind of inherently dependent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find this interface intuitive.

fair, for sure it can be improved.

p(crash) = (1 - biases.rollback) * biases.crash

That's totally true. I agree that this is not nice.

I will try to implement what you just described in a follow up pr!

@pepyakin pepyakin force-pushed the gm_torture_add_cli_snapshot_changeset_checks_options branch from 6531ee1 to 9b29ab0 Compare January 20, 2025 16:51
@pepyakin pepyakin force-pushed the gm_torture_add_rollbacks branch from 2ab0811 to a29e202 Compare January 20, 2025 16:51
@gabriele-0201 gabriele-0201 force-pushed the gm_torture_add_cli_snapshot_changeset_checks_options branch from 9b29ab0 to 42b968b Compare January 21, 2025 08:50
@gabriele-0201 gabriele-0201 force-pushed the gm_torture_add_rollbacks branch 2 times, most recently from 8a12a5f to 271ed0f Compare January 21, 2025 09:07
This was referenced Jan 21, 2025
Merged
Merged
Merged
Merged
This was referenced Jan 21, 2025
Merged
Merged
@pepyakin pepyakin force-pushed the gm_torture_add_cli_snapshot_changeset_checks_options branch 2 times, most recently from a526f84 to 63728ca Compare January 22, 2025 17:48
@pepyakin pepyakin force-pushed the gm_torture_add_rollbacks branch from 271ed0f to c2a12a6 Compare January 22, 2025 17:48
pepyakin
pepyakin reviewed Jan 22, 2025
View reviewed changes
torture/src/supervisor/workload.rs
async fn spawn_new_agent(&mut self) -> anyhow::Result<()> {
assert!(self.agent.is_none());
controller::spawn_agent_into(&mut self.agent).await?;
let workdir = self.workdir.path().display().to_string();
let rollback = self.state.biases.rollback > 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is fine for now, but in the future we want some workloads to be running with rollback enabled and some with rollback disabled.

Base automatically changed from gm_torture_add_cli_snapshot_changeset_checks_options to master January 23, 2025 08:49
@gabriele-0201 gabriele-0201 force-pushed the gm_torture_add_rollbacks branch from c2a12a6 to ac549d4 Compare January 23, 2025 08:53
@gabriele-0201 gabriele-0201 mentioned this pull request Jan 23, 2025
Merged
@rphmeier Graphite App
Copy link
Contributor

rphmeier commented Jan 23, 2025 •
edited
Loading

Merge activity

  • Jan 23, 2:16 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 23, 2:17 PM EST: A user merged this pull request with Graphite.

@rphmeier rphmeier merged commit 1b2fab8 into master Jan 23, 2025
8 checks passed
@rphmeier rphmeier deleted the gm_torture_add_rollbacks branch January 23, 2025 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepyakin pepyakin pepyakin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gabriele-0201 @rphmeier @pepyakin