Bug 833007 - [Patch] fixes for cases no CDS found and some enhancements for RHUI load balancer plugin
[Patch] fixes for cases no CDS found and some enhancements for RHUI load bala...
Status: CLOSED ERRATA
Product: Red Hat Update Infrastructure for Cloud Providers
Classification: Red Hat
Component: RHUA (Show other bugs)
2.1
Unspecified Unspecified
high Severity unspecified
: ---
: 2.1.1
Assigned To: James Slagle
mkovacik
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 07:37 EDT by Satoru SATOH
Modified: 2013-02-27 11:59 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
When clients tried to access CDS servers where none were accessible, the system will try to connect to a https://None/. This bug updates rhui-lb.py so that an appropriate error message displays.
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-02-27 11:59:14 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
[PATCH 1/4] added error checks and avoid access to the 'None' cds (1.14 KB, patch)
2012-06-18 07:43 EDT, Satoru SATOH
no flags Details | Diff
[PATCH 2/4] [] is evaluated as False in boolean context (871 bytes, patch)
2012-06-18 08:01 EDT, Satoru SATOH
no flags Details | Diff
[PATCH 3/4] introduced _get_lb_from_file() to make it exceptional safe while loading CDS from list files (1.34 KB, patch)
2012-06-18 08:14 EDT, Satoru SATOH
no flags Details | Diff
[PATCH 4/4] introduced a cached cds list file to save CDS found, and make it not trying to overwrite the cds list provided from RHUI and owned by client entitlement RPM package (2.67 KB, patch)
2012-06-18 08:22 EDT, Satoru SATOH
no flags Details | Diff

  None (edit)
Description Satoru SATOH 2012-06-18 07:37:56 EDT
Description of problem:

If no CDS is accessible from client, client trys to access the server 'None'
due to a possible bug in /etc/rhui/client-yum-plugin/rhui-lb.py.


Version-Release number of selected component (if applicable):
rh-rhui-tools-2.0.64-1.el6_2.noarch


How reproducible: Always if CDS is not accessible from clients.

Steps to Reproduce:
1. Make all CDS servers not accessible from clients
in which rhui entitlement rpms installed and have access rights to
these CDS servers
2. run yum on that client, e.g. yum repolist
  
Actual results: client trys to access to https://None/...


Expected results: Error messages such like 
"No accessible CDS found." and make clients not trying to 
access to https://None/...


Additional info:
I'll attach a series of patches fixes this issue and related fixes and enhancements.
Comment 1 Satoru SATOH 2012-06-18 07:43:17 EDT
Created attachment 592615 [details]
[PATCH 1/4] added error checks and avoid access to the 'None' cds

This patch fixes the issue I reported.

Also, this patch try to detect cases if CDS (load balancers)
not found in the cds list file asap.


I'm not sure how to use conduit.error, so if that is wrong,
please replace that w/ yum.Errors.RepoError or sometihng
more suitable one.
Comment 2 Satoru SATOH 2012-06-18 08:01:58 EDT
Created attachment 592619 [details]
[PATCH 2/4] [] is evaluated as False in boolean context

This is a trivial patch to make it confirming to the style guide line:
http://code.google.com/p/soc/wiki/PythonStyleGuide#True_False_evaluations


That is, [] is evaluated as False in boolean context so len() is
not needed in actual.
Comment 3 Satoru SATOH 2012-06-18 08:14:32 EDT
Created attachment 592621 [details]
[PATCH 3/4] introduced _get_lb_from_file() to make it exceptional safe while loading CDS from list files

Currently, there is no try..catch clause while loading CDS from list file.
So there is a possibility of crash of yum if permission-denied or 
file-not-existent or something cause IO/OSError while loading it
due to operational mistake and so on.

This patch adds a utility function to make it exceptional safe and
just returns [] (empty cds list) if something goes wrong w/ list files.


