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

Add transport implementation for agent #734

Merged
merged 1 commit into from
Jun 24, 2023

Conversation

chrisdoherty4
Copy link
Member

@chrisdoherty4 chrisdoherty4 commented May 15, 2023

Builds on #730.

The new agent requires a transport to retrieve workflows. The initial transport to be implemented is gRPC as its worked well for the tink-worker (the agents predecessor).

The transport is responsible for connecting to the Tink Server, identifying the node by submitting the nodes unique identifier and subsequently passing along workflows. Unlike the Tink Worker implementation, this transport provides a mechanism for publishing workflow events. The events are what provides insight into the progression of the workflow and are ultimately used by the Tink Server to update the workflow object in Kubernetes.

@chrisdoherty4 chrisdoherty4 force-pushed the feature/transport branch 2 times, most recently from 1e575b5 to 77e83f2 Compare May 17, 2023 13:32
@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #734 (6a593c7) into main (29052da) will decrease coverage by 4.41%.
The diff coverage is 38.49%.

❗ Current head 6a593c7 differs from pull request most recent head d857726. Consider uploading reports for the commit d857726 to get more accurate results

@@            Coverage Diff             @@
##             main     #734      +/-   ##
==========================================
- Coverage   56.31%   51.90%   -4.41%     
==========================================
  Files          28       32       +4     
  Lines        1243     1441     +198     
==========================================
+ Hits          700      748      +48     
- Misses        511      650     +139     
- Partials       32       43      +11     
Impacted Files Coverage Δ
internal/agent/mock.go 24.44% <0.00%> (ø)
internal/agent/transport/fake.go 0.00% <0.00%> (ø)
internal/agent/transport/grpc.go 23.85% <23.85%> (ø)
internal/agent/agent.go 62.96% <47.22%> (-29.04%) ⬇️
internal/agent/transport/mock.go 52.38% <52.38%> (ø)
internal/agent/run.go 83.67% <73.33%> (-16.33%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@chrisdoherty4 chrisdoherty4 force-pushed the feature/transport branch 5 times, most recently from 4e40e68 to fb1d172 Compare June 21, 2023 13:41
@chrisdoherty4 chrisdoherty4 marked this pull request as ready for review June 21, 2023 13:41
service WorkflowService {
// Stream creates a stream that will receive workflows intended for the agent identified
// by the StreamWorkflowsRequest.agent_id.
rpc StreamWorkflows(StreamWorkflowsRequest) returns (stream StreamWorkflowsResponse) {}
Copy link
Member

Choose a reason for hiding this comment

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

thought this was interesting. Its from the Buf website. https://buf.build/docs/best-practices/style-guide#recommendations

"While controversial, our recommendation is to avoid streaming RPCs. While they certainly have specific use cases that make them extremely valuable, on the whole they generally cause a lot of problems, push RPC framework logic up the stack, and usually prevent developing more reliable architectures."

Copy link
Member

Choose a reason for hiding this comment

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

Also, would you me open to a name change here? maybe StreamWorkflows -> GetWorkflows

Copy link
Member Author

@chrisdoherty4 chrisdoherty4 Jun 23, 2023

Choose a reason for hiding this comment

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

thought this was interesting. Its from the Buf website. buf.build/docs/best-practices/style-guide#recommendations

I'm familiar with this but chose to ignore it because the alternatives I thought about have tradeoffs I wanted to avoid right now. The options I came up with include: (1) polling from the agent to the server for workflows; (2) launching a server in the agent and have the tink-server (now a confusing name) issue unary calls to run workflows.

I wasn't fond of polling even with HTTP2s sticky connections and I wanted to, initially at least, avoid the complexity of launching a server and dealing with the agent going offline etc.

I very open to adopting a different approach and welcome more ideas. Long term I'm interested in migrating to the agent containing a server even though its more complex.

Also, would you me open to a name change here? maybe StreamWorkflows -> GetWorkflows

Yes, no problem.

jacobweinstock
jacobweinstock previously approved these changes Jun 23, 2023
Copy link
Member

@jacobweinstock jacobweinstock left a comment

Choose a reason for hiding this comment

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

one loosely held suggestion for a name change. other than that all good. Thanks for all the work on this!

Signed-off-by: Chris Doherty <chris.doherty4@gmail.com>
@chrisdoherty4
Copy link
Member Author

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Jun 24, 2023

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 9a45f7b

@mergify mergify bot merged commit 9a45f7b into tinkerbell:main Jun 24, 2023
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