Recreate listener Service when deleted#2387
Conversation
AryanP123
requested review from
c-kruse,
fgiorgetti and
nluaces
as code owners
|
|
||
| func (s *Site) EnsureListenerService(listener *skupperv2alpha1.Listener) { | ||
| if listener != nil && s.site != nil { | ||
| s.bindings.ListenerUpdated(listener) |
There was a problem hiding this comment.
Calling ListenerUpdated results in query to api server (via Expose) which would be good to avoid if possible.
Suggest looking at updating CheckListenerService to handle the delete case (e.g. svc == nil) and possibly use isHostExposed to distinguish deletion due to listener deletion versus user deletion.
The question will be whether there is enough context available to re-create the user deleted service (E.g. could isHostExposed return ports, etc. If not, maybe storing what is needed in a site scoped cm that tracks the service lifecycle might be an alternative.
There was a problem hiding this comment.
The HandleDeletedListenerService seems to be doing what is needed.
Is EnsureListenerService really needed?
There was a problem hiding this comment.
Ah, you're right. Didn't notice the redundancy, will fix. Thanks!
AryanP123
force-pushed
the
delete-service
branch
2 times, most recently
from
92edba4 to
6c45e69
Compare
| ctrl.eventProcessor.TestProcess() | ||
| } | ||
| deleteListenerService("mysvc", "test")(t, clients) | ||
| for !waitForService("mysvc", "test")(t, clients) { |
There was a problem hiding this comment.
Better to use a retry mechanism here (i.e.: internal/utils/retry.go).
| if existing, ok := a.exposed[listener.Spec.Host]; ok { | ||
| exposed = existing | ||
| } | ||
| if exposed := a.exposed.Expose(listener.Spec.Host, port); exposed != nil { |
There was a problem hiding this comment.
The original code was only going to call context.Expose if a change is needed,
now it seems to be calling it either way. I see that the context.Expose has mechanisms
to detect if something has changed or not, but once it is called it tries to retrieve the latest
service from the kube api.
Did you find a reason to change this (sorry if it is obvious but I am not seeing it)?
There was a problem hiding this comment.
So the only effect of the change is to recreate the Service earlier in the case where a Listener event is processed before the Service delete event, mainly added it as a defensive path during implementation. It does not add a path that’s required for correctness, the delete path (HandleDeletedListenerService) already guarantees recreation. Looking at it again, I do suppose this change here is unnecessary, so I can revert this. Thanks for pointing it out.
AryanP123
force-pushed
the
delete-service
branch
from
6c45e69 to
4b47524
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
3 participants
Fixes #2383