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

Download session/credentials locking -- inform user if locking is "failing" to be obtained, fail upon ~5min timeout #5884

Merged
merged 5 commits into from
Aug 18, 2021

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Aug 12, 2021

I think it could be considered to resolve #5099 - user will be given INFO level log messages (just now realized -- may be the last one at least should be a warning??? or should we have a progress bar? ;-)) with all possible information we can provide, even which processes have that lock ATM (well -- might not universally work... windows - I am talking to you).

I also decided that "heck with that locking" if we fail to get the lock within 5 minutes! at least this way may be some processes would complete (but possibly hide the problem).

Please see individual commits for more information. It seems to come out to be again yet another "one time use locking helper", but since we have not come up with a better way to address this locking "issue", I think it is a small price to pay ;)

  • [-] I have positioned against maint but it grew a bit too big. I will be fine to reposition it against master -- we should get one out of the door soon.
  • [-] I wonder if I should add some randomization so that not all parallel processes which fail to acquire lock start asking for credentials at the same time ... ;-)

… if failing

Also allow for a mode to  proceed_unlocked (not enabled by default) so we could
indeed ignore locking in some circumstances where clearly something is just odd
(should not take over 5 minutes to enter the password!)
…to acquire

Since this locking was primarily introduced just to avoid multiple parallel processes
querying for the same credential if it was not set etc, I think it would be just fine
to proceed unlocked after the default 3 attempts taking 5 minutes. After initial 5 seconds
user would be informed that locking is failing, and let user know the location of the
lock, and even (where it can figure out) which processes have it.  So, user should be
free to finish/kill those processes  or remove the lock having been given all information
we could provide
@yarikoptic yarikoptic changed the title Nf try lock informatively Download session/credentials locking -- inform user if locking is "failing" to be obtained, allow to proceed without locking Aug 12, 2021
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Aug 12, 2021
@yarikoptic yarikoptic added credentials Issues concerning credential(-management) UX user experience labels Aug 12, 2021
@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #5884 (136939c) into maint (2d53dd1) will decrease coverage by 7.59%.
The diff coverage is 72.72%.

❗ Current head 136939c differs from pull request most recent head 7324d71. Consider uploading reports for the commit 7324d71 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5884      +/-   ##
==========================================
- Coverage   90.32%   82.73%   -7.60%     
==========================================
  Files         300      297       -3     
  Lines       42396    42458      +62     
==========================================
- Hits        38293    35126    -3167     
- Misses       4103     7332    +3229     
Impacted Files Coverage Δ
datalad/support/locking.py 75.64% <48.64%> (-24.36%) ⬇️
datalad/support/tests/test_locking.py 91.48% <89.79%> (-4.26%) ⬇️
datalad/cmdline/tests/test_main.py 93.05% <100.00%> (-5.56%) ⬇️
datalad/downloaders/base.py 78.16% <100.00%> (-0.08%) ⬇️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 19.35% <0.00%> (-80.65%) ⬇️
datalad/metadata/extractors/xmp.py 12.96% <0.00%> (-79.63%) ⬇️
datalad/metadata/extractors/exif.py 18.75% <0.00%> (-78.13%) ⬇️
datalad/metadata/extractors/image.py 19.35% <0.00%> (-77.42%) ⬇️
datalad/metadata/extractors/audio.py 20.00% <0.00%> (-77.15%) ⬇️
... and 149 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d53dd1...7324d71. Read the comment docs.

assert_in(f'CWD={os.getcwd()} CMDLINE=', res['stderr'])
except AssertionError:
# we must have had the other one then
assert_in('failed to determine one', res['stderr'])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: no coverage here is odd since it should be happening on OSX... aren't we pushing coverage from those???

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, looking at /mnt/btrfs/datasets/datalad/ci/logs/2021/08/12/pr/5884/9827dc3/github-Test on macOS-1723-success/test (brew)/9_Upload coverage report to Codecov.txt -- coverage is successfully uploaded, and points to https://codecov.io/github/datalad/datalad/commit/9827dc38edf72106d429ee78dfa8d632a92bed43 but that one is not "run specific". I wonder if combining from multiple submissions works correctly, or just the last one to arrive (travis? ;)) wins it all?

@yarikoptic
Copy link
Member Author

I think I feel brave enough with this to proceed against maint without any further changes.

@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Aug 13, 2021
@yarikoptic yarikoptic added this to the 0.14.8 milestone Aug 13, 2021
@yarikoptic yarikoptic requested a review from mih August 14, 2021 17:19
@mih
Copy link
Member

mih commented Aug 16, 2021

I posted an extensive review yesterday. But there is no trace of it, wtf!? I have no time right now to rewrite it, but the bottom line was a "request changes", because ignoring locking in this fashion might pave over actual problems that we better get fixed. That being said, I unconditionally like the more informative output!

Per @mih opinion and my agreement with that
@yarikoptic
Copy link
Member Author

I posted an extensive review yesterday. But there is no trace of it, wtf!?

Happens to the best of us (did you grow to carry over a dozen of tabs in the browser?).

... ignoring locking in this fashion might pave over actual problems that we better get fixed.

I was on the edge in this but even though I thought to argue "but that is only for us to lock for credentials input!", experience in troubleshooting git-annex in #5400 (where git-annex tries to lock for quite a while to only give up eventually and proceed) kinda tilted me to the opinion, that yeah -- let's just error out so no such abnormal hard-to-debug cases emerge, and behavior would be somewhat consistent with current maint where it would hang/"error out by being killed". Pushing 1 line change

@yarikoptic yarikoptic changed the title Download session/credentials locking -- inform user if locking is "failing" to be obtained, allow to proceed without locking Download session/credentials locking -- inform user if locking is "failing" to be obtained, fail upon ~5min timeout Aug 18, 2021
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thx! Let's give it a go!

@yarikoptic yarikoptic merged commit d1a19e2 into datalad:maint Aug 18, 2021
@yarikoptic yarikoptic deleted the nf-try-lock-informatively branch October 8, 2021 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
credentials Issues concerning credential(-management) merge-if-ok OP considers this work done, and requests review/merge semver-patch Increment the patch version when merged UX user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants