Fixes #25186 - hotfix check #236

kgaikwad · 2018-11-29T12:03:43Z

No description provided.

theforeman-bot · 2018-11-29T12:03:51Z

iNecas · 2018-11-30T11:43:31Z

definitions/checks/check_hotfix_installed.rb

+  end
+
+  def regex_for_files_check
+    '^(..5....T.){1}(.{2}[^c])+.+\.(rb|py|erb|js)$'


This thing is hard to understand: would it be possible to put some explanatory comments on how that works?

iNecas · 2018-11-30T11:50:17Z

definitions/checks/check_hotfix_installed.rb

+
+  # only for downstream
+  def find_hotfix_rpms_installed
+    cmd = "rpm -qa 'HOTFIXRHBZ*'"


this doesn't seem to match packages, that have HOTFIX state in release version (see https://bugzilla.redhat.com/show_bug.cgi?id=1642369 as an example for testing with some hotfix installed)

It doesn't seem lik rpm -qa provides the capability. We could perhaps use the find_installed_packages and then search for presence of HOTFIX there.

Modified search using HOTFIX.
I found few issues where hot fixes delivered using RPM which not included HOTFIX string in release. I am not sure what is the reason behind that but in such cases, this check will fail to find out hot fixes applied using RPM.
Any suggestions on this?

iNecas · 2018-11-30T11:57:15Z

definitions/checks/check_hotfix_installed.rb

+  end
+
+  def find_installed_packages(version)
+    repolist_regexstr = feature(:downstream).repolist_for_hotfix_verify(version).join('|')


Could we make it in a way, that the version is not needed: for this case, we could rather look at the enabled repos and filter those that include satellite or rhscl

Now it will work without version. I have considered all repos that includes satellite/rhscl instead of enabled one. It might be chance that user disabled repos.

kgaikwad · 2018-11-30T12:34:17Z

Thank you @iNecas
I will address above comments. Addition to this, @mbacovsky suggested to execute linux commands using ruby library like Open3.popen3/exec which provides kind of streams.

mbacovsky · 2018-11-30T14:09:19Z

definitions/checks/check_hotfix_installed.rb

+  end
+
+  def find_installed_packages(version)
+    repolist_regexstr = feature(:downstream).repolist_for_hotfix_verify(version).join('|')


@kgaikwad here is my take on the repoqueries https://gist.github.com/mbacovsky/889459a71244d04135bcd71cd62fd692. I didn't tested how the performance differ and it should be subject for further refactoring. Memory-wise it should be okay.

Updated PR with the suggested changes. Please have look.

@ntkathole,
While testing this check, with other scenarios it would be good to test this check multiple times with some time interval for performance check.

upadhyeammit · 2018-12-19T13:00:46Z

definitions/checks/check_hotfix_installed.rb

+    changed_files
+  end
+
+  def installed_packages


I got another way to get these details,

To get packages from satellite and rhscl repos you can avoid if statement for each package name,

yumdb search from_repo satellite rhscl
rh-ruby23-runtime-2.2-7.el7.x86_64
from_repo = rhel-server-rhscl-7-rpms
rubygem-ansi-1.4.3-3.el7sat.noarch
from_repo = rhel-7-server-satellite-6.3-rpms

The yumdb comes with yum-utils only, and this is very quick :)

time yumdb search from_repo satellite rhscl
real 0m0.563s
user 0m0.356s
sys 0m0.197s

We just need to disable yum plugins before query and enable it after getting the packages to make it faster else its unnecessary waits for plugins which we dont use while querying yum database,

to disable,

sed -i -e 's/plugins=1/plugins=0/g' /etc/yum.conf

to enable,

sed -i -e 's/plugins=0/plugins=1/g' /etc/yum.conf

To get the hotfix packages installed you can use,

rpm -qa release="HOTFIX"

I have updated command to get hotfix rpms using rpm -qa release="*HOTFIX*" command in pull-request.
Kept repoquery command as yumdb is not giving packages names but having extra input lines.

mbacovsky

@kgaikwad Well done. Looks good overall. I've left just a few nitpicks and questions inline. I'll mock some system to test on tomorrow.
@ntkathole, will you have some spare cycles to test this on properly registered system. With hotfixes and patches?

mbacovsky · 2019-01-04T00:40:52Z

definitions/checks/check_hotfix_installed.rb

+  end
+
+  def run
+    if feature(:downstream) && feature(:downstream).subscribed_using_activationkey?


Presence of :downstream is ensured by confine block.

Good catch 👍
Forgot to remove first condition.

mbacovsky · 2019-01-04T00:45:23Z

definitions/checks/check_hotfix_installed.rb

+
+  def run
+    if feature(:downstream) && feature(:downstream).subscribed_using_activationkey?
+      skip 'Your system is subscribed using custom activationkey'


