Bug 593276 - IPVSADM init script load modules when unneeded
Summary: IPVSADM init script load modules when unneeded
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: ipvsadm
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Matthias Saou
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 593279
TreeView+ depends on / blocked
 
Reported: 2010-05-18 12:13 UTC by Jan Friesse
Modified: 2011-07-26 03:29 UTC (History)
3 users (show)

Fixed In Version: ipvsadm-1.26-2.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-07-26 03:29:44 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Init file (4.84 KB, text/plain)
2010-05-18 12:15 UTC, Jan Friesse
no flags Details
Config file (774 bytes, text/plain)
2010-05-18 12:15 UTC, Jan Friesse
no flags Details
Spec file diff (1.62 KB, patch)
2010-05-18 12:15 UTC, Jan Friesse
no flags Details | Diff
Better init file (4.85 KB, text/plain)
2010-05-18 15:05 UTC, Jan Friesse
no flags Details

Description Jan Friesse 2010-05-18 12:13:11 UTC
This is clone of relevant information from RHEL5 bug.

Original Description of problem:
The original problem was that sosreport loaded kernel modules unexpectedly.
sosreport is a command to collect system information for
problem analysis by Red Hat support.
If an execution of sosreport changes the system configuration,
customers will likely be reluctant to run it.
That could result in more support cost, less customer satisfaction.

See below how ipvsadm loads the modules:

[root@dm120ei2 ~]# rpm -q ipvsadm
ipvsadm-1.24-10
[root@dm120ei2 ~]# lsmod > lsmod.1
[root@dm120ei2 ~]# ipvsadm -L
IP Virtual Server version 1.2.1 (size=4096)
Prot LocalAddress:Port Scheduler Flags
-> RemoteAddress:Port           Forward Weight ActiveConn InActConn
[root@dm120ei2 ~]# lsmod > lsmod.2
[root@dm120ei2 ~]# diff lsmod.1 lsmod.2
1a2
> ip_vs                 122113  0 

My Description of problem:

It is clear that loading module if somebody ask fro ipvsadm -L MAKES clear sense (how to gen status if module is not loaded?). So I don't think main problem is with ipvsadm itself, but rather with init script.

Included is my version of init script + ipvsadm-config + spec file change, which includes:
- More LSB compliance
- Don't call ipvsadm -L if module is not loaded
- Add ipvsadm-config file with options like IPVS_MODULES_UNLOAD (unload modules on stop), IPVS_SAVE_ON_STOP (save rules on stop) and IPVS_STATUS_NUMERIC

I'm taking this bug also as code review, because it really make sense to hear another maintainer opinion.

Patches included.

Regards,
  Honza

Comment 1 Jan Friesse 2010-05-18 12:15:13 UTC
Created attachment 414828 [details]
Init file

It really doesn't make too much sense to make patch file, because patch file have more than 90% replacements

Comment 2 Jan Friesse 2010-05-18 12:15:28 UTC
Created attachment 414829 [details]
Config file

Comment 3 Jan Friesse 2010-05-18 12:15:54 UTC
Created attachment 414830 [details]
Spec file diff

Spec file diff

Comment 5 Jan Friesse 2010-05-18 15:05:32 UTC
Created attachment 414886 [details]
Better init file

After review of Lon, better init file included.

It changes:
- Start ipvsadm after network
- Better handle of module name in rmmod_r
- Fix some typos

Comment 6 Matthias Saou 2010-05-23 14:56:38 UTC
I don't get the initial sosreport and module loading comment. The ipvsadm service doesn't start automatically when the package is installed, so the module gets loaded only if someone enabled or ran the service (and not just the command). If your complaint is about the module being loaded by the command, well... that's the same even after your changes, right?

Is there some other issue I'm missing?

I'll have a look at the changes, since most seem sane. Note that I don't know how many people still use this init script to configure and control LVS. I for instance mostly use keepalived's LVS capabilities in order to have LVS as well as HA using VRRP with a single configuration file.

