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

Improve exception tracing when setting up a k8s-cloud #1404

Merged

Conversation

addyess
Copy link
Member

@addyess addyess commented Aug 16, 2023

We continue to see errors in the validation tests where k8s_cloud fixture is involved, but the only error coming back has to do with a failed async. This is because the fixture is trying too hard to catch exceptions -- when it should really bubble those out

There's also a bit of flakiness in many tests to get the kubeconfig from the control-plane unit. lets setup one fixture that can do that for every test. If we don't like it being module scoped -- that's fine we can remove that.

Comment on lines +305 to +309
clouds = await tools.run(
"juju", "clouds", "--format", "yaml", "-c", tools.controller_name
)
if tools.k8s_cloud in yaml.safe_load(clouds[0]):
yield tools.k8s_cloud
return
Copy link
Member Author

Choose a reason for hiding this comment

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

first -- let's see if the cloud we want already exists -- to prevent a failure when adding it a second time

Comment on lines +315 to 321
await tools.run(
"juju",
"add-k8s",
"--skip-storage",
"-c",
tools.controller_name,
tools.connection,
proxy=tools.juju_ssh_proxy,
tools.k8s_cloud,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

next, let's add-k8s. The kubeconfig fixture will handle adding the correct kubeconfig file into the os.environment

@addyess addyess force-pushed the akd/validation/error-propigation-setting-up-k8s-cloud branch from 4cdc739 to 967750b Compare August 16, 2023 21:16
Comment on lines +325 to +326
if _created:
click.echo("Removing k8s cloud")
Copy link
Member Author

@addyess addyess Aug 16, 2023

Choose a reason for hiding this comment

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

the remove portion is only run if _created was set

We're careful not to return during the finally so as to not lose any exceptions that may still need to bubble out

click.echo("Cleaning up k8s model")
try:
for relation in k8s_model.relations:
if _model_created:
Copy link
Member Author

Choose a reason for hiding this comment

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

we're careful here to not return in the finally, so exceptions can freely bubble out

return
await tools.run("juju-crashdump", "-a", "config", "-m", tools.k8s_connection)
click.echo("Cleaning up k8s model")
try:
Copy link
Member Author

Choose a reason for hiding this comment

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

also -- stop trying to catch errors in the finally -- just let the exceptions flow so we can find the issues

Comment on lines +683 to +684
path = Path.home() / ".local/share/juju"
with NamedTemporaryFile(dir=path) as f:
Copy link
Member Author

Choose a reason for hiding this comment

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

because juju is a strictly confined snap, there are use cases where this temporary file really should be in a path that juju can read it. That's really only under $HOME/.local/share/juju
so it's a temporary file in here

let's add it to the os.environ for things like kubectl or juju to be able to use naturally without have to feed its filename into the env

@kwmonroe kwmonroe merged commit 42f9837 into main Oct 24, 2023
6 checks passed
@kwmonroe kwmonroe deleted the akd/validation/error-propigation-setting-up-k8s-cloud branch October 24, 2023 13:41
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