canonical
Derive a context for use in operations
Operations should never be using the request context inside their run function body. The request context may have been cancelled before the operation is started.
When calling operations.OperationCreate, a context is passed in which is used to set the operation Requestor. The original context is not persisted beyond this.
This PR changes the operations.Operation to remove the cancel.Canceller in favour of using a cancellable context. The operation context is just a background context with some key information copied from the input context (e.g. the Requestor). Operations can still monitor the daemon shutdown context via their state field if necessary.
I've then updated all operations in LXD to use (Operation).Context() wherever a context.Context argument is called for inside an operation.
This is another part of https://github.com/canonical/lxd/issues/16613#issuecomment-3547843653
I'm not sure if we even need cancel.HTTPRequestCanceller anywhere in the code base. Although I notice, with dismay, that its part of the public SDK via BackupFileRequest and ImageFileRequest structs.
Just looked into this. What a nightmare. We can't change this really without introducing a context.Context parameter across the whole client interface.
I'm not sure if we even need cancel.HTTPRequestCanceller anywhere in the code base. Although I notice, with dismay, that its part of the public SDK via BackupFileRequest and ImageFileRequest structs.
Just looked into this. What a nightmare. We can't change this really without introducing a context.Context parameter across the whole client interface.
At the very least we should aim to de-couple it from the Operation and allow for cancel.HTTPRequestCanceller to be cancelled by a context (from the Operation).
introducing a context.Context parameter across the whole client interface
This has been on the "TODO" list for quite some time too.
introducing a context.Context parameter across the whole client interface
This has been on the "TODO" list for quite some time too.
Shall we bite the bullet? I'm making the same change everywhere else. Of course this is a big breaking change on the client. We can wait until after the next LTS if preferred.
introducing a context.Context parameter across the whole client interface
This has been on the "TODO" list for quite some time too.
Shall we bite the bullet? I'm making the same change everywhere else. Of course this is a big breaking change on the client. We can wait until after the next LTS if preferred.
Lets discuss in the team meeting.
It shouldn't need to hold this change up though as we should be able to untangle cancel.HTTPRequestCanceller and Operation at least.
This PR is now updated to:
- Add a context to an operations' main "run" hook. This context contains the operation requestor and can be cancelled in some situations.
- Change how operations are instantiated. We now how
CreateUserOperationandCreateServerOperation, with the former requiring arequest.Requestorto ensure that the owner of the operation is explicitly set for auditing and access control purposes. - Remove the channel that is returned from operation.Cancel. As far as I can tell this was not used. Callers should use op.Wait.
- Cancel image download operations via a HTTPRequestCanceller that automatically cancels requests when a given context is cancelled.
I have not added a context argument to any other hooks other than the "run" hook. It seemed redundant in the "cancel" hook, and the onConnect hook has an *http.Request argument which has it's own context.
Test failures fixed. Opened a new issue for the underlying problem: https://github.com/canonical/lxd/issues/17220
@markylaing this is looking really great, its going to be much easier to use and reason about operations once we get your work merged. Thanks!
@tomponline tests finally ran and passed. I've also changed the operation delete endpoint to emit the lifecycle event before waiting on the operation.
Ready for review when you're ready 😌