8000 Filter query: has-agent connecting, connected, disconnected, timeout by mtojek · Pull Request #5145 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

Filter query: has-agent connecting, connected, disconnected, timeout #5145

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 30 commits into from
Nov 24, 2022
Merged

Filter query: has-agent connecting, connected, disconnected, timeout #5145

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ee6577d
WIP
mtojek Nov 21, 2022
7d61519
has-agent:connecting, connected
mtojek Nov 22, 2022
b5a1ecc
Fix
mtojek Nov 22, 2022
dca2b8b
Fix
mtojek Nov 22, 2022
0a4746c
has-agent:disconnected, timeout
mtojek Nov 22, 2022
70952e1
Fix: typo
mtojek Nov 22, 2022
7965a54
Fix
mtojek Nov 22, 2022
c1bd839
TODOs
mtojek Nov 22, 2022
2af4133
databasefake
mtojek Nov 22, 2022
f9e2167
Fix: typo
mtojek Nov 22, 2022
45957c1
More TODOs
mtojek Nov 22, 2022
66892d7
databasefake
mtojek Nov 22, 2022
fc93f90
Timeout tests
mtojek Nov 22, 2022
e587b5d
Address PR comments
mtojek Nov 22, 2022
458a8eb
Implement FIXMEs
mtojek Nov 23, 2022
81d9fef
Renamings
mtojek Nov 23, 2022
5ca8c17
Address PR comments
mtojek Nov 23, 2022
ec2d571
Fix: readability
mtojek Nov 23, 2022
4b2f831
Fix: refactor CASE logic
mtojek Nov 23, 2022
3c0d4fe
CASE logic
mtojek Nov 23, 2022
9067a9e
Fix
mtojek Nov 23, 2022
c14663c
Use CTE
mtojek Nov 23, 2022
9c2f64a
Polishing
mtojek Nov 23, 2022
44bf343
Comment
mtojek Nov 24, 2022
598b140
WIP
mtojek Nov 24, 2022
f3787d1
Merge branch 'main' into 4679-surface-agent
mtojek Nov 24, 2022
ba13e03
IS NOT NULL
mtojek Nov 24, 2022
d74b072
Without CTE
mtojek Nov 24, 2022
4255448
One more optimization
mtojek Nov 24, 2022
4ace659
2nd optimization
mtojek Nov 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address PR comments
  • Loading branch information
