Bug 17833

Summary: _initilize_memcached() warns if errors
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: Architecture, internals, and plumbingAssignee: Olli-Antti Kivilahti <olli-antti.kivilahti>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, m.de.rooy, nick
Version: master   
Hardware: All   
OS: All   
Whiteboard:
Change sponsored?: --- Patch complexity: Trivial patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Attachments: Bug 17833 - Memcached silently fails to _initilize_memcached() on the second time it is invoked
Bug 17833 - _initilize_memcached() warns if errors
Bug 17833 - _initilize_memcached() warns if errors
Bug 17833 - _initilize_memcached() warns if errors
Bug 17833: Make sure this warning will not be ignored

Description Olli-Antti Kivilahti 2017-01-02 11:13:37 UTC
When _initilize_memcached() is called the first time, it sets the 'ismemcached'-key and returns 1.

When _initilize_memcached() is called the second time, the 'ismemcached'-key already exists and '' is returned (this default to False in Perl).
Koha thinks that memcached is not in use and decides to use some other caching mechanism instead.

Cache::Memcached::Fast returns undef on server error:
https://metacpan.org/pod/Cache::Memcached::Fast#add

Koha MUST listen to undefined, instead of False to define whether or not memcached is properly working.


Also added simple helpful diagnostics to help identify and log when this memcached server connection problem arises.


Here is a small test run to verify this behaviour:
root@kohadev:~# echo "fresh memcached start"
fresh memcached start
root@kohadev:~# perl l.pl
127.0.0.1:4545 1.4.14 (Ubuntu)
def  : '1'
getkey: 'text'
root@kohadev:~# echo "val already cached"
val already cached
root@kohadev:~# perl l.pl
127.0.0.1:4545 1.4.14 (Ubuntu)
def  : ''
getkey: 'text'
root@kohadev:~# echo "memcached killed and not responding"
memcached killed and not responding
root@kohadev:~# perl l.pl
undef
getkey: ''
root@kohadev:~# echo "The return value is undefined"
The return value is undefined
root@kohadev:~#



Here is the code snippet used to test the Memcached-server in the previous example output:

kivilahtio@koha_ci_1 /h/k/Koha> cat t.pl
use Cache::Memcached::Fast;
use Modern::Perl;

my $memd = new Cache::Memcached::Fast({
    servers => [ '/lxcBindMount/tmp/memcached.sock' ],
    namespace => 'koha_ci_1:',
    connect_timeout => 0.2,
    io_timeout => 0.5,
    close_on_error => 1,
    max_failures => 3,
    failure_timeout => 2,
    ketama_points => 150,
    nowait => 1,
    hash_namespace => 1,
});


# Get server versions.
my $versions = $memd->server_versions;
while (my ($server, $version) = each %$versions) {
    print "$server $version\n";
}

# Store scalars.
my $rv = $memd->add('skey', 'text');
print "undef\n" if not defined $rv;
print "def  : '$rv'\n" if defined $rv;
print "getkey: '".$memd->get('skey')."'\n";
Comment 1 Olli-Antti Kivilahti 2017-01-02 11:28:12 UTC
Created attachment 58554 [details] [review]
Bug 17833 - Memcached silently fails to _initilize_memcached() on the second time it is invoked

When _initilize_memcached() is called the first time, it sets the
'ismemcached'-key and returns 1.

When _initilize_memcached() is called the second time, the 'ismemcached'-key
already exists and '' is returned (this default to False in Perl).
Koha thinks that memcached is not in use and decides to use some other caching
mechanism instead.

Cache::Memcached::Fast returns undef on server error:
https://metacpan.org/pod/Cache::Memcached::Fast#add

Koha MUST listen to undefined, instead of False to define whether or not memcached
is properly working.

Also added simple helpful diagnostics to help identify and log when this memcached
server connection problem arises.
Comment 2 Jonathan Druart 2017-01-02 12:01:44 UTC
You are mixing add and set, replace ->add with ->set in your snippet and it will work as expected.
Set will return 1 even if the key already exists.

It seems that this is invalid, waiting for Olli's confirmation.
Comment 3 Olli-Antti Kivilahti 2017-01-02 12:21:58 UTC
Thanks for digging into this Jonathan!

You are correct. The culprit was misconfigured socket permission. Same behaviour is expected on any connection issue to the memcached server.

I changed the bug title and content to only add warnings when a server/connection problem occurs.
Comment 4 Olli-Antti Kivilahti 2017-01-02 12:22:38 UTC
Created attachment 58556 [details] [review]
Bug 17833 - _initilize_memcached() warns if errors

If memcached or the connection to it is misconfigured, show simple warnings to help
identify the problem.
Comment 5 Katrin Fischer 2017-10-15 10:52:34 UTC
Created attachment 68162 [details] [review]
Bug 17833 - _initilize_memcached() warns if errors

If memcached or the connection to it is misconfigured, show simple warnings to help
identify the problem.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2017-10-15 10:52:58 UTC
Small change, didn't appear to break something in my testing.
Comment 7 Marcel de Rooy 2018-01-05 07:55:32 UTC
Created attachment 70281 [details] [review]
Bug 17833 - _initilize_memcached() warns if errors

If memcached or the connection to it is misconfigured, show simple warnings to help
identify the problem.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Jonathan Druart 2018-01-05 17:03:10 UTC
Created attachment 70307 [details] [review]
Bug 17833: Make sure this warning will not be ignored
Comment 9 Jonathan Druart 2018-01-09 20:27:47 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 10 Nick Clemens 2018-01-16 12:24:37 UTC
Enhancement, skipping for 17.11.x.
Awesome work everybody!