Problem/Motivation
We have multiple issues with possible malconfiguration or unmet requirements.
First, we don't want to check everything and have false positive error in system requirements.
All plugins have config based instances.
Problems could be:
- IMAP contains wrong credentials.
- Many unprocessed items in IMAP or any other queue based fetcher.
- Unmet php library dependency, example pgp signing / check library.
Some of the issues can be covered by Monitoring project. Others need new coverage, typically the hook_requirements.
Proposed resolution
Let inmail implement hook_requirements.
Let inmail cycle through all instances of plugins (deliverer, analyser, handler) and ask each handler if it is healthy.
Could be checkRequirements().
For this, we will introduce a new method on the interface.
Checking for a library dependency is easy.
Make sure to not check if an item is disabled. thus if no pgp analyser is enabled, no error if the library is missing.
Manually disabling is a different thing to being unable based on runtime checks.
We can add an argument to checkRequirements, identifying "runtime" for runtime checks and "requirements" for the global hook.
The requirements check can be verbose, even trying to connect to each service configured to warn about wrong credentials setup.
The runtime check will only quickly check the situation to avoid fatal errors. It need to be fast and should be performed prior to interacting with the plugin instance. Not sure where exactle the API should be called, but for sure in check / fetch of IMAP.
May be, we even want to create two different methods?
- For hook_requirements, we need the requirements return value.
- For the runtime check, we might want to throw an exception?
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#19 | provide_a-2769617-19-interdiff.txt | 7.84 KB | mbovan |
#19 | provide_a-2769617-19.patch | 17.99 KB | mbovan |
| |||
#14 | provide_a-2769617-14-interdiff.txt | 9.33 KB | mbovan |
#14 | provide_a-2769617-14.patch | 19.11 KB | mbovan |
| |||
#12 | provide_a-2769617-12-interdiff.txt | 9.66 KB | mbovan |
Comments
Comment #2
miro_dietikerSince we will use Monitoring sensors to make sure there are not too many unprocessed mails. (However we need the sensors. ;-) )
Also, we want to avoid double monitoring and having two errors on our dashboard (requirements + specific sensor)...
I just realised that the count of unprocessed mails is not the best measure.
Instead the best to know would be the age of the oldest unprocessed mail. The sensor message value would be the age (simlar to cron) and the message could still contain the total count and the remaining count. BTW it's one sensor per deliverer instance. Or a sensor per plugin listing the aggregate of its instances.
Explanation: A threshold based on count doesn't fit sites as a high traffic site would soon go critical and a threshold of 0 or 1 for a small site would also result in many wrong warnings / escalations... and higher numbers add no value because you never know if the single mail you just sent is processed or not.
Comment #3
Primsi CreditAttribution: Primsi at MD Systems GmbH commentedOne additional remark: our hook_requirements implementation will have to run in the "runtime" phase, because the other two options would mean that we are checking requirements only on inmail install or update, which is not very useful.
One possible way would be to invoke the method once when submitting the plugin form in Inmail for every plugin and then instead of the argument on the method have a new entry on the plugin annotation, "validate_runtime" or something similar, so that inmail knows which plugins to check runtime and which only on enable. The idea of throwing exceptions on runtime is still applicable, we just catch them in inmail and handle them accordingly for runtime checks.
Comment #4
mbovan CreditAttribution: mbovan as a volunteer and for Google Summer of Code commentedI'm uploading the initial patch as an idea. There are some @todo's added.
IIRC, "runtime" phase of
hook_requirements()
executes onadmin/config
andadmin/reports/status
only.Comment #5
miro_dietikerDon't concatenate here. Define the name in full in the static list.
If this collects things, it should return some more structure than just a string. Requirements are keyed by a name. Possibly even more structure including verbosity... Possibly we even want to pass in the structure by reference?
At least the return bool|string is a bad idea. We need an interface that tells us more about severity, count, allow us to iterate through individual info and so on.
Also i think this check should receive information about if it's a simple lightweight check of in full.
Comment #6
miro_dietiker@Primsi The proposal to extend checkRequirements would be to allow
Oh also, now we are triggering checkRequirements() per instance.
If the plugin has a requirement to a library such as PGP and it has two instances, the error would appear twice.
I think we should create two methods, one is static per plugin to checkRequirements and one is per instance to check configuration.
Aha... now i see why you avoided the inmail_ prefix above. Requirement keys are nicer...
Yeah really ugly. Better conditionally add it, the problem is the variable types of the return statement.
But with my proposal to define a uniform return type in the interface, you can always merge.
Trying to be more specific: hook_requirements expects an associative array:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
So the key could be calculated by the caller...
IMHO checkRequirements should always return an array with the keys as defined in hook_req like title, value, severity, description. If nothing to report, then return empty array.
Comment #7
mbovan CreditAttribution: mbovan as a volunteer and for Google Summer of Code commentedProviding a patch with
checkRequirements()
updates and tests (not sure about the naming there).Re #5.2, #6:
Oh.. I was returning the array structure, but forgot to update the documentation... Updated the documentation.
Re #6:
We could aslo modify
inmail_get_plugin_requirements
and filter out duplicates?Comment #8
miro_dietikerThus, you are attaching empty arrays to the hook_requirements for plugins that have no (enabled) instance.
This reads so much crazy...
This should somehow be more explicit, such as setting a property in each subclass constructor.
Hmm... you should add a separate API for this.
The hook_requirements (check admin reports requirements) can easily also contain a status message that is no error. And that is totally OK... could be queue size or last sync time or whatever...
So we need some extra TRUE|FALSE API that only stops if misconfigured.
However, this now leads to silent skipping.
I guess we should skipping to the analyzer / handler at least on the processing result.
Comment #9
mbovan CreditAttribution: mbovan as a volunteer and for Google Summer of Code commentedI'm uploading an updated patch.
Here is the short summary:
- I added a new method on
PluginRequirementsInterface
-isAvailable()
to get a boolean value of plugin's availability- Implemented
getPluginInstance()
on a config entity- Added plugin type property (deliverer, handler, analyzer)
- Fixed hook_requirements to list requirements by plugin (instantiated plugins only) so we can avoid possible problem of multiple same-message problem.
- Also, I added a new
PluginBase
which implementsPluginRequirementsInterface
(didn't find better name) and it's used as a base class for Inmail plugin types. Not sure about this DRY change though...Comment #10
miro_dietikerHmm...
Now we can no more check if entered passwords are OK of a plugin instance!
Comment #11
miro_dietikerOK Again, to sum up, we have two categories of things to check in requirements:
1) If plugin dependencies are met, such as
- php-imap module installed, for IMAP fetching (deliverer)
- php-pgp module installed, for crypto signing (anaylzer)
2) If settings are right, such as
- IMAP credentials are right
This issue should IMHO cover both cases, with dealing properly
- No repeating messages, per plugin check
- Allow per-instance checks.
Comment #12
mbovan CreditAttribution: mbovan as a volunteer and for Google Summer of Code commentedMade the changes with 2 new methods:
checkPluginRequirements()
andcheckInstanceRequirements()
. Also, updated deliverers code to checkisAvailable()
before using it.Comment #13
miro_dietikerThis call should be a static plugin call.
But i would simply first loop over instances and collect the plugin types with instances and then do the static call once.
That's a bit ugly.. :-)
How about a cleaner merge before?
Plz avoid name clashes and name it something like InmailPluginBase.
Comment #14
mbovan CreditAttribution: mbovan as a volunteer and for Google Summer of Code commentedTried to make it cleaner but not sure if it actually is. :)
Re #13.1:
Made
checkPluginRequirements()
a static method, but had to removePluginRequirementsInterface
fromPluginConfig
because of non-object context... I could update it with optional parameters but it would be too complex on the plugin side IMHO. So for now, we haveisAvailable()
andcheckInstanceRequirements()
methods available on PluginConfig entity too. They serve mostly as shortcuts...I'm doing it now in one loop (together with instances). I guess we want to check only those plugins that are already in use.
Re #13.2:
Because of the 1 loop above, I left this as is... I could check
!empty()
instead of filter, not sure..Re #13.3:
Fixed.
Comment #15
miro_dietikerHmmmm... ;-)
Comment #16
BerdirNot 100% sure about doing it like this (vs. if checks above) or not but I do see a benefit: Only with this approach the logic to avoid repeated calls above makes sense, otherwise you'd have to collect them and do another loop. So fine I guess.
wouldn't this be much easier if you just set $protected pluginType in each class? much fewer code to copy & paste.
Comment #17
miro_dietikerthe $pluginInstance is unused / never initialised.
Comment #18
BerdirDiscussed a bit with miro.
I think the one-off checkInstanceRequirements() method on the entity isn't very useful. you need the plugin anyway in your requirements helper function, just get it, then call it directly and then also call the static method on it.
Additionally, the if ($plugin_instance) check is kinda pointless right now. createInstance() doesn't return NULL, it returns an exception, so there is no scenario where that would ever be NULL/FALSE. Instead, wrap it in a try/catch in your function and report the absense of a plugin also as an error.
Comment #19
mbovan CreditAttribution: mbovan as a volunteer and for Google Summer of Code commentedAddressed: #16.2, #17 and #18.
I left it on the entity mostly because the plugin is not aware of the attached entity. Thus, to solve the problem of repeating messages (#11.2.1) and make a difference between messages, I had to add to add something specific to the entity (ID and label). Also, I can add this in the helper method too, so moved it there...
Comment #20
BerdirLooks fine to me now.
Comment #21
miro_dietikerOK, committing, unlocking the other related / postponed ones. Thx!