[crmsh-4.6] Dev: ui_context: Skip querying CIB when in a sublevel or help command #1300
Conversation
liangxin1300
force-pushed
the
20240109_crm_configure_service_stopped
branch
from
8588d84 to
ff8dbf0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## crmsh-4.6 #1300 +/- ##
=============================================
+ Coverage 52.92% 53.04% +0.12%
=============================================
Files 79 79
Lines 23951 23957 +6
=============================================
+ Hits 12676 12708 +32
+ Misses 11275 11249 -26 ☔ View full report in Codecov by Sentry. |
liangxin1300
force-pushed
the
20240109_crm_configure_service_stopped
branch
2 times, most recently
from
37ed5da to
1b63ed3
Compare
liangxin1300
force-pushed
the
20240109_crm_configure_service_stopped
branch
from
1b63ed3 to
95b52d5
Compare
liangxin1300
changed the title
|
I'm surprised that this patch modifies |
| @@ -248,8 +251,6 @@ def enter_level(self, level): | |||
| self._in_transit = True | |||
|
|
|||
| entry = level() | |||
| if 'requires' in dir(entry) and not entry.requires(): | |||
| self.fatal_error("Missing requirements") | |||
There was a problem hiding this comment.
Currently, all sublevels will run this check.
And both maintenance and configure sublevel will call cib_factory.initialize, which will call cibadmin to query the CIB
This is the root cause of this issue
| if self.command_name not in constants.NON_FUNCTIONAL_COMMANDS: | ||
| entry = self.current_level() | ||
| if 'requires' in dir(entry) and not entry.requires(): | ||
| self.fatal_error("Missing requirements") |
There was a problem hiding this comment.
I moved the requires check into here, before running any subcommands under this level.
Apparently, 'help', 'cd', 'ls', 'quit', and 'up' don't need such checking
There was a problem hiding this comment.
It is OK to move the requires check. However, blacklisting 'help', 'cd', 'ls', 'quit', and 'up', which belongs to subcommand configure, breaks the layer design of ui_context.
There was a problem hiding this comment.
These non-functional commands belong to all subcommands, not only for configure
| ok = self.current_level().end_game() | ||
| ok = True | ||
| if self.command_name and self.command_name not in constants.NON_FUNCTIONAL_COMMANDS: | ||
| ok = self.current_level().end_game() |
There was a problem hiding this comment.
Inside the end_game implementation, it will query cibadmin, too
When cluster service is not running, entering some crm sublevel requires live cib, and it will fail to show the help info under this sublevel; To fix that, should skip querying cib when the current stack is just a sublevel, or when the current command is the non functional command (like help, ls, cd, up, quit)
liangxin1300
force-pushed
the
20240109_crm_configure_service_stopped
branch
from
95b52d5 to
0583b54
Compare
#1300 tried to remove unnecessary dependencies on a running pacemaker service when crmsh is not actually going to call the service. However, it removed the call to `end_game()` from `up()` and `quit()` incorrectly. This made sublevel `configure` not to have a chance to check uncommitted changes anymore, leading to a regression described in #1466. #1481 tried to fix this regression by adding the call to `end_game()` back to `up()`, and check if pacemaker service is running before calling `end_game()`. This is not optimal as `up()` is a common method used for all sublevels and the status of pacemaker service is only related sublevel `configure`. Checking the status of pacemaker service when using other sublevels does not make sense. This pull request uses a more straightforward method to fix the problem: improve how to detect uncommitted changes in `has_cib_changes()`. The original implementation is to check if there is any elements in change queue. This requires the in memory representation of cib to be populated, leading to an indirect dependency to pacemaker service. However, we can take advantage of a property: the in memory cib needs to be populated before doing any changes. So in the implementation can be optimized: if the in memory cib is not populated, we will know there is not any change.
When cluster service is not running, entering some crm sublevel requires live cib, such as:
And it will fail to show the help info under this sublevel, like:
To fix that, should skip querying cib when the current stack is just level, or when the current command is the non functional command (like help, ls, cd, up, quit)