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.

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 lacks a prompt asking whether to commit the changes #1466

Closed
liangxin1300 opened this issue Jun 20, 2024 · 0 comments
Closed

crmsh lacks a prompt asking whether to commit the changes #1466

liangxin1300 opened this issue Jun 20, 2024 · 0 comments

Comments

@liangxin1300
Copy link
Collaborator

Which should be a regression issue

on 15SP5

crm(live/15sp5-1)configure# primitive d Dummy
crm(live/15sp5-1)configure# up
There are changes pending. Do you want to commit them (y/n)? n
crm(live/15sp5-1)# configure
crm(live/15sp5-1)configure# edit
crm(live/15sp5-1)configure# up
There are changes pending. Do you want to commit them (y/n)?

For master code

crm(live/alp-2)configure# primitive d Dummy
crm(live/alp-2)configure# up
crm(live/alp-2)# configure
crm(live/alp-2)configure# edit
crm(live/alp-2)configure# up
aleksei-burlakov pushed a commit to aleksei-burlakov/crmsh that referenced this issue Jul 4, 2024
The regression was introduced in beb26f3
Before beb26f3 it was like
$ crm(live/15sp5-1)configure# primitive d Dummy
$ crm(live/15sp5-1)configure# up
$ There are changes pending. Do you want to commit them (y/n)?
After beb26f3 there is no prompt 'There are changes pending... (y/n)?'
This change brings the prompt back.
liangxin1300 added a commit that referenced this issue Jul 8, 2024
The regression was introduced in beb26f3
Before beb26f3 it was like
```
crm(live/15sp5-1)configure# primitive d Dummy
crm(live/15sp5-1)configure# up
There are changes pending. Do you want to commit them (y/n)?
```
After beb26f3 there is no prompt `There are changes pending. Do you
want to commit them (y/n)?`
This change brings the prompt back.
aleksei-burlakov pushed a commit to aleksei-burlakov/crmsh that referenced this issue Jul 8, 2024
The regression was introduced in beb26f3
Before beb26f3 it was like
$ crm(live/15sp5-1)configure# primitive d Dummy
$ crm(live/15sp5-1)configure# up
$ There are changes pending. Do you want to commit them (y/n)?
After beb26f3 there is no prompt 'There are changes pending... (y/n)?'
This change brings the prompt back.
aleksei-burlakov pushed a commit to aleksei-burlakov/crmsh that referenced this issue Jul 8, 2024
backport ClusterLabs#1481

The regression was introduced in beb26f3
Before beb26f3 it was like
$ crm(live/15sp5-1)configure# primitive d Dummy
$ crm(live/15sp5-1)configure# up
$ There are changes pending. Do you want to commit them (y/n)?
After beb26f3 there is no prompt 'There are changes pending... (y/n)?'
This change brings the prompt back.
liangxin1300 added a commit that referenced this issue Jul 9, 2024
backport #1481

The regression was introduced in beb26f3
Before beb26f3 it was like
$ crm(live/15sp5-1)configure# primitive d Dummy
$ crm(live/15sp5-1)configure# up
$ There are changes pending. Do you want to commit them (y/n)? After
beb26f3 there is no prompt 'There are changes pending... (y/n)?' This
change brings the prompt back.
nicholasyang2022 added a commit to nicholasyang2022/crmsh that referenced this issue Jul 10, 2024
nicholasyang2022 added a commit to nicholasyang2022/crmsh that referenced this issue Jul 10, 2024
liangxin1300 added a commit that referenced this issue Jul 11, 2024
#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.
nicholasyang2022 added a commit to nicholasyang2022/crmsh that referenced this issue Jul 11, 2024
nicholasyang2022 added a commit to nicholasyang2022/crmsh that referenced this issue Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant