mooli tayer has posted comments on this change. Change subject: tools: Support snmp trap as a notification method. ......................................................................
Patch Set 18: (12 comments) http://gerrit.ovirt.org/#/c/22909/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/UpDownEventFilter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/UpDownEventFilter.java: Line 18: // @Override Line 19: // public boolean isSubscribed(AuditLogEvent event) { Line 20: // return event.getLogTypeName().equals(eventUpName) || event.getLogTypeName().equals(eventDownName); Line 21: // } Line 22: //} > no need to commit such code. Removed http://gerrit.ovirt.org/#/c/22909/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/WhitelistFilter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/WhitelistFilter.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import java.util.Set; Line 4: Line 5: public class WhitelistFilter implements EventFilter { > no need to abstract this, why filter cannot return true/false based on data You suggested to represent data exactly like the DB: one value with include exclude. The problem is it does not make sense; a blacklist of !(a,b,c) cannot be translated to 3 discrete blacklists. !a !b !c (I am sure u see why). What I did was one of your original proposals: translating blacklist to white. Line 6: Line 7: private Set<String> values; Line 8: Line 9: public void setValues(Set<String> values) { http://gerrit.ovirt.org/#/c/22909/18/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/LocalConfig.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/LocalConfig.java: Line 280: * allowMissing is false. Line 281: */ Line 282: public String getProperty(String key, String optionalSuffix, boolean allowMissing) { Line 283: String property = getProperty(key + "_" + optionalSuffix, true); Line 284: if(property == null) { > use StringUtils and isBlank to check for both null or "" Done Line 285: property = getProperty(key , allowMissing); Line 286: } Line 287: return property; Line 288: } http://gerrit.ovirt.org/#/c/22909/18/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java: Line 126: if (subscriber.isSubscribed(event)) { Line 127: log.debug(sb.append("address:") Line 128: .append(subscriber.getMethodAddress()) Line 129: .append(" subscribed to event: ") Line 130: .append(event.getLogTypeName()).toString() > String.format is much better. Done Line 131: ); Line 132: EventSender sender = notificationMethodsMapper.getEventSender(subscriber.getEventNotificationMethod()); Line 133: EventSenderResult sendResult; Line 134: try { http://gerrit.ovirt.org/#/c/22909/18/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 36: Line 37: try { Line 38: // Create a new session and define it's transport. Line 39: snmp = new Snmp(new DefaultUdpTransportMapping()); Line 40: snmp.listen(); > why do you listen? There's no need for it, will remove. Line 41: } catch (IOException e) { Line 42: throw new NotificationServiceException("error creating " + getClass().getName()); Line 43: } Line 44: } http://gerrit.ovirt.org/#/c/22909/18/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/subscribers/ConfigurationSubscribersProvider.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/subscribers/ConfigurationSubscribersProvider.java: Line 74: String method = matcher.group("METHOD"); Line 75: EventNotificationMethod eventNotificationMethod = null; Line 76: if (method != null) { Line 77: switch (method) { Line 78: case "MAIL": > can we have this smtp and snmp lower case and consistent based on protocol? Are you talking about their names? The problem is There was already in the configuration MAIL_SERVER MAIL_PORT MAIL_USER etc. I do not want to change the enum either to contain mail so I reluctantly a different name in the configuration. Regarding casing It will also be more consistent with what we already have uppercased. Line 79: subscribers.add(createSubscriber(EventNotificationMethod.EMAIL, profile, names, include));//eventNotificationMethod = EventNotificationMethod.EMAIL; Line 80: break; Line 81: case "SNMP": Line 82: subscribers.add(createSubscriber(EventNotificationMethod.SNMP_TRAP, profile, names, include));//eventNotificationMethod = EventNotificationMethod.SNMP_TRAP; Line 114: Line 115: private String resolveAddress(EventNotificationMethod eventNotificationMethod, String profile) { Line 116: switch (eventNotificationMethod) { Line 117: case EMAIL: Line 118: return properties.getProperty(NotificationProperties.MAIL_ADDRESS, profile, false); > I think this is too match farther.... no need for profile to email. it shou I don't understand your comment in the context of the code you wrote. Do you want: case EMAIL: if (isblank(profile)) { return getProperty(NotificationProperties.MAIL_ADDRESS); } return properties.getProperty(NotificationProperties.MAIL_ADDRESS, profile, false); Line 119: case SNMP_TRAP: Line 120: return profile; Line 121: } Line 122: return null; http://gerrit.ovirt.org/#/c/22909/18/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 147: */ Line 148: public void validate() { Line 149: validateCommon(); Line 150: Line 151: validateEmailBasic(); > This should be called only if MAIL_SERVER is entered I went for validating basic properties always and if a method is defined I also check it's availability. Not sure what is better Line 152: if (isConfigured(NotificationProperties.MAIL_SERVER)) { Line 153: validateEmailAvailability(); Line 154: } Line 155: validateSNMPBasic(); Line 191: } Line 192: Line 193: private void validateEmailBasic() { Line 194: // validate MAIL_PORT Line 195: requireAll(MAIL_PORT); > Please add here mail port validation, you dropped this by mistake on rebase Done. I was sure I somehow added it by mistake prematurely to your patch. Line 196: Line 197: // validate MAIL_USER value Line 198: String emailUser = getProperty(NotificationProperties.MAIL_USER, true); Line 199: if (StringUtils.isEmpty(emailUser) Line 324: /** Line 325: * Returns {@code true} if mail transport encryption type is correctly specified, Line 326: * otherwise {@code false} Line 327: */ Line 328: public boolean isSMTPEncryptionOptionValid() { > Please undo this renaming I don't mind how it is but i'm trying to be consistent: I also have validateSNMPAvailability and validateSNMPBasic here. Do we have a guideline for these cases? 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/18/packaging/dbscripts/upgrade/03_04_0510_event_notification_methods.sql File packaging/dbscripts/upgrade/03_04_0510_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; > well, after discussion with barak, I truly think we should not touch the da You should have discussed with me as well. It's not about string or int. The original code kept a foreign key to a table the duplicated the EventNotificationMethod enum. There were also 4 classes dealing with converting the string to int. I see no sense in that. http://gerrit.ovirt.org/#/c/22909/18/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in File packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in: Line 149: # Line 150: # 3.) add a new snmp subscriber to all events using default definitions, overriding only the oid. Line 151: # SNMP_OID_DAVE=1.2.3.4 Line 152: # FILTER="${FILTER} -(SNMP:DAVE)" Line 153: FILTER= > this is not what we actually discussed, you tried to optimize the syntax bu Correct me if I'm wrong but I think what we agreed upon makes no sense: please see my comment on WhitelistFilter. I can remove the default filter - I kept It to try and also satisfy PM original req I'M not sure if it makes sense? I prefer not using parenthesis here especially for cases with out transport & profile Line 154: Line 155: #----------------------------------# Line 156: # Engine Monitoring Configuration: # Line 157: #----------------------------------# -- 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: 18 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
