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

feat: Restart ceilometer-agent-compute after nova-compute upgrade #435

Merged
merged 9 commits into from
Jun 19, 2024
Merged

feat: Restart ceilometer-agent-compute after nova-compute upgrade #435

merged 9 commits into from
Jun 19, 2024

Conversation

jneo8
Copy link
Contributor

@jneo8 jneo8 commented Jun 11, 2024

Fix: #427

@jneo8 jneo8 force-pushed the fix/restart-ceilometer-agent-compute-after-nova-compute-upgrade branch from 8b612c1 to 46a9dcc Compare June 11, 2024 10:18
@jneo8 jneo8 force-pushed the fix/restart-ceilometer-agent-compute-after-nova-compute-upgrade branch from 46a9dcc to 97cd898 Compare June 12, 2024 09:53
@jneo8 jneo8 marked this pull request as ready for review June 12, 2024 09:53
gabrielcocenza
gabrielcocenza requested changes Jun 12, 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.

I'm not sure which way it's easier to implement, but we can't rely upon application name. I see two options:

  1. Somehow include the charm name during the instantiation of SubordinateUnit
  2. Don't implement the SubordinateUnit class and pass just the nova-compute units to a function that will access the juju status, find the ceilometer-agents and restart if necessary

I like the option 1 because it looks flexible if we need to do more things with subordinates units when upgrading a principal, but this might require more unit tests

cou/utils/juju_utils.py Outdated Show resolved Hide resolved
cou/apps/core.py Outdated Show resolved Hide resolved
cou/utils/juju_utils.py Outdated Show resolved Hide resolved
cou/apps/core.py Show resolved Hide resolved
gabrielcocenza
gabrielcocenza reviewed Jun 13, 2024
View reviewed changes
cou/apps/core.py Show resolved Hide resolved
cou/utils/juju_utils.py Outdated Show resolved Hide resolved
cou/utils/juju_utils.py Outdated Show resolved Hide resolved
cou/utils/juju_utils.py Outdated Show resolved Hide resolved
gabrielcocenza
gabrielcocenza reviewed Jun 13, 2024
View reviewed changes
cou/utils/juju_utils.py Outdated Show resolved Hide resolved
cou/utils/juju_utils.py Outdated Show resolved Hide resolved
gabrielcocenza
gabrielcocenza requested changes Jun 13, 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.

I realized that for some reason the new post upgrade steps are not showing into the mocked_plans, hypervisors and core unit tests. Probably you need to add ceilometer subordinate units.

You also need to include the subordinates new field into the string representation of OpenStackApplication so in the future we can get this new information and replicate into mocked tests. I'm ok not updating on all mocked plans now, but I would like to see at least one changed and working as expected.

@jneo8 jneo8 requested a review from a team as a code owner June 14, 2024 05:27
chanchiwai-ray
chanchiwai-ray previously approved these changes Jun 14, 2024
View reviewed changes
rgildein
rgildein suggested changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

Adding one comment, which can simplify this approach, but it needs to be discussed. Currently, this changes bring new features, which also needs to be discussed.

tests/mocked_plans/sample_plans/018346c5-f95c-46df-a34e-9a78bdec0018.yaml Show resolved Hide resolved
cou/apps/core.py Outdated Show resolved Hide resolved
@jneo8
Copy link
Contributor Author

jneo8 commented Jun 17, 2024

Hi @gabrielcocenza , can I get your review again? All the comments from you are been addressed.

samuelallan72
samuelallan72 requested changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@samuelallan72 samuelallan72 left a comment

Choose a reason for hiding this comment

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

This looks good to me - just some small nits/suggestions.

tests/unit/apps/test_core.py Outdated Show resolved Hide resolved
tests/unit/apps/test_core.py Outdated Show resolved Hide resolved
tests/unit/utils/test_juju_utils.py Outdated Show resolved Hide resolved
chanchiwai-ray
chanchiwai-ray previously approved these changes Jun 18, 2024
View reviewed changes
samuelallan72
samuelallan72 previously approved these changes Jun 18, 2024
View reviewed changes
@jneo8 jneo8 force-pushed the fix/restart-ceilometer-agent-compute-after-nova-compute-upgrade branch from 8e4d021 to 8a2fe23 Compare June 18, 2024 08:17
gabrielcocenza
gabrielcocenza approved these changes Jun 18, 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.

LGTM. Thanks

rgildein
rgildein approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

LGTM

@jneo8 jneo8 merged commit a966c9d into canonical:main Jun 19, 2024
4 checks passed
@chanchiwai-ray chanchiwai-ray mentioned this pull request Jun 19, 2024
Closed
@samuelallan72 samuelallan72 mentioned this pull request Jul 19, 2024
Closed
chanchiwai-ray added a commit that referenced this pull request Sep 12, 2024
@chanchiwai-ray
Partially revert "feat: Restart ceilometer-agent-compute after nova-c…
ec55300 
…ompute upgrade (#435)"

This partially reverts commit a966c9d.
@chanchiwai-ray chanchiwai-ray mentioned this pull request Sep 12, 2024
Merged
chanchiwai-ray added a commit that referenced this pull request Sep 17, 2024
@chanchiwai-ray
Remove unused code and small refactoring (#534)
80158fb 
* Revert "Add subordinate units information for Application. (#497)"

This reverts commit 98950e7.

* Partially revert "feat: Restart ceilometer-agent-compute after nova-compute upgrade (#435)"

This partially reverts commit a966c9d.

* Small refactoring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chanchiwai-ray chanchiwai-ray chanchiwai-ray approved these changes

@rgildein rgildein rgildein approved these changes

@gabrielcocenza gabrielcocenza gabrielcocenza approved these changes

@samuelallan72 samuelallan72 samuelallan72 approved these changes

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@jneo8 @samuelallan72 @gabrielcocenza @rgildein @chanchiwai-ray