Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvs: return errors to callers on asynchronous load/store failures #1836

Merged
merged 14 commits into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@chu11
Copy link
Contributor

chu11 commented Nov 13, 2018

Previously, all asynchronous loads/stores in the kvs that failed would not return an error to the original kvs rpc request. The rpc request would just hang. Example errors are when a blobref is invalid or if the store size is larger than the maximum allowed.

The fix for this was semi involved.

  • support the ability for an error callback to be registered on waiters. On an error condition, that callback can be called on a waiter. Ultimately, these are used so that set_aux_errnum() functions can be called, leading to replays that can be informed that an error occurred during a load/store.

  • functions in the cache were added so that the error callbacks above can be called on specific cache entries and their waitqueues.

  • stores were a little bit more complex to handle than loads, b/c flux_content_store_get() returns the cache blobref on success, but no blobref on an error. In the event of the error, we need to the blobref to get the cache entry that is associated with the error. So changes were made in the cache so that a cache entry knows the blobref it was stored under, and subsequently that blobref can be retrieved under error scenarios.

  • things sort of fell into place at that point and tests were added.

  • included are a few cleanup patches too.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2018

hit another hang, restarting

  python/t0009-security.py:  PASS: N=2   PASS=2   FAIL=0 SKIP=0 XPASS=0 XFAIL=0
make[3]: Leaving directory '/usr/src/t'
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 14, 2018

I still haven't walked through this in detail, but one possibly naive question is: since futures are a container that can hold either an error or a result, and they can signify "not ready", should the cache just contain the future used for the content load or content store RPC?

Then when a KVS request is replayed, if it hits a cache entry that has been fulfilled with an error, it can get the errnum from the future and fail the request?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2018

That could be an option. The main reason I did it this way is that lookup_set/get_aux_errnum() and kvstxn_set/get_aux_errnum() were already in place and already used for passing some errors to replays. So it seemed wise to keep that infrastructure in place.

I did consider the option of refactoring the kvs cache to have the ability to attach an error to a cache entry (not so different than your attaching a future idea), so that anything that acted on that cache entry could see the error. But removing / cleaning up the cache would add some trickiness.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 14, 2018

OK, that sounds reasonable.

On cache_entry_get_blobref(), we end up with two copies of the key because zhash_t internally duplicates it as well. If we switched to zhashx_t, and set the key duplicator and key destructor to NULL, cache_insert() can just pass the duplicated key to zhashx_insert() and avoid the extra copy. I don't think converting would add much bulk here, and as discussed in #1804, zhashx_t is a bit smarter about managing expansion.

@garlick
Copy link
Member

garlick left a comment

Looks good, just some random comments that you don't need to take very seriously if they do not seem like good ideas.

Show resolved Hide resolved src/modules/kvs/kvs.c Outdated
Show resolved Hide resolved src/modules/kvs/kvs.c Outdated
Show resolved Hide resolved src/modules/kvs/kvs.c Outdated
Show resolved Hide resolved src/modules/kvs/cache.c
Show resolved Hide resolved src/modules/kvs/cache.c Outdated
Show resolved Hide resolved src/modules/kvs/cache.h Outdated
Show resolved Hide resolved src/modules/kvs/kvs.c
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2018

On cache_entry_get_blobref(), we end up with two copies of the key because zhash_t internally duplicates it as well. If we switched to zhashx_t, and set the key duplicator and key destructor to NULL, cache_insert() can just pass the duplicated key to zhashx_insert() and avoid the extra copy. I don't think converting would add much bulk here, and as discussed in #1804, zhashx_t is a bit smarter about managing expansion.

I like this idea!

@chu11 chu11 force-pushed the chu11:issue792 branch from 88ac1c7 to ab6e83a Nov 14, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2018

just a checkpoint push fixing all the small typos and what not above. These changes are all small so I went ahead and squashed the changes.

@chu11 chu11 force-pushed the chu11:issue792 branch from ab6e83a to 76ebd91 Nov 14, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 14, 2018

Codecov Report

Merging #1836 into master will increase coverage by 0.06%.
The diff coverage is 80.79%.

@@            Coverage Diff             @@
##           master    #1836      +/-   ##
==========================================
+ Coverage   79.84%   79.91%   +0.06%     
==========================================
  Files         197      197              
  Lines       35212    35309      +97     
==========================================
+ Hits        28116    28216     +100     
+ Misses       7096     7093       -3
Impacted Files Coverage Δ
src/modules/kvs/waitqueue.c 88.28% <100%> (+2.43%) ⬆️
src/modules/kvs/kvstxn.c 77.5% <40%> (-0.42%) ⬇️
src/modules/kvs/kvs.c 65.85% <64.06%> (+0.44%) ⬆️
src/modules/kvs/cache.c 90.61% <95%> (+0.55%) ⬆️
src/common/libflux/response.c 79.62% <0%> (-1.24%) ⬇️
src/common/libflux/mrpc.c 86.71% <0%> (-1.18%) ⬇️
src/broker/module.c 83.56% <0%> (-0.28%) ⬇️
src/cmd/flux-module.c 85.88% <0%> (+0.3%) ⬆️
src/common/libflux/message.c 81.76% <0%> (+0.36%) ⬆️
... and 4 more

@chu11 chu11 force-pushed the chu11:issue792 branch from 246eb7f to 5de4a13 Nov 14, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2018