@mtojek
mtojek committed Nov 22, 2022
commit e587b5d555d2a3b40930838f110d8375f5676013
8000 30 changes: 15 additions & 15 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 15 additions & 15 deletions coderd/database/queries/workspaces.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ FROM
workspaces
LEFT JOIN LATERAL (
SELECT
build_number,
workspace_builds.job_id,
build_number,
workspace_builds.job_id,
workspace_builds.transition,
provisioner_jobs.started_at,
provisioner_jobs.updated_at,
Expand All @@ -64,13 +64,13 @@ LEFT JOIN LATERAL (
ON
provisioner_jobs.id = workspace_builds.job_id
LEFT JOIN
workspace_resources
workspace_resources
ON
workspace_resources.job_id = provisioner_jobs.id
workspace_resources.job_id = provisioner_jobs.id
LEFT JOIN
workspace_agents
workspace_agents
ON
workspace_agents.resource_id = workspace_resources.id
workspace_agents.resource_id = workspace_resources.id
WHERE
workspace_builds.workspace_id = workspaces.id
ORDER BY
Expand Down Expand Up @@ -163,7 +163,7 @@ WHERE
-- Use the organization filter to restrict to 1 org if needed.
AND CASE
WHEN @template_name :: text != '' THEN
template_id = ANY(SELECT id FROM templates WHERE lower(name) = lower(@template_name) AND deleted = false)
template_id = ANY(SELECT id FROM templates WHERE lower(name) = lower(@template_name) AND deleted = false)
ELSE true
END
-- Filter by template_ids
Expand All @@ -187,7 +187,7 @@ WHERE
WHEN @has_agent = 'timeout' THEN
latest_build.first_connected_at IS NULL AND (latest_build.created_at + latest_build.connection_timeout_seconds * INTERVAL '1 second' < NOW())
WHEN @has_agent = 'connecting' THEN
latest_build.first_connected_at IS NULL
latest_build.first_connected_at IS NULL
WHEN @has_agent = 'disconnected' THEN
(
latest_build.disconnected_at IS NOT NULL AND
Expand All @@ -197,22 +197,22 @@ WHERE
latest_build.last_connected_at + 6 * INTERVAL '1 second' < NOW() -- FIXME agentInactiveDisconnectTimeout = 6
)
WHEN @has_agent = 'connected' THEN
latest_build.last_connected_at IS NOT NULL
latest_build.last_connected_at IS NOT NULL
Copy link
Member

Choose a reason for hiding this comment

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

Should this case also verify that the state isn't actually disconnected either via timeout or disonnected_at?

Copy link
Member Author

Choose a reason for hiding this comment

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

I extended the condition.

Frankly speaking,, I'm thinking about refactoring that part of the code and creating an extra "runtime" column for agent status. I'm not quite sure if it's possible to use SQL clauses to reflect this logic. I really wouldn't like to create functions...

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that you can take a look at the CASE logic implementation one more time as I've rewritten it to look similar to the Go code.

Copy link
Member

Choose a reason for hiding this comment

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

@mtojek I think the case logic is exactly what you want. But you would have to add that case logic to all reads. So either remove all GetWorkspaceAgentsByXXX and make 1 with a filter (like workspaces/template/etc). Or you could also make a view. I looked into this before with sqlc in an issue here:

#2201

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @Emyrk, thanks for jumping in!

I had a chat about this issue with @mafredri to try WITH expression first. I guess that I can transform it into a VIEW if there are performance/clean-sql concerns. Otherwise, I am happy to do this in a follow-up.

Copy link
Member
@Emyrk Emyrk Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The only reason I mentioned a view is because sqlc creates our model types from the sql.

So you have GetWorkspaceAgentByAuthToken, GetWorkspaceAgentByID, GetWorkspaceAgentByInstanceID, GetWorkspaceAgentsByResourceIDs, GetWorkspaceAgentsCreatedAfter. If you update 1 to select this extra "status" column dynamically, sqlc creates a new model type for this. It will not be type WorkspaceAgent. I think it has the name of the function, eg GetWorkspaceAgentByXXX (iirc?). So you will need to add this new column to all the calls, and I think each call gets a unique model type. A way around this is to consolidate all these calls too.

A "view" is a way to repackage the extra dynamic column in a way sqlc still sees it as a single type. So all the above queries go from ... FROM workspace_agents ... to ... FROM workspace_agents_view .... The sqlc type will be type WorkspaceAgentsView.

So tl;dr the view is just a way to keep the sqlc clean. You'll see when you run a make gen with a dynamic column (SELECT *, "unknown" as status FROM workspace_agents).

Copy link
Member Author

Choose a reason for hiding this comment

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

So either remove all GetWorkspaceAgentsByXXX and make 1 with a filter (like workspaces/template/etc)

Fortunately, at moment we only need it to search workspaces using filtering. I guess that we don't need to hurry up and inject the case logic everywhere.

The only reason I mentioned a view is because sqlc creates our model types from the sql.

In terms of clean code and the benefits of sqlc, I totally agree with you. I admit that I wouldn't like to get stuck in this pull request with too many changes and I'm wondering if I can split it into multiple PRs.

Follow-up ideas:

  1. Replace the WITH expression with a view. Adjust the rest of the codebase to use it.
  2. Make agent status dynamically set in SQL. Remove the agent status logic from Go code - we don't need it in two places, except for databasefake.

Frankly speaking, if there aren't big performance or clean code concerns around this PR, I would leave it as is, and focus on improvements once the requested feature is delivered.

Let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Emyrk @mafredri After further research I noticed a possible blocker for a view. The CASE logic
requires agent_inactive_disconnect_timeout_seconds, which is a coderd option, and it's currently provided via query.

Unless we can change it to a static value, then I suppose we can't use views here.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point @mtojek, it definitely hampers the use case. Ideally this value would come from either the terraform provider or be stored in the database by some other means, at which point it can be part of the query.

I don't find it unthinkable that we would serialize certain coder server runtime properties in the database, either as a temp table or one that is (re)written on startup. The inactivity seconds could then be stored there and joined.

ELSE true
END
ELSE true
END
-- Authorize Filter clause will be injected below in GetAuthorizedWorkspaces
-- @authorize_filter
ORDER BY
last_used_at DESC
last_used_at DESC
LIMIT
CASE
WHEN @limit_ :: integer > 0 THEN
@limit_
END
CASE
WHEN @limit_ :: integer > 0 THEN
@limit_
END
OFFSET
@offset_
@offset_
;

-- name: GetWorkspaceByOwnerIDAndName :one
Expand Down
4 changes: 1 addition & 3 deletions coderd/workspaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,9 +963,7 @@ func TestWorkspaceFilterManual(t *testing.T) {
workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{
FilterQuery: fmt.Sprintf("has-agent:%s", "timeout"),
})
if !assert.NoError(t, err) {
return false
}
require.NoError(t, err)
return workspaces.Count == 1
}, testutil.IntervalMedium, "agent status timeout")
})
Expand Down
0