Enhance charm's life cycle. #119

chanchiwai-ray · 2023-11-29T10:00:53Z

Description of changes:

Improved install_or_upgrade event (addresses 4)
- Set Maintenance status when install or upgrade
- Set Blocked status and defer when install or upgrade event failed (both resource and exporter)
- Skip installing exporter if it's already installed (e.g. during upload resource)
Improved remove event
- Provide warning message when exporter files are not properly removed
Improved update_status event
- Add check for relating more than one grafana agent (addresses 1)
- Add check for validating exporter's health, and try to recover it if it fails. If it cannot be recovered set Error status. (addresses 2,3)
- Add check for validating if the tools are properly installed
Improved config_changed event
- Add check for validating exporter configs
Miscellaneous updates to hw_tool.py for adding tool validation logic

Maybe closes:

chanchiwai-ray · 2023-11-29T10:03:53Z

Refactoring can be contributed by someone else

Pjack

Error status definition: any situation entering error status, we can assume there is a bug in charm. In general, if the step fails, it should go to blocked status.

If this is still the logic we want to follow, I think we should enter blocked status in most of case. But I see most of failture cause the charm enter error status now.

src/service.py

src/charm.py

src/hw_tools.py

linting. Co-authored-by: jneo8 <[email protected]> Continue to remove refactoring code from the previous branch, and fix status handling.

chanchiwai-ray · 2023-11-30T04:16:33Z

Further removed refactoring code from this PR, and make as less changes as possible. Also cherry-pick @jneo8 strategy checks into this PR.

chanchiwai-ray · 2023-11-30T04:31:46Z

Updated description

Pjack · 2023-11-30T05:21:25Z

Will this MR also cover this one?
#84

chanchiwai-ray · 2023-11-30T05:32:49Z

Will this MR also cover this one? #84

No, the implementation was removed from this PR

Pjack

nitpick

src/charm.py

Pjack

If there is no further big change, please implement unit test

jneo8

Command and I don't see where does #114 been fixed?

src/charm.py

jneo8 · 2023-11-30T09:14:52Z

src/charm.py

+                )
+                return
+
+        hw_tool_ok, error_msg = self.hw_tool_helper.check_installed()


Need fix:

Shouldn't we check the resources before checking the service?
Imagine we restart a service with wrong resource.

dashmage

Just one small comment and the tests would need to be updated to reflect these changes.
Apart from that, LGTM :)

dashmage · 2023-11-30T09:42:30Z

src/hw_tools.py

@@ -127,6 +147,10 @@ def name(self) -> HWTool:
        """Name."""
        return self._name

+    @abstractmethod
+    def check(self) -> bool:


Would it be possible to rename this to be more descriptive? Maybe check_status? I'm open to other names as well.

This is coming from this commit, I didn't authored that changes, and it's only internal use, so it's not so important

Ah, never mind then. I still think it could use a nicer name since check sounds a bit ambiguous but it's alright even if we keep it check.

chanchiwai-ray · 2023-11-30T12:05:59Z

Command and I don't see where does #114 been fixed?

Maybe the implementation was removed from this PR, I'll updated the description

dashmage · 2023-11-30T13:21:53Z

Since we're merging to the feat/life-cycle branch, there's no automated CI happening here. So it will be hard to verify whether the unit + func tests are passing. How do we validate this since it's not a PR to main/ master?

chanchiwai-ray · 2023-11-30T13:55:15Z

Since we're merging to the feat/life-cycle branch, there's no automated CI happening here. So it will be hard to verify whether the unit + func tests are passing. How do we validate this since it's not a PR to main/ master?

As I understand, this PR only includes the implementation with ops-testing (with hardness, you can test events). Further improvement / manual testing / refactoring / test mindmap update / etc will be handle later. And proper functional test and manual verification are required for merging into main branch.

jneo8

Overall LGTM now. Please add more details in test function name & doc-string.

tests/unit/test_charm.py

- Fixed some warning message in unittest - Patched exporter retry constants

