Bug 1526774

Summary: While executing "foreman-maintain upgrade run" , n(no) and q(quit) performs same action so they should be merged
Product: Red Hat Satellite Reporter: Gauravi <gapatil>
Component: Satellite MaintainAssignee: Kavita <kgaikwad>
Status: CLOSED NOTABUG QA Contact: Katello QA List <katello-qa-list>
Severity: medium Docs Contact:
Priority: medium    
Version: 6.3.0CC: apatel, dgross, inecas, kgaikwad, sabnave
Target Milestone: UnspecifiedKeywords: Triaged
Target Release: Unused   
Hardware: x86_64   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-01-10 15:39:41 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Gauravi 2017-12-17 12:00:50 UTC
Description of problem:
It seems that n(no), q(quit) performs same action while executing "foreman-maintain upgrade run".


[root@dhcp210-253 ~]# foreman-maintain upgrade run --target-version 6.3
Running preparation steps required to run the next scenarios
================================================================================
Procedures::Packages::Install: 
Loaded plugins: product-id, search-disabled-repos, subscription-manager
Package hdparm-9.43-5.el7.x86_64 already installed and latest version
No package fio available.
Nothing to do
                                                                      [OK]
--------------------------------------------------------------------------------


Running Checks before upgrading to Satellite 6.3
================================================================================
Check for paused tasks:                                               [OK]
--------------------------------------------------------------------------------
Check whether all services are running using hammer ping:             [OK]
--------------------------------------------------------------------------------
Check for running tasks:                                              [OK]
--------------------------------------------------------------------------------
Check for old tasks in paused/stopped state:                          [OK]
--------------------------------------------------------------------------------
Check for pending tasks which are safe to delete:                     [OK]
--------------------------------------------------------------------------------
Check for tasks in planning state:                                    [OK]
--------------------------------------------------------------------------------
Check for recommended disk speed of pulp, mongodb, pgsql dir.: 
\ Finished                                                            [OK]      
--------------------------------------------------------------------------------


The pre-upgrade checks indicate that the system is ready for upgrade.
It's recommended to perform a backup at this stage.
Confirm to continue with the the modification part of the upgrade, [y(yes), n(no), q(quit)] q
[root@dhcp210-253 ~]#
[root@dhcp210-253 ~]# foreman-maintain upgrade run --target-version 6.3
Running preparation steps required to run the next scenarios
================================================================================
Procedures::Packages::Install: 
Loaded plugins: product-id, search-disabled-repos, subscription-manager
Package hdparm-9.43-5.el7.x86_64 already installed and latest version
No package fio available.
Nothing to do
                                                                      [OK]
--------------------------------------------------------------------------------


Running Checks before upgrading to Satellite 6.2.z
================================================================================
Check for paused tasks:                                               [OK]
--------------------------------------------------------------------------------
Check whether all services are running using hammer ping:             [OK]
--------------------------------------------------------------------------------
Check for running tasks:                                              [OK]
--------------------------------------------------------------------------------
Check for old tasks in paused/stopped state:                          [OK]
--------------------------------------------------------------------------------
Check for pending tasks which are safe to delete:                     [OK]
--------------------------------------------------------------------------------
Check for tasks in planning state:                                    [OK]
--------------------------------------------------------------------------------
Check for recommended disk speed of pulp, mongodb, pgsql dir.: 
\ Finished                                                            [OK]      
--------------------------------------------------------------------------------


The pre-upgrade checks indicate that the system is ready for upgrade.
It's recommended to perform a backup at this stage.
Confirm to continue with the the modification part of the upgrade, [y(yes), n(no), q(quit)] n
[root@dhcp210-253 ~]# 

Version-Release number of selected component (if applicable):


How reproducible:
Always

Steps to Reproduce:
1. Run below command -
   #foreman-maintain upgrade run --target-version 6.3
2. It will ask to -
   Confirm to continue with the the modification part of the upgrade, [y(yes), n(no), q(quit)]
3. Either of n(no) and q(quit) executing one at a time will perform same action.


Additional info:
After giving either of n(no) and q(quit), upgrade halts and return to the prompt so there should be only one option.

Comment 1 Kavita 2017-12-18 11:59:05 UTC
why priority and severity set to high for this BZ? 

I don't think that it is causing an issue with upgrade execution and nothing is blocked due to this. If I misunderstood anything, please correct me.

There is a behaviour difference between these options i.e n(no) and q(quit) while running checks.
n(no) -> exit from current execution & run next task. If no next task, will exit from command execution.
q(quit) -> fully exit from command execution

But yes, here confirmation is to run phase so we can show either n(no) or q(quit).

Modifying a priority and severity from high to medium.

Comment 3 Dylan Gross 2020-09-24 13:26:34 UTC
Re:  "There is a behaviour difference between these options i.e n(no) and q(quit) while running checks.
n(no) -> exit from current execution & run next task. If no next task, will exit from command execution.
q(quit) -> fully exit from command execution"


With the above description, I would expect a "n(no)" to skip that check and move to the next.
And q(quit) to exit the foreman-maintain command completely.   

Unless I'm misunderstanding, the below example shows that NOT happening.
It's at a different prompt than the original bug report (and in that case there may not be a distinction needed).

But the sub-checks don't appear to obey the q(quit)

(Note:  I can reproduce this only because I have tasks older than 30 days which prompt me, so I don't know if it's specific to that sub-check or if it's a re-used prompt)


Running Checks before upgrading to Satellite 6.7.z
================================================================================
Check number of fact names in database:                               [OK]
--------------------------------------------------------------------------------
Check for verifying syntax for ISP DHCP configurations:               [SKIPPED]
DHCP feature is not enabled
--------------------------------------------------------------------------------
Check whether all services are running:                               [SKIPPED]
--------------------------------------------------------------------------------
Check whether all services are running using the ping call:           [FAIL]
Some components are failing: foreman_tasks
--------------------------------------------------------------------------------
Continue with step [Restart applicable services]?, [y(yes), n(no), q(quit)] q        <--------------
Check for paused tasks:                                               [OK]      
--------------------------------------------------------------------------------
Check to verify no empty CA cert requests exist:                      [OK]
--------------------------------------------------------------------------------
Check whether system is self-registered or not:                       [OK]
--------------------------------------------------------------------------------
Check to make sure root(/) partition has enough space:                [OK]
--------------------------------------------------------------------------------
Check to validate candlepin database:                                 [OK]
--------------------------------------------------------------------------------
Check for running tasks:                                              [OK]
--------------------------------------------------------------------------------
Check for old tasks in paused/stopped state:                          [FAIL]
Found 60 paused or stopped task(s) older than 30 days
--------------------------------------------------------------------------------
Continue with step [Delete old tasks]?, [y(yes), n(no), q(quit)] q
Check for pending tasks which are safe to delete:                     [OK]      
--------------------------------------------------------------------------------
Check for tasks in planning state:                                    [OK]
--------------------------------------------------------------------------------
Check to verify if any hotfix installed on system:

Comment 4 Kavita 2020-09-25 18:07:06 UTC
Hello Dylan,

Yes, you are correct that q(quit) action should exit the foreman-maintain command.

> But the sub-checks don't appear to obey the q(quit)

This is because running Checks before upgrading comes under fail_slow strategy so foreman-maintain should not display q(quit) action.
Foreman Maintain will show q(quit) action only in case of fail_fast.

There is one more issue BZ#1640974 opened for an improvement around this. 
Please check my comment - https://bugzilla.redhat.com/show_bug.cgi?id=1640974#c7

Let me know if you have any further queries.

- Kavita