Per discussion above, did two extra additions:

  • refactor cache_entry_create() to take a ref as input and it gets stored in the entry. cache_insert() no longer needs to take a reference as input as it's already in the entry.

  • use zhashx_t instead of zhash_t, so that we don't have to duplicate the hash keys, as they are already duplicated instead each cache entry. Bonus, we can set a global hash/cache destructor on a zhashx_t to destroy cache entries on deletion.

@garlick
Copy link
Member

garlick left a comment

Nice work! A few comments inline. Nothing major.

Show resolved Hide resolved src/modules/kvs/kvs.c
Show resolved Hide resolved src/modules/kvs/kvs.c
Show resolved Hide resolved src/modules/kvs/test/waitqueue.c
Show resolved Hide resolved src/modules/kvs/cache.c Outdated
Show resolved Hide resolved src/modules/kvs/cache.c Outdated
Show resolved Hide resolved src/modules/kvs/cache.c Outdated

@chu11 chu11 force-pushed the chu11:issue792 branch from 5de4a13 to 7fabdac Nov 15, 2018

chu11 added some commits Nov 9, 2018

modules/kvs: Add aux errnum and cb to waitqueue
Add ability to set/get an auxiliary errnum to waitqueue.  Also
allow an optional callback to be called when a aux errnum is set.
modules/kvs: Rename/refactor content_store_get()
Rename/refactor content_store_get() to content_store_completion()
and remove prior content_store_completion() function.  Prior
refactoring removed the need for a synchronous contet_store_get(),
so any code related to the need for both a synchronous and asynchronous
store is removed.
modules/kvs: Add wait_queue_iter()
Add ability to iterate through all wait queue waiters.
modules/kvs: Add functions to set waiter errnums
Add function cache_entry_set_errnum_on_valid() and
cache_entry_set_errnum_on_notdirty() to allow users to
inform all waiters for a valid or notdirty cache entry
that an error has occurred.
modules/kvs: Return async load errors
Allow most errors during an asynchronous load to be returned
to the original caller, instead of just hanging.

The only errors that cannot be returned will be under catastrophic
circumstances when a cache entry can't be found.
modules/kvs: Add cache_entry_get_blobref()
Add function to retrieve the blobref that stores a particular
cache entry.  Requires change to cache_insert() that sets an
entry's blobref upon insertion and results in cache_insert()
returning an int instead of a void.

Adjust all callers of cache_insert() appropriately.
modules/kvs: Add blobref check in store
In content_store_completion(), add a double check to ensure the
blobref returned from the content store is identical to the one
used when storing an entry to the cache.
modules/kvs: Return async store errors
Allow many errors during an asynchronous store to be returned
to the original caller, instead of just hanging.  A few catastrophic
errors cannot be recovered from.

Fixes #792

@chu11 chu11 force-pushed the chu11:issue792 branch from 7fabdac to 6e12e4d Nov 15, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 15, 2018

pushed with changes & rebased

@@ -89,6 +89,11 @@ int cache_entry_set_errnum_on_notdirty (struct cache_entry *entry, int errnum);
int cache_entry_wait_notdirty (struct cache_entry *entry, wait_t *wait);
int cache_entry_wait_valid (struct cache_entry *entry, wait_t *wait);

/* Get the blobref this entry is stored in the cache with. Returns
* blobref on success, NULL if not yet stored in the cache.

This comment has been minimized.

@garlick

garlick Nov 15, 2018

Member

Comment about NULL return is out of date

This comment has been minimized.

@chu11

chu11 Nov 15, 2018

Author Contributor

doh! thanks for the catch

This comment has been minimized.

@chu11

chu11 Nov 15, 2018

Author Contributor

hmmm, now that I think of it, with the change to cache_entry_create() taking a blobref, and cache_insert() no longer taking a ref, cache_insert() doesn't have to return an int. It can go back to returning a void. That would remove a lot of changes I did.

But then again, things could change in the future. Perhaps best to have cache_insert() return an int still.

This comment has been minimized.

@garlick

garlick Nov 15, 2018

Member

I think leaving the possibility of returning an error is probably not a bad thing, since it's somewhat dependent on the hash implementation whether an error is possible.

chu11 added some commits Nov 14, 2018

modules/kvs: Refactor cache entry create/insert
As cache entries now store the reference it is cached under, refactor
cache_entry_create() to be passed the reference at creation time
and stored within the entry.  Then refactor cache_insert() to no longer
take a reference as input at insertion time, since the reference
is stored in the entry.

Update all callers and tests appropriately.
modules/kvs: Cleanup cache iterations
Use FOREACH_ZHASH macro in locations when no entries in the cache
are modified while iterating.

Add additional comment why FOREACH_ZHASH can't be used in one
location.
modules/kvs: Use zhashx_t in cache
Replace use of zhash_t with zhashx_t in cache.  By doing so,
we avoid duplicating the hash key/blobref that is already duplicated
in each cache entry.  The keys are also not duplicated when
cache entries are checked for expiration.

A global destructor for cache entries is also set, removing the need
to set a free function on every item in the hash.

@chu11 chu11 force-pushed the chu11:issue792 branch from 6e12e4d to 8c23603 Nov 15, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 15, 2018

re-pushed with the comment change

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 15, 2018

Excellent! Ready to merge this?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 15, 2018

yup, glad to finally resolve this long lingering issue. When you hit it on #1799, thought we should finally get this solved.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 15, 2018

Agreed, nice work!

@garlick garlick merged commit 33f764e into flux-framework:master Nov 15, 2018

3 checks passed

codecov/patch 80.26% of diff hit (target 79.84%)
Details
codecov/project 79.88% (+0.04%) compared to 2bc53a7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.