* Enhance charm's life cycle. (#119) * Enhance charm's life cycle. * Remove unused property. * Cherry-pick from 7513f18, and fix linting. Co-authored-by: jneo8 <[email protected]> Continue to remove refactoring code from the previous branch, and fix status handling. * Move "exporter health check failed and restart" logic into charm.py * add resource check in update_status event, and change Error -> Blocked * Fix typo * Add unittests. * Update unit test docstring and name. * - Used `parameterized` - Fixed some warning message in unittest - Patched exporter retry constants * Separate enabling logic of individual ipmi services monitoring (#123) * Separate enabling logic of individual ipmi services monitoring * Add and refactor unit tests * Fix/resoure install failed (#135) * fix: lifecycle bug fix - Remove install retry - Add cache to hw_white_list to avoid repeat calls - Fix missing resource failed msg in update status hook * fix: Remove resource_failed_msg * fix: Fix type checking comments and defer relation_join hook if checking not pass * fix: Missing return after grafana-agent relation join event defer * fix: Force re-config exporter when upgrade (#140) * refactor: Move restart exporter logic as single function (#139) * Fix/skip remove freeipmi tools (#138) * fix: Skip removing pkg on IPMI strategy Skip removing package because we afraid this cause dependency error for any other services on the same machine. * fix: Skip remove ssacli pkg and repo * test: Fix failed functional test (#141) Fix failure functional test caused because of life-cycle changing. * style: Linter fix (#142) * fix: apt install freeipmi-tools failed on focal (#143) * fix: apt install freeipmi-tools failed on focal Fix: #128 --------- Co-authored-by: Ray Chan <[email protected]> Co-authored-by: Sudeep Bhandari <[email protected]>

chanchiwai-ray requested review from a team, Pjack, aieri, agileshaw, jneo8 and rgildein November 29, 2023 10:00

chanchiwai-ray mentioned this pull request Nov 29, 2023

Enhance charm's life-cycle #118

Closed

chanchiwai-ray force-pushed the dev-without-refactor branch from 24fc040 to fd7915d Compare November 29, 2023 10:27

Enhance charm's life cycle.

611ecfc

chanchiwai-ray force-pushed the dev-without-refactor branch from fd7915d to 611ecfc Compare November 29, 2023 10:32

Remove unused property.

342aac8

Pjack requested changes Nov 30, 2023

View reviewed changes

src/service.py Outdated Show resolved Hide resolved

src/charm.py Outdated Show resolved Hide resolved

src/charm.py Outdated Show resolved Hide resolved

src/hw_tools.py Outdated Show resolved Hide resolved

src/hw_tools.py Outdated Show resolved Hide resolved

chanchiwai-ray added 2 commits November 30, 2023 12:00

Cherry-pick from 7513f18, and fix

2a1d20e

linting. Co-authored-by: jneo8 <[email protected]> Continue to remove refactoring code from the previous branch, and fix status handling.

Move "exporter health check failed and restart" logic into charm.py

488fe7a

chanchiwai-ray requested a review from Pjack November 30, 2023 04:16

add resource check in update_status event, and change Error -> Blocked

6c9b299

Pjack requested changes Nov 30, 2023

View reviewed changes

src/charm.py Outdated Show resolved Hide resolved

Pjack requested changes Nov 30, 2023

View reviewed changes

Fix typo

a46cfe5

jneo8 requested changes Nov 30, 2023

View reviewed changes

dashmage reviewed Nov 30, 2023

View reviewed changes

Add unittests.

cda5783

chanchiwai-ray requested review from Pjack and jneo8 November 30, 2023 12:06

jneo8 requested changes Dec 1, 2023

View reviewed changes

tests/unit/test_charm.py Outdated Show resolved Hide resolved

Update unit test docstring and name.

c057bec

chanchiwai-ray requested a review from jneo8 December 1, 2023 03:59

- Used parameterized

c5c60d4

- Fixed some warning message in unittest - Patched exporter retry constants

jneo8 approved these changes Dec 1, 2023

View reviewed changes

jneo8 merged commit 5f734cf into canonical:feat/life-cycle Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance charm's life cycle. #119

Enhance charm's life cycle. #119

chanchiwai-ray commented Nov 29, 2023 •

edited by jneo8

Loading

chanchiwai-ray commented Nov 29, 2023

Pjack left a comment •

edited

Loading

chanchiwai-ray commented Nov 30, 2023

chanchiwai-ray commented Nov 30, 2023

Pjack commented Nov 30, 2023

chanchiwai-ray commented Nov 30, 2023 •

edited

Loading

Pjack left a comment

Pjack left a comment •

edited

Loading

jneo8 left a comment

jneo8 Nov 30, 2023

dashmage left a comment

dashmage Nov 30, 2023

chanchiwai-ray Nov 30, 2023 •

edited

Loading

dashmage Nov 30, 2023 •

edited

Loading

chanchiwai-ray commented Nov 30, 2023

dashmage commented Nov 30, 2023

chanchiwai-ray commented Nov 30, 2023 •

edited

Loading

jneo8 left a comment

Enhance charm's life cycle. #119

Enhance charm's life cycle. #119

Conversation

chanchiwai-ray commented Nov 29, 2023 • edited by jneo8 Loading

chanchiwai-ray commented Nov 29, 2023

Pjack left a comment • edited Loading

Choose a reason for hiding this comment

chanchiwai-ray commented Nov 30, 2023

chanchiwai-ray commented Nov 30, 2023

Pjack commented Nov 30, 2023

chanchiwai-ray commented Nov 30, 2023 • edited Loading

Pjack left a comment

Choose a reason for hiding this comment

Pjack left a comment • edited Loading

Choose a reason for hiding this comment

jneo8 left a comment

Choose a reason for hiding this comment

jneo8 Nov 30, 2023

Choose a reason for hiding this comment

dashmage left a comment

Choose a reason for hiding this comment

dashmage Nov 30, 2023

Choose a reason for hiding this comment

chanchiwai-ray Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

dashmage Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

chanchiwai-ray commented Nov 30, 2023

dashmage commented Nov 30, 2023

chanchiwai-ray commented Nov 30, 2023 • edited Loading

jneo8 left a comment

Choose a reason for hiding this comment

chanchiwai-ray commented Nov 29, 2023 •

edited by jneo8

Loading

Pjack left a comment •

edited

Loading

chanchiwai-ray commented Nov 30, 2023 •

edited

Loading

Pjack left a comment •

edited

Loading

chanchiwai-ray Nov 30, 2023 •

edited

Loading

dashmage Nov 30, 2023 •

edited

Loading

chanchiwai-ray commented Nov 30, 2023 •

edited

Loading