upgrades-migrations / preupgrade-assistant-modules

Modules for the Preupgrade Assistant tool. Disclaimer: It was fun. R.I.P. :-)
GNU General Public License v3.0
3 stars 11 forks source link

system/java-1.8.0-ibm - check for package being present before update #103

Closed rmetrich closed 6 years ago

rmetrich commented 6 years ago

java-1.8.0-ibm EL6 is incompatible with EL7. Upon update, this causes broken symlinks to persist and makes Java unusable.

AloisMahdal commented 6 years ago

qa_ack: The presence of the package is all that influences the result; ie. there are just 2 cases. Manual testing should be enough; automated would be better but problematic unless you can easily install the java package (which also must be signed) from within the test.

Here is the full case table:

CASE TABLE

Note that java-1.8.0-ibm is in "supplementary" repo which is not normally available on testing machines. For testing, it should be enough to just download it from Brew and install it manually.

Table

INPUT FACTORS || BEHAVIORS
--------------||--------------.------.----------
package       || result       | risk | allowed
--------------||--------------|------|----------
present       || needs_action | high | no
non-present   || pass         | none | yes
similar       || pass         | none | yes

Legend

Notes

The allowed behavior means whether or not redhat-upgrade-tool would be allowed to continue; the module achieves this by dropping a handler script; these scripts are executed by r-u-t as first thing, resulting in error and refusal to continue.

So this behavior can be verified by running redhat-upgrade-tool manually after preupg, or (probably better) via upgrade test.

rmetrich commented 6 years ago

Hello, I'll move and update the code tomorrow. Regarding the high risk, which should prevent r-u-t to run, is the script handler really capable of disallowing the tool from running? I'm asking because while testing I only found there was some message being printed, but still I could continur the upgrade. Maybe I need to review this also.

Renaud.

Out of the office / Sent from my phone.

Le lun. 27 août 2018 19:22, Petr Stodůlka notifications@github.com a écrit :

@pirat89 requested changes on this pull request.

Please look at my comments above. Additionaly move please the module under the system directory:

git mv RHEL6_7/3rdparty/java-1.8.0-ibm RHEL6_7/system/

The 3rdparty dir is good for testing but we want to keep it empty just for the custom or 3rdparty modules. I will put it then on_qe and merge it once the tests will be done.

Thanks for the contribution!

In RHEL6_7/3rdparty/java-1.8.0-ibm/READY https://github.com/upgrades-migrations/preupgrade-assistant-modules/pull/103#discussion_r213044586 :

@@ -0,0 +1 @@ +jhornice

Please remove this file completely. It is not needed anymore and the content says thetexts in module have been verified by jhornice.

In RHEL6_7/3rdparty/java-1.8.0-ibm/check https://github.com/upgrades-migrations/preupgrade-assistant-modules/pull/103#discussion_r213047584 :

  • if efunc is exit_error:
  • _use_exit_func = exit_error
  • elif efunc is exit_fail and _use_exit_func is not exit_error:
  • _use_exit_func = exit_fail
  • elif efunc is exit_fixed and _use_exit_func not in [exit_error, exit_fail]:
  • _use_exit_func = exit_fixed
  • elif efunc is exit_informational and _use_exit_func is exit_pass:
  • _use_exit_func = exit_informational
  • +############################################################################## +##### MAIN ##### +##############################################################################

  • +# check if package is installed +if is_pkg_installed("java-1.8.0-ibm"):

Correctly you should take care only about RPMs that are native (signed by RH):

if is_pkg_installed("java-1.8.0-ibm") and is_dist_native("java-1.8.0-ibm"):

in case you expect to handle RHEL el6 RPM provided by RH.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/upgrades-migrations/preupgrade-assistant-modules/pull/103#pullrequestreview-149796841, or mute the thread https://github.com/notifications/unsubscribe-auth/ABHBczO1gVMYt-LSbKwEpFp1JjYt3E6Jks5uVCrQgaJpZM4WJlEp .

rmetrich commented 6 years ago

I've made the necessary changes. I've also modified the remove-java-1.8.0-ibm-pre-upgrade.sh so that it only complains if java-1.8.0-ibm is from Red Hat. When package is installed, redhat-upgrade-tool fails with the following error:

Error: A java-1.8.0-ibm package has been found installed on the system. The package must be removed before the upgrade to a new system. See the report from the Preupgrade Assistant.
Following preupgrade script(s) failed:

/root/preupgrade/preupgrade-scripts/remove-java-1.8.0-ibm-pre-upgrade.sh exited with status 1
exiting
pirat89 commented 6 years ago

@rmetrich I will test it manually this night as part of review. In case I find an issue I will write you feedback with patch :) High risk is not preventing user from upgrade (that do extreme risk and error). But the pre-upgrade script that is processed by r-u-t will inhibit the upgrade when returns non-zero exit code.

pirat89 commented 6 years ago

@rmetrich: Hi Renaud, I tested the module and it works as expected. Moving on_qe. Final testing will be done this week and then I will do new release. Thanks for contribution.

