From 46e20edbc2d52a93eeb7dc78de1949a3be4b31ca Mon Sep 17 00:00:00 2001
From: avandras <avandras@gmail.com>
Date: Tue, 10 Dec 2024 12:04:47 +0100
Subject: [PATCH] Show only the members to be restarted upon restart
 confirmation (#3226)

When doing `patronictl restart <clustername> --pending`, the confirmation lists all members, regardless if their restart is really pending:

```
> patronictl restart pgcluster --pending
+ Cluster: pgcluster (7436691039717365672) ----+----+-----------+-----------------+---------------------------------+
| Member | Host     | Role         | State     | TL | Lag in MB | Pending restart | Pending restart reason          |
+--------+----------+--------------+-----------+----+-----------+-----------------+---------------------------------+
| win1   | 10.0.0.2 | Sync Standby | streaming |  8 |         0 | *               | hba_file: [hidden - too long]   |
|        |          |              |           |    |           |                 | ident_file: [hidden - too long] |
|        |          |              |           |    |           |                 | max_connections: 201->202       |
+--------+----------+--------------+-----------+----+-----------+-----------------+---------------------------------+
| win2   | 10.0.0.3 | Leader       | running   |  8 |           | *               | hba_file: [hidden - too long]   |
|        |          |              |           |    |           |                 | ident_file: [hidden - too long] |
|        |          |              |           |    |           |                 | max_connections: 201->202       |
+--------+----------+--------------+-----------+----+-----------+-----------------+---------------------------------+
| win3   | 10.0.0.4 | Replica      | streaming |  8 |         0 |                 |                                 |
+--------+----------+--------------+-----------+----+-----------+-----------------+---------------------------------+
When should the restart take place (e.g. 2024-11-27T08:27)  [now]:
Restart if the PostgreSQL version is less than provided (e.g. 9.5.2)  []:
Are you sure you want to restart members win1, win2, win3? [y/N]:
```

When we proceed with the restart despite the scary message mentioning all members, not just the ones needing a restart, there will be an error message stating the node not to be restarted was indeed not restarted:

```
Are you sure you want to restart members win1, win2, win3? [y/N]: y
Restart if the PostgreSQL version is less than provided (e.g. 9.5.2)  []:
Success: restart on member win1
Success: restart on member win2
Failed: restart for member win3, status code=503, (restart conditions are not satisfied)
```

The misleading confirmation message can also be seen when using the `--any` flag.

The current PR is fixing this.

However, we do not apply filtering in case of scheduled pending restart, because the condition must be evaluated at the scheduled time.
---
 patroni/ctl.py | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/patroni/ctl.py b/patroni/ctl.py
index b55cfd4d..2ce4b024 100644
--- a/patroni/ctl.py
+++ b/patroni/ctl.py
@@ -711,7 +711,7 @@ def get_members(cluster: Cluster, cluster_name: str, member_names: List[str], ro
 
 
 def confirm_members_action(members: List[Member], force: bool, action: str,
-                           scheduled_at: Optional[datetime.datetime] = None) -> None:
+                           scheduled_at: Optional[datetime.datetime] = None, pending: Optional[bool] = None) -> None:
     """Ask for confirmation if *action* should be taken by *members*.
 
     :param members: list of member which will take the *action*.
@@ -730,8 +730,13 @@ def confirm_members_action(members: List[Member], force: bool, action: str,
     """
     if scheduled_at:
         if not force:
-            confirm = click.confirm('Are you sure you want to schedule {0} of members {1} at {2}?'
-                                    .format(action, ', '.join([m.name for m in members]), scheduled_at))
+            if pending:
+                confirm = click.confirm('The nodes needing a restart will be identified at the scheduled time, and '
+                                        'might be different from the ones showing pending restart right now. '
+                                        'Is this fine?')
+            else:
+                confirm = click.confirm('Are you sure you want to schedule {0} of members {1} at {2}?'
+                                        .format(action, ', '.join([m.name for m in members]), scheduled_at))
             if not confirm:
                 raise PatroniCtlException('Aborted scheduled {0}'.format(action))
     else:
@@ -1104,20 +1109,25 @@ def restart(cluster_name: str, group: Optional[int], member_names: List[str],
                                  type=str, default='now')
 
     scheduled_at = parse_scheduled(scheduled)
-    confirm_members_action(members, force, 'restart', scheduled_at)
+
+    content: Dict[str, Any] = {}
+    if pending:
+        content['restart_pending'] = True
+        # For scheduled restarts we don't filter the members now. If they really need a restart might change
+        # until the scheduled time.
+        if not scheduled_at:
+            members = [m for m in members if m.data.get('pending_restart', False)]
 
     if p_any:
         random.shuffle(members)
         members = members[:1]
 
+    confirm_members_action(members, force, 'restart', scheduled_at, pending)
+
     if version is None and not force:
         version = click.prompt('Restart if the PostgreSQL version is less than provided (e.g. 9.5.2) ',
                                type=str, default='')
 
-    content: Dict[str, Any] = {}
-    if pending:
-        content['restart_pending'] = True
-
     if version:
         try:
             postgres_version_to_int(version)