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

More flexible destructors [investigation] #583

Open
joshua-berry-ntnx opened this issue Jan 4, 2021 · 6 comments
Open

More flexible destructors [investigation] #583

joshua-berry-ntnx opened this issue Jan 4, 2021 · 6 comments

Comments

@joshua-berry-ntnx
Copy link
Contributor

Similar to what was done in #505, we may want to offer more flexible options for how to destroy an entity. This is a placeholder issue to investigate what can/should be done along these lines.

Some ideas to explore [please feel free to edit/add to this list]:

@joshua-berry-ntnx
Copy link
Contributor Author

Assigning to Nitesh to come up with a proposal for what custom destructors might look like from a user perspective, then we can discuss.

@nitesh-idnani1
Copy link
Contributor

nitesh-idnani1 commented Feb 24, 2021

Based on the use-case discussion for destructors with @ShrikantJadhav-Ntnx, here are the few requirements that I have scoped out:

  • Input parameters in destructors:
    We have requirements for passing some input field to the destructor for an entity, so that we can update the associated entities and store information related to the reason for failure in those.
    This can be achieved in the same way as the constructors input i.e. specify the input schema for the on_delete handler and pass the input in the delete API call.

  • Intentful destructors:
    So far, I don't see any use case for making the destructors intentful, an ideal implementation for destructor would simply be to free up the resource/dependent entities associated with the entity to be deleted and trigger their respective intent resolvers.

  • Pre vs Post delete handler:
    Current implementation uses a pre-delete handler which I think works for the use-case. Regardless, we can transform this to a post-delete handler provided we've sufficient information about the deleted entity in the context.

  • Custom validation:
    Dependent on Feature request: Custom validation of user-provided specs #389, will update with it's progress.

Feel free to add your thoughts to this and I will start working on the new destructor implementation.

@shlomi-nx
Copy link
Contributor

shlomi-nx commented Feb 24, 2021

Destructors generally do not accept any parameters, so I am not sure that adding parameters is a good abstraction. Usually if parameters are needed, there would be a formal method such as close or release that must be called prior to the destructor. We need to dig deeper to understand the requirement for input parameters on destructors. More than that, although RFC does not prohibit bodies on DELETE, it is not a fully supported feature and we shouldn't assume that it is. Some random elements on the way may discard the DELETE body.

The main point to consider is how can a destructor successfully finish in the face of errors. Say the provider wants to implement some form of cascade delete, and during the delete process the process dies. How can the destructor be invoked again on another papiea instance? how would we be able to avoid race conditions?

@joshua-berry-ntnx
Copy link
Contributor Author

Requirements (Nitesh's post)

Intentful destructors

I think intentful destructors could be useful but I'm not sure how they would work. For example, cleaning up a resource may require some configuration change somewhere which could fail, etc. To implement this, though, we would basically have to do some kind of asynchronous deletion where the entity simply disappears at some unspecified time when the intent is resolved, and I think that might be quite surprising to users.

Pre vs Post delete handler

I agree the pre-delete handler is sufficient, and even preferable (because of the cascade-delete example raised by Shlomi).

Premise (Shlomi's post)

Destructors generally do not accept any parameters, so I am not sure that adding parameters is a good abstraction.

I understand the analogy to programming languages, but I think this is a different situation. In a language which practices RAII like C++, destructors are generally implicit because whole idea is the programmer shouldn't have to remember to free the resource at all, and it should happen magically at the right (predictable!) time. That implicitness is what drives the convention that destructors do not throw exceptions, do not take parameters, etc.

But for Papiea, I don't think we have that requirement. Often these are end-user resources we are talking about, where deletion may have significant consequences. So I think it's perfectly fine and even desirable to design for explicit deletion.

We need to dig deeper to understand the requirement for input parameters on destructors.

So in the specific case of Customer-1, we would like clients to be able to provide a reason why an entity is being deleted, because there are some actions we need to take on the back end depending on the reason.

The main point to consider is how can a destructor successfully finish in the face of errors.

I think it's OK to fail the deletion entirely and force the caller to retry. (AFAIK this is what Papiea does now and Customer-1 is already relying on this precisely for the cascade-delete example you mention.)

@nitesh-idnani1
Copy link
Contributor

nitesh-idnani1 commented Mar 1, 2021

Here are the general updates to sum up the discussion so far, and further thoughts on how should the destructors work:

Parameters in delete call:

We decided not to add parameters to the delete call, since the support is not fully completed. Instead, provider capabilities to register and invoke pre-delete handler as a specialized method and keep the delete API call as it is. An example usage for this design should look something like this

Location = new Kind(...)
Location.pre_delete_procedure(input_schema, handler_func)
EntityAPIForLocation.invoke_destructor(input)

Internally, the pre delete procedure would work as an entity procedure taking the input schema and handler func as input and a set name like "__Location_delete" (similar to current conventions). The handler_func for the destructor would perform the cleanup activities for that entity and simply return. Then, it would be the engine's responsibility to delete the entity successfully.

Intentful Destructors:

As per my understanding, the use of intent resolvers is associated with some change in state within the entity. In case of destructors, the change in state that happens is not on an entity rather it's the kind's state that changes (to put it in terms of papiea). So, the destructors are not truly resolving any state change themselves and are only performing actions to trigger intent resolver for other entities (if any). Hence, I'm not entirely convinced about making destructors intentful, and the failure handling part which @joshua-berry-ntnx mentioned should be separated from the intentful discussion, since it is related to handling general operation failures and not just intent resolution failures.

Handling failures in destructors:

I did some research on how to handle failures, more specifically how to rollback state of a system in case of failures and found out two design patterns used to achieve rollback:

Memento pattern [url]:

Involves saving state of an entity, and restoring the state to a previously saved state in case of failures. Example:

destructor_handler(ctx, entity_list, input) {
    // Memento req - Save the entity state
    bucket = GetBucketEntity()
    entity_list.add(bucket)
    
    UpdateBucketEntity()
}

// papiea-engine delete entity method
Delete_Entity(...) {
    try {
        destructor_handler(ctx, entity_list, input)
    } catch {
        // Rollback in case of failure, read from entity list and restore the bucket entity
        rollback(entity_list) 
    }
    DeleteObjectEntity()
}

Command pattern [url]:

destructor_handler(ctx, action_list, input) {
    // Command req - Encapsulate action and entity info to action object
    bucket = GetBucketEntity()
    action_list.add({ UPDATE_ACTION, bucket })
}

// papiea-engine delete entity method
Delete_Entity(...) {
    destructor_handler(ctx, action_list, input)
    action_list.forEach(action => 
        execute(action)
        undo(action if failure)
    )
    DeleteObjectEntity()

}
Involves encapsulating information related to the entity and the operation into an action interface which supports the rollback feature.

In both cases, however we'll have to pass some context to the destructor handler (entity list in case of memento and action object list in case of command). And then in case of failures, invoke the undo/rollback method to restore the state of the system based on the design pattern implementation. I feel the command pattern is more clear and has less overhead in terms of the user actions, since in memento the user will have to save entity in order to rollback successfully.

@joshua-berry-ntnx
Copy link
Contributor Author

IIRC we decided some time back to drop this since HTTP doesn't officially support passing parameters to DELETE. Should this be closed or simply placed on the back burner?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Customer-1's asks
Should have 1.0
Development

No branches or pull requests

3 participants