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

Add timeout for juju controllers refresh #286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guoqiao
Copy link
Contributor

@guoqiao guoqiao commented Sep 4, 2019

If you have a dead controller, which is already deleted but still has a
local record in juju, it will cause a few problems without hints:

  1. juju destroy-model will freeze forever
  2. juju kill-model will fail with unuseful message
  3. juju controllers --refresh will freeze forever

The only way to fix so far is juju unregister.

A related bug has been reported:

https://bugs.launchpad.net/juju-core/+bug/1593506

functest-prepare will also freeze forever because of 3, without any
message. To avoid wasting time on waiting, add timeout and help message
for people to find the problem earlier.

Signed-off-by: Joe Guo [email protected]

If you have a dead controller, which is already deleted but still has a
local record in juju, it will cause a few problems without hints:

1. juju destroy-model will freeze forever
2. juju kill-model will fail with unuseful message
3. juju controllers --refresh will freeze forever

The only way to fix so far is `juju unregister`.

A related bug has been reported:

    https://bugs.launchpad.net/juju-core/+bug/1593506

`functest-prepare` will also freeze forever because of 3, without any
message. To avoid wasting time on waiting, add timeout and help message
for people to find the problem earlier.

Signed-off-by: Joe Guo <[email protected]>
@guoqiao guoqiao closed this Sep 4, 2019
@guoqiao guoqiao reopened this Sep 4, 2019
Copy link
Collaborator

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

Please use async IO functions in async function definitions as otherwise the async loop will stall.

Comment on lines +36 to +42

# if you have a dead controller, this cmd will wait forever.
# add timeout to avoid waiting.
# use `juju unregister` to remove your dead controller and try again
# related bug: https://bugs.launchpad.net/juju-core/+bug/1593506
subprocess.check_call(['juju', 'list-controllers', '--refresh'],
timeout=60)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nice change, but there is a major fault in the original code, which is that there is a non-async subprocess.check_call(...) in an async function. This will stall the async loop which basically forces it to sync. This should be changed to use the asyncio equivalent.

@ajkavanagh ajkavanagh added the stale Nothing happening with the PR for a long time; pending delete. label Jun 29, 2022
@ajkavanagh
Copy link
Collaborator

Marking as stale as nothing has happened with the PR in over 2 years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Nothing happening with the PR for a long time; pending delete.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants