asim: add assertion, mutation-assertion events #109316

wenyihu6 · 2023-08-23T08:57:57Z

Previously, the simulator relied on test code to create functions to be called
for delayed events. As we want to introduce more dynamic events through the
randomized testing framework, this approach becomes increasingly complex and
error-prone. In addition, we want to introduce new event types, such as
assertion events and mutation-assertion events to validate the simulation’s
state and verify the effects of prior mutation events.

To achieve these goals, the following changes were made in this patch:

Instead of directly adding functions to be invoked to simulator’s
delayEventList, events are now organized into different struct
(SetSpanConfigEvent, AddNodeEvent, SetNodeLivenessEvent, and
SetCapacityOverrideEvent). Each struct is equipped to generate its
corresponding function which can be called externally for event execution. These
events can be scheduled with StaticEvents using ScheduleEvent method with
just the required parameters.
AssertionEvent struct can now be used to schedule assertion events as part
of the simulation using ScheduleEvent.
MutationWithAssertionEvent can also be used now to schedule mutation events
coupled with subsequent assertion events using the
ScheduleMutationWithAssertionEvent method.

Under the hood, these events all implement the event interface. This interface
outlines

Func() returns a function that can be called externally.
String() returns a formatted output string. To accommodate varying function
types returned by Func(), we introduced another interface for different
function types to be used as an event func.

When event executor executes these events at their scheduled time, it retrieves
the functions calling Func() and calls them with the simulator's current state
and history.

Part of: #106192
Release note: none

cockroach-teamcity · 2023-08-23T08:58:20Z

This change is

Previously, the history struct (capturing store metrics at each tick and simulator’s current state) resided within the asim package. Another PR is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events use simulation’s history to conduct assertion checks, the event package would have to depend on the asim package. However, the asim package would also have to depend on the event package’s event executor for event ticking, creating a circular dependency. To address this issue, this patch moves the history component out of the asim package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: cockroachdb#109316 Part of: cockroachdb#106192 Release note: none

Previously, simulation assertions resided within the tests package. Another PR is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events require the assertion structs to conduct assertion checks, the event package would have to depend on the test package. However, the test package would also have to depend on the event package’s exported struct to initialize structures for event generation, creating a circular dependency. To address this issue, this patch moves the assertion component out of the test package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: cockroachdb#109316 Part of: cockroachdb#106192 Release note: none

wenyihu6 · 2023-08-28T16:17:01Z

Please only review the last commit. The preceding commits are from #109337.

