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.

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

Endpoint to allow users to delete pending on-demand tasks #1653

Merged
merged 8 commits into from Dec 7, 2017

Conversation

Projects
None yet
3 participants
@pschoenfelder
Copy link
Contributor

commented Nov 16, 2017

No description provided.

@pschoenfelder pschoenfelder requested a review from ssalinas Nov 16, 2017

@ssalinas ssalinas added this to the 0.19.0 milestone Nov 17, 2017

SingularityTaskRequest taskRequest = getPendingTask(user, taskId);
SingularityRequest request = taskRequest.getRequest();

authorizationHelper.checkForAuthorization(request, user, SingularityAuthorizationScope.ADMIN);

This comment has been minimized.

Copy link
@ssalinas

ssalinas Nov 17, 2017

Member

this will need to be check by request and a scope of WRITE. The user just needs to be authorized for the request they are operating on, not necessarily for admin actions

authorizationHelper.checkForAuthorization(request, user, SingularityAuthorizationScope.ADMIN);
checkBadRequest(request.getRequestType() == RequestType.ON_DEMAND, "Only ON_DEMAND tasks may be deleted.");

taskManager.deletePendingTask(taskRequest.getPendingTask().getPendingTaskId());

This comment has been minimized.

Copy link
@ssalinas

ssalinas Nov 17, 2017

Member

This is a very basic way of doing this. However, it opens us up to some odd cases of inconsistent state with the pollers. Here's an example:

  • Two Singularity scheduler instances are running, 1 is the leader 2 is just serving API calls
  • The leader grabs the list of pending tasks from zk to schedule them
  • API call comes in to delete pending task 123 on instance 2, and is removed from zk
  • The leader may schedule it, or maybe fail to, and if it tries to delete it is no longer there

For this particular case, I think we would still get the desired outcome in the end. However, it is good to think about how readable and debuggable this is. An alternative solution is to create a new node in zk using the TaskManager, for pending tasks that are marked as deleted. Then, on the next run of the poller (which normally consumes pending tasks), the actual delete can take place. This keeps all of the change of state with the leader to make it more consistent and easier to reason about if something were to go wrong and we had to debug it.

This comment has been minimized.

Copy link
@pschoenfelder

pschoenfelder Nov 17, 2017

Author Contributor

To clarify, is the code here correctly limiting state change to the leader? i.e. Is the primary concern debugability/semantic cohesiveness when looking at the zk structure?

This comment has been minimized.

Copy link
@ssalinas

ssalinas Nov 17, 2017

Member

In general Singularity's model of the pollers is based on the fact that certain things can only be edited/deleted by the leading instance. This way the state is easier to manage since we don't have to think about another server editing something we just loaded into memory to operate on.

The code here as it stands will run on whatever instance gets hit by the api call, which may or may not be the current leader. Even if we hit the leader we could have problems of state in that the api call would run on a different thread than the poller. So, the api call thread could be deleting while the poller is operating on the same data.

So the primary concern in this case is keeping the state (i.e. list of pending/active tasks) clean by only allowing the leader to operate it in a locked way.

return maybeProxyToLeader(requestContext, Void.class, null, () -> deleteScheduledTask(taskId, user));
}

private Void deleteScheduledTask(String taskId, SingularityUser user) {

This comment has been minimized.

Copy link
@ssalinas

ssalinas Nov 21, 2017

Member

Two things here:

  • We should keep this public instead of private. These methods are called when the endpoint is hit, so I would generally consider them publicly accessible. It's also useful to be able to test the methods for endpoints end to end (i.e. with the validation/etc) in other parts of the code when needed
  • Rather than returning a void, it might be useful to return an actual object. If you look in other DELETE methods in Singularity we generally return some sort of 'receipt' for the delete. For example, killing a task returns the task cleanup object that was created. Deleting a request returns the request data that was deleted. For this case maybe we can return the SingularityPendingTaskId that is being deleted?
}

private Void deleteScheduledTask(String taskId, SingularityUser user) {
SingularityTaskRequest taskRequest = getPendingTask(user, taskId);

This comment has been minimized.

Copy link
@ssalinas

ssalinas Nov 21, 2017

Member

getPendingTask is the other resource method on this class that does it's own separate checks and validations before returning a value. Rather than relying on that endpoint's checks, we should prefer interacting directly with task manager here. The situations in which each endpoint are used can vary greatly, so we would generally prefer interacting with the data vs interacting with another 'endpoint' (even though that endpoint here is just another method not an api call). This way our new endpoint can manage its own behavior for situations like when the pending task is not found, and not have to worry about catching and processing exceptions from the other endpoint to determine what happened.

taskManager has a getPendingTask as well, that will return an Optional, making our check for a pending task that wasn't found easier. Also, related to the comment above about return type, we can then more easily return an Optional<SingularityPendingTaskId> in the case where the pending task didn't exist. (As a note, jax-rs automatically maps an absent/empty Optional to a 404, whereas a Void will be mapped to a 204)

Also, we don't need the full SingularityTaskRequest data, so calling TaskResource.getPendingTask is doing extra work. The authorization helper has a checkForAuthorizationByRequestId method, and the pending task id has the request id present in it. So, we don't actually need the extra task requests call that TaskResource.getPendingTask is doing.

This comment has been minimized.

Copy link
@pschoenfelder

pschoenfelder Nov 21, 2017

Author Contributor

So returning an Optional<X> from an endpoint will automatically produce 404's, eliminating the need to do something like checkNotFound(x.isPresent(), "Couldn't find something");?

This comment has been minimized.

Copy link
@pschoenfelder

pschoenfelder Nov 21, 2017

Author Contributor

I think we still need the SingularityTaskRequest because we're checking ON_DEMAND type in addition to auth. Is there another way to check type without that?

This comment has been minimized.

Copy link
@ssalinas

ssalinas Nov 21, 2017

Member

Ah, forgot about the type check. You're correct we need more info. We need the SingularityRequest, but not necessarily the SingularityDeploy (which also comes with the task request and can be a lot more data). You can use the requestManager to grab that (and consequently continue using the same authorizer method so it doesn't have to re-fetch the request from the id)

@@ -10,8 +10,8 @@ master:
net: host
environment:
MESOS_ZK: zk://localhost:2181/mesos
MESOS_HOSTNAME: localhost
MESOS_IP: 127.0.0.1
MESOS_HOSTNAME: 192.168.99.100

This comment has been minimized.

Copy link
@ssalinas

ssalinas Nov 21, 2017

Member

Have a fix for this in #1655 , I'll merge through to master so you can pull it in

Paul Schoenfelder added some commits Nov 21, 2017

Paul Schoenfelder

@pschoenfelder pschoenfelder changed the title WIP: Endpoint to allow users to delete pending on-demand tasks Endpoint to allow users to delete pending on-demand tasks Nov 27, 2017

@pschoenfelder pschoenfelder requested a review from kwm4385 Nov 27, 2017

};

static defaultProps = {
children: (

This comment has been minimized.

Copy link
@kwm4385

kwm4385 Nov 27, 2017

Contributor

Defining the children here seems weird, especially since the component is only being used in one place. Do we need the ability to override the button's contents? Either way, I would move the logic into the render function rather than defaultProps.

This comment has been minimized.

Copy link
@pschoenfelder

pschoenfelder Nov 27, 2017

Author Contributor

No override necessary that I can think of. Curious, why is it done this way in the PauseButton?

This comment has been minimized.

Copy link
@kwm4385

kwm4385 Nov 28, 2017

Contributor

I'm not sure, this UI has a lot of demons ha. I don't blame you for copying it though. In this case I think it's fine if you just hardcode it.

This comment has been minimized.

Copy link
@pschoenfelder

pschoenfelder Nov 29, 2017

Author Contributor

Ahh, figured out why it's defined here - the getClickComponent hooks up the onClick logic based on props.children. I'll leave for now unless you have better alternative?

This comment has been minimized.

Copy link
@kwm4385

kwm4385 Nov 29, 2017

Contributor

That's fine if it'll be a bigger refactor. Better to have it be consistent for now.

};

render() {
if (this.props.requestType == "ON_DEMAND") {

This comment has been minimized.

Copy link
@kwm4385

kwm4385 Nov 27, 2017

Contributor

Rather than making this check here, I would only render the button in the first place if the type is ON_DEMAND. Then you don't need to pass the requestType as a prop at all.

This comment has been minimized.

Copy link
@pschoenfelder

pschoenfelder Nov 27, 2017

Author Contributor

Since the DeletePendingTaskButton is placed inside the ScheduledActions constant, how can I make it dynamic?

This comment has been minimized.

Copy link
@kwm4385

kwm4385 Nov 28, 2017

Contributor

You could wrap it in a conditional, like

{cellData.pendingTask.pendingTaskId.id &&
    <DeletePendingTaskButton 
        taskId={cellData.pendingTask.pendingTaskId.id}
        requestType={cellData.request.requestType}
    />
}

render() {
return (
<FormModal

This comment has been minimized.

Copy link
@kwm4385

kwm4385 Nov 27, 2017

Contributor

Do we not already have a confirm modal? Feels bad doing it as a FormModal with no fields.

This comment has been minimized.

Copy link
@pschoenfelder

pschoenfelder Nov 27, 2017

Author Contributor

Ah yes, we do have one! I'll swap it out.

@@ -307,6 +308,9 @@ export const ScheduledActions = (
cellRender={(cellData) => (
<div className="hidden-xs">
<RunNowButton requestId={cellData.pendingTask.pendingTaskId.requestId} />
<DeletePendingTaskButton taskId={cellData.pendingTask.pendingTaskId.id}

This comment has been minimized.

Copy link
@kwm4385

kwm4385 Nov 27, 2017

Contributor

Style: taskId should be on a new line here.

@pschoenfelder pschoenfelder added the hs_qa label Nov 30, 2017

@ssalinas

This comment has been minimized.

Copy link
Member

commented Nov 30, 2017

🚢

@pschoenfelder

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

🚢

@ssalinas ssalinas merged commit 6e50c50 into master Dec 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ssalinas ssalinas deleted the delete_pending_on_demand_tasks branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.