And if [] is returned, administrators still can find such errors
as I added logging code to notify CDS list is empty in previous
patch (#1: 1/4), I guess.
Comment 4 Satoru SATOH 2012-06-18 08:22:27 EDT
Created attachment 592625 [details]
[PATCH 4/4] introduced a cached cds list file to save CDS found, and make it not trying to overwrite the cds list provided from RHUI and owned by client entitlement RPM package

This patch adds some experimental code to make CDS found but not 
in the CDS list provided by client entitlement RPMs originally
built w/ rhui-manager, into the other cached version of CDS list
(/var/cache/yum/rhui-load-balancers for example in this patch).

The file /etc/yum.repos.d/rhui-load-balancers is owned by
client entitlement RPMs and IMHO it is not intended to be 
overwritten.

Also, this patch makes use of newer 'with' statement can be 
run in python > 2.5 but it's not essential. So it should be
able to replaced w/ try..finally caluse if needed.
Comment 5 James Slagle 2012-06-20 15:18:48 EDT
I think we could probably incorporate patches 1 and 2 in a future release.  

Patch 3 has the effect of hiding the error that caused the exception.  Since an error at this step is unlikely, I think we want to let the exception bubble up so that it's displayed to the user and we know what went wrong.  Although, we could most likely make that more user friendly. 

Patch 4, I'm not sure what the intent is.  Just to not modify the packaged file?  I actually think this is ok.  The rhui-load-balancers file should be considered like a config file.  We should make sure this is marked as %config in our spec file.  Also, we can't make use of the "with" keyword as we still have to support Python 2.4 which is what's available in RHEL 5.
Comment 6 Satoru SATOH 2012-06-20 23:32:10 EDT
Thanks a lot for reaviewing these!


Comments inline below.

> Patch 4, I'm not sure what the intent is.  Just to not modify the packaged file?

I intended that (not try to touch files owned by RPMs) in this patch.

> I actually think this is ok.  The rhui-load-balancers file should be 
> considered like a config file.  We should make sure this is marked as
> %config in our spec file.

IMHO, it does not look true, as far as my understanding and rpm in rhel 6.

Configuration files not marked as "noreplace" are owned by RPM packages
and user and program should not touch it as it is replaced anytime
the package updated in general (this behavior might change depends on
rpm version: rpm in rhel6 seems like so as far as I read its source code).

And if files may be modified, special flag '%verify(not md5 size mtime)'
should be needed for that files in RPM specs.

If user or program modified it, verification may be failed such like:

[root@rhel-6-client-0 ~]# rpm -qf /etc/yum.repos.d/rhui-load-balancers
rhui-client-entitlement-rhel-6-2.0-1.el6.noarch
[root@rhel-6-client-0 ~]# vim /etc/yum.repos.d/rhui-load-balancers
... (add CDS instance) ...
[root@rhel-6-client-0 ~]# rpm -V rhui-client-entitlement-rhel-6
S.5....T.  c /etc/yum.repos.d/rhui-load-balancers
[root@rhel-6-client-0 ~]#


Also, FHS says /etc is to keep *static* local files used to control
the operation of a program:
http://www.pathname.com/fhs/pub/fhs-2.3.html#ETCHOSTSPECIFICSYSTEMCONFIGURATION

(Unfortunatelly, there is some exceptional cases such like
/etc/blkid.tab, /etc/resolv.conf, /etc/mtab - but it's just a symlink to
/proc/mounts these days - because histrocal reasons or something, however.)


And I feel unconfortable program may change config files under /etc
anytime before we know it and it does not have special reason for that
such as historical limitation or something. IMHO, it should be placed
under /var if program may modify it as needed.

> Also, we can't make use of the "with" keyword as we still
> have to support Python 2.4 which is what's available in RHEL 5.

Sorry, I forgot it. Then, how about replace that with statements w/ 
try ... finally clause?
Comment 7 wes hayutin 2012-07-30 15:17:48 EDT
Confirmed patch #1 works..

Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity
Could not retrieve mirrorlist https://None/pulp/mirror/path/test error was
14: PYCURL ERROR 6 - "Couldn't resolve host 'None'"
repo id                                     repo name                                                          status
rhui-custom-10001                           Custom Repositories - 10001                                        0
rhui-rhui-2.0                               Red Hat Update Infrastructure 2.0 (RPMs)                           0
repolist: 0
[root@ip-10-190-159-157 ~]# patch   /usr/lib/yum-plugins/rhui-lb.py /root/yum.patch 
patching file /usr/lib/yum-plugins/rhui-lb.py
Hunk #1 succeeded at 49 (offset 2 lines).
Hunk #2 succeeded at 89 (offset 2 lines).
[root@ip-10-190-159-157 ~]# yum repolist
Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity
No CDS load balancers found in the lists.
rhui-custom-10001                                                                             | 2.6 kB     00:00     
rhui-custom-10001/primary_db                                                                  | 7.4 kB     00:00     
rhui-rhui-2.0                                                                                 | 2.6 kB     00:00     
rhui-rhui-2.0/primary_db                                                                      |  23 kB     00:00     
repo id                                     repo name                                                          status
rhui-custom-10001                           Custom Repositories - 10001                                         1
rhui-rhui-2.0                               Red Hat Update Infrastructure 2.0 (RPMs)                           37
Comment 8 wes hayutin 2012-07-30 15:23:37 EDT
sorry.. adding a snippet of yum working w/ an empty load balancer file 


[root@ip-10-190-159-157 ~]# cat /etc/yum.repos.d/rhui-load-balancers
[root@ip-10-190-159-157 ~]# yumdownloader gofer
Loaded plugins: product-id, rhui-lb
No CDS load balancers found in the lists.
gofer-0.64-1.el6.noarch.rpm
Comment 9 wes hayutin 2012-07-30 15:38:28 EDT
regarding patch #3

w/o patching...

[root@ip-10-190-159-157 yum.repos.d]# rm -Rf /etc/yum.repos.d/rhui-load-balancers
[root@ip-10-190-159-157 yum.repos.d]# yum repolist
Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity

[Errno 2] No such file or directory: '/etc/yum.repos.d/rhui-load-balancers'

** on a rhel6 client, this seems fine to me w/o a patch, also tested removing the contents of the file and patch#1 handled that.

+1 on the patch in https://bugzilla.redhat.com/show_bug.cgi?id=833007#c1
Comment 10 wes hayutin 2012-07-30 16:11:23 EDT
moving to version 2.1.1
Comment 11 James Slagle 2013-01-25 17:11:01 EST
Again, thanks for submitting these patches!

I've committed patches 1 and 2 to our cloude repo (bce25888274a6efe2b46c9a9b26d7de06cefa4ab).

Patch 3: I don't want to commit for the reason I mentioned above.  We want to fail in this case.  That file should never be missing, or unreadable.  If it is, we don't want the yum transaction to continue.  If the customer has some reason why this can't be, they can always disable the rhui-lb plugin to accomplish whatever they're trying to accomplish.

Patch 4: I understand your concerns about the violation of the FHS and modifying rpm owned files.  I'm not sure I know the full history as to why these choices were made initially for rhui-lb.  However, I don't want to introduce a lot of change, which would include migrations for existing customers unless their is a specific blocking issue we're  trying to solve.
Comment 13 Vitaly Kuznetsov 2013-02-05 10:56:35 EST
[root@rhua ~]# rpm -q rh-rhui-tools
rh-rhui-tools-2.1.15-1.el6_3.noarch

[root@cds1 ~]# service pulp-cds stop
Stopping httpd:                                            [  OK  ]
Stopping goferd                                            [  OK  ]

[root@cli1 ~]# yum repolist
Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity


Could not contact any CDS load balancers: cds1.example.com.
Comment 15 errata-xmlrpc 2013-02-27 11:59:14 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2013-0571.html

Note You need to log in before you can comment on or make changes to this bug.