Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 4: Verified+1 Verified by running latest engine and vdsm and by creating and suspending a vm. -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
automat...@ovirt.org has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 5: * Update tracker::IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found. -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Dan Kenigsberg has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 4: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Dan Kenigsberg has submitted this change and it was merged. Change subject: events: make sure to send only one event per status .. events: make sure to send only one event per status Initially we made an assumption that engine can handle more than one event containing the same status. The assumption is not true and there are flows where engine logic assumes only one status change. One of the problematic flows is vm destroy for which having duplicate means that the operation failed and recovery flow is triggered. Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Signed-off-by: pkliczewski piotr.kliczew...@gmail.com Reviewed-on: https://gerrit.ovirt.org/42579 Continuous-Integration: Jenkins CI Reviewed-by: Francesco Romani from...@redhat.com Reviewed-by: Dan Kenigsberg dan...@redhat.com --- M vdsm/virt/vm.py 1 file changed, 20 insertions(+), 21 deletions(-) Approvals: Piotr Kliczewski: Verified Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved Francesco Romani: Looks good to me, but someone else must approve -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Francesco Romani has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Francesco Romani has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 3: Code-Review+1 (1 comment) looks ok, thanks for the changes. Minor comment inside. https://gerrit.ovirt.org/#/c/42579/3/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 29: import threading Line 30: import time Line 31: import uuid Line 32: Line 33: # 3rd party libs importss unneeded change Line 34: import libvirt Line 35: Line 36: # vdsm imports Line 37: from vdsm import constants -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 3: (1 comment) https://gerrit.ovirt.org/#/c/42579/3/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 29: import threading Line 30: import time Line 31: import uuid Line 32: Line 33: # 3rd party libs importss unneeded change Sure. Will remove. Line 34: import libvirt Line 35: Line 36: # vdsm imports Line 37: from vdsm import constants -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
automat...@ovirt.org has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 4: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has posted comments on this change.
Change subject: events: make sure to send only one event per status
..
Patch Set 2:
(4 comments)
https://gerrit.ovirt.org/#/c/42579/2//COMMIT_MSG
Commit Message:
Line 6:
Line 7: events: make sure to send only one event per status
Line 8:
Line 9: Initially we made an assumption that engine can handle more than one
Line 10: event containing the same status. The assumtion is not true and there
typo: assumption
Done
Line 11: are flows where engine logic assumes only one status change.
Line 12:
Line 13: One of the problematic flows is vm distroy for which having duplicate
Line 14: means that the operation failed and recovery flow is triggered.
Line 9: Initially we made an assumption that engine can handle more than one
Line 10: event containing the same status. The assumtion is not true and there
Line 11: are flows where engine logic assumes only one status change.
Line 12:
Line 13: One of the problematic flows is vm distroy for which having duplicate
typo: destroy
Done
Line 14: means that the operation failed and recovery flow is triggered.
Line 15:
Line 16:
Line 17: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
https://gerrit.ovirt.org/#/c/42579/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 381: self._lastStatus = value
Line 382:
Line 383: def send_status_event(self):
Line 384: vm_status = self._getVmStatus()['status']
Line 385: with self._eventLock:
ok with the concept, but...
:)
Line 386: if vm_status != self._evaluatedStatus:
Line 387: self._evaluatedStatus = vm_status
Line 388: stats = {
Line 389: 'status': vm_status,
Line 399: if 'exitMessage' in self.conf:
Line 400: stats['exitMessage'] =
self.conf['exitMessage']
Line 401: if 'exitReason' in self.conf:
Line 402: stats['exitReason'] = self.conf['exitReason']
Line 403:
until here, we're copying data around, or doing easy computations (hash()).
Done
Line 404: self.log.debug('Last status %s and evaluated status
%s',
Line 405:self.lastStatus, vm_status)
Line 406: self._notify('VM_status', stats)
Line 407:
--
To view, visit https://gerrit.ovirt.org/42579
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Arik Hadas aha...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com
Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
automat...@ovirt.org has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 3: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Francesco Romani has posted comments on this change.
Change subject: events: make sure to send only one event per status
..
Patch Set 2:
(4 comments)
-1 for visibility/discussion
I'd prefer to notify(), and log, without the lock held, and should be easily
done. Other than that, looks OK. Please see inline comments for suggestions.
Some typos in the commit message, which I don't really care too much about.
https://gerrit.ovirt.org/#/c/42579/2//COMMIT_MSG
Commit Message:
Line 6:
Line 7: events: make sure to send only one event per status
Line 8:
Line 9: Initially we made an assumption that engine can handle more than one
Line 10: event containing the same status. The assumtion is not true and there
typo: assumption
Line 11: are flows where engine logic assumes only one status change.
Line 12:
Line 13: One of the problematic flows is vm distroy for which having duplicate
Line 14: means that the operation failed and recovery flow is triggered.
Line 9: Initially we made an assumption that engine can handle more than one
Line 10: event containing the same status. The assumtion is not true and there
Line 11: are flows where engine logic assumes only one status change.
Line 12:
Line 13: One of the problematic flows is vm distroy for which having duplicate
typo: destroy
Line 14: means that the operation failed and recovery flow is triggered.
Line 15:
Line 16:
Line 17: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
https://gerrit.ovirt.org/#/c/42579/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 381: self._lastStatus = value
Line 382:
Line 383: def send_status_event(self):
Line 384: vm_status = self._getVmStatus()['status']
Line 385: with self._eventLock:
ok with the concept, but...
Line 386: if vm_status != self._evaluatedStatus:
Line 387: self._evaluatedStatus = vm_status
Line 388: stats = {
Line 389: 'status': vm_status,
Line 399: if 'exitMessage' in self.conf:
Line 400: stats['exitMessage'] =
self.conf['exitMessage']
Line 401: if 'exitReason' in self.conf:
Line 402: stats['exitReason'] = self.conf['exitReason']
Line 403:
until here, we're copying data around, or doing easy computations (hash()). So
far so good.
I'm a bit worried by logging and _notify()ing with a lock held.
I'm sure most of time will be OK, but still...
It should be easy to move them both outside the lock, maybe with an helper
variable for self.lastStatus.
So, it will read something like:
with. self._eventLock:
# produce stats. If not stats needed, let's do stats = {}
# ...
current_status = self.lastStatus # to log the same thing we used to do
logic
if stats:
self.log.debug('Last status %s and evaluated status %s',
current_status, vm_status)
self._notify('VM_status', stats)
# method ends here
this way should be a bit safer.
Line 404: self.log.debug('Last status %s and evaluated status
%s',
Line 405:self.lastStatus, vm_status)
Line 406: self._notify('VM_status', stats)
Line 407:
--
To view, visit https://gerrit.ovirt.org/42579
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Arik Hadas aha...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com
Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Arik Hadas has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 2: Verified+1 (1 comment) got one event for Down (the bug on engine side where the event is received on each host still exist, but with 1 host it works properly) https://gerrit.ovirt.org/#/c/42579/2//COMMIT_MSG Commit Message: Line 10: event containing the same status. The assumtion is not true and there Line 11: are flows where engine logic assumes only one status change. Line 12: Line 13: One of the problematic flows is vm distroy for which having duplicate Line 14: means that the operation failed and recovery flow is triggered. As I described in above commit message. The engine will make an assumption I think there are two cases: 1. The timestamp on the event is earlier than the one we got on poll - the engine will ignore the event 2. The timestamp on the poll is earlier than the one on the event - then the engine process the event. So engine assumes that status that is returned by polling was sent by an event before. Line 15: Line 16: Line 17: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Arik Hadas aha...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 2: (1 comment) https://gerrit.ovirt.org/#/c/42579/2//COMMIT_MSG Commit Message: Line 10: event containing the same status. The assumtion is not true and there Line 11: are flows where engine logic assumes only one status change. Line 12: Line 13: One of the problematic flows is vm distroy for which having duplicate Line 14: means that the operation failed and recovery flow is triggered. I don't really mind the vdsm-side patch, but what would happen on Engine it As I described in above commit message. The engine will make an assumption that vm was not destroyed and will attempt to recover it. The engine is making a lot of assumptions around vm life cycle and this is one of many. Line 15: Line 16: Line 17: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Dan Kenigsberg has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 2: (1 comment) https://gerrit.ovirt.org/#/c/42579/2//COMMIT_MSG Commit Message: Line 10: event containing the same status. The assumtion is not true and there Line 11: are flows where engine logic assumes only one status change. Line 12: Line 13: One of the problematic flows is vm distroy for which having duplicate Line 14: means that the operation failed and recovery flow is triggered. I don't really mind the vdsm-side patch, but what would happen on Engine it gets a single event with Down state right after having polled Down? Line 15: Line 16: Line 17: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has uploaded a new change for review.
Change subject: events: make sure to send only one event per status
..
events: make sure to send only one event per status
Initially we made an assumption that engine can handle more than one
event containing the same status. The assumtion is not true and there
are flows where engine logic assumes only one status change.
Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
Signed-off-by: pkliczewski piotr.kliczew...@gmail.com
---
M vdsm/virt/vm.py
1 file changed, 22 insertions(+), 23 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/79/42579/1
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 063bc3a..25a9b10 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -306,6 +306,7 @@
else:
self._lastStatus = vmstatus.WAIT_FOR_LAUNCH
self._evaluatedStatus = None
+self._eventLock = threading.Lock()
self._migrationSourceThread = migration.SourceThread(self)
self._kvmEnable = self.conf.get('kvmEnable', 'true')
self._incomingMigrationFinished = threading.Event()
@@ -380,32 +381,30 @@
self._lastStatus = value
def send_status_event(self):
-vm_status = self._getVmStatus()['status']
+with self._eventLock:
+vm_status = self._getVmStatus()['status']
-# this check is to reduce number of send events
-# It is not safe since evaluatedStatus can change but
-# in the worst case we will send the same event twice
-if vm_status != self._evaluatedStatus:
-self._evaluatedStatus = vm_status
-stats = {
-'status': vm_status,
-'hash': str(hash((self._domain.devices_hash,
- self.guestAgent.diskMappingHash))),
-}
+if vm_status != self._evaluatedStatus:
+self._evaluatedStatus = vm_status
+stats = {
+'status': vm_status,
+'hash': str(hash((self._domain.devices_hash,
+ self.guestAgent.diskMappingHash))),
+}
-# TODO: DOWN and exitCode must be set atomically. Once this is done
-# we can remove the multiple conditions from this code.
-if vm_status == vmstatus.DOWN:
-if 'exitCode' in self.conf:
-stats['exitCode'] = self.conf['exitCode']
-if 'exitMessage' in self.conf:
-stats['exitMessage'] = self.conf['exitMessage']
-if 'exitReason' in self.conf:
-stats['exitReason'] = self.conf['exitReason']
+# TODO: DOWN and exitCode must be set atomically. Once this is
+# done we can remove the multiple conditions from this code.
+if vm_status == vmstatus.DOWN:
+if 'exitCode' in self.conf:
+stats['exitCode'] = self.conf['exitCode']
+if 'exitMessage' in self.conf:
+stats['exitMessage'] = self.conf['exitMessage']
+if 'exitReason' in self.conf:
+stats['exitReason'] = self.conf['exitReason']
-self.log.debug('Last status %s and evaluated status %s',
- self.lastStatus, vm_status)
-self._notify('VM_status', stats)
+self.log.debug('Last status %s and evaluated status %s',
+ self.lastStatus, vm_status)
+self._notify('VM_status', stats)
def _notify(self, operation, params):
sub_id = '|virt|%s|%s' % (operation, self.id)
--
To view, visit https://gerrit.ovirt.org/42579
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
automat...@ovirt.org has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 1: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
automat...@ovirt.org has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 2: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has posted comments on this change.
Change subject: events: make sure to send only one event per status
..
Patch Set 1:
(2 comments)
https://gerrit.ovirt.org/#/c/42579/1//COMMIT_MSG
Commit Message:
Line 7: events: make sure to send only one event per status
Line 8:
Line 9: Initially we made an assumption that engine can handle more than one
Line 10: event containing the same status. The assumtion is not true and there
Line 11: are flows where engine logic assumes only one status change.
could you elaborate on the Engine use flows broken by this?
I was told that there are issues with Destroy vm when we receive more than
single event with 'Down' status.
Will update commit message.
Line 12:
Line 13: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
https://gerrit.ovirt.org/#/c/42579/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 381: self._lastStatus = value
Line 382:
Line 383: def send_status_event(self):
Line 384: with self._eventLock:
Line 385: vm_status = self._getVmStatus()['status']
usually I prefer holding locks for the shortest possible time. do we really
We don't. Will change.
Line 386:
Line 387: if vm_status != self._evaluatedStatus:
Line 388: self._evaluatedStatus = vm_status
Line 389: stats = {
--
To view, visit https://gerrit.ovirt.org/42579
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com
Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 1: Verified+1 Verified by running latest vdsm and the engine and checking that there was single message send for status changes during start of a vm. -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Piotr Kliczewski has posted comments on this change. Change subject: events: make sure to send only one event per status .. Patch Set 2: Verified+1 Verified by running latest engine and vdsm and stopping a vm to see that event was triggered. -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Francesco Romani from...@redhat.com Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: events: make sure to send only one event per status
Dan Kenigsberg has posted comments on this change.
Change subject: events: make sure to send only one event per status
..
Patch Set 1:
(2 comments)
https://gerrit.ovirt.org/#/c/42579/1//COMMIT_MSG
Commit Message:
Line 7: events: make sure to send only one event per status
Line 8:
Line 9: Initially we made an assumption that engine can handle more than one
Line 10: event containing the same status. The assumtion is not true and there
Line 11: are flows where engine logic assumes only one status change.
could you elaborate on the Engine use flows broken by this?
How are they not broken by repeated polls of an unchanging status?
Line 12:
Line 13: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
https://gerrit.ovirt.org/#/c/42579/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 381: self._lastStatus = value
Line 382:
Line 383: def send_status_event(self):
Line 384: with self._eventLock:
Line 385: vm_status = self._getVmStatus()['status']
usually I prefer holding locks for the shortest possible time. do we really
need to call _getVmStatus under the lock?
Line 386:
Line 387: if vm_status != self._evaluatedStatus:
Line 388: self._evaluatedStatus = vm_status
Line 389: stats = {
--
To view, visit https://gerrit.ovirt.org/42579
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Francesco Romani from...@redhat.com
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek mskri...@redhat.com
Gerrit-Reviewer: Piotr Kliczewski piotr.kliczew...@gmail.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
