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.

Sign up for GitHub

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

rsc: Implement several bug fixes #1543

Merged
merged 2 commits into from
Apr 18, 2024
Merged

rsc: Implement several bug fixes #1543

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e024eb
rsc: Implement several bug fixes
V-FEXrt Apr 16, 2024
d6e35f0
Address comments
V-FEXrt Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions rust/rsc/src/rsc/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,8 +333,10 @@ fn launch_blob_eviction(
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
// setup a subscriber for logging
let subscriber = tracing_subscriber::FmtSubscriber::new();
tracing::subscriber::set_global_default(subscriber)?;
// TODO: The logging is incredibly spammy right now and causes significant slow down.
// for now, logging is disabled but this should be turned back on once logging is pruned.
Comment on lines +336 to +337
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably just make this an option right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, though I want to spend some time to get it right. i.e. a matrix of where to log X what level to log at. I feel like it'll be totally fine for "log errors to file" as the default. I just wanted to focus on making it work first before circling back around to that.

// let subscriber = tracing_subscriber::FmtSubscriber::new();
// tracing::subscriber::set_global_default(subscriber)?;

// Parse the arguments
let args = ServerOptions::parse();
Expand Down
72 changes: 42 additions & 30 deletions share/wake/lib/system/remote_cache_runner.wake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,66 +162,79 @@ export def mkRemoteCacheRunner (rscApi: RemoteCacheApi) (hashFn: Result RunnerIn
else failWithError "rsc: Failed to make output directory"

def doDownload (CacheSearchOutputFile path mode blob) =
rscApiGetFileBlob blob "{input.getRunnerInputDirectory}/{path}" mode
rscApiGetFileBlob blob path mode
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to write down in the RunnerInput/Plan tuple documentation that only Command is relative to Directory both Visible/Input and Output are relative to workspace. It seems reasonable to assume either and I don't see it written down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a comment/note on this line but it would probably be good for us to put together a high level document covering all these details/designs/limitations.

We have a bunch of smaller docs, but I don't think we have the top level doc covering all the content. Mainly because most of this was just iterating on the local cache learnings/design


# We don't actually care about the result here but we need to ensure that all
# downloads have completed and succeeded before we attempt any symlinks/continue.
require Pass _ =
outputFiles
| map doDownload
| findFail
| addErrorContext "rsc: Failed to download a blob"

# TODO: These probably need to be wakeroot relative
# Link must point to dest. We do the reverse here of what is done for posting a job
def doMakeSymlink (CacheSearchOutputSymlink dest link) =
# Link must point to path. We do the reverse here of what is done for posting a job
def doMakeSymlink (CacheSearchOutputSymlink path link) =
require True =
makeExecPlan
(
"ln",
# create symbolic link
"-s",
dest,
# overwrite <link> on disk if a file already exists at that path.
# Ideally, we could just fail but its very common to delete wake.db
# without cleaning all the outputs. This causes problems since the link
# would already exist on disk
"-f",
path,
link,
)
Nil
| setPlanLabel "rsc: symlink {link} to {dest}"
| setPlanLabel "rsc: symlink {link} to {path}"
| runJobWith localRunner
| isJobOk
else failWithError "rsc: Failed to link {link} to {dest}"
else failWithError "rsc: Failed to link {link} to {path}"

Pass Unit

require Pass _ =
def outputFilesDownload =
outputFiles
| map doDownload

# Symlinks don't need to wait for files, the symlinks will just momentarily be invalid if created first
def outputSymlinksDownload =
outputSymlinks
| map doMakeSymlink
| findFail
| addErrorContext "rsc: Failed to create a symlink"

require Pass stdout =
stdoutDownload
| addErrorContext "rsc: Failed to download stdout for '{label}'"

require Pass stderr =
stderrDownload
| addErrorContext "rsc: Failed to download stderr for '{label}'"

def resolvedOutputs =
outputFiles
| map getCacheSearchOutputFilePath

def resolvedSymlinks =
outputSymlinks
| map getCacheSearchOutputSymlinkPath
| map getCacheSearchOutputSymlinkLink

def resolvedDirectories =
outputDirs
| map getCacheSearchOutputDirectoryPath

def outputs = resolvedOutputs ++ resolvedDirectories ++ resolvedSymlinks
def predict = Usage status runtime cputime mem ibytes obytes

require Pass stdout =
stdoutDownload
| addErrorContext "rsc: Failed to download stdout for '{label}'"

require Pass stderr =
stderrDownload
| addErrorContext "rsc: Failed to download stderr for '{label}'"

def _ = virtual job stdout stderr status runtime cputime mem ibytes obytes
def inputs = map getPathName (input.getRunnerInputVisible)

# We don't actually care about the result here but we need to ensure that all
# downloads have completed before returning.
require Pass _ =
outputFilesDownload
| findFail
| addErrorContext "rsc: Failed to download a blob"

require Pass _ =
outputSymlinksDownload
| findFail
| addErrorContext "rsc: Failed to create a symlink"

Pass (RunnerOutput inputs outputs predict)

def _ =
Expand Down Expand Up @@ -341,7 +354,7 @@ def postJob (rscApi: RemoteCacheApi) (job: Job) (_wakeroot: String) (hidden: Str
# The path output by the job itself is the created symlink. The contents of the symlink
# is the original file on disk. This is reversed when downloading a job.
def makeSymlink (Pair link _) =
require Pass dest =
require Pass path =
makeExecPlan
(
"readlink",
Expand All @@ -353,8 +366,7 @@ def postJob (rscApi: RemoteCacheApi) (job: Job) (_wakeroot: String) (hidden: Str
| runJobWith localRunner
| getJobStdout

# TODO: Pretty sure this needs to be wakeroot relative
CachePostRequestOutputSymlink dest link
CachePostRequestOutputSymlink path link
| Pass

def makeDirectory (Pair path (Stat _ mode _)) =
Expand Down
Loading