NotificationsYou must be signed in to change notification settings
Fork905
Star10k

feat(agent/agentcontainers): update containers periodically#17972

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

mafredri merged 11 commits intomainfrommafredri/wip

May 22, 2025

Merged

feat(agent/agentcontainers): update containers periodically#17972

mafredri merged 11 commits intomainfrommafredri/wip

May 22, 2025

Conversation

Copy link

Member

mafredri commentedMay 21, 2025•
edited
Loading

This change introduces a significant refactor to the agentcontainers API
and enables periodic updates of Docker containers rather than on-demand.
Consequently this change also allows us to move away from using a
locking channel and replace it with a mutex, which simplifies usage.

Additionally a previous oversight was fixed, and testing added, to clear
devcontainer running/dirty status when the container has been removed.

Updates#16424
Updatescoder/internal#621

github-actionsbot assignedmafredri

May 21, 2025

mafredri force-pushed themafredri/wip branch 4 times, most recently from84e7fb1 to25ead7bCompare

May 21, 2025 15:51

mafredri requested a review fromCopilot

May 21, 2025 15:52

This comment was marked as resolved.

feat(agent/agentcontainers): update containers periodically

0b002ec

This change introduces a significant refactor to the agentcontainers APIand enabled periodic updates of Docker containers rather than on-demand.Consequently this change also allows us to move away from using alocking channel and replace it with a mutex, which simplifies usage.Additionally a previous oversight was fixed, and testing added, to cleardevcontainer running/dirty status when the container has been removed.Updates#16424Updatescoder/internal#621

mafredri force-pushed themafredri/wip branch from25ead7b to0b002ecCompare

May 21, 2025 15:54

mafredri marked this pull request as ready for review

May 21, 2025 16:02

mafredri requested review fromjohnstcn andDanielleMaywood

May 21, 2025 16:02

mafredri added2 commits

May 21, 2025 16:06

try to fix potential race in test

58b65a1

fix bug in marking container as dirty

3cd9cc1

mafredri force-pushed themafredri/wip branch fromcb28d90 to3cd9cc1Compare

May 21, 2025 16:12

mafredri added2 commits

May 21, 2025 16:15

make sure watcher is synced

1aa6a48

improve readability of markDevcontainerDirty

0122e43

johnstcn reviewed

May 21, 2025

View reviewed changes

agent/agentcontainers/api.go Outdated

Comment on lines 264 to 266

		defer func() { sema <- struct{}{} }()

		return api.updateContainers(api.ctx)

Copy link

Member

johnstcnMay 21, 2025

Choose a reason for hiding this comment

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

Would it make sense instead to just return early fromdoUpdate() if we're not able to read fromsema? Queueing multiple updates seems like it could cause increased memory usage over time, and I'm not sure what value we get from running it right again after one instance has just completed.

Copy link

MemberAuthor

mafredriMay 22, 2025

Choose a reason for hiding this comment

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

If we did that, we can't wait for it to complete in therefreshContainers function. I'll see if there's any point in keeping a refresh method around after I refactor the recreate endpoint.