Change in vdsm[master]: Ifcfg: Fix file writing races

[amuller]

Assaf Muller has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1: Code-Review+1

Wow, big up Antoni... Sounds like a painful debug session.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4887/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4002/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4812/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/665/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[asegurap]

Antoni Segura Puimedon has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1: Verified+1

Passes the whole functional test suite on my host.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4887/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4002/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4812/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/666/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4887/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4002/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4812/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/668/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1: -Verified

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4887/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4002/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4812/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/669/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[danken]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Ifcfg: Fix file writing races
..


Ifcfg: Fix file writing races

On a fast enough computer, it could very well happen that we'd do:
open('foo', 'w').write('moo')
execCmd(['cat', 'foo'])

And that the content that 'cat' would show for foo would not be 'moo'
but whatever was there before the write, due to the file closing
happening at a non deterministic point of time.

How was this horribly edgy case unearthed? Good question!

Let's say we have a bond0 with slaves em2 and em1. We are in the
process of removing the bond completely.

removeNic basically stripes the nic ifcfg file of everything except
the hwaddr, device type, etc. So, if there was previously for nic em1
MASTER=bond0

after removeNic, there is an ifup of em1 (to leave the link state up)
that would still see bond0 as master. That in turn would make
initscripts trigger ifup of bond0, but that ifcfg file no longer
exists, ifup fails, raises an exception, and the whole removal is
rolled back.

Needless to say, this was unfindable with the debugger, as the file
close would be fast enough due to the debugger slowing down operation.

Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Signed-off-by: Antoni S. Puimedon asegu...@redhat.com
Reviewed-on: http://gerrit.ovirt.org/20050
Reviewed-by: Assaf Muller amul...@redhat.com
Reviewed-by: Dan Kenigsberg dan...@redhat.com
---
M vdsm/netconf/ifcfg.py
1 file changed, 14 insertions(+), 9 deletions(-)

Approvals:
  Antoni Segura Puimedon: Verified
  Assaf Muller: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[asegurap]

Antoni Segura Puimedon has uploaded a new change for review.

Change subject: Ifcfg: Fix file writing races
..

Ifcfg: Fix file writing races

On a fast enough computer, it could very well happen that we'd do:
open('foo', 'w').write('moo')
execCmd(['cat', 'foo'])

And that the content that 'cat' would show for foo would not be 'moo'
but whatever was there before the write, due to the file closing
happening at a non deterministic point of time.

How was this horribly edgy case unearthed? Good question!

Let's say we have a bond0 with slaves em2 and em1. We are in the
process of removing the bond completely.

removeNic basically stripes the nic ifcfg file of everything except
the hwaddr, device type, etc. So, if there was previously for nic em1
MASTER=bond0

after removeNic, there is an ifup of em1 (to leave the link state up)
that would still see bond0 as master. That in turn would make
initscripts trigger ifup of bond0, but that ifcfg file no longer
exists, ifup fails, raises an exception, and the whole removal is
rolled back.

Needless to say, this was unfindable with the debugger, as the file
close would be fast enough due to the debugger slowing down operation.

Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Signed-off-by: Antoni S. Puimedon asegu...@redhat.com
---
M vdsm/netconf/ifcfg.py
1 file changed, 14 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/50/20050/1

diff --git a/vdsm/netconf/ifcfg.py b/vdsm/netconf/ifcfg.py
index d2032a7..b278183 100644
--- a/vdsm/netconf/ifcfg.py
+++ b/vdsm/netconf/ifcfg.py
@@ -267,7 +267,8 @@
 os.makedirs(dirName)
 os.chown(dirName, vdsm_uid, 0)
 
-open(backup, 'w').write(content)
+with open(backup, 'w') as backupFile:
+backupFile.write(content)
 os.chown(backup, vdsm_uid, 0)
 logging.debug(Persistently backed up %s 
   (until next 'set safe config'), backup)
@@ -337,14 +338,15 @@
 
 def restoreAtomicBackup(self):
 logging.info(Rolling back configuration (restoring atomic backup))
-for confFile, content in self._backups.iteritems():
+for confFilePath, content in self._backups.iteritems():
 if content is None:
-utils.rmFile(confFile)
+utils.rmFile(confFilePath)
 logging.debug('Removing empty configuration backup %s',
-  confFile)
+  confFilePath)
 else:
-open(confFile, 'w').write(content)
-logging.info('Restored %s', confFile)
+with open(confFilePath, 'w') as confFile:
+confFile.write(content)
+logging.info('Restored %s', confFilePath)
 
 def _devType(self, content):
 if re.search('^TYPE=Bridge$', content, re.MULTILINE):
@@ -479,7 +481,8 @@
 self._backup(fileName)
 logging.debug('Writing to file %s configuration:\n%s' % (fileName,
   configuration))
-open(fileName, 'w').write(configuration)
+with open(fileName, 'w') as confFile:
+confFile.write(configuration)
 os.chmod(fileName, 0664)
 try:
 selinux.restorecon(fileName)
@@ -559,7 +562,8 @@
 
 # create the bonding device to avoid initscripts noise
 if bond.name not in open(netinfo.BONDING_MASTERS).read().split():
-open(netinfo.BONDING_MASTERS, 'w').write('+%s\n' % bond.name)
+with open(netinfo.BONDING_MASTERS, 'w') as bondingMasters:
+bondingMasters.write('+%s\n' % bond.name)
 
 def addNic(self, nic, **opts):
  Create ifcfg-* file with proper fields for NIC 
@@ -606,7 +610,8 @@
if line.startswith('HWADDR=')]
 l = ['DEVICE=%s\n' % nic, 'ONBOOT=yes\n',
  'MTU=%s\n' % netinfo.DEFAULT_MTU] + hwlines
-open(cf, 'w').writelines(l)
+with open(cf, 'w') as nicFile:
+nicFile.writelines(l)
 except IOError:
 pass
 


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Ifcfg: Fix file writing races

[oVirt Jenkins CI Server]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Ifcfg: Fix file writing races
..


Patch Set 1: Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/664/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4887/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4002/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4812/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6aabfbba44f1250f305ef6ec06f715e4782c4d1b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Assaf Muller amul...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Livnat Peer lp...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches