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

Add final upgrade step to check if reboot needed #773

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Sep 25, 2023

This is intended to automate into the procedure and be able to drop from our documentation running the command explicitly. I have included example outputs below. My only concern is whether this is too "hidden" of output for the user and if instead we need more of special post upgrade output (similar to the installer) that clearly articulates any upgrade considerations.

Output if reboot needed:

Running Checks after upgrading to Satellite 6.15.z
================================================================================
Clean old Kernel and initramfs files from tftp-boot:                  [OK]
--------------------------------------------------------------------------------
Check number of fact names in database:                               [OK]
--------------------------------------------------------------------------------
Check whether all services are running:                               [OK]
--------------------------------------------------------------------------------
Check whether all services are running using the ping call:           [OK]
--------------------------------------------------------------------------------
Check for paused tasks:                                               [OK]
--------------------------------------------------------------------------------
Check whether system is self-registered or not:                       [OK]
--------------------------------------------------------------------------------
Check if only installed assets are present on the system: 
| Checking for presence of non-original assets...                     [OK]      
--------------------------------------------------------------------------------
Check if system needs reboot: 
Updating Subscription Management repositories.
Core libraries or services have been updated since boot-up:
  * dbus
  * dbus-daemon
  * kernel
  * linux-firmware
  * systemd

Reboot is required to fully utilize these updates.
More information: https://access.redhat.com/solutions/27943           [OK]
--------------------------------------------------------------------------------


--------------------------------------------------------------------------------
Upgrade finished.

Example if no reboot:

Running Checks after upgrading to Satellite 6.15.z
================================================================================
Clean old Kernel and initramfs files from tftp-boot:                  [OK]
--------------------------------------------------------------------------------
Check number of fact names in database:                               [OK]
--------------------------------------------------------------------------------
Check whether all services are running:                               [OK]
--------------------------------------------------------------------------------
Check whether all services are running using the ping call:           [OK]
--------------------------------------------------------------------------------
Check for paused tasks:                                               [OK]
--------------------------------------------------------------------------------
Check whether system is self-registered or not:                       [OK]
--------------------------------------------------------------------------------
Check if only installed assets are present on the system: 
| Checking for presence of non-original assets...                     [OK]      
--------------------------------------------------------------------------------
Check if system needs reboot: 
No core libraries or services have been updated since boot-up.
Reboot should not be necessary.
More information: https://access.redhat.com/solutions/27943           [OK]
--------------------------------------------------------------------------------


--------------------------------------------------------------------------------
Upgrade finished.

@@ -87,6 +87,7 @@ class PostUpgradeChecks < Abstract
def compose
add_steps(find_checks(:default))
add_steps(find_checks(:post_upgrade))
add_step(Procedures::Packages::CheckForReboot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why isn't the step just tagged with post_upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want it to be the final step so the user sees it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hence my question above:

My only concern is whether this is too "hidden" of output for the user and if instead we need more of special post upgrade output (similar to the installer) that clearly articulates any upgrade considerations.

@ehelms ehelms requested a review from evgeni September 27, 2023 14:36
@ehelms
Copy link
Member Author

ehelms commented Oct 9, 2023

Here is the updated output:

Check if system needs reboot:                                         [WARNING]
Updating Subscription Management repositories.
Core libraries or services have been updated since boot-up:
  * dbus
  * dbus-daemon
  * glibc
  * kernel
  * linux-firmware
  * systemd

Reboot is required to fully utilize these updates.
More information: https://access.redhat.com/solutions/27943
--------------------------------------------------------------------------------
Scenario [Checks after upgrading to Satellite 6.15.z] failed.

The following steps ended up in warning state:

  [packages-check-for-reboot]

The steps in warning state itself might not mean there is an error,
but it should be reviewed to ensure the behavior is expected

I am not the biggest fan of that "what does warning mean" output, but we can live with it.

@ehelms
Copy link
Member Author

ehelms commented Oct 9, 2023

I could look into introducing a different output for this to add before or after this change? Maybe that's a bigger follow up to tackle as part of some overall changes.

@ehelms
Copy link
Member Author

ehelms commented Oct 9, 2023

I learned that this results in an exit code 78 as that is what we issue for a warning state, I wonder if that will result in this being interpreted as failed by users / scripts?

@evgeni
Copy link
Member

evgeni commented Oct 9, 2023

If they take notice, this is what we wanted, no? :)

(Didn't you yourself pondered about an exit code at some point?)

@ehelms ehelms merged commit 6640166 into theforeman:master Oct 9, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants