Gal Hammer has uploaded a new change for review.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
BZ#845994 VM failed to start after file injection.
The "before_vm_start" hook created a file inside the guest's image file using the GuestFS library. The image was not unmounted explicitly and this caused libvirt to fail when it tried to read the image file.
Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Signed-off-by: Gal Hammer ghammer@redhat.com --- M vdsm_hooks/fileinject/before_vm_start.py 1 file changed, 25 insertions(+), 37 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/56/6956/1 -- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com
Gal Hammer has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 2: Verified
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 2: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 2: (3 inline comments)
.................................................... File vdsm_hooks/fileinject/before_vm_start.py Line 34: sys.stderr.write('fileinject: [error in inject_file]: %s\n' % e) why is this function swallowing so many exceptions, while its calling function has a global "except" of its own?
Line 46: try: I believe this "try" should come right after mount_options()
Line 50: gfs.upload(temp.name, filepath) why are we not using
gfs.write(filepath, content)
instead of creating the tempfile?
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Gal Hammer has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 2: (3 inline comments)
.................................................... File vdsm_hooks/fileinject/before_vm_start.py Line 34: sys.stderr.write('fileinject: [error in inject_file]: %s\n' % e) I preferred not to change the function's behavior too much in one patch. The calling code expect the inject_file function not to fail and return a boolean value.
Line 46: try: Done
Line 50: gfs.upload(temp.name, filepath) Done. I've used the existing code and not too familiar with the GuestFS library's functions.
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Peter V. Saveliev has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 3: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(2 inline comments)
there's still a big place for improvement, but I like the direction. thanks.
BTW, any idea how this code passed the automated build? I get
./vdsm_hooks/fileinject/before_vm_start.py:8: 'tempfile' imported but unused
.................................................... File vdsm_hooks/fileinject/before_vm_start.py Line 4: import sys Line 5: import hooking Line 6: import traceback Line 7: import guestfs Line 8: import tempfile thanks for dropping its usage, now please drop the import of tempfile. Line 9: Line 10: ''' Line 11: fileinject vdsm hook Line 12: ====================
Line 23: syntax: Line 24: fileinject=/<target file name> : <file content> Line 25: fileinject=/myfile:some file content\netc... Line 26: ''' Line 27: pep8 WANTED the newline that you've deleted. Line 28: def inject_file(filepath, content, drive, diskformat): Line 29: injected = False Line 30: gfs = guestfs.GuestFS() Line 31: try:
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Gal Hammer has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 3: (2 inline comments)
.................................................... File vdsm_hooks/fileinject/before_vm_start.py Line 4: import sys Line 5: import hooking Line 6: import traceback Line 7: import guestfs Line 8: import tempfile Done Line 9: Line 10: ''' Line 11: fileinject vdsm hook Line 12: ====================
Line 23: syntax: Line 24: fileinject=/<target file name> : <file content> Line 25: fileinject=/myfile:some file content\netc... Line 26: ''' Line 27: Done Line 28: def inject_file(filepath, content, drive, diskformat): Line 29: injected = False Line 30: gfs = guestfs.GuestFS() Line 31: try:
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Gal Hammer has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 4: Verified; Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 4:
Build Failed
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/446/ : ABORTED
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
Patch Set 4: Looks good to me, approved
Thanks!
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has submitted this change and it was merged.
Change subject: BZ#845994 VM failed to start after file injection. ......................................................................
BZ#845994 VM failed to start after file injection.
The "before_vm_start" hook created a file inside the guest's image file using the GuestFS library. The image was not unmounted explicitly and this caused libvirt to fail when it tried to read the image file.
Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Signed-off-by: Gal Hammer ghammer@redhat.com --- M vdsm_hooks/fileinject/before_vm_start.py 1 file changed, 22 insertions(+), 39 deletions(-)
Approvals: Gal Hammer: Verified; Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/6956 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: Ibb705a7153b1d2662a1c113bbbb5c694c568cabd Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org