Prevent kafka controller from running into NodePort service deletion and re-creation cycles indefinitely #928
Conversation
…and re-creation cycles indefinitely
|
Found another minor issue during development - if the external listeners were set to Will open another PR to address this issue since it's not part of #922 |
There was a problem hiding this comment.
Don't be discouraged by the comments, as those are just recommendations and optional stuff, I would be okay approving the PR as is, I just feel like we could still add some worthwhile improvements at relatively low cost.
Also I don't keep my suggestions in me, rather write it out even if it is just a personal practice (some optionals), but if you disagree with anything - that is not arequired change of which there is non right now - I'm not attached to these and am ready to drop those.
…oud/koperator into deletion/non-headless/svc
…oud/koperator into deletion/non-headless/svc
|
|
||
| // if NodePort is used for any of the external listeners, the corresponding services need to remain | ||
| // so that clients from outside the Kubernetes cluster can reach the brokers | ||
| filteredSvcsToDelete := services |
There was a problem hiding this comment.
I think we dont have to use another variable for this.
we can use the services all way long.
later: services = nonNodePortServices(services)
...
There was a problem hiding this comment.
yea...but having this little extra variable named filteredSvcsToDelete would make the implementation more readable
There was a problem hiding this comment.
It is clear enough to me services = nonNodePortServices(services).
It is just my opinion and a suggestion. Your solution is ok for me.
There was a problem hiding this comment.
We can also use here (
) name like: filteredSvcsToDeleteand filteredSvcsToDelete = nonNodePortBrokerServices(filteredSvcsToDelete).
This
"services" already filtered by label selector so it is equal filteredSvcsToDelete
There was a problem hiding this comment.
"services" already filtered by label selector so it is equal filteredSvcsToDelete
I guess I have a slightly different interpretation - we use the label selectors to query all of the services that match the labels (and they are stored in the services variable), then we filter them based on the external listener configuration, which results in filteredSvcsToDelete and these are the services that we will actually remove at the end
Using a different variable with a more declarative name would make it more understandable to people that are not familiar with the existing codebase IMO - so I' d prefer to keep it as it is
…and re-creation cycles indefinitely (#928) * Prevent kafka controller from running into NodePort service deletion and re-creation cycles indefinitely * Rename variable * Refactor existing implementation to improve readability * Fix unintended change * Update logs and comments to reduce confusion
What's in this PR?
To fix #922
Why?
When using
NodePortas the service type for external listeners, the corresponding NodePort services get into deletion-recreation cycles indefinitelyAdditional context
Also refactor the relevant logic - moving the headless service related logic from
pkg/resources/kafka/allBrokerService.gotopkg/resources/kafka/headlessService.goalong with additional unit testsChecklist
- [x] User guide and development docs updated (if needed)