Skip to content

Commit e706c92

Browse files
aslilacaslilac
committed
Merge branch 'main' into lilac/dynamic-parameters-endpoint
2 parents 20e6e08 + e5ba8b7 commit e706c92

File tree

91 files changed

+3212
-648
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

91 files changed

+3212
-648
lines changed

‎.gitattributes

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# Generated files
22
agent/agentcontainers/acmock/acmock.go linguist-generated=true
33
agent/agentcontainers/dcspec/dcspec_gen.go linguist-generated=true
4+
agent/agentcontainers/testdata/devcontainercli/*/*.log linguist-generated=true
45
coderd/apidoc/docs.go linguist-generated=true
56
docs/reference/api/*.md linguist-generated=true
67
docs/reference/cli/*.md linguist-generated=true

‎.github/actions/setup-tf/action.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,5 @@ runs:
77
- name: Install Terraform
88
uses: hashicorp/setup-terraform@b9cd54a3c349d3f38e8881555d616ced269862dd # v3.1.2
99
with:
10-
terraform_version: 1.11.3
10+
terraform_version: 1.11.4
1111
terraform_wrapper: false

‎.github/workflows/typos.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,6 @@ extend-exclude = [
4242
"site/src/pages/SetupPage/countries.tsx",
4343
"provisioner/terraform/testdata/**",
4444
# notifications' golden files confuse the detector because of quoted-printable encoding
45-
"coderd/notifications/testdata/**"
45+
"coderd/notifications/testdata/**",
46+
"agent/agentcontainers/testdata/devcontainercli/**"
4647
]

‎agent/agent.go

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -229,13 +229,21 @@ type agent struct {
229229
// we track 2 contexts and associated cancel functions: "graceful" which is Done when it is time
230230
// to start gracefully shutting down and "hard" which is Done when it is time to close
231231
// everything down (regardless of whether graceful shutdown completed).
232-
gracefulCtx context.Context
233-
gracefulCancel context.CancelFunc
234-
hardCtx context.Context
235-
hardCancel context.CancelFunc
236-
closeWaitGroup sync.WaitGroup
232+
gracefulCtx context.Context
233+
gracefulCancel context.CancelFunc
234+
hardCtx context.Context
235+
hardCancel context.CancelFunc
236+
237+
// closeMutex protects the following:
237238
closeMutex sync.Mutex
239+
closeWaitGroup sync.WaitGroup
238240
coordDisconnected chan struct{}
241+
closing bool
242+
// note that once the network is set to non-nil, it is never modified, as with the statsReporter. So, routines
243+
// that run after createOrUpdateNetwork and check the networkOK checkpoint do not need to hold the lock to use them.
244+
network *tailnet.Conn
245+
statsReporter *statsReporter
246+
// end fields protected by closeMutex
239247

240248
environmentVariables map[string]string
241249

@@ -259,9 +267,7 @@ type agent struct {
259267
reportConnectionsMu sync.Mutex
260268
reportConnections []*proto.ReportConnectionRequest
261269

262-
network *tailnet.Conn
263-
statsReporter *statsReporter
264-
logSender *agentsdk.LogSender
270+
logSender *agentsdk.LogSender
265271

266272
prometheusRegistry *prometheus.Registry
267273
// metrics are prometheus registered metrics that will be collected and
@@ -274,6 +280,8 @@ type agent struct {
274280
}
275281

276282
func (a *agent) TailnetConn() *tailnet.Conn {
283+
a.closeMutex.Lock()
284+
defer a.closeMutex.Unlock()
277285
return a.network
278286
}
279287

@@ -1205,15 +1213,15 @@ func (a *agent) createOrUpdateNetwork(manifestOK, networkOK *checkpoint) func(co
12051213
}
12061214
a.closeMutex.Lock()
12071215
// Re-check if agent was closed while initializing the network.
1208-
closed := a.isClosed()
1209-
if !closed {
1216+
closing := a.closing
1217+
if !closing {
12101218
a.network = network
12111219
a.statsReporter = newStatsReporter(a.logger, network, a)
12121220
}
12131221
a.closeMutex.Unlock()
1214-
if closed {
1222+
if closing {
12151223
_ = network.Close()
1216-
return xerrors.New("agent is closed")
1224+
return xerrors.New("agent is closing")
12171225
}
12181226
} else {
12191227
// Update the wireguard IPs if the agent ID changed.
@@ -1328,8 +1336,8 @@ func (*agent) wireguardAddresses(agentID uuid.UUID) []netip.Prefix {
13281336
func (a *agent) trackGoroutine(fn func()) error {
13291337
a.closeMutex.Lock()
13301338
defer a.closeMutex.Unlock()
1331-
if a.isClosed() {
1332-
return xerrors.New("track conn goroutine: agent is closed")
1339+
if a.closing {
1340+
return xerrors.New("track conn goroutine: agent is closing")
13331341
}
13341342
a.closeWaitGroup.Add(1)
13351343
go func() {
@@ -1547,7 +1555,7 @@ func (a *agent) runCoordinator(ctx context.Context, tClient tailnetproto.DRPCTai
15471555
func (a *agent) setCoordDisconnected() chan struct{} {
15481556
a.closeMutex.Lock()
15491557
defer a.closeMutex.Unlock()
1550-
if a.isClosed() {
1558+
if a.closing {
15511559
return nil
15521560
}
15531561
disconnected := make(chan struct{})
@@ -1772,7 +1780,10 @@ func (a *agent) HTTPDebug() http.Handler {
17721780

17731781
func (a *agent) Close() error {
17741782
a.closeMutex.Lock()
1775-
defer a.closeMutex.Unlock()
1783+
network := a.network
1784+
coordDisconnected := a.coordDisconnected
1785+
a.closing = true
1786+
a.closeMutex.Unlock()
17761787
if a.isClosed() {
17771788
return nil
17781789
}
@@ -1849,7 +1860,7 @@ lifecycleWaitLoop:
18491860
select {
18501861
case <-a.hardCtx.Done():
18511862
a.logger.Warn(context.Background(), "timed out waiting for Coordinator RPC disconnect")
1852-
case <-a.coordDisconnected:
1863+
case <-coordDisconnected:
18531864
a.logger.Debug(context.Background(), "coordinator RPC disconnected")
18541865
}
18551866

@@ -1860,8 +1871,8 @@ lifecycleWaitLoop:
18601871
}
18611872

18621873
a.hardCancel()
1863-
if a.network != nil {
1864-
_ = a.network.Close()
1874+
if network != nil {
1875+
_ = network.Close()
18651876
}
18661877
a.closeWaitGroup.Wait()
18671878

‎agent/agent_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,54 @@ func TestMain(m *testing.M) {
6868

6969
var sshPorts = []uint16{workspacesdk.AgentSSHPort, workspacesdk.AgentStandardSSHPort}
7070

71+
// TestAgent_CloseWhileStarting is a regression test for https://github.com/coder/coder/issues/17328
72+
func TestAgent_ImmediateClose(t *testing.T) {
73+
t.Parallel()
74+
75+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
76+
defer cancel()
77+
78+
logger := slogtest.Make(t, &slogtest.Options{
79+
// Agent can drop errors when shutting down, and some, like the
80+
// fasthttplistener connection closed error, are unexported.
81+
IgnoreErrors: true,
82+
}).Leveled(slog.LevelDebug)
83+
manifest := agentsdk.Manifest{
84+
AgentID: uuid.New(),
85+
AgentName: "test-agent",
86+
WorkspaceName: "test-workspace",
87+
WorkspaceID: uuid.New(),
88+
}
89+
90+
coordinator := tailnet.NewCoordinator(logger)
91+
t.Cleanup(func() {
92+
_ = coordinator.Close()
93+
})
94+
statsCh := make(chan *proto.Stats, 50)
95+
fs := afero.NewMemMapFs()
96+
client := agenttest.NewClient(t, logger.Named("agenttest"), manifest.AgentID, manifest, statsCh, coordinator)
97+
t.Cleanup(client.Close)
98+
99+
options := agent.Options{
100+
Client: client,
101+
Filesystem: fs,
102+
Logger: logger.Named("agent"),
103+
ReconnectingPTYTimeout: 0,
104+
EnvironmentVariables: map[string]string{},
105+
}
106+
107+
agentUnderTest := agent.New(options)
108+
t.Cleanup(func() {
109+
_ = agentUnderTest.Close()
110+
})
111+
112+
// wait until the agent has connected and is starting to find races in the startup code
113+
_ = testutil.RequireRecvCtx(ctx, t, client.GetStartup())
114+
t.Log("Closing Agent")
115+
err := agentUnderTest.Close()
116+
require.NoError(t, err)
117+
}
118+
71119
// NOTE: These tests only work when your default shell is bash for some reason.
72120

73121
func TestAgent_Stats_SSH(t *testing.T) {

‎agent/agentcontainers/containers.go

Lines changed: 76 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import (
99

1010
"golang.org/x/xerrors"
1111

12+
"github.com/go-chi/chi/v5"
13+
1214
"github.com/coder/coder/v2/coderd/httpapi"
1315
"github.com/coder/coder/v2/codersdk"
1416
"github.com/coder/quartz"
@@ -20,9 +22,10 @@ const (
2022
getContainersTimeout = 5 * time.Second
2123
)
2224

23-
type devcontainersHandler struct {
25+
type Handler struct {
2426
cacheDuration time.Duration
2527
cl Lister
28+
dccli DevcontainerCLI
2629
clock quartz.Clock
2730

2831
// lockCh protects the below fields. We use a channel instead of a mutex so we
@@ -32,20 +35,26 @@ type devcontainersHandler struct {
3235
mtime time.Time
3336
}
3437

35-
// Option is a functional option for devcontainersHandler.
36-
type Option func(*devcontainersHandler)
38+
// Option is a functional option for Handler.
39+
type Option func(*Handler)
3740

3841
// WithLister sets the agentcontainers.Lister implementation to use.
3942
// The default implementation uses the Docker CLI to list containers.
4043
func WithLister(cl Lister) Option {
41-
return func(ch *devcontainersHandler) {
44+
return func(ch *Handler) {
4245
ch.cl = cl
4346
}
4447
}
4548

46-
// New returns a new devcontainersHandler with the given options applied.
47-
func New(options ...Option) http.Handler {
48-
ch := &devcontainersHandler{
49+
func WithDevcontainerCLI(dccli DevcontainerCLI) Option {
50+
return func(ch *Handler) {
51+
ch.dccli = dccli
52+
}
53+
}
54+
55+
// New returns a new Handler with the given options applied.
56+
func New(options ...Option) *Handler {
57+
ch := &Handler{
4958
lockCh: make(chan struct{}, 1),
5059
}
5160
for _, opt := range options {
@@ -54,7 +63,7 @@ func New(options ...Option) http.Handler {
5463
return ch
5564
}
5665

57-
func (ch *devcontainersHandler) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
66+
func (ch *Handler) List(rw http.ResponseWriter, r *http.Request) {
5867
select {
5968
case <-r.Context().Done():
6069
// Client went away.
@@ -80,7 +89,7 @@ func (ch *devcontainersHandler) ServeHTTP(rw http.ResponseWriter, r *http.Reques
8089
}
8190
}
8291

83-
func (ch *devcontainersHandler) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
92+
func (ch *Handler) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
8493
select {
8594
case <-ctx.Done():
8695
return codersdk.WorkspaceAgentListContainersResponse{}, ctx.Err()
@@ -149,3 +158,61 @@ var _ Lister = NoopLister{}
149158
func (NoopLister) List(_ context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
150159
return codersdk.WorkspaceAgentListContainersResponse{}, nil
151160
}
161+
162+
func (ch *Handler) Recreate(w http.ResponseWriter, r *http.Request) {
163+
ctx := r.Context()
164+
id := chi.URLParam(r, "id")
165+
166+
if id == "" {
167+
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
168+
Message: "Missing container ID or name",
169+
Detail: "Container ID or name is required to recreate a devcontainer.",
170+
})
171+
return
172+
}
173+
174+
containers, err := ch.cl.List(ctx)
175+
if err != nil {
176+
httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{
177+
Message: "Could not list containers",
178+
Detail: err.Error(),
179+
})
180+
return
181+
}
182+
183+
containerIdx := slices.IndexFunc(containers.Containers, func(c codersdk.WorkspaceAgentContainer) bool {
184+
return c.Match(id)
185+
})
186+
if containerIdx == -1 {
187+
httpapi.Write(ctx, w, http.StatusNotFound, codersdk.Response{
188+
Message: "Container not found",
189+
Detail: "Container ID or name not found in the list of containers.",
190+
})
191+
return
192+
}
193+
194+
container := containers.Containers[containerIdx]
195+
workspaceFolder := container.Labels[DevcontainerLocalFolderLabel]
196+
configPath := container.Labels[DevcontainerConfigFileLabel]
197+
198+
// Workspace folder is required to recreate a container, we don't verify
199+
// the config path here because it's optional.
200+
if workspaceFolder == "" {
201+
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
202+
Message: "Missing workspace folder label",
203+
Detail: "The workspace folder label is required to recreate a devcontainer.",
204+
})
205+
return
206+
}
207+
208+
_, err = ch.dccli.Up(ctx, workspaceFolder, configPath, WithRemoveExistingContainer())
209+
if err != nil {
210+
httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{
211+
Message: "Could not recreate devcontainer",
212+
Detail: err.Error(),
213+
})
214+
return
215+
}
216+
217+
w.WriteHeader(http.StatusNoContent)
218+
}

‎agent/agentcontainers/containers_internal_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ func TestContainersHandler(t *testing.T) {
277277
ctrl = gomock.NewController(t)
278278
mockLister = acmock.NewMockLister(ctrl)
279279
now = time.Now().UTC()
280-
ch = devcontainersHandler{
280+
ch = Handler{
281281
cacheDuration: tc.cacheDur,
282282
cl: mockLister,
283283
clock: clk,

0 commit comments

Comments
 (0)