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

fix: add maximum activation time for extensions #3446

Merged
merged 2 commits into from Aug 22, 2023

Conversation

benoitf
Copy link
Collaborator

@benoitf benoitf commented Aug 7, 2023

after reaching that time, loader continue without waiting for this extension extension is flagged as a failed extension

fixes #3280 fixes #2993

What does this PR do?

add maximum activation time for extensions (configurable)

after reaching that time, loader continue without waiting for this extension
extension is flagged as a failed extension

Screenshot/screencast of this PR

image

What issues does this PR fix or reference?

fixes #3280
fixes #2993

How to test this PR?

Unit test is provided

but you can also add a await new Promise(r => setTimeout(r, 10000)); instruction in one activate method of an extension
(extension should report as having failed)

@benoitf benoitf requested a review from a team as a code owner August 7, 2023 08:59
@benoitf benoitf requested review from jeffmaury and cdrage and removed request for a team August 7, 2023 08:59
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Looks strange to me because:

  • the default 5s causes the OpenShift Local to fail
  • even if the extension is marked as failed, registrations are performed so I was able to create Kind or OpenShift Local clusters

@benoitf
Copy link
Collaborator Author

benoitf commented Aug 8, 2023

I'll check

Yes, it does not prevent the activation (but it's not waiting that activate method returns before proceeding)

If OpenShift local extension is not being activated in 5s I guess there is a big issue in the extension

Copy link
Contributor

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Was able to test this with https://github.com/crc-org/crc-extension but didn't run into the timeout issue (maybe I had less stuff running).

Should we change it to 10 seconds instead?

You're right though, there is something wrong with the extension if it's taking that long (should load fast, and THEN do whatever call is making it time out).

after reaching that time, loader continue without waiting for this extension
extension is flagged as a failed extension

fixes containers#3280
fixes containers#2993

Signed-off-by: Florent Benoit <fbenoit@redhat.com>
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf benoitf merged commit 5a1f5e1 into containers:main Aug 22, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.4.0 milestone Aug 22, 2023
mairin pushed a commit to mairin/podman-desktop that referenced this pull request Aug 29, 2023
* fix: add maximum activation time for extensions

after reaching that time (default is 10s), loader continue without waiting for this extension
extension is flagged as a failed extension

fixes containers#3280
fixes containers#2993

Signed-off-by: Florent Benoit <fbenoit@redhat.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.

Better handling for poor extension activation extensions-started event should always be fired
4 participants