Update return code behavior #218

bocekm · 2017-02-07T21:40:50Z

Update Preupgrade Assistant to return code per table in: #218 (comment)

With this update Preupgrade Assistant terminates with INTERNAL_EXCEPTION when it detects unknown module result
A module can report an inplace risk only when the result of the module is "fail"

Cleanup

Remove:
- clearly dead code
- unused settings.ERROR_RETURN_VALUES
- superfluous settins.ORDERED_LIST
- preupg_diff part of the test XMLs so the inplace_risks tests work as expected (and update preupg_diff tests to still provide value)
Rename:
Per rhbz#1414359:
- settings.ReturnValues->PreupgReturnCodes
- settings.ModuleValues->ResultBasedReturnCodes

bocekm · 2017-02-10T20:06:49Z

After talking to @AloisMahdal, the return codes from preupg --riskcheck should be the same as the preupg return codes (normal run). These are described here: #190. I'll update the "riskcheck code" accordingly.

@AloisMahdal

@AloisMahdal hasn't seen the comment (#218 (comment)) that says there will be another changes to this PR.

bocekm · 2017-05-11T14:25:14Z

Per @pirat89 the return codes should be:

    INPUT                  || BEHAVIOR
    -------.---------------||--------.-------------.
    exit_x | log_x_risk    || result | return code |
    =======|===============||========|=============|
    fail   | <not called>  ||  error |  2          |
    fail   | slight        ||  n_ins |  0          |
    fail   | medium        ||  n_ins |  0          |
    fail   | high          ||  n_act |  1          |
    fail   | extreme       ||   fail |  2          |
    -------|---------------||--------|-------------|
    fixed  | <not called>  ||  fixed |  0          |
    fixed  | <any risk>    ||  error |  2          |
    -------|---------------||--------|-------------|
    info   | <not called>  ||   info |  0          |
    info   | <any risk>    ||  error |  2          |
    -------|---------------||--------|-------------|
    n_app  | <not called>  ||   napp |  0          |
    n_app  | <any risk>    ||  error |  2          |
    -------|---------------||--------|-------------|
    pass   | <not called>  ||   pass |  0          |
    pass   | <any risk>    ||  error |  2          |
    -------|---------------||--------|-------------|
    error  | <not called>  ||  error |  2          |
    error  | <any risk>    ||  error |  2          |
    -------|---------------||--------|-------------|
    <not called>|<any risk>||  error |  2          |
    -------|---------------||--------|-------------|

I still need to figure out how or when the not_selected and not_checked results are set.

AloisMahdal · 2017-05-30T18:21:12Z

I'm sorry to beat this horse again, but here's few things I think could be done better:

Preupgrade Assistant now terminates with INTERNAL_EXCEPTION when it detects
unknown module result

OK.

A module can have inplace risks only when the module result is "fail"

OK. I do agree and sympathize with this change, but I would not call it "update return code behavior". This seems a different PR to me.

Then there's this "typo" thing (first commit). The problem with that is, that you can't just fix a typo and move on, it has to be clear what are the (intended) functional implications of that in order for us to be able to review code properly.

As you know, the legacy code base is far from being very readable. Most of code is hard to wrap one's head around. To combat with that, what helps is to have as each change as small as possible and again, with very clear and obvious aim.

But...

Remove:

clearly dead code

unused settings.ERROR_RETURN_VALUES

superfluous settins.ORDERED_LIST

preupg_diff part of the test XMLs so the inplace_risks tests work as expected (and update > * preupg_diff tests to still provide value)

Rename:

settings.ReturnValues->PreupgReturnCodes

settings.ModuleValues->ResultBasedReturnCodes

Now there are 7 changes that each add "mist" over the above changes, making the actual functional changes harder to see in the diff. At the same time, they are obviously needed, they don't need almost no explaining and each of them is trivial to review. Seeing them 7-commit "cleanup" PR (or even 7 PRs) would make them just go away with basically zero friction.

OTOH, having to review all this one/two diffs is a headache.

So, if @pirat89 (who knows the codebase much better than me) is able to do the review, I'm not goiong to block it; I'll just run some tests downstream. However, if you want to get more eyeballs on this, please consider re-splitting them as separate commits (or even PRs -- the price of having more PRs if they are easy is definitely worth it.)

bocekm · 2017-06-14T12:58:50Z

More than half of the changed code is just updating tests, so I think this fact adds up to the impression that there're so many changes for one PR.
You can forget the "typo thing", it's not relevant anymore - now, the code with the typo is not there at all. I'll try to get rid of that commit.
Anyway, the purpose of the PR now (as opposed to just the typo fix in the past) is to have Preupgrade Assistant return the code as per the table a few commits above.
Sorry, I couldn't do it in less lines changed. Or I just didn't want to, I wouldn't push such a change out just because it took me a long time to wrap my head around the original code, as it was totally obfuscated. It begged for some refactoring. I think that if you rule out the test changes, there's not really that much of a code changed.