AloisMahdal commented 6 years ago

Tested with preupgrade-assistant-el6toel7-0.7.5-0.201808291151Z.PR103.el6_8.noarch.

TL;DR: The behavior is not as the code intends.

When the java-1.8.0-ibm package is not present, the result is notapplicable, not pass. This is most probably due to this:

applies_to = java-1.8.0-ibm

When the package is not there, the applies_to mechanism just marks the module as notapplicable and rest of the check script is skipped.

When package is there, check script is executed, but then most of it is again just doing the same as applies_to already does. IOW, the result would be identical if the check script just went straight to the risk emitting and hook deployment.

@pirat89, is applies_to the right mechanism to use here? If so (it looks like that to me), the check script could be reduced to just few lines. If not, then the mentioned line should be removed from module.ini.

It's worth noting that the effect on user is basically the same, but the truth is that the script does not behave as meant. I'm not going to treat this as blocking issue, but I'd prefer fixing this unless we're really out of time.

rmetrich commented 6 years ago

@AloisMahdal I believe using applies_to is adequate here: if package is not installed, then module is notapplicable; if package is installed but not from Red Hat, then module will be pass. If from Red Hat, module will be fail.

pirat89 commented 6 years ago

@rmetrich @AloisMahdal : Yes, applies_to is wanted here and the code can be much simplier in that way. Thanks for info. I completely overlooked that. I believe it is not blocking but it can be simplified significantly and would be nice to keep it as simple as possible in that case.

Just to correct you about behaviour of applies_to. The result will be not_applicable when the rpm is not installed or is is not from Red Hat - the check is already included. But I guess that it is still ok and pass is not needed at all here. Correct?

rmetrich commented 6 years ago

I understand the code is complicated and can be simplified. I did so (it's just basic copy/paste from tomcat) in case this needs to be enhanced. But maybe it's overkill indeed.

pirat89 commented 6 years ago

@rmetrich w8 a sec. I did refactoring. I am finishing testing and will send you PR into your repo

AloisMahdal commented 6 years ago

@pirat89 I'm not sure what you are asking. Both applies_to and calling is_dist_native do the same thing: check if package is native (=signed by Red Hat), packages that are not installed are irrelevant. So, the pass is unreachable, and the if condition on line 58 cannot be False.

Roughly, the script could be reduced to:

#!/usr/bin/python
# -*- coding: utf-8 -*-

from preupg.script_api import *

#END GENERATED SECTION

import os
import shutil

PRE_UPGRADE_SCRIPT = "remove-java-1.8.0-ibm-pre-upgrade.sh"
PRE_UPGRADE_DIR    = os.path.join(VALUE_TMP_PREUPGRADE, "preupgrade-scripts")
log_high_risk("The java-1.8.0-ibm package is installed."
              " Remove it before the in-place upgrade.")
solution_file("* The java-1.8.0-ibm package is installed and conflicts with the upgrade."
              " Remove the package before the upgrade.")
shutil.copy2(PRE_UPGRADE_SCRIPT, PRE_UPGRADE_DIR)
os.chmod(os.path.join(PRE_UPGRADE_DIR, PRE_UPGRADE_SCRIPT), 0775)
exit_fail()
AloisMahdal commented 6 years ago

@rmetrich Yes, the code is complicated, I don't think it's your fault though. Many check scripts (even including templates in the doc, I think) are pretty messy here and the API is pretty much confusing and incomplete. So, yeah, sorry for the pain this causes you.

pirat89 commented 6 years ago

@AloisMahdal : exactly that's the solution. I have been just curious about different result I've got and wanted to be sure that I did not a mistake. E.g. pylint and flake checks crashes on invalid token because of 0775 instead of 775. Ok. Then there was several PEBKAC issues and in the end I was curious why the utf8 is required in the end.

@rmetrich: The utf8 is required because part of "composing" is adding the preamble & some checks based on the content of the module.ini file. So the preamble is generated like that:

"""Preupgrade Assistant performs system upgradability assessment
and gathers information required for successful operating system upgrade.
Copyright (C) 2013 Red Hat Inc.
Renaud Métrich <rmetrich@redhat.com>

This program is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation, either version 3 of the License, or
(at your option) any later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program.  If not, see <http://www.gnu.org/licenses/>."""
check_applies_to (check_applies="java-1.8.0-ibm")

And the non-ascii character é is the thing why you need utf8 specified.

@AloisMahdal: It's friday afternoon, my brain is in the kitchen blush. Coffee :coffee: ?

rmetrich commented 6 years ago

@pirat89 Don't bother with a PR in my repo, just push your commit directly to main workspace. I don't care about authoring at all

AloisMahdal commented 6 years ago

Tested with preupgrade-assistant-el6toel7-0.7.5-0.201808311445Z.PR103.el6_8.noarch:

If java-1.8.0-ibm is installed and signed, risk is emitted and r-u-t will inhibit upgrade. If the package is not installed or not signed, the relevant module ends with notapplicable. A package with similar name (java-1.8.0-openjdk) was tested as well, and it does not trigger the module.