Previously, the history struct (capturing store metrics at each tick and simulator’s current state) resided within the asim package. Another PR(cockroachdb#109316) is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events use simulation’s history to conduct assertion checks, the event package would have to depend on the asim package. However, the asim package would also have to depend on the event package’s event executor for event ticking, creating a circular dependency. To address this issue, this patch moves the history component out of the asim package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: cockroachdb#109316 Part of: cockroachdb#106192 Release note: none

Previously, simulation assertions resided within the tests package. Another PR(cockroachdb#109316) is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events require the assertion structs to conduct assertion checks, the event package would have to depend on the test package. However, the test package would also have to depend on the event package’s exported struct to initialize structures for event generation, creating a circular dependency. To address this issue, this patch moves the assertion component out of the test package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: cockroachdb#109316 Part of: cockroachdb#106192 Release note: none

Previously, the history struct (capturing store metrics at each tick and simulator’s current state) resided within the asim package. Another PR(cockroachdb#109316) is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events use simulation’s history to conduct assertion checks, the event package would have to depend on the asim package. However, the asim package would also have to depend on the event package’s event executor for event ticking, creating a circular dependency. To address this issue, this patch moves the history component out of the asim package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: cockroachdb#109316 Part of: cockroachdb#106192 Release note: none

Previously, simulation assertions resided within the tests package. Another PR(cockroachdb#109316) is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events require the assertion structs to conduct assertion checks, the event package would have to depend on the test package. However, the test package would also have to depend on the event package’s exported struct to initialize structures for event generation, creating a circular dependency. To address this issue, this patch moves the assertion component out of the test package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: cockroachdb#109316 Part of: cockroachdb#106192 Release note: none

kvoli

Reviewed 10 of 10 files at r1, 22 of 25 files at r4, 1 of 29 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)

-- commits line 20 at r1:
nit: You can format to just #109316, which will link on github.

pkg/kv/kvserver/asim/event/assertion_event.go line 57 at r4 (raw file):

// when the event executor summarizes the executed events in the end.
func (ag assertionEvent) String() string {
	return ""

Should this be ag.reason instead of ""?

pkg/kv/kvserver/asim/event/mutation_event.go line 88 at r4 (raw file):

func (ae AddNodeEvent) String() string {
	return ""

Should this return an event description?

pkg/kv/kvserver/asim/event/mutation_event.go line 92 at r4 (raw file):

func (sne SetNodeLivenessEvent) Func() EventFunc {
	var fn MutationFunc = func(ctx context.Context, s state.State) {

nit: return inline.

pkg/kv/kvserver/asim/event/mutation_event.go line 102 at r4 (raw file):

func (sne SetNodeLivenessEvent) String() string {
	return "'"

Should this return the event description?

pkg/kv/kvserver/asim/event/mutation_event.go line 106 at r4 (raw file):

func (sce SetCapacityOverrideEvent) Func() EventFunc {
	var fn MutationFunc = func(ctx context.Context, s state.State) {

nit: return inline.

pkg/kv/kvserver/asim/event/mutation_event.go line 114 at r4 (raw file):

func (sce SetCapacityOverrideEvent) String() string {
	return "'"

Same as above.

pkg/kv/kvserver/asim/gen/event_generator.go line 27 at r4 (raw file):

	// Generate returns a list of events, which should be exectued at the delay specified.
	Generate(seed int64, settings *config.SimulationSettings) scheduled.EventExecutor
	String() string /**/

Remove the inline /**/.

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 11 at r4 (raw file):

// licenses/APL.txt.

package scheduled

Should we add some testing for this functionality, or do you feel its sufficiently tested via the existing datadriven test code?

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 28 at r4 (raw file):

// managing scheduled events, allowing event registration, tick-based
// triggering. Please use NewExecutorWithNoEvents for proper initialization.
type EventExecutor interface {

nit: could you also copy these comments onto the implementation? This is a personal preference.

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 34 at r4 (raw file):

	// Start sorts the scheduled list chronologically, ready to execute events for
	// simulation.
	Start()

This doesn't feel necessary as an additional function, given it isn't triggering any sort of continuous runner.

Could you instead maintain a flag on eventExecutor, which is true if ticking has begun, and false otherwise.

That way, you could check the flag and sort them on the first tick.

What do you think?

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 87 at r4 (raw file):

func (e *eventExecutor) PrintEventsExecuted() string {
	return ""

Could you add a TODO here, or add the events executed.

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 135 at r4 (raw file):


# We expect ranges to be randomly allocated across stores with a replication
# factor of 1. Assertion failures on some samples are expected due to this

I think this description could be more specific.

"When the replication factor is 1, rebalancing falls back to adding a replica in order to rebalance. The original replica is expected to transfer its lease, and then be removed. In this setup, it is always possible to be overreplicated if rebalancing is occurring -- as we catch ranges in the middle of rebalancing."

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 139 at r4 (raw file):

# leaseholder, it is locked into the current replica distribution. The
# rebalancer falls back to adding one replica, hoping the rebalancing would
# happen next time this range is checked. This can case thrashing where settling

Not from this PR, but s/case/cause/

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 142 at r4 (raw file):

# is difficult. In addition, we expect all output details to be displayed upon
# test failure.

nit: rm empty line.

pkg/kv/kvserver/asim/tests/datadriven_simulation_test.go line 252 at r4 (raw file):

				scanIfExists(t, d, "stores", &numStores)
				scanIfExists(t, d, "locality", &localityString)
				eventGen.ScheduleEvent(settingsGen.Settings.StartTime, delay, event.AddNodeEvent{

Nice, this is much cleaner now!

109337: asim: extract history, assertion into their own packages r=kvoli a=wenyihu6 **asim: extract history into its own package** Previously, the history struct (capturing store metrics at each tick and simulator’s current state) resided within the asim package. Another PR(#109316) is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events use simulation’s history to conduct assertion checks, the event package would have to depend on the asim package. However, the asim package would also have to depend on the event package’s event executor for event ticking, creating a circular dependency. To address this issue, this patch moves the history component out of the asim package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: #109316 Part of: #106192 Release note: none ---- **asim: extract assertions into its own package** Previously, simulation assertions resided within the tests package. Another PR(#109316) is introducing assertion registration which enables scheduled assertion checks to run as delayed events. Since these assertion events require the assertion structs to conduct assertion checks, the event package would have to depend on the test package. However, the test package would also have to depend on the event package’s exported struct to initialize structures for event generation, creating a circular dependency. To address this issue, this patch moves the assertion component out of the test package to its own package. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. See also: #109316 Part of: #106192 Release note: none ---- **asim: refactor liveness parsing to be handled by scanArg** Previously, the test code directly parsed the string to derive a `livenessStatus`. To make this cleaner, this patch delegates string parsing and creation of `livenessStatus` to `scanArg`. Note that this commit does not change any existing behavior, and the main purpose is to make future commits cleaner. Epic: none Release note: none 109611: sql,schemachanger: disallow dropping indexes when referenced in UDF/View r=Xiang-Gu a=Xiang-Gu This PR disallows schema changes (in both legacy and declarative schema changers) that would drop indexes referenced explicitly via index hinting in UDF or view. They include `DROP INDEX`, `ADD COLUMN`, `DROP COLUMN`, and `ALTER PRIMARY KEY`. Fixes #108974 Epic: None Release note: None Co-authored-by: wenyihu6 <[email protected]> Co-authored-by: Xiang Gu <[email protected]>

wenyihu6 · 2023-08-29T17:06:27Z

pkg/kv/kvserver/asim/event/assertion_event.go line 57 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Should this be ag.reason instead of ""?

Printing output issues are handled in this subsequent PR #109602. It also tests the changes introduced in this PR.

wenyihu6 · 2023-08-29T18:01:14Z

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 11 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Should we add some testing for this functionality, or do you feel its sufficiently tested via the existing datadriven test code?

Changes introduced in this PR is tested in #109602 where mutation and assertion events are executed.

wenyihu6 · 2023-08-29T18:02:43Z

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 135 at r4 (raw file):

Previously, kvoli (Austen) wrote…

I think this description could be more specific.

"When the replication factor is 1, rebalancing falls back to adding a replica in order to rebalance. The original replica is expected to transfer its lease, and then be removed. In this setup, it is always possible to be overreplicated if rebalancing is occurring -- as we catch ranges in the middle of rebalancing."

opps resolving merge conflicts messed this comment up. Didn't mean to change this. Is the current wording better?

wenyihu6 · 2023-08-29T18:02:48Z

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 139 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Not from this PR, but s/case/cause/

Done.

wenyihu6 · 2023-08-29T18:02:54Z

pkg/kv/kvserver/asim/tests/datadriven_simulation_test.go line 252 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Nice, this is much cleaner now!

Done.

wenyihu6

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)

pkg/kv/kvserver/asim/event/mutation_event.go line 88 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Should this return an event description?

Same as above (Resolved in #109602).

pkg/kv/kvserver/asim/event/mutation_event.go line 102 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Should this return the event description?

Same as above (Resolved in #109602).

pkg/kv/kvserver/asim/event/mutation_event.go line 114 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Same as above.

Same as above (Resolved in #109602).

pkg/kv/kvserver/asim/gen/event_generator.go line 27 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Remove the inline /**/.

Done.

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 87 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Could you add a TODO here, or add the events executed.

Same as above (Resolved in #109602).

wenyihu6 · 2023-08-29T18:15:00Z

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 34 at r4 (raw file):

Previously, kvoli (Austen) wrote…

This doesn't feel necessary as an additional function, given it isn't triggering any sort of continuous runner.

Could you instead maintain a flag on eventExecutor, which is true if ticking has begun, and false otherwise.

That way, you could check the flag and sort them on the first tick.

What do you think?

Agreed. I added a flag called hasStarted in eventExecutor which does that.

kvoli

minor nits on the commit message.

Nice stuff!

Reviewed 20 of 25 files at r8, 9 of 11 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)

-- commits line 4 at r9:
nit: the simulator relied on

-- commits line 11 at r9:
nit the following changes were made

pkg/kv/kvserver/asim/scheduled/scheduled_event_executor.go line 34 at r4 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

Agreed. I added a flag called hasStarted in eventExecutor which does that.

Thanks!

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 135 at r4 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

opps resolving merge conflicted messed this comment up. Didn't mean to change this. Is the current wording better?

Looks good!

pkg/kv/kvserver/asim/tests/datadriven_simulation_test.go line 252 at r4 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

Done.

?

Previously, the simulator relied on test code to create functions to be called for delayed events. As we want to introduce more dynamic events through the randomized testing framework, this approach becomes increasingly complex and error-prone. In addition, we want to introduce new event types, such as assertion events and mutation-assertion events to validate the simulation’s state and verify the effects of prior mutation events. To achieve these goals, the following changes were made in this patch: 1. Instead of directly adding functions to be invoked to simulator’s delayEventList, events are now organized into different struct (`SetSpanConfigEvent`, `AddNodeEvent`, `SetNodeLivenessEvent`, and `SetCapacityOverrideEvent`). Each struct is equipped to generate its corresponding function which can be called externally for event execution. These events can be scheduled with `StaticEvents` using `ScheduleEvent` method with just the required parameters. 2. `AssertionEvent` struct can now be used to schedule assertion events as part of the simulation using `ScheduleEvent`. 3. `MutationWithAssertionEvent` can also be used now to schedule mutation events coupled with subsequent assertion events using the `ScheduleMutationWithAssertionEvent` method. Under the hood, these events all implement the event interface. This interface outlines 1. Func() returns a function that can be called externally. 2. String() returns a formatted output string. To accommodate varying function types returned by `Func()`, we introduced another interface for different function types to be used as an event func. When event executor executes these events at their scheduled time, it retrieves the functions calling Func() and calls them with the simulator's current state and history. Part of: cockroachdb#106192 Release note: none

wenyihu6 · 2023-08-29T18:28:21Z

pkg/kv/kvserver/asim/tests/datadriven_simulation_test.go line 252 at r4 (raw file):

Previously, kvoli (Austen) wrote…

?

Was trying to clear "to_reply" on reviewable to zero haha

wenyihu6 · 2023-08-29T19:41:05Z

TFTR!

bors r=kvoli

craig · 2023-08-29T20:47:41Z

Build succeeded:

Bazel Essential CI (Cockroach)

wenyihu6 force-pushed the event-register-new branch from 79b1810 to 50ea4a7 Compare August 23, 2023 11:05

wenyihu6 mentioned this pull request Aug 23, 2023

asim: extract history, assertion into their own packages #109337

Merged

wenyihu6 force-pushed the event-register-new branch from 50ea4a7 to a2d0bff Compare August 28, 2023 15:14

wenyihu6 changed the title ~~[wip] asim: add event and assertion registration~~ asim: add assertion, mutation-assertion events Aug 28, 2023

wenyihu6 mentioned this pull request Aug 28, 2023

asim: enable event series generation #109602

Merged

wenyihu6 force-pushed the event-register-new branch from a2d0bff to 10d828f Compare August 28, 2023 17:42

wenyihu6 self-assigned this Aug 28, 2023

wenyihu6 requested a review from kvoli August 28, 2023 22:44

wenyihu6 force-pushed the event-register-new branch 2 times, most recently from 61dca6b to 2ca810f Compare August 29, 2023 14:37

kvoli requested changes Aug 29, 2023

View reviewed changes

wenyihu6 force-pushed the event-register-new branch from 2ca810f to c4a0dfc Compare August 29, 2023 17:58

wenyihu6 commented Aug 29, 2023

View reviewed changes

wenyihu6 force-pushed the event-register-new branch from c4a0dfc to 2b6b798 Compare August 29, 2023 18:14

wenyihu6 requested a review from kvoli August 29, 2023 18:15

wenyihu6 marked this pull request as ready for review August 29, 2023 18:15

wenyihu6 requested a review from a team as a code owner August 29, 2023 18:15

wenyihu6 force-pushed the event-register-new branch from 2b6b798 to 67831fb Compare August 29, 2023 18:17

kvoli approved these changes Aug 29, 2023

View reviewed changes

wenyihu6 force-pushed the event-register-new branch from 67831fb to 843943d Compare August 29, 2023 18:26

craig bot merged commit 72636bd into cockroachdb:master Aug 29, 2023

wenyihu6 deleted the event-register-new branch August 29, 2023 20:49

wenyihu6 mentioned this pull request Sep 20, 2023

asim: add zone config satisfiability check #110968

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asim: add assertion, mutation-assertion events #109316

asim: add assertion, mutation-assertion events #109316

wenyihu6 commented Aug 23, 2023 •

edited

Loading

cockroach-teamcity commented Aug 23, 2023

wenyihu6 commented Aug 28, 2023

kvoli left a comment

wenyihu6 commented Aug 29, 2023

wenyihu6 commented Aug 29, 2023 •

edited

Loading

wenyihu6 commented Aug 29, 2023 •

edited

Loading

wenyihu6 commented Aug 29, 2023

wenyihu6 commented Aug 29, 2023

wenyihu6 left a comment

wenyihu6 commented Aug 29, 2023

kvoli left a comment

wenyihu6 commented Aug 29, 2023

wenyihu6 commented Aug 29, 2023

craig bot commented Aug 29, 2023

asim: add assertion, mutation-assertion events #109316

asim: add assertion, mutation-assertion events #109316

Conversation

wenyihu6 commented Aug 23, 2023 • edited Loading

cockroach-teamcity commented Aug 23, 2023

wenyihu6 commented Aug 28, 2023

kvoli left a comment

Choose a reason for hiding this comment

wenyihu6 commented Aug 29, 2023

wenyihu6 commented Aug 29, 2023 • edited Loading

wenyihu6 commented Aug 29, 2023 • edited Loading

wenyihu6 commented Aug 29, 2023

wenyihu6 commented Aug 29, 2023

wenyihu6 left a comment

Choose a reason for hiding this comment

wenyihu6 commented Aug 29, 2023

kvoli left a comment

Choose a reason for hiding this comment

wenyihu6 commented Aug 29, 2023

wenyihu6 commented Aug 29, 2023

craig bot commented Aug 29, 2023

wenyihu6 commented Aug 23, 2023 •

edited

Loading

wenyihu6 commented Aug 29, 2023 •

edited

Loading

wenyihu6 commented Aug 29, 2023 •

edited

Loading