Bug 1260747 - Create the filesystem layout at install time in the places where needed
Create the filesystem layout at install time in the places where needed
Status: ON_QA
Product: vdsm
Classification: oVirt
Component: General (Show other bugs)
4.17.9
Unspecified Unspecified
medium Severity low (vote)
: ovirt-4.2.0
: 4.20.8
Assigned To: Yaniv Bronhaim
Petr Kubica
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-07 11:22 EDT by Fabian Deutsch
Modified: 2017-11-29 03:47 EST (History)
13 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: Infra
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rule-engine: ovirt‑4.2+
rule-engine: planning_ack+
mperina: devel_ack+
pstehlik: testing_ack+


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
oVirt gerrit 81898 master ABANDONED Remove mkdir /rhev from init scripts 2017-11-01 05:17 EDT
oVirt gerrit 81901 master MERGED Remove old code that was required by spice server 2017-10-17 06:57 EDT
oVirt gerrit 83473 master MERGED Remove from init script the mkdir of /var/log/vdsm/backup 2017-11-09 05:56 EST
oVirt gerrit 83474 master ABANDONED Remove from init script the mkdir of /var/run/vdsm 2017-11-05 07:07 EST

  None (edit)
Description Fabian Deutsch 2015-09-07 11:22:13 EDT
Description of problem:
While looking at vdsm/vdsmd_init_common.sh.in I notice that some
directories are created at runtime (when the service comes up in this
case).

Creating the dirs at installation time would be very beneficial for image based deliveries like Node or containers, because in those scenarios the filesystem tree is often read-only at runtime.

However, for some parts it still makes sense to check at runtime if the FS layout needs to be created, i.e. for /run.
Comment 1 Fabian Deutsch 2015-09-24 08:51:56 EDT
Some more details are given here: http://0pointer.net/blog/projects/stateless.html
Comment 2 Yaniv Bronhaim 2015-10-18 09:02:15 EDT
Dan, any historical reason why we create those folders on first run and not as part of the installation?