Comment 7 Matthias Saou 2010-05-23 15:06:41 UTC
Everything I've seen so far looks good, except for the "check if ipvs module load is deactivated" in the init script. Its scope is too narrow and its implementation fragile. Any system where the module is blacklisted but someone is nevertheless trying to run the service shouldn't be expected to work IMHO. I'd remove that part. Maybe try to catch any module loading problem in some cleaner and more global way somehow, though I personally don't think it's worth it.

Comment 8 Jan Friesse 2010-05-24 07:34:13 UTC
Hi,

> I don't get the initial sosreport and module loading comment. The ipvsadm
> service doesn't start automatically when the package is installed, so the

right

> module gets loaded only if someone enabled or ran the service (and not just the
> command). If your complaint is about the module being loaded by the command,
> well... that's the same even after your changes, right?

right

> 
> Is there some other issue I'm missing?
> 

Ya. sosreport is running something like
foreach services; do /sbin/service $service status;done
even the service is not enabled for running.

So problem is, that old scripts runs ipvsadm -L -n, which (of course) loads module.

This is something what new script doesn't do, because it is testing, if /proc/net/ip_vs exists. If so, ipvsadm -L -n is started. If not, only message is printed.

Main idea of this is taken from iptables.

> I'll have a look at the changes, since most seem sane. Note that I don't know
> how many people still use this init script to configure and control LVS. I for
> instance mostly use keepalived's LVS capabilities in order to have LVS as well
> as HA using VRRP with a single configuration file.    

I don't think there are *many* people using real ipvsadm init script. But it doesn't mean, that it cannot be as much as possible LSB compliant and doesn't do something what sosreport doesn't like.

Comment 9 Jan Friesse 2010-05-24 07:42:12 UTC
(In reply to comment #7)
> Everything I've seen so far looks good, except for the "check if ipvs module
> load is deactivated" in the init script. Its scope is too narrow and its
> implementation fragile. Any system where the module is blacklisted but someone
> is nevertheless trying to run the service shouldn't be expected to work IMHO.
> I'd remove that part. Maybe try to catch any module loading problem in some
> cleaner and more global way somehow, though I personally don't think it's worth
> it.    

Agree that this way is not perfect. Again, I took that from iptables init script. Please feel free to remove that 6 lines.

Also please let me know if/when you will push init script to Fedora. I would like to keep Fedora/RHEL package as close as possible, because the current situation (RHEL5 has many init script improvements (start ipvsadm after network, more LSB compliant, ...) but non of them is in Fedora) is just very bad (and yes, I think it was fault of previous RHEL package maintainer).

Comment 10 Bug Zapper 2010-07-30 11:39:34 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 14 development cycle.
Changing version to '14'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 11 Matthias Saou 2011-07-11 11:08:41 UTC
I was about to include those changes, as well as updating to 1.26, but I'd also like to include the F15+ ipvsadm.service file for systemd. See #720175 for the details and don't hesitate to comment there, since the simple default file would revert lots of the changes implemented in the sysvinit script (optional saving, optional module reloading etc.)

Comment 12 Fedora Update System 2011-07-11 11:55:32 UTC
ipvsadm-1.26-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/ipvsadm-1.26-2.fc15

Comment 13 Jan Friesse 2011-07-11 12:06:53 UTC
(In reply to comment #11)
> I was about to include those changes, as well as updating to 1.26, but I'd also
> like to include the F15+ ipvsadm.service file for systemd. See #720175 for the
> details and don't hesitate to comment there, since the simple default file
> would revert lots of the changes implemented in the sysvinit script (optional
> saving, optional module reloading etc.)

Mathias,
from my point of view, best what we can do is wait for iptables conversion and then implement similar ideas (what is likely what you wanted to do as described in #720175). I personally don't have any experience with systemd so I probably cannot help there too much.

btw. doesn't systemd handle iptables by itself? (hopefully not, but I'm really not sure what is hiding behind ~3MB of source code)

Comment 14 Jóhann B. Guðmundsson 2011-07-11 13:02:26 UTC
Given comment 1 the actual problem here is sosreport not the legacy sysvinit
script or ipvsadm well first of all modules should be loaded via autoloading
based on bus information, or via /etc/modules-load.d/*.conf. and unloading a
module from the kernel should not be done except for debugging purposes. 

Secondly the issue here seems to be that "ipvsadm -L -n" is being called and it
only is called in the status() section of the legacy init script. 

In systemd you no longer can create a custom status output ( and other custom
() functions ) hence this is no longer relevant ( in F16+ ) since the command
never gets called...

Comment 15 Jan Friesse 2011-07-11 14:06:52 UTC
(In reply to comment #14)
> Given comment 1 the actual problem here is sosreport not the legacy sysvinit
> script or ipvsadm well first of all modules should be loaded via autoloading
> based on bus information, 

Ya right, but ipvs is like iptables, so no bus

> or via /etc/modules-load.d/*.conf. and unloading a

in other words you mean that ipvs/iptables must carry it's own *.conf file to that directory? What if user simply runs iptables/ipvsadm? Module is then loaded automatically even without /etc/modules-load/*.conf file, what systemd does then?

> module from the kernel should not be done except for debugging purposes. 
> 
> Secondly the issue here seems to be that "ipvsadm -L -n" is being called and it
> only is called in the status() section of the legacy init script. 
> 
> In systemd you no longer can create a custom status output ( and other custom
> () functions ) hence this is no longer relevant ( in F16+ ) since the command
> never gets called...

great, then what systemd status does? ipvsadm is very same as iptables so there is no notion of "daemon" or better, daemon is kernel module. I can understand what systemd does for standalone daemon app where systemd can save pid and simply look to processes if pid is still alive, but how systemd get status of something what is not daemon? 

Also one of the very nice feature of iptables script, which is also implemented in ipvsadm, is ability to call init script with save/restore command resulting in storing/restoring rules. How does systemd handles that?

Comment 16 Jóhann B. Guðmundsson 2011-07-11 14:28:23 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > Given comment 1 the actual problem here is sosreport not the legacy sysvinit
> > script or ipvsadm well first of all modules should be loaded via autoloading
> > based on bus information, 
> 
> Ya right, but ipvs is like iptables, so no bus
> 
> > or via /etc/modules-load.d/*.conf. and unloading a
> 
> in other words you mean that ipvs/iptables must carry it's own *.conf file to
> that directory? What if user simply runs iptables/ipvsadm? Module is then
> loaded automatically even without /etc/modules-load/*.conf file, what systemd
> does then?

Not must rather preferred 

> 
> > module from the kernel should not be done except for debugging purposes. 
> > 
> > Secondly the issue here seems to be that "ipvsadm -L -n" is being called and it
> > only is called in the status() section of the legacy init script. 
> > 
> > In systemd you no longer can create a custom status output ( and other custom
> > () functions ) hence this is no longer relevant ( in F16+ ) since the command
> > never gets called...
> 
> great, then what systemd status does? ipvsadm is very same as iptables so there
> is no notion of "daemon" or better, daemon is kernel module. I can understand
> what systemd does for standalone daemon app where systemd can save pid and
> simply look to processes if pid is still alive, but how systemd get status of
> something what is not daemon? 

Status is a part of systemd and clearly defined the man page for systemctl

status [NAME...|PID...]
           Show terse runtime status information about one or more units. This function is intended to generate human-readable output.
           If you are looking for computer-parsable output, use show instead. If a PID is passed information about the unit the process
           of the PID belongs to is shown.


> 
> Also one of the very nice feature of iptables script, which is also implemented
> in ipvsadm, is ability to call init script with save/restore command resulting
> in storing/restoring rules. How does systemd handles that?

There are no such things as "custom" commands ( save or otherwize )  systemd hence you cant other then calling the exact command when stopping the service 
Which is what I did in the submitted service file for ipvsadm. 

"ExecStop=/bin/bash -c "exec /sbin/ipvsadm-save -n > /etc/sysconfig/ipvsadm""

which saves the running configuration before stopping the service.

Comment 17 Jan Friesse 2011-07-11 15:08:58 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Given comment 1 the actual problem here is sosreport not the legacy sysvinit
> > > script or ipvsadm well first of all modules should be loaded via autoloading
> > > based on bus information, 
> > 
> > Ya right, but ipvs is like iptables, so no bus
> > 
> > > or via /etc/modules-load.d/*.conf. and unloading a
> > 
> > in other words you mean that ipvs/iptables must carry it's own *.conf file to
> > that directory? What if user simply runs iptables/ipvsadm? Module is then
> > loaded automatically even without /etc/modules-load/*.conf file, what systemd
> > does then?
> 
> Not must rather preferred 
> 
> > 
> > > module from the kernel should not be done except for debugging purposes. 
> > > 
> > > Secondly the issue here seems to be that "ipvsadm -L -n" is being called and it
> > > only is called in the status() section of the legacy init script. 
> > > 
> > > In systemd you no longer can create a custom status output ( and other custom
> > > () functions ) hence this is no longer relevant ( in F16+ ) since the command
> > > never gets called...
> > 
> > great, then what systemd status does? ipvsadm is very same as iptables so there
> > is no notion of "daemon" or better, daemon is kernel module. I can understand
> > what systemd does for standalone daemon app where systemd can save pid and
> > simply look to processes if pid is still alive, but how systemd get status of
> > something what is not daemon? 
> 
> Status is a part of systemd and clearly defined the man page for systemctl
> 
> status [NAME...|PID...]
>            Show terse runtime status information about one or more units. This
> function is intended to generate human-readable output.
>            If you are looking for computer-parsable output, use show instead.
> If a PID is passed information about the unit the process
>            of the PID belongs to is shown.
> 
> 

Ok, but my question is little different. ipvsadm isn't daemon, so what systemd will generate?

> > 
> > Also one of the very nice feature of iptables script, which is also implemented
> > in ipvsadm, is ability to call init script with save/restore command resulting
> > in storing/restoring rules. How does systemd handles that?
> 
> There are no such things as "custom" commands ( save or otherwize )  systemd
> hence you cant other then calling the exact command when stopping the service 
> Which is what I did in the submitted service file for ipvsadm. 
> 
> "ExecStop=/bin/bash -c "exec /sbin/ipvsadm-save -n > /etc/sysconfig/ipvsadm""
> 
> which saves the running configuration before stopping the service.

Great, but what if this should be configurable? something like /bin/bash -c '. /etc/sysconfig/ipvsadm && [ $save == yes ] && ipvsadm -save -n"?

Comment 18 Jóhann B. Guðmundsson 2011-07-11 15:20:56 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> Ok, but my question is little different. ipvsadm isn't daemon, so what systemd
> will generate?

Precisely this ....

[root@valhalla system]# systemctl start ipvsadm.service 
[root@valhalla system]# systemctl status ipvsadm.service 
ipvsadm.service - Initialise the Linux Virtual Server
	  Loaded: loaded (/lib/systemd/system/ipvsadm.service)
	  Active: active (exited) since Mon, 11 Jul 2011 15:16:21 +0000; 3s ago
	 Process: 26042 ExecStart=/bin/bash -c exec /sbin/ipvsadm-restore < /etc/sysconfig/ipvsadm (code=exited, status=0/SUCCESS)
	 Process: 26040 ExecStartPre=/sbin/ipvsadm -C (code=exited, status=0/SUCCESS)
	  CGroup: name=systemd:/system/ipvsadm.service

> Great, but what if this should be configurable? something like /bin/bash -c '.
> /etc/sysconfig/ipvsadm && [ $save == yes ] && ipvsadm -save -n"?

Now why would you want to make it configurable mean seriously why not then just hash out that line in the unit file and restart the daemon?

Comment 19 Fedora Update System 2011-07-12 05:21:29 UTC
Package ipvsadm-1.26-2.fc15:
* should fix your issue,
* was pushed to the Fedora 15 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing ipvsadm-1.26-2.fc15'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/ipvsadm-1.26-2.fc15
then log in and leave karma (feedback).

Comment 20 Fedora Update System 2011-07-26 03:29:29 UTC
ipvsadm-1.26-2.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.


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