Yaniv Bronhaim has posted comments on this change. Change subject: Separate MOM to an external process ......................................................................
Patch Set 8: (8 comments) https://gerrit.ovirt.org/#/c/41602/8//COMMIT_MSG Commit Message: Line 18: number 54322. Line 19: Line 20: mom-vdsm will be restarted when vdsmd restarts and started when Line 21: vdsmd starts (but errors in mom-vdsm do not propagate to vdsmd). Line 22: better to have https://gerrit.ovirt.org/#/c/40726/ in first but not so important Line 23: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1227714 Line 24: Change-Id: I99b81d21080834b7031824dab0a26f45e7eac7af https://gerrit.ovirt.org/#/c/41602/8/init/systemd/mom-vdsm.service.in File init/systemd/mom-vdsm.service.in: Line 6: Line 7: [Service] Line 8: Type=simple Line 9: LimitCORE=infinity Line 10: ExecStart=/usr/sbin/momd -c /etc/vdsm/mom.conf consider to use the daemonAdapter that being used for vdsmd and supervdsmd to manipulate std outputs and nice level although, Im quite sure that all the parameters we set with this adapter can be configured by attributes here in the service file.. but didn't check that anyhow, the stdout and stderr need to be directed explicitly somewhere Line 11: Restart=on-failure Line 12: RestartSec=10 Line 13: User=@VDSMUSER@ Line 14: Group=@VDSMGROUP@ Line 10: ExecStart=/usr/sbin/momd -c /etc/vdsm/mom.conf Line 11: Restart=on-failure Line 12: RestartSec=10 Line 13: User=@VDSMUSER@ Line 14: Group=@VDSMGROUP@ why does it run as vdsm user? it contains all its sudoer permissions.. better to have separate user for mom operations Line 15: TimeoutStopSec=@SERVICE_STOP_TIMEOUT@ Line 16: Line 17: [Install] https://gerrit.ovirt.org/#/c/41602/8/init/systemd/vdsmd.service.in File init/systemd/vdsmd.service.in: Line 4: iscsid.service rpcbind.service supervdsmd.service sanlock.service \ Line 5: vdsm-network.service Line 6: After=multipathd.service libvirtd.service iscsid.service rpcbind.service \ Line 7: supervdsmd.service sanlock.service vdsm-network.service Line 8: Conflicts=libvirt-guests.service ksmtuned.service now we can remove the ksmtuned conflict from here? Line 9: Wants=mom-vdsm.service Line 10: Line 11: [Service] Line 12: Type=simple Line 5: vdsm-network.service Line 6: After=multipathd.service libvirtd.service iscsid.service rpcbind.service \ Line 7: supervdsmd.service sanlock.service vdsm-network.service Line 8: Conflicts=libvirt-guests.service ksmtuned.service Line 9: Wants=mom-vdsm.service why not in requires? Line 10: Line 11: [Service] Line 12: Type=simple Line 13: LimitCORE=infinity https://gerrit.ovirt.org/#/c/41602/8/vdsm/momIF.py File vdsm/momIF.py: Line 35: class MomNotAvailableError(RuntimeError): Line 36: pass Line 37: Line 38: Line 39: class MomThread(object): MomServer could be better name for it.. Line 40: Line 41: def __init__(self, momconf): Line 42: if not _momAvailable: Line 43: raise MomNotAvailableError() Line 51: if port == "-1": Line 52: self.log.error("MOM's RPC interface is disabled") Line 53: raise MomNotAvailableError() Line 54: Line 55: self._mom = xmlrpclib.ServerProxy("http://localhost:"+port) souldn't you have some kind of self._mom.stop somewhere? Line 56: self._policy = {} Line 57: Line 58: def getKsmStats(self): Line 59: """ Line 70: ret['memShared'] = stats['ksm_pages_sharing'] * PAGE_SIZE_BYTES Line 71: ret['memShared'] /= Mbytes Line 72: ret['ksmCpu'] = stats['ksmd_cpu_usage'] Line 73: except (AttributeError, socket.error): Line 74: self.log.warning("MOM not available, KSM stats will be missing.") better to print traceback to know what attribute is missing Line 75: Line 76: return ret Line 77: Line 78: def setPolicy(self, policyStr): -- To view, visit https://gerrit.ovirt.org/41602 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I99b81d21080834b7031824dab0a26f45e7eac7af Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Sivák <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
