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

fix: resolve indefinitely queued (STOPPING_COMPLETED) trials #9605

Merged
merged 6 commits into from
Jul 18, 2024
Merged

fix: resolve indefinitely queued (STOPPING_COMPLETED) trials #9605

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b575482
fix: resolve indefinitely queued (STOPPING_COMPLETED) trials
carolinaecalderon Jun 24, 2024
43dc1a1
switching experiment to see if that helps
carolinaecalderon Jul 16, 2024
6a6643b
fixing test
carolinaecalderon Jul 16, 2024
3ba496e
linting
carolinaecalderon Jul 16, 2024
05357f8
fixing up test
carolinaecalderon Jul 16, 2024
6d109be
edits
carolinaecalderon Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions e2e_tests/tests/cluster/test_master_restart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,42 @@ def test_master_restart_reattach_recover_experiment_k8s(
_test_master_restart_reattach_recover_experiment(k8s_managed_cluster, downtime)


@pytest.mark.managed_devcluster
def test_master_agent_restart_reattach_recover_experiment(
restartable_managed_cluster: managed_cluster.ManagedCluster,
) -> None:
sess = api_utils.user_session()

try:
# Start an experiment
exp_id = exp.create_experiment(
sess,
conf.fixtures_path("no_op/single-medium-train-step.yaml"),
conf.fixtures_path("no_op"),
None,
)

# exp.wait_for_experiment_state(sess, exp_id, bindings.experimentv1State.RUNNING)
carolinaecalderon marked this conversation as resolved.
Show resolved Hide resolved

# Kill the agent & master
restartable_managed_cluster.kill_agent()
Copy link
Contributor

Choose a reason for hiding this comment

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

does the order matter here that which one dies first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if you try to kill the agent after the master, it gives you an error (can't remember what it is off the top of my head though)

restartable_managed_cluster.kill_master()

# Restart the agent & master
restartable_managed_cluster.restart_master()
restartable_managed_cluster.restart_agent(True)

exp.wait_for_experiment_state(
sess, exp_id, bindings.experimentv1State.COMPLETED, max_wait_secs=60
)
trials = exp.experiment_trials(sess, exp_id)
assert (trials[0].trial.state) == bindings.trialv1State.COMPLETED
except Exception:
restartable_managed_cluster.restart_master()
restartable_managed_cluster.restart_agent()
raise


@pytest.mark.managed_devcluster
def test_master_restart_generic_task(
managed_cluster_restarts: managed_cluster.ManagedCluster,
Expand Down
7 changes: 4 additions & 3 deletions master/internal/trial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,9 +748,10 @@ func (t *trial) transition(s model.StateWithReason) error {
})
default:
if action, ok := map[model.State]task.AllocationSignal{
model.StoppingCanceledState: task.TerminateAllocation,
model.StoppingKilledState: task.KillAllocation,
model.StoppingErrorState: task.KillAllocation,
model.StoppingCompletedState: task.TerminateAllocation,
model.StoppingCanceledState: task.TerminateAllocation,
model.StoppingKilledState: task.KillAllocation,
model.StoppingErrorState: task.KillAllocation,
carolinaecalderon marked this conversation as resolved.
Show resolved Hide resolved
}[t.state]; ok {
t.syslog.Infof("decided to %s trial", action)
err := task.DefaultService.Signal(*t.allocationID, action, s.InformationalReason)
Expand Down
5 changes: 3 additions & 2 deletions master/internal/trial_intg_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ func TestTrial(t *testing.T) {
Complete: false,
Closed: true,
}))
require.True(t, alloc.AssertExpectations(t))
require.NotNil(t, tr.allocationID)

// Running stage.
require.NoError(t, tr.PatchSearcherState(experiment.TrialSearcherState{
Expand All @@ -57,6 +55,8 @@ func TestTrial(t *testing.T) {
Complete: true,
Closed: true,
}))
require.True(t, alloc.AssertExpectations(t))
require.NotNil(t, tr.allocationID)

dbTrial, err := internaldb.TrialByID(context.TODO(), tr.id)
require.NoError(t, err)
Expand Down Expand Up @@ -135,6 +135,7 @@ func setup(t *testing.T) (
"StartAllocation", mock.Anything, mock.Anything, mock.Anything,
mock.Anything, mock.Anything, mock.Anything, mock.Anything,
).Return(nil)
as.On("Signal", mock.Anything, mock.Anything, mock.Anything).Return(nil)

a, _, _ := setupAPITest(t, nil)
j := &model.Job{JobID: model.NewJobID(), JobType: model.JobTypeExperiment}
Expand Down
Loading