Skip to content

[WB-8588] Add username to debug-cli log file to prevent conflicts of multiple users#3301

Merged
raubitsj merged 12 commits intomasterfrom
security/debug-cli-log
Apr 2, 2022
Merged

[WB-8588] Add username to debug-cli log file to prevent conflicts of multiple users#3301
raubitsj merged 12 commits intomasterfrom
security/debug-cli-log

Conversation

@zythosec
Copy link
Copy Markdown
Contributor

Fixes WB-8588

Description

  • Uses mkstemp to generate a random filename with identifiable prefix/suffix for logging. This enables multiple clis to log on same machine by logging to different directories.

Testing

How was this PR tested?

Checklist

  • Name PR "[WB-NNNN][WB-MMMM] Add support for..." similar to entries in CHANGELOG.md
  • Include reference to internal ticket "Fixes WB-NNNN" (and github issue "Fixes #NNNN" if applicable)

@zythosec zythosec force-pushed the security/debug-cli-log branch from ea3395f to 207e3c2 Compare February 25, 2022 20:07
@zythosec zythosec marked this pull request as draft February 25, 2022 20:07
Copy link
Copy Markdown
Contributor

@raubitsj raubitsj left a comment

Choose a reason for hiding this comment

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

i think this is going to add too much noise in the wandb_dir. (it is bad enough with runs.. but every cli log will be too much noise i think)
maybe a directory where the randomized log goes. and then if supported a symlink from the run dir named the original debug-cli.log... This will clobber the previous symlink but all the logs will be in the wandb/cli-logs/ directory.

The current structure:

rwxr-xr-x  41 jeff  staff  1312 Feb 25 12:02 ../
-rw-r--r--   1 jeff  staff     0 Feb 23 13:15 debug-cli.log
lrwxr-xr-x   1 jeff  staff    52 Feb 23 14:25 debug-internal.log@ -> run-20220223_142515-2k3yz9hl/logs/debug-internal.log
lrwxr-xr-x   1 jeff  staff    43 Feb 23 14:25 debug.log@ -> run-20220223_142515-2k3yz9hl/logs/debug.log
lrwxr-xr-x   1 jeff  staff    28 Feb 23 14:25 latest-run@ -> run-20220223_142515-2k3yz9hl
drwxr-xr-x   5 jeff  staff   160 Feb 23 13:13 run-20220223_131333-2m5racrz/
drwxr-xr-x   5 jeff  staff   160 Feb 23 13:13 run-20220223_131333-2p8nnqyq/
drwxr-xr-x   5 jeff  staff   160 Feb 23 13:13 run-20220223_131333-2v4yzlgz/
drwxr-xr-x   5 jeff  staff   160 Feb 23 13:13 run-20220223_131333-3k5kmcti/

We actually do a similar thing with sweeps when running locally. (iirc)
sweep-....
latest-sweep@ -> sweep-something/

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 25, 2022

Codecov Report

Merging #3301 (fa7d92f) into master (1ef7ae2) will increase coverage by 0.05%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3301      +/-   ##
==========================================
+ Coverage   81.62%   81.67%   +0.05%     
==========================================
  Files         236      236              
  Lines       29108    29113       +5     
==========================================
+ Hits        23758    23779      +21     
+ Misses       5350     5334      -16     
Flag Coverage Δ
functest 58.18% <60.00%> (+0.02%) ⬆️
unittest 71.84% <60.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
wandb/cli/cli.py 66.27% <60.00%> (-0.04%) ⬇️
wandb/integration/tensorboard/monkeypatch.py 87.65% <0.00%> (-4.94%) ⬇️
wandb/sdk/lib/git.py 76.35% <0.00%> (ø)
wandb/sdk/internal/internal_api.py 83.55% <0.00%> (+1.49%) ⬆️
wandb/sdk/internal/meta.py 90.74% <0.00%> (+3.08%) ⬆️
wandb/sdk/internal/artifacts.py 83.15% <0.00%> (+6.31%) ⬆️

Copy link
Copy Markdown
Member

@dmitryduev dmitryduev left a comment

Choose a reason for hiding this comment

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

How about instead creating a user-specific log? E.g., get the current user, and append that?

import getpass
user = getpass.getuser()

Otherwise, I agree with @raubitsj, that will add too much noise.

@zythosec
Copy link
Copy Markdown
Contributor Author

@raubitsj the issue with only having one file is that it prevents multiple users from running on the same machine at the same time. if they both try to wandb.login for instance, it will fail out. Looking deeper into it, it looks like there may still be additional pieces that would prevent two users from being able to run at the same time anyway.

@dmitryduev I wonder if all logs should be scoped and permissioned to the user that is running?

Also another question for both of you, is there anything else that may prevent two users from running at the same time?

@raubitsj
Copy link
Copy Markdown
Contributor

raubitsj commented Mar 1, 2022

@raubitsj the issue with only having one file is that it prevents multiple users from running on the same machine at the same time. if they both try to wandb.login for instance, it will fail out. Looking deeper into it, it looks like there may still be additional pieces that would prevent two users from being able to run at the same time anyway.

I was suggesting multiple files. But a symlink to the most recent one. We do this for our other logs. It should be safe

@dmitryduev
Copy link
Copy Markdown
Member

@zythosec good point, debug.log should also be scoped to each user IMO.
Also, latest-run symlink should also be scoped to the user. Do you agree @raubitsj?

@zythosec
Copy link
Copy Markdown
Contributor Author

zythosec commented Mar 2, 2022

@raubitsj Makes sense about symlinking to latest file. I think the symlink would still need to be scoped to the user as @dmitryduev mentions above.

The main problem I'm trying to solve is multiple users running on the same machine, which I now realize might be a bigger conversation than just log files. Happy to open up a separate issue if it makes more sense to have that conversation there.

@manuel-delverme
Copy link
Copy Markdown

I can't run any cli command on our cluster due to this..
is there a workaround?

@vanpelt
Copy link
Copy Markdown
Contributor

vanpelt commented Mar 16, 2022

@manuel-delverme you can set the TMPDIR environment variable to a unique directory that's writable by you as a quick and dirty workaround, i.e.

mkdir /tmp/$(whoami)
export TMPDIR=/tmp/$(whoami)

@vanpelt
Copy link
Copy Markdown
Contributor

vanpelt commented Mar 16, 2022

CC @dmitryduev @raubitsj let's make sure a fix for this makes it into the next release. I would think scoping the file to a well known name that's unique per user on the machine should do it.

@dmitryduev dmitryduev marked this pull request as ready for review March 19, 2022 01:07
@dmitryduev
Copy link
Copy Markdown
Member

I scoped the cli-debug log to the user - does it look good? @vanpelt @zythosec @raubitsj

@dmitryduev dmitryduev requested a review from raubitsj March 21, 2022 08:00
@zythosec
Copy link
Copy Markdown
Contributor Author

Thanks @dmitryduev looks good to me. I can't approve my own PR, but approve the changes.

@dmitryduev dmitryduev added this to the sdk-2022-04.1 milestone Mar 21, 2022
@raubitsj raubitsj merged commit 1bc04df into master Apr 2, 2022
@raubitsj raubitsj deleted the security/debug-cli-log branch April 2, 2022 21:02
@raubitsj raubitsj changed the title [WB-8588] Add random element to debug-cli log file to prevent conflicts of multiple users [WB-8588] Add username to debug-cli log file to prevent conflicts of multiple users Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants