Feature/freshen up windows workers #126
Conversation
stagnation
force-pushed
the
feature/freshen-up-windows-workers
branch
3 times, most recently
from
e0d5056 to
5ae118c
Compare
pkg/builder/local_build_executor.go
Outdated
| func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesystem.FilePool, monitor access.UnreadDirectoryMonitor, digestFunction digest.Function, request *remoteworker.DesiredState_Executing, executionStateUpdates chan<- *remoteworker.CurrentState_Executing) *remoteexecution.ExecuteResponse { | ||
| func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesystem.FilePool, monitor access.UnreadDirectoryMonitor, digestFunction digest.Function, request *remoteworker.DesiredState_Executing, executionStateUpdates chan<- *remoteworker.CurrentState_Executing) (response *remoteexecution.ExecuteResponse) { | ||
| // Timeout handling. | ||
| response := NewDefaultExecuteResponse(request) | ||
| response = NewDefaultExecuteResponse(request) |
There was a problem hiding this comment.
Why is this change needed? If it's not needed, please revert.
There was a problem hiding this comment.
No it is not needed 👍. I assumed when first trying to modify the response in the defer closure that we needed to have the name captured as a named output. Then forgot it when I rewrote the code to just log, and now that we send the error again I find that this change is not needed.
pkg/builder/local_build_executor.go
Outdated
| defer func() { | ||
| err := buildDirectory.Close() | ||
| if err != nil { | ||
| log.Printf("Failed to clean up build environment: %v\n", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Can't we simply do:
defer func() {
if err := buildDirectory.Close(); err != nil {
attachErrorToExecuteResponse(response, util.StatusWrap(err, "Failed to close build directory"))
}
}()?
There was a problem hiding this comment.
Yes. I have a commit to do just that, and am looking into a better way to avoid Bazel retrying and printing follow up errors to the user.
This is the commit message if you find it useful, or I can prune it if it is too much information.
Send failure to clean up build directory to the client
The client may choose to retry the action and then receive a different
follow-up error.
Action: http://.../remote-execution/blobs/sha256/action/6068c5370fdcca9dfb70936f5ea481cf53092c5c3ea328e351a69641d5c7fbfa-169/
ExecuteResponse: {...,
"status":{
"code":13,
"message":"Failed to close build directory: Failed to remove build directory \"6068c5370fdcca9d\": Failed to remove \"root/external/com_google_absl/absl/base/internal\": directory not empty"
}}
Action: http://.../remote-execution/blobs/sha256/action/6068c5370fdcca9dfb70936f5ea481cf53092c5c3ea328e351a69641d5c7fbfa-169/
ExecuteResponse: {...,
"status":{
"code":13,
"message":"Failed to acquire build environment: Failed to create build directory \"6068c5370fdcca9d\": Cannot create a file when that file already exists."
}}
There was a problem hiding this comment.
Concurrency is also an important note here if we want to accurately describe the current state of affairs, or we can punt that and look at the required improvements instead.
pkg/runner/local_runner_windows.go
Outdated
| @@ -25,6 +26,24 @@ func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator { | |||
| return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) { | |||
There was a problem hiding this comment.
If we can't get rid of this CommandCreator implementation, can we at least ensure it's as similar to the one we use on UNIX as possible, in terms of comments, ordering of statements, etc. etc. etc.
pkg/runner/local_runner_windows.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| arguments[0] = "\\nonexistent-place-back-later" |
There was a problem hiding this comment.
This is mutating one of the input arguments of this function. Is this safe/desirable? Considering that we call exec.CommandContext() with arguments[0] and arguments[1:] separately, why not just pass in the string there?
There was a problem hiding this comment.
Yes. With the shared command creator that is much better. This was an artifact from also setting the argv0 on Windows to match cmd.Path. But It could have been implemented better. Now though we do not choose to do so.
stagnation
force-pushed
the
feature/freshen-up-windows-workers
branch
2 times, most recently
from
36aefe2 to
e44ccf7
Compare
pkg/builder/local_build_executor.go
Outdated
| @@ -2,6 +2,7 @@ package builder | |||
|
|
|||
| import ( | |||
| "context" | |||
| "log" | |||
There was a problem hiding this comment.
I think this should be removed.
| @@ -160,7 +161,14 @@ func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesyste | |||
| util.StatusWrap(err, "Failed to acquire build environment")) | |||
| return response | |||
| } | |||
| defer buildDirectory.Close() | |||
| defer func() { | |||
| err := buildDirectory.Close() | |||
There was a problem hiding this comment.
if err := buildDirectory.Close(); err != nil {
The client may choose to retry the action and then receive a different
follow-up error.
Action: http://.../remote-execution/blobs/sha256/action/6068c5370fdcca9dfb70936f5ea481cf53092c5c3ea328e351a69641d5c7fbfa-169/
ExecuteResponse: {...,
"status":{
"code":13,
"message":"Failed to close build directory: Failed to remove build directory \"6068c5370fdcca9d\": Failed to remove \"root/external/com_google_absl/absl/base/internal\": directory not empty"
}}
Action: http://.../remote-execution/blobs/sha256/action/6068c5370fdcca9dfb70936f5ea481cf53092c5c3ea328e351a69641d5c7fbfa-169/
ExecuteResponse: {...,
"status":{
"code":13,
"message":"Failed to acquire build environment: Failed to create build directory \"6068c5370fdcca9d\": Cannot create a file when that file already exists."
}}
stagnation
force-pushed
the
feature/freshen-up-windows-workers
branch
from
e44ccf7 to
3ab02e3
Compare
.