bocekm · 2017-06-14T13:07:37Z

A module can have inplace risks only when the module result is "fail"

OK. I do agree and sympathize with this change, but I would not call it "update return code behavior". This seems a different PR to me.

IMO it's interconnected - what directly affects the return code is the fact that an in-place risk is allowed only for the result fail . Before, the in-place risk did not have any effect on the return code, just the result had (fail, pass, ...).

bocekm · 2017-07-27T18:20:38Z

This is the latest version of the return codes behaviour:

   .--------------------------------.--------------------------------.
   | Module called functions        | BEHAVIOR                       |
   .----------------.---------------|------------------.-------------.
   | exit_x         | log_x_risk    | result in XML    | return code |
   |================|===============|==================|=============|
   | fail           | <not called>  | error            |  2          |
   | fail           | slight        | needs_inspection |  0          |
   | fail           | medium        | needs_inspection |  0          |
   | fail           | high          | needs_action     |  1          |
   | fail           | extreme       | fail             |  2          |
   |----------------|---------------|------------------|-------------|
   | fixed          | <not called>  | fixed            |  0          |
   | fixed          | <any risk>    | error            |  2          |
   |----------------|---------------|------------------|-------------|
   | informational  | <not called>  | informational    |  0          |
   | informational  | <any risk>    | error            |  2          |
   |----------------|---------------|------------------|-------------|
   | not_applicable | <not called>  | notapplicable    |  0          |
   | not_applicable | <any risk>    | error            |  2          |
   |----------------|---------------|------------------|-------------|
   | pass           | <not called>  | pass             |  0          |
   | pass           | <any risk>    | error            |  2          |
   |----------------|---------------|------------------|-------------|
   | error          | <not called>  | error            |  2          |
   | error          | <any risk>    | error            |  2          |
   |----------------|---------------|------------------|-------------|
   | N/A*           | N/A*          | notselected      |  0          |
   | N/A*           | N/A*          | notchecked       |  0          |
   |----------------|---------------|------------------|-------------|
   | <not called>   | <not called>  | undefined**      |  ?          |
   | <not called>   | <any risk>    | undefined**      |  ?          |
   .----------------.---------------.------------------.-------------.
   * OpenSCAP hasn't run the module's check script
   ** If the exit_x is not called in the check script the result is undefined.
      To be more precise, the result will depend on with which code the check
      script exits. See the exit codes 1-9 accepted by OpenSCAP below. OpenSCAP
      evaluates exit codes other than 1-9 as error(3).

Module set API function exit_x tells OpenSCAP to set one of the following
results:
  pass(101), fail(102), error(103), notapplicable(105), informational(108), fixed(109)
Under certain circumnstances, OpenSCAP can use the following additional results:
  unknown(104) .. Could not tell what happened
  notchecked(106) .. Rule doesn't have any check script defined
  notselected(107) .. Rule was not selected in the XCCDF Benchmark

See https://github.com/upgrades-migrations/preupgrade-assistant/wiki/How-modules-affect-the-Preupgrade-Assistant-return-code for more details.

pirat89

Incomplete change. The preupg/application.py still contains few "ReturnValues" instead of "PreupgReturnCodes".

pirat89 · 2017-07-30T22:29:25Z

preupg/xccdf.py

+            if found_result not in settings.RESULT_BASED_RETURN_CODES.keys():
+                sys.stderr.write("Unknown result: %s\n" % found_result)
+                sys.exit(settings.PreupgReturnCodes.INTERNAL_EXCEPTION)
+                continue


The continue doesn't make sense here as the sys.exit is called before. It is dead code.

pirat89 · 2017-07-30T22:41:14Z

It's suspicious, that tests on jenkins succeeded.

python ./setup.py test
running test
running egg_info
writing preupgrade_assistant.egg-info/PKG-INFO
writing top-level names to preupgrade_assistant.egg-info/top_level.txt
writing dependency_links to preupgrade_assistant.egg-info/dependency_links.txt
reading manifest file 'preupgrade_assistant.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
writing manifest file 'preupgrade_assistant.egg-info/SOURCES.txt'
running build_ext
preupg.log.ERROR: Configuration file /etc/preupgrade-assistant.conf is missing or is not readable!
Traceback (most recent call last):
  File "./setup.py", line 71, in <module>
    test_suite='tests.suite'
  File "/usr/lib64/python2.7/distutils/core.py", line 151, in setup
    dist.run_commands()
  File "/usr/lib64/python2.7/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "/usr/lib64/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/usr/lib/python2.7/site-packages/setuptools/command/test.py", line 211, in run
    self.run_tests()
  File "/usr/lib/python2.7/site-packages/setuptools/command/test.py", line 234, in run_tests
    **exit_kwarg
  File "/usr/lib64/python2.7/unittest/main.py", line 94, in __init__
    self.parseArgs(argv)
  File "/usr/lib64/python2.7/unittest/main.py", line 149, in parseArgs
    self.createTests()
  File "/usr/lib64/python2.7/unittest/main.py", line 158, in createTests
    self.module)
  File "/usr/lib64/python2.7/unittest/loader.py", line 130, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib64/python2.7/unittest/loader.py", line 115, in loadTestsFromName
    test = obj()
  File "/home/pstodulk/special/preupgrade-assistant/tests/__init__.py", line 27, in suite
    from tests import test_api
  File "/home/pstodulk/special/preupgrade-assistant/tests/test_api.py", line 6, in <module>
    from preupg import script_api
  File "/home/pstodulk/special/preupgrade-assistant/preupg/script_api.py", line 806, in <module>
    load_pa_configuration()
  File "/home/pstodulk/special/preupgrade-assistant/preupg/script_api.py", line 681, in load_pa_configuration
    exit_error()
  File "/home/pstodulk/special/preupgrade-assistant/preupg/script_api.py", line 439, in exit_error
    sys.exit(int(os.environ['XCCDF_RESULT_ERROR']))
  File "/usr/lib64/python2.7/UserDict.py", line 40, in __getitem__
    raise KeyError(key)
KeyError: u'XCCDF_RESULT_ERROR'

bocekm · 2017-09-08T12:58:54Z

@pirat89, as requested, the PR:

the PR has been fixed per the review comments,
the PR it is rebased,
the PR Fix copying README files due to their recent location change #294 has already been merged to master.
You can finish the review.
Thanks.

pirat89 · 2017-09-17T20:56:59Z

I found important issues in modules during testing of the new functionality that require changes. Currently there are 18 modules that has to be modified, some of them will need significant rewrite. Because of prohibited combination log_slight_risk and exit_(fixed,informational) exit codes, some FIXED and INFORMATIONAL results will be replaced by the needs_inspection result probably.

Modules are not prepared currently for this change and suggest to keep it un-merged (but up-to-date) until the modules will be fixed and changes will be covered by tests. See the fix_risks branch where the modules are fixed partially (some fixes are discussable and probably will be modifed yet).

NOTE: I checked only subset of all modules, it's possible that number of modules that need to be fixed is bigger.

bocekm · 2017-09-18T12:42:14Z

@pirat89, sure, I agree, let's first fix the modules according to the changes of this PR.

bocekm · 2017-09-19T14:14:56Z

We'll wait with merging this PR until the API for modules is changed per issue #318.

- rename per rhbz#1414359: - settings.ReturnValues->PreupgReturnCodes - settings.ModuleValues->ResultBasedReturnCodes

- refactor check_inplace_risk function - terminate with INTERNAL_EXCEPTION when unknown module result is detected - a module can now report an inplace risk only when the result of the module is "fail" - the purpose of the function is and has been to return a number (0-2) that corresponds to a severity of the issues found by Preupgrade Assistant modules - the returned number is determined by the modules' result (exit_x) and inplace risk (log_x_risk), whichever has higher severity - refactor replace_inplace_risk - its purpose is to replace particular modules' results in the report XML generated by OpenSCAP - it now replaces the result of a module: - from UNKNOWN to ERROR - from FAIL to ERROR if the module has no risk - from FAIL to NEEDS_INSPECTION if the module has SLIGHT or MEDIUM risk - from FAIL to NEEDS_ACTION if the module has HIGH risk - to ERROR if the module has some risk but the result isn't FAIL - remove update_inplace_risk function - dead code - fix incorrect OpenSCAP module results (not_applicable -> notapplicable, etc.) in settings.py

AloisMahdal · 2017-11-16T18:41:18Z

qa_ack: Update downstream tests: IIUC api/mseverity and preupg/result-files.

- remove preupg_diff part of the test XMLs so the inplace_risks tests work as expected - update preupg_diff tests to still provide value

AloisMahdal · 2018-04-04T16:13:38Z

Cleaning up qa_ack (acks are milestone-specific).

bocekm requested review from AloisMahdal, pirat89 and phracek February 7, 2017 21:40

AloisMahdal previously approved these changes Feb 21, 2017

View reviewed changes

bocekm force-pushed the fix_highest_risk_found branch from c89fdb8 to 5f6b081 Compare May 5, 2017 18:52

bocekm changed the title ~~Typo in a function returning highest risk~~ Update return code behavior May 11, 2017

bocekm force-pushed the fix_highest_risk_found branch from 0d7ba15 to b074eca Compare May 11, 2017 18:39

bocekm force-pushed the fix_highest_risk_found branch from b074eca to e164903 Compare June 14, 2017 13:23

