clustermesh: refactor MCS-API derived service controller #35039
Conversation
dc0bb1d to
c91dc0c
Compare
|
/test |
|
hey @MrFreezeex I was a bit short on cycles the last two days, I will get to the review of this PR early next week. |
So hmm this derived service controller was supposed to create the derived service if there is either a ServiceExport or a ServiceImport (and adding the owner referrence to both if present at the last reconcile time) but in hindsight I am not entirely sure this a great idea anymore as it complexify the logic for not much and setting the shared annotation only if there is a ServiceExport as we currently do is not correct/not in line with what the other controllers are doing :/. Sooo let me rework a bit the logic here first. Thanks for the question prompting me to dig more into this :D. |
1fb7ded to
6316d50
Compare
596680e to
6ebff1e
Compare
|
/test |
|
cc @giorio94 in case you would like to review this as well |
a443702 to
397ad2d
Compare
This commit refactors the MCS-API Service controller to only create the derived Services if the ServiceImport already exists. Before this commit we had this notion of creating the derived Service if either the ServiceExport or the ServiceImport was created and depending if the ServiceExport is present marks the Service as shared. The aim was that the local Service endpoint would be accessible but not shared until the ServiceExport was created. Also the initial design of how exported Services info was shared in remote cluster was relying on adding those fields to `serviceStore.ClusterService` which required to always create the derived Service. However those assumptions does not make a lot of sense now that we have the full picture with the other controllers which rely on a specific MCS-API data structure different from `serviceStore.ClusterService`. Also this not in line with the fact that the derived Service should globally have the same behavior as we were including unexported local service endpoints. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
397ad2d to
1c64884
Compare
There was a problem hiding this comment.
Thanks for updating, as Marco already approved, feel free to run tests and it will get merged.
Now it looks more in line of what I was assuming I should see, although I am still missing how all of that fits into the bigger picture. Let's take it offline, I would love to understand it a bit better for the future :) I will start a thread on Slack.
|
/test |
|
/ci-e2e-upgrade |
|
/ci-ginkgo |
This commit refactors the MCS-API Service controller to only create the derived Services if the ServiceImport already exists.
Before this commit we had this notion of creating the derived Service if either the ServiceExport or the ServiceImport was created and depending if the ServiceExport is present marks the Service as shared. The aim was that the local Service endpoint would be accessible but not shared until the ServiceExport was created. Also the initial design of how exported Services info was shared in remote cluster was relying on adding those fields to
serviceStore.ClusterServicewhich required to always createthe derived Service.
However those assumptions does not make a lot of sense now that we have the full picture with the other controllers which rely on a specific MCS-API data structure different from
serviceStore.ClusterService. Also this not in line with the fact that the derived Service should globally have the same behavior as we were including unexported local service endpoints.