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.

Sign up for GitHub

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

Jump to bottom

Wait for units to settle after charm refresh #528

Merged
merged 11 commits into from
Sep 4, 2024
Merged

Wait for units to settle after charm refresh #528

merged 11 commits into from
Sep 4, 2024

Conversation

samuelallan72
Copy link
Contributor

Avoids hitting race conditions in charms,
such as https://bugs.launchpad.net/charm-barbican/+bug/2039604

@samuelallan72 samuelallan72 force-pushed the fix-race branch 4 times, most recently from 6888ba0 to 71fe724 Compare August 23, 2024 05:24
@samuelallan72
Copy link
Contributor Author

@canonical/soleng-reviewers I'd appreciate your thoughts on the implementation here. I haven't updated all the tests yet, but please see the first commit in this PR for the actual app code changes. I'm not sure if this is the best way to implement it, but the goal is to get COU to wait for the charm to finish any upgrade-charm hooks after running a charm refresh.

Specifically, it appears that waiting between Upgrade <charm> from <old channel> to the new channel: <new channel> and Change charm config of <charm> 'openstack-origin' to 'cloud:focal-<new release>' is a workaround to https://bugs.launchpad.net/charm-barbican/+bug/2039604

In general, it will be better for COU to wait for the charm to finish processing hooks after any changes (change config, refresh charm, etc.).

@samuelallan72 samuelallan72 marked this pull request as ready for review August 23, 2024 05:38
@samuelallan72 samuelallan72 requested a review from a team as a code owner August 23, 2024 05:38
Pjack
Pjack previously approved these changes Aug 27, 2024
View reviewed changes
chanchiwai-ray
chanchiwai-ray reviewed Aug 27, 2024
View reviewed changes
Copy link
Contributor

@chanchiwai-ray chanchiwai-ray left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The idea is safe to me since it's only adding wait steps after upgrade. However, just to leave it as note, we might need to increase global default timeout.

@samuelallan72
Copy link
Contributor Author

However, just to leave it as note, we might need to increase global default timeout.

I don't think there is a global timeout?

@chanchiwai-ray
Copy link
Contributor

I don't think there is a global timeout?

okay, maybe I was wrong. anyways, it's out of scope now

chanchiwai-ray
chanchiwai-ray previously approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@chanchiwai-ray chanchiwai-ray left a comment

Choose a reason for hiding this comment

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

LGTM, but I believe there should be some doc change some where

gabrielcocenza
gabrielcocenza requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@gabrielcocenza gabrielcocenza left a comment

Choose a reason for hiding this comment

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

@samuelallan72 so if I understood correctly if you manually upgrade the cloud the issue doesn't happen because you give more time to the application settle?

cou/apps/auxiliary.py Outdated Show resolved Hide resolved
tests/functional/tests/smoke.py Show resolved Hide resolved
tests/mocked_plans/sample_plans/018346c5-f95c-46df-a34e-9a78bdec0018.yaml Outdated Show resolved Hide resolved
cou/apps/base.py Outdated Show resolved Hide resolved
@samuelallan72
Copy link
Contributor Author

@gabrielcocenza

so if I understood correctly if you manually upgrade the cloud the issue doesn't happen because you give more time to the application settle?

Correct. And waiting for the unit to settle in between making changes to it is good practice in general. :)

gabrielcocenza
gabrielcocenza reviewed Sep 2, 2024
View reviewed changes
Copy link
Member

@gabrielcocenza gabrielcocenza left a comment

Choose a reason for hiding this comment

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

Thanks LGTM, but linting is failing

@samuelallan72 samuelallan72 merged commit 3fe94a4 into main Sep 4, 2024
6 checks passed
@samuelallan72 samuelallan72 deleted the fix-race branch September 4, 2024 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabrielcocenza gabrielcocenza gabrielcocenza approved these changes

@Pjack Pjack Pjack approved these changes

@aieri aieri Awaiting requested review from aieri aieri is a code owner automatically assigned from canonical/soleng-reviewers

@jneo8 jneo8 Awaiting requested review from jneo8 jneo8 is a code owner automatically assigned from canonical/soleng-reviewers

@rgildein rgildein Awaiting requested review from rgildein rgildein is a code owner automatically assigned from canonical/soleng-reviewers

@chanchiwai-ray chanchiwai-ray Awaiting requested review from chanchiwai-ray

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samuelallan72 @chanchiwai-ray @Pjack @gabrielcocenza