-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 watch to in-memory server multiplexer #8851
🌱 Add watch to in-memory server multiplexer #8851
Conversation
1d2b2eb
to
cc5f262
Compare
a32a0c0
to
dd89113
Compare
5547a4c
to
dfac09e
Compare
/retitle 🌱 Add watch to in-memory server multiplexer |
dfac09e
to
4e7372a
Compare
62c4acd
to
e9cf72f
Compare
/retest |
test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller_test.go
Show resolved
Hide resolved
select { | ||
case <-ctx.Done(): | ||
return | ||
case <-timeoutTimer.C: | ||
return |
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.
Q: Why do we need a context and a timer to timeout?
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.
I'm not 100 percent on this - need to test it more. But this is what is done in the K8s APIServer. I haven't tested the timeouts on this properly - as they were being interfered with by the client timeout (and still are for our actual workload clusters). Would like to take it as a follow up though.
Hm interesting. Unit tests failed, maybe because of the change in the deletion logic. (before the rebase unit tests of the in-memory provider were not run) |
Yeah - mentioned here: #8851 (comment) |
a019df8
to
e79e813
Compare
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
e79e813
to
69a2ced
Compare
/lgtm Let's merge this. I think it's a huge step forward as it gives us functioning watches. Let's have a follow-up iteration for the smaller improvements suggested above |
LGTM label has been added. Git tree hash: d91153493ef4521e80e9734ae16f677ef9ba2005
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
/area provider/infrastructure-in-memory
Add watch to the CAPI in-memory provider's API server. This will allow the in-memory apiserver to deliver watch events when CAPI is watching resources like nodes on the workload cluster.
Part of #8814