task_mkdirs(){                                                                  

    _mk_data_center - /rhev - ? any reason we don't create it in installation time?
    _mk_core_path   - /var/log/core ? we do ""install -dDm 1777 %{buildroot}%{_localstatedir}/log/core"" only in rhel. why not always?

    _mk_dom_backup  - /var/log/vdsm/backup - we create it in spec - this can be removed (line 1057) no?

    _mk_run_path    - we do create those subfolders /var/run/vdsm subfolders as part of installation - we can remove the mkdir, no?

    _mk_console_path - /var/run/ovirt-vmconsole-console - isn't ovirt-vmconsole should take care of creating this folder? how can it not be there? why do we have this condition there?

    "/usr/bin/chmod" 1777 /dev/shm                                              
}
Comment 3 Dan Kenigsberg 2015-10-19 03:25:36 EDT
(In reply to Yaniv Bronhaim from comment #2)
> Dan, any historical reason why we create those folders on first run and not
> as part of the installation?

There might have been special considerations on ovirt-node. Placing mkdir in startup gives a little bit more protection against someone deleting a required directory by mistake, or chaning its permission. It also provides a more precise error log in case the directory does not exist and cannot be created.

However, these are mostly excuses. We should conform to standards. At most, we can check (and fail startup) if these directories are missing.


> task_mkdirs(){                                                              
> 
> 
>     _mk_data_center - /rhev - ? any reason we don't create it in
> installation time?
>     _mk_core_path   - /var/log/core ? we do ""install -dDm 1777
> %{buildroot}%{_localstatedir}/log/core"" only in rhel. why not always?

commit 670503889: conform to Fedora standards. We need to remove this dropbox completely.

> 
>     _mk_dom_backup  - /var/log/vdsm/backup - we create it in spec - this can
> be removed (line 1057) no?
> 
>     _mk_run_path    - we do create those subfolders /var/run/vdsm subfolders
> as part of installation - we can remove the mkdir, no?

Just double check they exist on boot time (with /var/run being tmpfs) and on the node.

> 
>     _mk_console_path - /var/run/ovirt-vmconsole-console - isn't
> ovirt-vmconsole should take care of creating this folder? how can it not be
> there? why do we have this condition there?

I believe it was only Francesco trying to be nicer and not recreate an existing directory.

> 
>     "/usr/bin/chmod" 1777 /dev/shm                                          

Long long ago this was required by spice server. I think it can (and should!) be gone.
Comment 4 Yaniv Bronhaim 2016-04-25 06:52:32 EDT
it mostly about cleanup and arrangement of vdsm init script. moving that to 4.1
Comment 5 Yaniv Kaul 2017-09-04 07:46:10 EDT
(In reply to Yaniv Bronhaim from comment #4)
> it mostly about cleanup and arrangement of vdsm init script. moving that to
> 4.1

What's the latest?
Comment 6 Yaniv Bronhaim 2017-09-05 07:11:12 EDT
I wouldn't try to achieve this in 4.1, I missed that request for awhile.. 
it touches few aspects that require deeper check - probably most of it is for node setup, so it requires some verification, depends of our support back, but that's basically it. I believe it can be done for 4.2 if we want to remove that code, but it's only for the cleanup, if we do those mkdirs during installation already and it works on node.
Comment 7 Yaniv Kaul 2017-09-05 07:46:54 EDT
(In reply to Yaniv Bronhaim from comment #6)
> I wouldn't try to achieve this in 4.1, I missed that request for awhile.. 
> it touches few aspects that require deeper check - probably most of it is
> for node setup, so it requires some verification, depends of our support
> back, but that's basically it. I believe it can be done for 4.2 if we want
> to remove that code, but it's only for the cleanup, if we do those mkdirs
> during installation already and it works on node.

We missed 4.1 with this. I'm wondering if we are going to do it in 4.2.
If we do, now's the time.
Comment 8 Fabian Deutsch 2017-09-05 10:50:14 EDT
Technically we don't need this anymore. It would still help Node NG a little, but it's not as urgent as before.
Comment 9 Francesco Romani 2017-09-18 06:01:57 EDT
(In reply to Dan Kenigsberg from comment #3)
> >     _mk_console_path - /var/run/ovirt-vmconsole-console - isn't
> > ovirt-vmconsole should take care of creating this folder? how can it not be
> > there? why do we have this condition there?
> 
> I believe it was only Francesco trying to be nicer and not recreate an
> existing directory.

The problem here is who - between Vdsm and ovirt-vmconsole owns this directory.
Vdsm has optional support for ovirt-vmconsole, so it takes care of setting up the directory as it needs.

In order to remove the snippet from init_common.sh we need to fix Vdsm to
1. assume the directory is present and with right attributes (permissions, labels) if the support for ovirt-vmconsole is enabled
2. use a different path - and set it once? if not

BTW, back in time, I remember we discussed that checking paths, permissions and labels every startup added a bit of security; otherwise Vdsm will blindly trust what happened at install time. Not sure if this is relevant anymore.
Comment 10 Yaniv Bronhaim 2017-11-01 05:45:50 EDT
The status is:

1. /rhev - we won't touch (see nir's comment on the abandoned patch - http://gerrit.ovirt.org/81898)

2. For /var/run/vdsm  /var/log/backup and /var/log/core we have:

https://gerrit.ovirt.org/#/c/81899/ - Dan suggests something not related to the removal. still wait for approval on that change

Just posted: https://gerrit.ovirt.org/#/c/83473/ and https://gerrit.ovirt.org/83474 - for the rest. 

This was verified with ost using https://gerrit.ovirt.org/82424

3. /var/run/ovirt-vmconsole-console - we won't touch (see comment #9)
"/usr/bin/chmod" 1777 /dev/shm - already removed


So basically we wait to merge https://gerrit.ovirt.org/#/c/81899/ https://gerrit.ovirt.org/#/c/83473/ and ttps://gerrit.ovirt.org/83474
Comment 11 Yaniv Bronhaim 2017-11-08 09:57:50 EST
https://gerrit.ovirt.org/81899 - was changed to remove all current vdsm handling with /var/log/core , basically after abrt integration abrt service configures the system where to output core dumps.
Rest of the patches are merged. Moving to MODIFIED

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