bocekm force-pushed the fix_highest_risk_found branch from e164903 to 871eb0a Compare July 25, 2017 22:30

bocekm requested a review from MajoDurco July 26, 2017 13:49

bocekm force-pushed the fix_highest_risk_found branch 9 times, most recently from 6287bdd to 0cf9483 Compare July 28, 2017 12:36

pirat89 suggested changes Jul 30, 2017

View reviewed changes

pirat89 removed the request for review from phracek July 30, 2017 21:35

pirat89 reviewed Jul 30, 2017

View reviewed changes

bocekm force-pushed the fix_highest_risk_found branch from 4c3bc1b to 5f729a4 Compare August 29, 2017 17:21

bocekm added the needs_verify label Aug 29, 2017

bocekm mentioned this pull request Aug 30, 2017

Catch if exit_fail does not contain risk #131

Open

bocekm force-pushed the fix_highest_risk_found branch 2 times, most recently from 547887a to 6040296 Compare September 1, 2017 16:17

bocekm force-pushed the fix_highest_risk_found branch from 6040296 to 5e25dd1 Compare September 19, 2017 18:59

bocekm added affects_modules on_qe and removed on_qe labels Sep 19, 2017

bocekm added this to the RHEL 6.9 -> RHEL 7.5 GA milestone Oct 11, 2017

bocekm mentioned this pull request Oct 18, 2017

riskcheck will not run without upgrade path at /usr #320

Closed

bocekm force-pushed the fix_highest_risk_found branch from 5e25dd1 to 1830e04 Compare October 18, 2017 17:33

bocekm force-pushed the fix_highest_risk_found branch from 1830e04 to 248c68e Compare November 13, 2017 16:01

bocekm added 2 commits November 13, 2017 18:52

remove unused settings.ERROR_RETURN_VALUES

c19395a

- rename per rhbz#1414359: - settings.ReturnValues->PreupgReturnCodes - settings.ModuleValues->ResultBasedReturnCodes

bocekm force-pushed the fix_highest_risk_found branch from 248c68e to cc5e9af Compare November 13, 2017 21:34

jmazanek mentioned this pull request Nov 14, 2017

networking/squid upgrades-migrations/preupgrade-assistant-modules#41

Merged

AloisMahdal added the qa_ack label Nov 16, 2017

pirat89 force-pushed the fix_highest_risk_found branch from cc5e9af to 3a4434e Compare November 22, 2017 14:06

Update tests to follow the updated return codes

bdbff7c

- remove preupg_diff part of the test XMLs so the inplace_risks tests work as expected - update preupg_diff tests to still provide value

pirat89 force-pushed the fix_highest_risk_found branch from 3a4434e to bdbff7c Compare November 22, 2017 18:25

bocekm removed this from the el69toel75b0 milestone Feb 21, 2018

AloisMahdal removed the qa_ack label Apr 4, 2018

jmazanek mentioned this pull request Jul 2, 2018

Risk_ fix upgrades-migrations/preupgrade-assistant-modules#50

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update return code behavior #218

Update return code behavior #218

bocekm commented Feb 7, 2017 •

edited

bocekm commented Feb 10, 2017

bocekm commented May 11, 2017 •

edited

AloisMahdal commented May 30, 2017

bocekm commented Jun 14, 2017

bocekm commented Jun 14, 2017 •

edited

bocekm commented Jul 27, 2017 •

edited

pirat89 left a comment

pirat89 Jul 30, 2017

pirat89 commented Jul 30, 2017

bocekm commented Sep 8, 2017

pirat89 commented Sep 17, 2017 •

edited

bocekm commented Sep 18, 2017

bocekm commented Sep 19, 2017

AloisMahdal commented Nov 16, 2017

AloisMahdal commented Apr 4, 2018

Update return code behavior #218

Are you sure you want to change the base?

Update return code behavior #218

Conversation

bocekm commented Feb 7, 2017 • edited

bocekm commented Feb 10, 2017

bocekm commented May 11, 2017 • edited

AloisMahdal commented May 30, 2017

bocekm commented Jun 14, 2017

bocekm commented Jun 14, 2017 • edited

bocekm commented Jul 27, 2017 • edited

pirat89 left a comment

Choose a reason for hiding this comment

pirat89 Jul 30, 2017

Choose a reason for hiding this comment

pirat89 commented Jul 30, 2017

bocekm commented Sep 8, 2017

pirat89 commented Sep 17, 2017 • edited

bocekm commented Sep 18, 2017

bocekm commented Sep 19, 2017

AloisMahdal commented Nov 16, 2017

AloisMahdal commented Apr 4, 2018

bocekm commented Feb 7, 2017 •

edited

bocekm commented May 11, 2017 •

edited

bocekm commented Jun 14, 2017 •

edited

bocekm commented Jul 27, 2017 •

edited

pirat89 commented Sep 17, 2017 •

edited