Skip to content

Commit

Permalink
Remove timeout logic around cvd create in host orchestrator.
Browse files Browse the repository at this point in the history
- This would be managed at the cvd layer.
  • Loading branch information
ser-io committed May 28, 2024
1 parent 68d613d commit 519a949
Show file tree
Hide file tree
Showing 9 changed files with 1 addition and 95 deletions.
2 changes: 0 additions & 2 deletions docker/orchestration/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ RUN apt install -y -f \

RUN echo "num_cvd_accounts=100" >> /etc/default/cuttlefish-host-resources

RUN echo "orchestrator_cvd_creation_timeout_secs=1800" >> /etc/default/cuttlefish-host_orchestrator

RUN usermod -aG kvm root
RUN usermod -aG cvdnetwork root

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ orchestrator_listen_address=0.0.0.0
# Defaults to "/tmp/<uid>/cvd_artifacts"
orchestrator_cvd_artifacts_dir=/var/lib/cuttlefish-common
#
# Timeout for CVD creation
# Defaults to 7 mins
# orchestrator_cvd_creation_timeout_secs=420
#
# Where web UI in the application is from
# If it isn't empty, the application uses web UI from the url,
# else, it uses web pages which is generated during the build
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,6 @@ start_orchestrator() {
if [[ -n "${orchestrator_cvd_artifacts_dir}" ]]; then
args+=("--cvd_artifacts_dir=${orchestrator_cvd_artifacts_dir}")
fi
if [[ -n "${orchestrator_cvd_creation_timeout_secs}" ]]; then
args+=("--cvd_creation_timeout_secs=${orchestrator_cvd_creation_timeout_secs}")
fi
if [[ -n "${operator_http_port}" ]]; then
args+=("--operator_http_port=${operator_http_port}")
fi
Expand Down
2 changes: 0 additions & 2 deletions frontend/src/host_orchestrator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ func newOperatorProxy(port int) *httputil.ReverseProxy {
func main() {
httpPort := flag.Int("http_port", 2081, "Port to listen on for HTTP requests.")
cvdUser := flag.String("cvd_user", "", "User to execute cvd as.")
cvdCreationTimeoutSecs := flag.Int("cvd_creation_timeout_secs", 420, "CVD creation timeout in seconds")
operatorPort := flag.Int("operator_http_port", 1080, "Port where the operator is listening.")
abURL := flag.String("android_build_url", defaultAndroidBuildURL, "URL to an Android Build API.")
imRootDir := flag.String("cvd_artifacts_dir", defaultCVDArtifactsDir(), "Directory where cvd will download android build artifacts to.")
Expand Down Expand Up @@ -130,7 +129,6 @@ func main() {
Config: orchestrator.Config{
Paths: imPaths,
AndroidBuildServiceURL: *abURL,
CVDCreationTimeout: time.Duration(*cvdCreationTimeoutSecs) * time.Second,
CVDUser: *cvdUser,
},
OperationManager: om,
Expand Down
2 changes: 0 additions & 2 deletions frontend/src/host_orchestrator/orchestrator/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ const HeaderBuildAPICreds = "X-Cutf-Host-Orchestrator-BuildAPI-Creds"
type Config struct {
Paths IMPaths
AndroidBuildServiceURL string
CVDCreationTimeout time.Duration
CVDUser string
}

Expand Down Expand Up @@ -195,7 +194,6 @@ func (h *createCVDHandler) Handle(r *http.Request) (interface{}, error) {
ArtifactsFetcher: artifactsFetcher,
CVDBundleFetcher: cvdBundleFetcher,
UUIDGen: func() string { return uuid.New().String() },
CVDStartTimeout: h.Config.CVDCreationTimeout,
CVDUser: h.Config.CVDUser,
UserArtifactsDirResolver: h.UADirResolver,
BuildAPICredentials: creds,
Expand Down
10 changes: 1 addition & 9 deletions frontend/src/host_orchestrator/orchestrator/createcvdaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"sync"
"sync/atomic"
"syscall"
"time"

"github.com/google/android-cuttlefish/frontend/src/host_orchestrator/orchestrator/artifacts"
"github.com/google/android-cuttlefish/frontend/src/host_orchestrator/orchestrator/cvd"
Expand All @@ -45,7 +44,6 @@ type CreateCVDActionOpts struct {
CVDBundleFetcher artifacts.CVDBundleFetcher
UUIDGen func() string
CVDUser string
CVDStartTimeout time.Duration
UserArtifactsDirResolver UserArtifactsDirResolver
BuildAPICredentials string
}
Expand All @@ -63,7 +61,6 @@ type CreateCVDAction struct {
artifactsMngr *artifacts.Manager
startCVDHandler *startCVDHandler
cvdUser string
cvdStartTimeout time.Duration
buildAPICredentials string

instanceCounter uint32
Expand All @@ -81,7 +78,6 @@ func NewCreateCVDAction(opts CreateCVDActionOpts) *CreateCVDAction {
cvdBundleFetcher: opts.CVDBundleFetcher,
userArtifactsDirResolver: opts.UserArtifactsDirResolver,
cvdUser: opts.CVDUser,
cvdStartTimeout: opts.CVDStartTimeout,
buildAPICredentials: opts.BuildAPICredentials,

artifactsMngr: artifacts.NewManager(
Expand All @@ -91,7 +87,6 @@ func NewCreateCVDAction(opts CreateCVDActionOpts) *CreateCVDAction {
execContext: cvdExecContext,
startCVDHandler: &startCVDHandler{
ExecContext: cvdExecContext,
Timeout: opts.CVDStartTimeout,
},
}
}
Expand Down Expand Up @@ -154,10 +149,7 @@ func (a *CreateCVDAction) launchWithCanonicalConfig(op apiv1.Operation) (*apiv1.
args = append(args, "--credential_source=gce")
}
}
opts := cvd.CommandOpts{
Timeout: a.cvdStartTimeout,
}
cmd := cvd.NewCommand(a.execContext, args, opts)
cmd := cvd.NewCommand(a.execContext, args, cvd.CommandOpts{})
if err := cmd.Run(); err != nil {
return nil, operator.NewInternalError(ErrMsgLaunchCVDFailed, err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,40 +413,6 @@ func TestCreateCVDFailsDueCVDSubCommandExecution(t *testing.T) {
}
}

func TestCreateCVDFailsDueTimeout(t *testing.T) {
dir := orchtesting.TempDir(t)
defer orchtesting.RemoveDir(t, dir)
execContext := execCtxCvdSubcmdDelays
paths := IMPaths{ArtifactsRootDir: dir + "/artifacts"}
om := NewMapOM()
buildAPI := &fakeBuildAPI{}
artifactsFetcher := newBuildAPIArtifactsFetcher(buildAPI)
cvdExecContext := newCVDExecContext(execContext, fakeCVDUser)
cvdBundleFetcher := newFetchCVDCommandArtifactsFetcher(cvdExecContext, "")
opts := CreateCVDActionOpts{
Request: &apiv1.CreateCVDRequest{CVD: &apiv1.CVD{BuildSource: androidCISource("1", "foo")}},
HostValidator: &AlwaysSucceedsValidator{},
Paths: paths,
OperationManager: om,
ExecContext: execContext,
BuildAPI: buildAPI,
ArtifactsFetcher: artifactsFetcher,
CVDBundleFetcher: cvdBundleFetcher,
CVDStartTimeout: testFakeBinaryDelayMs - (50 * time.Millisecond),
CVDUser: fakeCVDUser,
}
action := NewCreateCVDAction(opts)

op, err := action.Run()

orchtesting.FatalIfNil(t, err)
res, err := om.Wait(op.Name, 1*time.Second)
orchtesting.FatalIfNil(t, err)
if res.Error == nil {
t.Error("expected error")
}
}

type AlwaysFailsValidator struct{}

func (AlwaysFailsValidator) Validate() error {
Expand Down
36 changes: 0 additions & 36 deletions frontend/src/host_orchestrator/orchestrator/cvd/cvd.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,13 @@ import (
"log"
"os/exec"
"strings"
"sync/atomic"
"syscall"
"time"
)

type CVDExecContext = func(ctx context.Context, env []string, name string, arg ...string) *exec.Cmd

const (
CVDBin = "/usr/bin/cvd"
FetchCVDBin = "/usr/bin/fetch_cvd"

CVDCommandDefaultTimeout = 30 * time.Second
)

const (
Expand All @@ -44,7 +39,6 @@ type CommandOpts struct {
AndroidHostOut string
Home string
Stdout io.Writer
Timeout time.Duration
}

type Command struct {
Expand Down Expand Up @@ -77,46 +71,16 @@ func (e *CommandExecErr) Error() string {

func (e *CommandExecErr) Unwrap() error { return e.err }

type CommandTimeoutErr struct {
args []string
}

func (e *CommandTimeoutErr) Error() string {
return fmt.Sprintf("cvd execution with args %q timed out", strings.Join(e.args, " "))
}

func (c *Command) Run() error {
// TODO: Use `context.WithTimeout` if upgrading to go 1.19 as `exec.Cmd` adds the `Cancel` function field,
// so the cancel logic could be customized to continue sending the SIGINT signal.
cmd := c.execContext(context.TODO(), cvdEnv(c.opts.AndroidHostOut), c.cvdBin, c.args...)
stderr := &bytes.Buffer{}
cmd.Stdout = c.opts.Stdout
cmd.Stderr = stderr
if err := cmd.Start(); err != nil {
return err
}
var timedOut atomic.Value
timedOut.Store(false)
timeout := CVDCommandDefaultTimeout
if c.opts.Timeout != 0 {
timeout = c.opts.Timeout
}
go func() {
select {
case <-time.After(timeout):
// NOTE: Do not use SIGKILL to terminate cvd commands. cvd commands are run using
// `sudo` and contrary to SIGINT, SIGKILL is not relayed to child processes.
if err := cmd.Process.Signal(syscall.SIGINT); err != nil {
log.Printf("error sending SIGINT signal %+v", err)
}
timedOut.Store(true)
}
}()
if err := cmd.Wait(); err != nil {
LogStderr(cmd, stderr.String())
if timedOut.Load().(bool) {
return &CommandTimeoutErr{c.args}
}
return &CommandExecErr{c.args, stderr.String(), err}
}
return nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"path/filepath"
"strconv"
"strings"
"time"

"github.com/google/android-cuttlefish/frontend/src/host_orchestrator/orchestrator/artifacts"
"github.com/google/android-cuttlefish/frontend/src/host_orchestrator/orchestrator/cvd"
Expand Down Expand Up @@ -321,7 +320,6 @@ const (

type startCVDHandler struct {
ExecContext cvd.CVDExecContext
Timeout time.Duration
}

type startCVDParams struct {
Expand Down Expand Up @@ -359,7 +357,6 @@ func (h *startCVDHandler) Start(p startCVDParams) error {
opts := cvd.CommandOpts{
AndroidHostOut: p.MainArtifactsDir,
Home: p.RuntimeDir,
Timeout: h.Timeout,
}
cvdCmd := cvd.NewCommand(h.ExecContext, args, opts)
err := cvdCmd.Run()
Expand Down

0 comments on commit 519a949

Please sign in to comment.