Alon Bar-Lev has posted comments on this change. Change subject: tools: Support snmp trap as a notification method. ......................................................................
Patch Set 22: (10 comments) http://gerrit.ovirt.org/#/c/22909/22/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 146: NotificationProperties.INTERVAL_IN_SECONDS, Line 147: NotificationProperties.IS_HTTPS_PROTOCOL, Line 148: NotificationProperties.REPEAT_NON_RESPONSIVE_NOTIFICATION); Line 149: // validate mandatory and non empty properties Line 150: requireOne(NotificationProperties.MAIL_SERVER, NotificationProperties.SNMP_MANAGERS); well, will it work if I only have SNMP_MANAGER_alon ? Line 151: Line 152: // validate non negative args Line 153: for (String property : new String[] { Line 154: NotificationProperties.DAYS_TO_KEEP_HISTORY, Line 198: "'%s' must be set when SSL or TLS is enabled or when password is set", Line 199: NotificationProperties.MAIL_USER)); Line 200: } Line 201: Line 202: if (!isSmtpEncryptionOptionValid()) { this does not belong to this patch Line 203: throw new IllegalArgumentException( Line 204: String.format( Line 205: GENERIC_MESSAGE + "'%s' value has to be one of: '%s', '%s', '%s'.", Line 206: NotificationProperties.MAIL_SMTP_ENCRYPTION, Line 223: public boolean isConfigured(String property) { Line 224: return !StringUtils.isEmpty(getProperty(property)); Line 225: } Line 226: Line 227: // Availability this does not belong to this patch Line 228: private void validateEmailAvailability() { Line 229: // try to resolve MAIL_SERVER host Line 230: validateHost(NotificationProperties.MAIL_SERVER, getProperty(NotificationProperties.MAIL_SERVER)); Line 231: } Line 231: } Line 232: Line 233: private void validateSnmpBasic() { Line 234: // validate required SNMP parameters Line 235: requireAll(SNMP_OID, SNMP_COMMUNITY); not sure this will work if I only have SNMP_OID_alon etc... Line 236: try { Line 237: new OID(getProperty(SNMP_OID)); Line 238: } catch (Exception ex) { Line 239: throw new IllegalArgumentException(GENERIC_MESSAGE + "illegal value for '" + SNMP_OID + "'"); Line 241: } Line 242: Line 243: private void validateSnmpAvailability() { Line 244: for (String snmpManager : getProperty(SNMP_MANAGERS).split("\\s+")) { Line 245: validateHost(SNMP_MANAGERS, snmpManager.replaceAll(":.*", "")); not sure this is wise, as if dns is not available service will not start, while it will be able to send events once dns resumes. same for smtp. what you are doing here, is not allowing anything to be sent ever because of local issue when server was rebooted. Line 246: } Line 247: } Line 248: Line 249: private void validateFilter() { Line 324: Line 325: /** Line 326: * Returns {@code true} if mail transport encryption type is correctly specified, otherwise {@code false} Line 327: */ Line 328: public boolean isSmtpEncryptionOptionValid() { not belong to this patch. Line 329: return MAIL_SMTP_ENCRYPTION_NONE.equals(getProperty(MAIL_SMTP_ENCRYPTION, true)) Line 330: || MAIL_SMTP_ENCRYPTION_SSL.equals(getProperty(MAIL_SMTP_ENCRYPTION, true)) Line 331: || MAIL_SMTP_ENCRYPTION_TLS.equals(getProperty(MAIL_SMTP_ENCRYPTION, true)); Line 332: } http://gerrit.ovirt.org/#/c/22909/22/backend/manager/tools/src/main/modules/org/ovirt/engine/core/tools/main/module.xml File backend/manager/tools/src/main/modules/org/ovirt/engine/core/tools/main/module.xml: Line 18: <module name="org.ovirt.engine.core.compat"/> Line 19: <module name="org.ovirt.engine.core.utils"/> Line 20: <module name="org.postgresql"/> Line 21: <module name="sun.jdk"/> Line 22: <module name="org.snmp4j"/> sort() ? Line 23: </dependencies> Line 24: http://gerrit.ovirt.org/#/c/22909/22/packaging/dbscripts/event_sp.sql File packaging/dbscripts/event_sp.sql: Line 19: RETURNS VOID Line 20: AS $procedure$ Line 21: BEGIN Line 22: INSERT INTO event_notification_hist(audit_log_id, event_name, method_type, reason, sent_at, status) Line 23: VALUES(v_audit_log_id, v_event_name, v_method_type, v_reason, v_sent_at, v_status); I am unsure this belong to this patch, please separate all db related to different patch. Line 24: END; $procedure$ Line 25: LANGUAGE plpgsql; Line 26: Line 27: http://gerrit.ovirt.org/#/c/22909/22/packaging/dbscripts/upgrade/03_04_0620_event_notification_methods.sql File packaging/dbscripts/upgrade/03_04_0620_event_notification_methods.sql: Line 26: INNER JOIN users u ON es2.subscriber_id = u.user_id Line 27: WHERE (es.method_address is NULL OR trim(both from es.method_address) = ''); Line 28: Line 29: -- change events history table Line 30: ALTER TABLE event_notification_hist DROP COLUMN subscriber_id; this is different patch http://gerrit.ovirt.org/#/c/22909/22/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in File packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in: Line 120 Line 121 Line 122 Line 123 Line 124 why removed? should be in different patch if required. -- 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: 22 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: Eli Mesika <[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
