From 834b2d138482d6a4923866707aea9c7d55998138 Mon Sep 17 00:00:00 2001 From: kylecarbs Date: Mon, 27 Jun 2022 14:37:49 +0000 Subject: [PATCH 1/2] fix: Elongate agent disconnect timeout in tests This will fix the flake seen here: https://github.com/coder/coder/runs/7071719863?check_suite_focus=true --- coderd/coderd.go | 5 +++++ coderd/coderdtest/coderdtest.go | 3 +++ coderd/provisionerjobs.go | 2 +- coderd/workspaceagents.go | 20 ++++++++++---------- coderd/workspaceresources.go | 2 +- 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index dcfeb1f747c06..acf39fd8bbead 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -47,6 +47,7 @@ type Options struct { CacheDir string AgentConnectionUpdateFrequency time.Duration + AgentInactiveDisconnectTimeout time.Duration // APIRateLimit is the minutely throughput rate limit per user or ip. // Setting a rate limit <0 will disable the rate limiter across the entire // app. Specific routes may have their own limiters. @@ -69,6 +70,10 @@ func New(options *Options) *API { if options.AgentConnectionUpdateFrequency == 0 { options.AgentConnectionUpdateFrequency = 3 * time.Second } + if options.AgentInactiveDisconnectTimeout == 0 { + // Multiply the update by two to allow for some lag-time. + options.AgentInactiveDisconnectTimeout = options.AgentConnectionUpdateFrequency * 2 + } if options.APIRateLimit == 0 { options.APIRateLimit = 512 } diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index ea530c4de2c44..0e3a7da471d52 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -152,6 +152,9 @@ func NewWithAPI(t *testing.T, options *Options) (*codersdk.Client, *coderd.API) // We set the handler after server creation for the access URL. coderAPI := coderd.New(&coderd.Options{ AgentConnectionUpdateFrequency: 150 * time.Millisecond, + // Force a long disconnection timeout to ensure + // agents are not marked as disconnected during slow tests. + AgentInactiveDisconnectTimeout: 5 * time.Second, AccessURL: serverURL, Logger: slogtest.Make(t, nil).Leveled(slog.LevelDebug), Database: db, diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 1c64cffdb976e..38d1efeae43df 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -258,7 +258,7 @@ func (api *API) provisionerJobResources(rw http.ResponseWriter, r *http.Request, } } - apiAgent, err := convertWorkspaceAgent(agent, convertApps(dbApps), api.AgentConnectionUpdateFrequency) + apiAgent, err := convertWorkspaceAgent(agent, convertApps(dbApps), api.AgentInactiveDisconnectTimeout) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: "Internal error reading job agent.", diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index a029d113768f7..0701c21483dfa 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -50,7 +50,7 @@ func (api *API) workspaceAgent(rw http.ResponseWriter, r *http.Request) { }) return } - apiAgent, err := convertWorkspaceAgent(workspaceAgent, convertApps(dbApps), api.AgentConnectionUpdateFrequency) + apiAgent, err := convertWorkspaceAgent(workspaceAgent, convertApps(dbApps), api.AgentInactiveDisconnectTimeout) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: "Internal error reading workspace agent.", @@ -74,7 +74,7 @@ func (api *API) workspaceAgentDial(rw http.ResponseWriter, r *http.Request) { httpapi.ResourceNotFound(rw) return } - apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentConnectionUpdateFrequency) + apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentInactiveDisconnectTimeout) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: "Internal error reading workspace agent.", @@ -121,7 +121,7 @@ func (api *API) workspaceAgentDial(rw http.ResponseWriter, r *http.Request) { func (api *API) workspaceAgentMetadata(rw http.ResponseWriter, r *http.Request) { workspaceAgent := httpmw.WorkspaceAgent(r) - apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentConnectionUpdateFrequency) + apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentInactiveDisconnectTimeout) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: "Internal error reading workspace agent.", @@ -401,7 +401,7 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { httpapi.ResourceNotFound(rw) return } - apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentConnectionUpdateFrequency) + apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentInactiveDisconnectTimeout) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: "Internal error reading workspace agent.", @@ -689,7 +689,7 @@ func inetToNetaddr(inet pqtype.Inet) netaddr.IPPrefix { return ipp } -func convertWorkspaceAgent(dbAgent database.WorkspaceAgent, apps []codersdk.WorkspaceApp, agentUpdateFrequency time.Duration) (codersdk.WorkspaceAgent, error) { +func convertWorkspaceAgent(dbAgent database.WorkspaceAgent, apps []codersdk.WorkspaceApp, agentInactiveDisconnectTimeout time.Duration) (codersdk.WorkspaceAgent, error) { var envs map[string]string if dbAgent.EnvironmentVariables.Valid { err := json.Unmarshal(dbAgent.EnvironmentVariables.RawMessage, &envs) @@ -734,13 +734,13 @@ func convertWorkspaceAgent(dbAgent database.WorkspaceAgent, apps []codersdk.Work // If we've disconnected after our last connection, we know the // agent is no longer connected. workspaceAgent.Status = codersdk.WorkspaceAgentDisconnected - case agentUpdateFrequency*2 >= database.Now().Sub(dbAgent.LastConnectedAt.Time): - // The connection updated it's timestamp within the update frequency. - // We multiply by two to allow for some lag. - workspaceAgent.Status = codersdk.WorkspaceAgentConnected - case database.Now().Sub(dbAgent.LastConnectedAt.Time) > agentUpdateFrequency*2: + case database.Now().Sub(dbAgent.LastConnectedAt.Time) > agentInactiveDisconnectTimeout: // The connection died without updating the last connected. workspaceAgent.Status = codersdk.WorkspaceAgentDisconnected + case dbAgent.LastConnectedAt.Valid: + // The agent should be assumed connected if it's under inactivity timeouts + // and last connected at has been properly set. + workspaceAgent.Status = codersdk.WorkspaceAgentConnected } return workspaceAgent, nil diff --git a/coderd/workspaceresources.go b/coderd/workspaceresources.go index 8f38d4d9dc15a..66dcc89bfb977 100644 --- a/coderd/workspaceresources.go +++ b/coderd/workspaceresources.go @@ -69,7 +69,7 @@ func (api *API) workspaceResource(rw http.ResponseWriter, r *http.Request) { } } - convertedAgent, err := convertWorkspaceAgent(agent, convertApps(dbApps), api.AgentConnectionUpdateFrequency) + convertedAgent, err := convertWorkspaceAgent(agent, convertApps(dbApps), api.AgentInactiveDisconnectTimeout) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: "Internal error reading workspace agent.", From 0ac6cfc2e027df197f84534862c812ee275936ff Mon Sep 17 00:00:00 2001 From: kylecarbs Date: Mon, 27 Jun 2022 16:27:52 +0000 Subject: [PATCH 2/2] fix: Add test for SCP This was hanging due to the stdin pipe never being closed. A test has been added to make sure it works! --- agent/agent.go | 1 + agent/agent_test.go | 15 +++++++++++++++ go.mod | 1 + go.sum | 2 ++ 4 files changed, 19 insertions(+) diff --git a/agent/agent.go b/agent/agent.go index 3bdc46bf64e04..d596511dd1522 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -467,6 +467,7 @@ func (a *agent) handleSSHSession(session ssh.Session) error { } go func() { _, _ = io.Copy(stdinPipe, session) + _ = stdinPipe.Close() }() err = cmd.Start() if err != nil { diff --git a/agent/agent_test.go b/agent/agent_test.go index 612aa93b17034..22ee920c2c8f7 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -16,6 +16,7 @@ import ( "testing" "time" + scp "github.com/bramvdbogaerde/go-scp" "github.com/google/uuid" "github.com/pion/udp" "github.com/pion/webrtc/v3" @@ -149,6 +150,20 @@ func TestAgent(t *testing.T) { require.NoError(t, err) }) + t.Run("SCP", func(t *testing.T) { + t.Parallel() + sshClient, err := setupAgent(t, agent.Metadata{}, 0).SSHClient() + require.NoError(t, err) + scpClient, err := scp.NewClientBySSH(sshClient) + require.NoError(t, err) + tempFile := filepath.Join(t.TempDir(), "scp") + content := "hello world" + err = scpClient.CopyFile(context.Background(), strings.NewReader(content), tempFile, "0755") + require.NoError(t, err) + _, err = os.Stat(tempFile) + require.NoError(t, err) + }) + t.Run("EnvironmentVariables", func(t *testing.T) { t.Parallel() key := "EXAMPLE" diff --git a/go.mod b/go.mod index 52647ad624d7d..20f1c2799a57c 100644 --- a/go.mod +++ b/go.mod @@ -49,6 +49,7 @@ require ( github.com/armon/circbuf v0.0.0-20190214190532-5111143e8da2 github.com/awalterschulze/gographviz v2.0.3+incompatible github.com/bgentry/speakeasy v0.1.0 + github.com/bramvdbogaerde/go-scp v1.2.0 github.com/briandowns/spinner v1.18.1 github.com/charmbracelet/charm v0.12.1 github.com/charmbracelet/lipgloss v0.5.0 diff --git a/go.sum b/go.sum index 615b6c6236f5c..db1a2afa5c2b8 100644 --- a/go.sum +++ b/go.sum @@ -270,6 +270,8 @@ github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dR github.com/bombsimon/wsl/v3 v3.3.0/go.mod h1:st10JtZYLE4D5sC7b8xV4zTKZwAQjCH/Hy2Pm1FNZIc= github.com/bool64/shared v0.1.4 h1:zwtb1dl2QzDa9TJOq2jzDTdb5IPf9XlxTGKN8cySWT0= github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= +github.com/bramvdbogaerde/go-scp v1.2.0 h1:mNF1lCXQ6jQcxCBBuc2g/CQwVy/4QONaoD5Aqg9r+Zg= +github.com/bramvdbogaerde/go-scp v1.2.0/go.mod h1:s4ZldBoRAOgUg8IrRP2Urmq5qqd2yPXQTPshACY8vQ0= github.com/breml/bidichk v0.1.1/go.mod h1:zbfeitpevDUGI7V91Uzzuwrn4Vls8MoBMrwtt78jmso= github.com/bshuster-repo/logrus-logstash-hook v0.4.1/go.mod h1:zsTqEiSzDgAa/8GZR7E1qaXrhYNDKBYy5/dWPTIflbk= github.com/bshuster-repo/logrus-logstash-hook v1.0.0/go.mod h1:zsTqEiSzDgAa/8GZR7E1qaXrhYNDKBYy5/dWPTIflbk=