-
Notifications
You must be signed in to change notification settings - Fork 95
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
[crmsh-4.6] Dev: ui_context: Skip querying CIB when in a sublevel or help command #1300
[crmsh-4.6] Dev: ui_context: Skip querying CIB when in a sublevel or help command #1300
Conversation
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. |
37ed5da
to
1b63ed3
Compare
1b63ed3
to
95b52d5
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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)