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

better handling of subordinate departed states #214

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

kwmonroe
Copy link
Member

@kwmonroe kwmonroe commented Apr 7, 2022

Lots of things don't work when control plane subordinates depart. kubectl get $stuff is especially troublesome as it takes forever to timeout. When we know our subs are going away, don't run things that call kubectl get $stuff like get_dns_ip and configure_cdk_addons.

Fixes https://bugs.launchpad.net/charm-kubernetes-master/+bug/1968237

@kwmonroe kwmonroe changed the title guard pesky functions on cni.deparated better handling of subordinate departed states Apr 8, 2022
@kwmonroe
Copy link
Member Author

kwmonroe commented Apr 8, 2022

Looks good:

$ time juju destroy-model diehard -y
Destroying model
Waiting for model to be removed, 10 machine(s), 7 application(s)................
......
Waiting for model to be removed, 9 machine(s), 7 application(s)....
Waiting for model to be removed, 8 machine(s), 7 application(s)......
Waiting for model to be removed, 7 machine(s), 7 application(s)...
Waiting for model to be removed, 6 machine(s), 7 application(s)........
Waiting for model to be removed, 5 machine(s), 7 application(s).................
..
Waiting for model to be removed, 4 machine(s), 7 application(s)...
Waiting for model to be removed, 3 machine(s), 7 application(s)...
Waiting for model to be removed, 2 machine(s), 7 application(s).................
...............................
Waiting for model to be removed, 2 machine(s), 6 application(s)...............
Waiting for model to be removed, 2 machine(s), 5 application(s).............
Waiting for model to be removed, 2 machine(s), 4 application(s).................
.
Waiting for model to be removed, 2 machine(s), 3 application(s).............
Waiting for model to be removed, 2 machine(s), 2 application(s).........
Waiting for model to be removed, 1 machine(s), 2 application(s).................
.....
Waiting for model to be removed, 1 machine(s), 1 application(s).................
.......
Waiting for model to be removed, 1 machine(s)....
Model destroyed.

real	6m57.537s
user	0m0.240s
sys	0m0.295s

7 minutes is considerably better than the ♾️ minutes it was taking before.

@kwmonroe kwmonroe marked this pull request as ready for review April 8, 2022 04:22
@kwmonroe kwmonroe marked this pull request as draft April 8, 2022 14:06
@kwmonroe
Copy link
Member Author

kwmonroe commented Apr 8, 2022

/hold to flesh this out in charmed-kubernetes/layer-kubernetes-common#28

Copy link
Member

@addyess addyess left a comment

Choose a reason for hiding this comment

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

Yep, this look pretty nice.

Comment on lines +1401 to 1404
if dns_ip:
return True, dns_ip, 53, dns_domain
else:
return False, None, None, None
Copy link
Member

Choose a reason for hiding this comment

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

Random nit: this method could REALLY use some return typing

def get_dns_info() -> Tuple[bool, Optional[str], Optional[int], Optional[str]]:

Suggested change
if dns_ip:
return True, dns_ip, 53, dns_domain
else:
return False, None, None, None
if dns_ip:
return True, dns_ip, 53, dns_domain
return False, None, None, None

@addyess
Copy link
Member

addyess commented Apr 5, 2023

@kwmonroe i'm still good with this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants