Adam Litke has posted comments on this change. Change subject: Separate constants from rules to collector, read it from XML ......................................................................
Patch Set 1: Code-Review-1 (2 comments) Please see my alternate suggestion in the comments... http://gerrit.ovirt.org/#/c/34688/1/mom/Collectors/Collector.py File mom/Collectors/Collector.py: Line 75: Values are used only if hypervisor isn't able to provide its own values. Line 76: """ Line 77: Line 78: # do not anything if collector doesn't provide any constants Line 79: if not self.const_fiels: const_fields Line 80: return {} Line 81: Line 82: logger = logging.getLogger('mom.Collector._collect_const_fields') Line 83: ret_fields = {} http://gerrit.ovirt.org/#/c/34688/1/mom/Collectors/GuestMemory.py File mom/Collectors/GuestMemory.py: Line 29: hypervisor isn't able to provide its own values. Line 30: """ Line 31: return {'min_guest_free_percent': 0.2, Line 32: 'max_balloon_change_percent': 0.05, Line 33: 'min_balloon_change_percent': 0.0025} Hmm... I'm not convinced that trying these to the Collector is a good idea. There is supposed to be a very clean separation between the Collector/Controller part of MOM and the policy. This is tightly coupling our particular favored balloon policy with the Collector implementation itself. While I understand the desire to have per-VM constants, I think it should be done in a different way. My suggestion is to introduce a new Collector (GuestConstants). This collector would use a new call to the active HypervisorInterface (either vdsm or libvirt) to get the set of constants for a guest. All fields of this Collector would be considered optional so you'll want to sync with Roy Golan to make sure you're implementing it the same way he plans to do the GuestMemory Collector. This includes the usage of Entity.Stat() default values Finally, in your policy you'll retrieve to constant as shown below: # Here is the policy default value (defvar min_guest_free_percent 0.20) (def shrink_guest (guest) { # ... (defvar min_free guest.Stat('const_min_guest_free_percent', min_guest_free_percent)) # Now use min_free, which will either have a value from the guest-specific Collector constants or the policy default (with Guests guest (shrink_guest guest)) I'd be happy to set up a call with you, Martin Sivak, Roy Golan, and I to discuss this further if you are looking for more context. Line 34: Line 35: def getFields(self): Line 36: fields = self.hypervisor_iface.getStatsFields() Line 37: fields.update(self.getConstants().keys()) -- To view, visit http://gerrit.ovirt.org/34688 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c16d29f6e1d541808d9c1f4051c3de0cc746098 Gerrit-PatchSet: 1 Gerrit-Project: mom Gerrit-Branch: master Gerrit-Owner: Martin Pavlásek <mpavl...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Martin Pavlásek <mpavl...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
