[node-patches] Change in ovirt-node[master]: Mount kdump nfs targets before trying to use them

[fabiand at redhat.com]

Fabian Deutsch has posted comments on this change.

Change subject: Mount kdump nfs targets before trying to use them
......................................................................


Patch Set 7: Code-Review-1

(5 comments)

See my inline comments.

http://gerrit.ovirt.org/#/c/33439/7/scripts/ovirt-init-functions.sh.in
File scripts/ovirt-init-functions.sh.in:

Line 1587: 
Line 1588:         if is_persisted /etc/kdump.conf; then
Line 1589:             if [[ ! -z $OVIRT_KDUMP_NFS && $(major_release) -ge 7 ]]; then
Line 1590:                 [[ ! -d /tmp/kdump-nfs ]] && mkdir /tmp/kdump-nfs
Line 1591:                 mount /tmp/kdump-nfs
We should probably use /run/kdump-nfs/
Line 1592:             fi
Line 1593:             service kdump start
Line 1594:         fi
Line 1595: 


http://gerrit.ovirt.org/#/c/33439/7/src/ovirt/node/config/defaults.py
File src/ovirt/node/config/defaults.py:

Line 1120:                         "path": "/core"}
Line 1121: 
Line 1122:                 _set_values(vals)
Line 1123: 
Line 1124:         class MountNFS(utils.Transaction.Element):
MOUNT_TARGET="/run/kdump-nfs"

Please use  a variable for the mount target.
Line 1125:             title = "Mounting NFS volume for kdump configuration"
Line 1126: 
Line 1127:             def commit(self):
Line 1128:                 try:


Line 1125:             title = "Mounting NFS volume for kdump configuration"
Line 1126: 
Line 1127:             def commit(self):
Line 1128:                 try:
Line 1129:                     if not os.path.isdir("/tmp/kdump-nfs"):
We should probably use /run/kdump-nfs/.
And plese use the variable
Line 1130:                         os.makedirs("/tmp/kdump-nfs")
Line 1131:                     utils.process.check_call(["mount", "-t", "nfs", nfs,
Line 1132:                                               "/tmp/kdump-nfs"])
Line 1133: 


Line 1131:                     utils.process.check_call(["mount", "-t", "nfs", nfs,
Line 1132:                                               "/tmp/kdump-nfs"])
Line 1133: 
Line 1134:                     File("/etc/fstab").write(
Line 1135:                         "%s\t/tmp/kdump-nfs\tnfs\tdefaults\t0 0" % nfs, "a")
This will not work if we enable nfs kdump a couple of times. fstab will contain several of these lines.

A check needs to be done to see if it is already present.
Line 1136:                 except utils.process.CalledProcessError:
Line 1137:                     self.logger.warning("Failed to mount %s at " +
Line 1138:                                         "/tmp/kdump-nfs" %
Line 1139:                                         nfs, exc_info=True)


Line 1137:                     self.logger.warning("Failed to mount %s at " +
Line 1138:                                         "/tmp/kdump-nfs" %
Line 1139:                                         nfs, exc_info=True)
Line 1140: 
Line 1141:         class UmountNFS(utils.Transaction.Element):
This calss is not used, but it probably should, right?
Line 1142:             title = "Umounting NFS volume"
Line 1143: 
Line 1144:             def commit(self):
Line 1145:                 try:


-- 
To view, visit http://gerrit.ovirt.org/33439
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7b64094fe1c42638eac00ad81c79c043b884a528
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes