state store: better handling of job deletion

[pkazmierczak]

When jobs are deleted with -purge, all their deployments and allocations should be deleted from the state store, and the evals status should be set to complete. Otherwise we end up in a situation where users could re-submit previously failing jobs, but these new jobs would not get deployments allocated unless system gc got called.

Fixes #10502

[tgross]

This looks generally good but it's worth trying to reason through what happens if there are allocations on a client that's disconnected when the job is purged. I'm not sure that it's safe to delete allocations until we know they're terminal?

[pkazmierczak]

what happens if there are allocations on a client that's disconnected when the job is purged.

If these are failed allocs, they remain in dead state because there's no scheduler to reschedule their re-run. If they correspond to non-failing jobs, they remain running.

I'm not sure that it's safe to delete allocations until we know they're terminal?

Yeah so this to me is more about the intended behavior of the -purge flag. In my mind, purge should delete any and all allocations on all the clients.

The behavior introduced in this PR makes one unfortunate side-effect, that is, if a client is disconnected while the job gets purged, and if it's a failing job (with alloc-failure evals), after reconnecting this client will leak allocs (in dead state). GC cleans them of course.

[lgfa29]

Some small comments, but one general observation is that we usually have DeleteX() and DeleteXTxn() methods. I think it would nice to keep this pattern here for deployments and allocs.

[tgross]

LGTM once @lgfa29's comments are resolved

[pkazmierczak]

we usually have DeleteX() and DeleteXTxn() methods. I think it would nice to keep this pattern here for deployments and allocs.

Not sure I understand, are you suggesting to rename DeleteJobTxn to DeleteJobDeploymentsAndAllocsTxn or sth like that? This PR doesn't introduce a new method, it just modifies the DeleteJobTxn.

[lgfa29]

we usually have DeleteX() and DeleteXTxn() methods. I think it would nice to keep this pattern here for deployments and allocs.

Not sure I understand, are you suggesting to rename DeleteJobTxn to DeleteJobDeploymentsAndAllocsTxn or sth like that? This PR doesn't introduce a new method, it just modifies the DeleteJobTxn.

Nope, I was suggesting having DeleteJobTxn() look something like this:

func (s *StateStore) DeleteJobTxn(index uint64, namespace, jobID string, txn Txn) error {
  // ...
  for _, deployment := range deployments {
    err := s.DeleteDeploymentTxn(txn, deployment)
    // ...
  }
  // ....
  for _, alloc := range allocs {
    err := s.DeleteAllocTxn(txn, alloc)
    // ...
  }
}

That way the deployment and alloc delete logic is shared and consistent across methods (for example, you wouldn't need to update their index table in DeleteJobTxn).

[pkazmierczak]

That way the deployment and alloc delete logic is shared and consistent across methods (for example, you wouldn't need to update their index table in DeleteJobTxn).

oooh I get it now I think. Implemented in e918e3b, have a look if that's what you meant.

[lgfa29]

Last bit of nit-picking 😄