Souldn't we add something like 'Hotfixes can't be detected.'? Or is it clear?

mbacovsky · 2019-01-04T00:47:22Z

definitions/features/downstream.rb

@@ -54,6 +54,10 @@ def subscribed_using_activationkey?
    ENV['EXTERNAL_SAT_ACTIVATION_KEY'] && ENV['EXTERNAL_SAT_ORG']
  end

+  def repolist(sat_version)


Is this used somewhere?

mbacovsky · 2019-01-04T01:07:42Z

definitions/checks/check_hotfix_installed.rb

+  def msg_for_modified_files(files_modified)
+    message = "\n\nFound #{files_modified.length} file(s) modified on this system.\n"
+    if files_modified.length > 10
+      message += 'Here, it shows only 10 records. For complete result, please check a log file.'


Do we actually log the full list somewhere? How long the list is expected to be on patched system? Would it bve too obtrusive to keep the full list in the output?

+1
Yes, it would be ok I think if we display it in output only.

mbacovsky · 2019-01-04T01:18:10Z

definitions/checks/check_hotfix_installed.rb

+    end
+    message += "\n\nBefore package(s) update, please make sure the avaliablility of above file(s) "\
+      'modifications in latest.'\
+      "\nFor safe side, you can take backup of above file(s) belongs to package(s)\n"


There is likely some word missing in the message. My take with no guarantee would be
Before update make sure the updated packages contain the listed modifications otherwise the fixes will be lost. It is also recommended to backup the modified files prior update.

kgaikwad · 2019-01-04T09:49:35Z

@mbacovsky, Thank you for review!
Updated PR with suggested changes and removed unused code.

mbacovsky · 2019-01-04T11:16:49Z

definitions/checks/check_hotfix_installed.rb

-    if feature(:downstream) && feature(:downstream).subscribed_using_activationkey?
-      skip 'Your system is subscribed using custom activationkey'
+    if feature(:downstream).subscribed_using_activationkey?
+      skip "Hotfixes can't be detected"


@kgaikwad, sorry for the confusion, I think the original part is important too:
Your system is subscribed using custom activationkey. Hotfixes can't be detected..
This way the user knows the cause and can understand the result.

No problem! my bad I was thinking the same :-) but then I thought you expected this as main message.I will do that

mbacovsky · 2019-01-04T11:17:49Z

@kgaikwad thanks for the update. I'll do some more testing and this can get merged.

mbacovsky · 2019-01-04T12:18:35Z

definitions/checks/check_hotfix_installed.rb

+
+  def run
+    if feature(:downstream).subscribed_using_activationkey?
+      skip "Your system is subscribed using custom activationkey. Hotfixes can't be detected."


space is missing in "activation key"

definitions/features/downstream.rb

definitions/checks/check_hotfix_installed.rb

mbacovsky · 2019-01-04T23:00:16Z

@kgaikwad I have a few more findings from my testing.

the check is not tagged to run during pre-upgrade checks
"Before update make sure the updated packages contain the listed modifications otherwise these fixes will be lost. It is also recommended to backup the modified files prior update." is too long and breaks the text flow. We need to add newlines:
Before update make sure the updated packages contain the listed modifications\notherwise these fixes will be lost.\nIt is also recommended to backup the modified files prior update.
the result should be rather non-blocking warning as we don't expect the condition will be resolved (hotfixes removed) before upgrade
I'm not sure if we can move the message before the [FAIL] but it would IMO look better. After some investigation it is not this PR related and should be fixed separately. See, this way the message is sort of disconnected from the check:

Check to verify if any hotfix installed on system: 
\ Checking for presence of hotfix(es). It may take some time to verify.         
                                                                      [FAIL]

HOTFIX rpm(s) applied on this system:
pulp-selinux-2.13.4.9-2.HOTFIXRHBZ1590906.el7sat.noarch

Before update make sure the updated packages contain the listed modifications otherwise these fixes will be lost. It is also recommended to backup the modified files prior update.

jameerpathan111 · 2019-01-08T06:39:21Z

@kgaikwad
Tested PR for following scenarios :

with hotfix installed
with modified_files
with custom activation key
apart from issues mentioned by @mbacovsky in above comment, PR looks good to me.
Still testing it for other scenarios as well.

kgaikwad · 2019-01-08T13:07:12Z

Thank you @mbacovsky and @jameerpathan111
Updated PR with suggested changes by @mbacovsky.
In new commit, I modified failure type to "WARNING" instead of "FAIL" and also few changes to warning message.
@mbacovsky, please let me know if you have any comments/suggestions on new changes.

jameerpathan111 · 2019-01-10T12:44:57Z

Thank you @mbacovsky and @jameerpathan111
Updated PR with suggested changes by @mbacovsky.
In new commit, I modified failure type to "WARNING" instead of "FAIL" and also few changes to warning message.
@mbacovsky, please let me know if you have any comments/suggestions on new changes.

@kgaikwad I have retested PR
-the check is now tagged to run during pre-upgrade check.
-failure type is "WARNING" instead of "FAIL".
-changes in warning message .
new changes looks good to me. 👍

mbacovsky · 2019-01-14T15:55:53Z

This looks and works perfectly now. Well done @kgaikwad and thanks for your patience :)!
Thanks @jameerpathan111 for testing!

theforeman-bot added Needs testing Not yet reviewed labels Nov 29, 2018

iNecas reviewed Nov 30, 2018

View reviewed changes

kgaikwad added the Waiting on contributor label Nov 30, 2018

mbacovsky reviewed Nov 30, 2018

View reviewed changes

theforeman-bot removed the Waiting on contributor label Dec 13, 2018

kgaikwad force-pushed the 25186_verify_hotfix_installed branch from b68da7c to 9d55a7a Compare December 14, 2018 07:32

upadhyeammit reviewed Dec 19, 2018

View reviewed changes

mbacovsky suggested changes Jan 4, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jan 4, 2019

kgaikwad added 3 commits January 4, 2019 04:34

Fixes #25186 - [WIP] hotfix check

2a0bc77

Refs #25186 - modified changes using IO streaming

d2511a0

Refs #25186 - hotfix rpms by rpm command

1d25569

kgaikwad force-pushed the 25186_verify_hotfix_installed branch from 54bb688 to bbf0697 Compare January 4, 2019 09:36

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jan 4, 2019

kgaikwad force-pushed the 25186_verify_hotfix_installed branch from bbf0697 to 4f51d9b Compare January 4, 2019 09:41

kgaikwad changed the title ~~Fixes #25186 - [WIP] hotfix check~~ Fixes #25186 - hotfix check Jan 4, 2019

mbacovsky reviewed Jan 4, 2019

View reviewed changes

kgaikwad force-pushed the 25186_verify_hotfix_installed branch from 4f51d9b to 017586e Compare January 4, 2019 11:25

mbacovsky reviewed Jan 4, 2019

View reviewed changes

Refs #25186 - refactored the hotfix check as per review comments

23b542a

kgaikwad force-pushed the 25186_verify_hotfix_installed branch from 017586e to 23b542a Compare January 4, 2019 12:32

mbacovsky reviewed Jan 4, 2019

View reviewed changes

definitions/features/downstream.rb Show resolved Hide resolved

mbacovsky reviewed Jan 4, 2019

View reviewed changes

definitions/checks/check_hotfix_installed.rb Show resolved Hide resolved

Refs #25186 - modified check failure to warning instead failure

349dc69

mbacovsky approved these changes Jan 14, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Jan 14, 2019

mbacovsky merged commit fbb1fbe into theforeman:master Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #25186 - hotfix check #236

Fixes #25186 - hotfix check #236

kgaikwad commented Nov 29, 2018

theforeman-bot commented Nov 29, 2018

iNecas Nov 30, 2018

iNecas Nov 30, 2018

iNecas Nov 30, 2018

kgaikwad Dec 14, 2018

iNecas Nov 30, 2018

kgaikwad Dec 14, 2018

kgaikwad commented Nov 30, 2018

mbacovsky Nov 30, 2018

kgaikwad Dec 14, 2018 •

edited

upadhyeammit Dec 19, 2018

kgaikwad Dec 26, 2018

mbacovsky left a comment

mbacovsky Jan 4, 2019

kgaikwad Jan 4, 2019

mbacovsky Jan 4, 2019

mbacovsky Jan 4, 2019

mbacovsky Jan 4, 2019

kgaikwad Jan 4, 2019

mbacovsky Jan 4, 2019

kgaikwad commented Jan 4, 2019

mbacovsky Jan 4, 2019

kgaikwad Jan 4, 2019

mbacovsky commented Jan 4, 2019

mbacovsky Jan 4, 2019

mbacovsky commented Jan 4, 2019

jameerpathan111 commented Jan 8, 2019

kgaikwad commented Jan 8, 2019

jameerpathan111 commented Jan 10, 2019

mbacovsky commented Jan 14, 2019

Fixes #25186 - hotfix check #236

Fixes #25186 - hotfix check #236

Conversation

kgaikwad commented Nov 29, 2018

theforeman-bot commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Nov 30, 2018

Choose a reason for hiding this comment

kgaikwad Dec 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky commented Jan 4, 2019

Choose a reason for hiding this comment

mbacovsky commented Jan 4, 2019

jameerpathan111 commented Jan 8, 2019

kgaikwad commented Jan 8, 2019

jameerpathan111 commented Jan 10, 2019

mbacovsky commented Jan 14, 2019

kgaikwad Dec 14, 2018 •

edited