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

client: Pass a flag into queryOperation to skip event listener setup if not required. #12349

Merged
merged 9 commits into from Oct 10, 2023

Conversation

markylaing
Copy link
Contributor

@markylaing markylaing commented Oct 5, 2023

Is it not always necessary to set up an event listener when interacting with operations via the client. For example, many client methods call queryOperation and then call Wait on the returned operation. Instead of receiving all events, filtering by operation, then filtering by the ID of the operation, and checking the status code for "success" or "failure", we can just use /1.0/operations/wait.

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Can this change be pushed down in to ExecInstance itself?

@markylaing
Copy link
Contributor Author

Can this change be pushed down in to ExecInstance itself?

It's tricky because ExecInstance calls (*ProtocolLXD).queryOperation which always tries to set up an event listener. I don't think it's possible to restructure the client to only use the operations API when dealing with operations, because the events API is used for things like listening for updates on an operation (when an image is being unpacked for example). So in order to create an instance via the CLI you must have access to the events API, which may be counterintuitive.

lxc/exec.go Outdated Show resolved Hide resolved
@markylaing markylaing changed the title lxc/exec: Use operation to wait directly. client: Pass a flag into queryOperation to skip event listener setup if not required. Oct 6, 2023
@markylaing
Copy link
Contributor Author

@tomponline I've changed this to roughly what we discussed on IRC (unless I've misunderstood).

I have only skipped the event listener setup for ExecInstance and ConsoleInstance as this is sufficient for the OpenFGA work. Would you like me to go through all calls to queryOperation to check if the event listener is necessary or not?

client/lxd.go Outdated Show resolved Hide resolved
@tomponline
Copy link
Member

looks like tests are sad

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
Copy link
Contributor Author

@tomponline tests should pass now if you have time to re-review.

As a side note, I think a long-term solution to this problem might be to have a GET /1.0/operations/{uuid}/events endpoint which returns only events relevant to the given operation.

client/lxd.go Outdated Show resolved Hide resolved
client/lxd.go Outdated Show resolved Hide resolved
tomponline
tomponline previously approved these changes Oct 10, 2023
client/lxd.go Outdated Show resolved Hide resolved
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@tomponline tomponline merged commit 33693b9 into canonical:main Oct 10, 2023
26 checks passed
markylaing added a commit to markylaing/lxd that referenced this pull request Oct 12, 2023
After recent changes to the client in canonical#12349, when calling ExecInstance
we wait for the operation to complete by directly calling the operation
wait endpoint instead of listening for the event.

When calling exec on a VM, the client is used to connect with the LXD
agent over the devlxd socket. This was failing with a 404 because the
endpoint didn't exist.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
markylaing added a commit to markylaing/lxd that referenced this pull request Oct 12, 2023
After recent changes to the client in canonical#12349, when calling ExecInstance
we wait for the operation to complete by directly calling the operation
wait endpoint instead of listening for the event.

When calling exec on a VM, the client is used to connect with the LXD
agent via vsock. This was failing with a 404 because the
endpoint didn't exist.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
tomponline pushed a commit to tomponline/lxd that referenced this pull request Nov 30, 2023
After recent changes to the client in canonical#12349, when calling ExecInstance
we wait for the operation to complete by directly calling the operation
wait endpoint instead of listening for the event.

When calling exec on a VM, the client is used to connect with the LXD
agent via vsock. This was failing with a 404 because the
endpoint didn't exist.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
tomponline pushed a commit to tomponline/lxd that referenced this pull request Nov 30, 2023
After recent changes to the client in canonical#12349, when calling ExecInstance
we wait for the operation to complete by directly calling the operation
wait endpoint instead of listening for the event.

When calling exec on a VM, the client is used to connect with the LXD
agent via vsock. This was failing with a 404 because the
endpoint didn't exist.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants