Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement gateway class controller in Rust #360
base: main
Are you sure you want to change the base?
Implement gateway class controller in Rust #360
Changes from 1 commit
faf0f5a
47909e4
2d3639f
6fa67f4
a701a87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now I think this will introduce a bug because below in our
controller
function I don't see that we've set up any watch mechanism that will re-enqueue theGateway
once theGatewayClass
is finally accepted. We would need that to make sure thatGatewayClass
changes trigger their connectedGateways
getting enqueued 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just re-enqueue here? Wouldn't that cause the gateway to keep re-enqueuing until it's GatewayClass was accepted?
That's not ideal because it's tantamount to naively long polling for changes to your GatewayClass that may never come versus the GatewayClass change driving the behavior.
Is there a an API for the watch mechanism you describe? If so, I can implement that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still wouldn't be quite right because of the problem you described, and in a case where something is broken it would just endlessly re-enqueue.
Yes, there are ways to watch for changes to related or owned objects, and then enqueue the related object when the watch object changes. Look at the owns() (for when there's specifically an owner relationship) and watches() functions and other related functions in kube::runtime::Controller, there are examples in there as well. I think what we'll probably want to do is run a watch on
GatewayClass
that re-enqueues any relatedGateways
when that class changes whenever it matches ourcontrollerName
🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goal is to make a full clone, you should be able to do that with Deref and clone:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed the aim, unless I can get away with not cloning at all. But we ultimately would need a thread-safe mutable reference (
Arc<Mutex<GatewayClass>>
) to our gateway_class input then, but AFAICT from the kube.rs docs,reconcile
will always be passed anArc<>
of the resource it's watching. Is that so?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will always be an Arc, so if you want the inner of an arc you can do a deref clone (as shown above) OR an
as_ref().clone()
.