Doron Fediuck has posted comments on this change. Change subject: tools: support snmp trap as a notification method. ......................................................................
Patch Set 14: (4 comments) Mooli, I agree with Alom on the multiple trap destinations, and indeed putting it in a different patch may help you to ease the implementation. See inline for some comments. Also, we should start adding unit tests here. So if something breaks (DB change, etc) you'll be able to know. Last thing- the notifications are design to be selected from the UI. I suggest to close this gap for snmp as well. So admins should be able to choose which traps go to which destination using the UI. http://gerrit.ovirt.org/#/c/22909/14/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/sender/snmp/SNMPEventSender.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/sender/snmp/SNMPEventSender.java: Line 80: target.setAddress(new UdpAddress(split[0] + "/" + (split.length > 1 ? split[1] : 162))); Line 81: target.setCommunity(new OctetString(prop.getProperty(NotificationProperties.SNMP_COMMUNITY))); Line 82: target.setVersion(SnmpConstants.version2c); Line 83: target.setRetries(2); Line 84: target.setTimeout(5000); The above 2 settings should be configurable, so the admin will be able to override it. Line 85: snmp.send(v2pdu, target); Line 86: final EventSenderResult eventSenderResult = new EventSenderResult(); Line 87: eventSenderResult.setSent(true); Line 88: return eventSenderResult; http://gerrit.ovirt.org/#/c/22909/14/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java: Line 135: * Validates properties values. Line 136: * Line 137: * @throws IllegalArgumentException if some properties has invalid values Line 138: */ Line 139: public void validate() { This method became a monster. Since you already broke parts of it out, I suggest you keep doing it so you'll end up with a 5 lines method calling to- - validateCommon(); - validateMail(); - validateSNMP(); Line 140: Line 141: // validate mandatory and non empty properties Line 142: for (String property : new String[]{ Line 143: NotificationProperties.DAYS_TO_KEEP_HISTORY, Line 193: )); Line 194: } Line 195: Line 196: // try to resolve MAIL_SERVER host Line 197: validateHost(NotificationProperties.MAIL_SERVER, getProperty(NotificationProperties.MAIL_SERVER)); What happens if snmp only is configured? Looks like this will fail. Line 198: Line 199: // validate email addresses Line 200: for (String property : new String[]{ Line 201: NotificationProperties.MAIL_USER, http://gerrit.ovirt.org/#/c/22909/14/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in File packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in: Line 119: SNMP_OID=1.3.6.1.4.1.2312.13.1.1 Line 120: # 1[iso].3[organization].6[DoD].1[Internet].4[private].1[enterprises].2312[redhat].13[ovirt-engine].1[notifications]. Line 121: # 1[audit-log] Line 122: Line 123: # comma separated list of event names to notify on. You should specify here or elsewhere the list of available events, so the user can copy-paste it as needed. Line 124: #WHITELIST="" Line 125: Line 126: # notify on all but these comma separated list of events: Line 127: BLACKLIST="" -- To view, visit http://gerrit.ovirt.org/22909 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0cd22d022ae535f45e046b09a2cbfadd837b465c Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: mooli tayer <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: mooli tayer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
