Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Restricting Monitoring to a Single Cluster #160

Merged
merged 10 commits into from
Apr 27, 2023

Conversation

triarius
Copy link
Contributor

@triarius triarius commented Apr 23, 2023

Previously, the controller would poll the GraphQL API for all jobs in an organization that matched certain tags. However, if the controller were only equipped with a Cluster token, the buildkite agent in the resulting pods would not be able to acquire the Buildkite Job from a different (or no) cluster.

In this PR, we change the GraphQL query to either restrict jobs to a configured Cluster UUID, or if no Cluster UUID is in the config, to jobs NOT belonging to a cluster.

There is also some improvements to how the GraphQL client is generated. In particular, there is a recipe for downloading the schema file that may be used by those with access to the buildkite repositories.

@@ -1,7 +1,5 @@
package api

//go:generate go run github.com/Khan/genqlient

Copy link
Contributor Author

@triarius triarius Apr 23, 2023

Choose a reason for hiding this comment

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

Moved to api/client.go as it is what uses the generated client.

@triarius triarius requested a review from a team April 23, 2023 13:05
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from the bk/bk source code. I have no idea why it is so different. The diff only needs to be the new arguments to the job field, allowing filtering by cluster.

@triarius triarius changed the title Support Restriting Monitoring to a Single Cluster Support Restricting Monitoring to a Single Cluster Apr 24, 2023
Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

Looking good!

internal/monitor/monitor.go Outdated Show resolved Hide resolved
for _, job := range jobsResp.Organization.Jobs.Edges {
jobs = append(jobs, job.Node.(*api.JobJobTypeCommand))
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

[Optional] Probably worth considering dividing the the method into two sub-methods, one for each of the branches.

Copy link
Contributor Author

@triarius triarius Apr 25, 2023

Choose a reason for hiding this comment

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

I think this is a symptom of how the generated methods return 2 struct types, GetScheduledJobsReponse and GetScheduledJobsClusteredResponse that have the same structure and need to be handled identically, but the type system won't let us write only one block of code to handle both.

I decided to rectify this using an interface to abstract over the parts of the structs we need. There's still a bit of duplication, but it is limited to identical implementations of the interface methods that only do "one thing". If go had a more expressive syntax, both of these methods could be one-liners. In any case, this should be earlier to maintain that larger duplicated blocks that do multiple things.

@triarius triarius force-pushed the pdp-825-query-by-cluster-in-the-controllers branch from df0db7f to fded53f Compare April 25, 2023 06:56
@triarius triarius force-pushed the pdp-825-query-by-cluster-in-the-controllers branch from 0f62938 to 188931c Compare April 25, 2023 22:43
@triarius triarius force-pushed the pdp-825-query-by-cluster-in-the-controllers branch from 188931c to 22c830f Compare April 25, 2023 23:06
Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

Nice! 🍕

@triarius triarius merged commit 276adc6 into main Apr 27, 2023
@triarius triarius deleted the pdp-825-query-by-cluster-in-the-controllers branch April 27, 2023 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants