MCS API changes: does implementers use ServiceImport spec as desired state?

[MrFreezeex]

This issue is here to collect info about whether or not implementations use ServiceImport spec as a desired state.

This is part of the effort to update the current ServiceImport API before ultimately graduating MCS-API to v1beta1.

The plan is to get info first from implementations active in the SIG (or any other implementations that would see this and willing to answer ofc).

More context are available in this presentation about the context and the CIlium POV: https://docs.google.com/presentation/d/12A3i8OdJdpsHu4b_IenWgldsgYSnUGkfzLJVl93b9tY

Copying the last slide explaining the next step here for clarity:

- Does the current Spec (minus the IPs?) can be considered a desired state for your implementation?
  - Needs clear answer from at least Submariner, GKE and Azure/fleet (any other implementations are ofc VERY welcomed to also participate)
- If mostly yes, let’s discuss opinions and vote if there’s no clear majority
- If mostly no, let’s merge everything in the root
- Discuss if those change should go in v1alpha1 directly or v1alpha2 and timeline for v1beta1
- Release the API change
- Promote to v1beta1!

This issue is to address the first step, which is getting info from implementations if they can view in a way the ServiceImport spec as desired state and potentially a few sentence explaining why (see the presentation above for more context about what we do in Cilium).

Minimum input needed for this discussion from:

• Cilium
• Azure/Fleet
• Submariner
• GKE

Other implementations that we would love to have input if you happen to drop by:

• Antrea (tried to analyze on my side, but if a maintainer drop by feel free to correct the assumptions here)
• Karmada
• Alibaba
• Any implementation that we don't know about yet/or is still in the process of implementing MCS-API!

[MrFreezeex]

This is explained in way more details in the slides linked in the first post but to start of and give a quick example of what type of answer is expected I will give Cilium POV here too:

In Cilium, we use all the ServiceImport spec but the IPs field as desired state. We have two separate controllers involved here, one that create the ServiceImport (without the IPs) and the other one that act on the ServiceImport spec to create a underlying Service and then report back the IPs to the ServiceImport. In that sense, the current ServiceImport spec (but the IPs) can be viewed as a desired state for the first controller and applied by the second one with the IPs being the actual state reported back to the ServiceImport.

[MrFreezeex]

I was checking Antrea code and if I were to try to answer for them on the above question it's yes and no...

The code that handle this is here https://github.com/antrea-io/antrea/blob/e41b66dcd16f4722dcb0015b4d338b3268385e4f/multicluster/controllers/multicluster/member/resourceimport_controller.go#L142 and it seems that Antrea is creating some kind of derived Service and that Spec of ServiceImport and Service are similar like Cilium, so in that sense yes it could be considered a desired state. The thing that put Antrea in a more grey area for this question specifically is that the logic for all of these things are done in the the same controller (and even function) so it's less clear to say that this is actually a desired state as updates to ServiceImport and its "derived" Service are actually done in bulk.

[zhiying-lin]

Yes or not, from the Azure,

Controller resolves the conflicts and generates the serviceImport (in the hub cluster) by looking at all the serviceExports on the members. From this perspective, it's more like a status.

On the other side, another controller creates the derived service according to the serviceImport. From this perspective, it can be treated as a desired state.

@circy9 mentioned that "Azure allows manual creation of ServiceImport because clusters can be asymmetric". To make it more accurate, we allow cx to specify where to import the service by using another API, not the serviceImport. The serviceImport are always generated by our controller, not by human.

[skitt]

@zhiying-lin to clarify, would collapsing everything (no spec/status distinction) make some of your use cases impossible, or would it only involve refactoring to reference different fields? In other words, do the controllers that touch ServiceImport in Azure require a distinction between spec and status?

[zhiying-lin]

Putting into the root works for us. Comparing moving to root with only move IP into the status, personally i prefer to putting into the root, as moving only IP into the status would be weird from the API perspective. All other fields are also derived from the serviceExports, which is not spec.

[skitt]

@tpantelis or @vthapar, could you give the Submariner viewpoint?

[vthapar]

Submariner does't use Spec as desired state. All spec fields are derived from Service and ServiceExport.

[MrFreezeex]

Submariner does't use Spec as desired state. All spec fields are derived from Service and ServiceExport.

@vthapar Could you give more details, "All spec fields are derived from Service and ServiceExport" is basically what we are all doing 😅... For instance is there multiple controllers involved that update your ServiceImport resources? If yes what is each controller responsability in term of what they update/observe from the ServiceImport resource.

Or do you have one controller that add create/update every field from all the ServiceImport (spec/status including IPs and stuff like that) in basically one reconciliation?

[vthapar]

Sorry, should've been more clear. When I meant with "derived from Service and ServiceExport" is that those two are the source of "desired state". ServiceImport is created/updated by a single controller. Creation by controller in the cluster where Service is first exported, then similar controllers in other clusters update the Status.Clusters on creation of ServiceExport. So post creation there are no state changes other than updating clusters where service gets Exported/UnExported later.

So depending on what you mean by "one controller", answer can vary. We have controller running in each cluster but they're logically same.

[MrFreezeex]

Hi @lauralorenz @JeremyOT kind reminder about this 🙏

[lauralorenz]

I've (finally) initiated a conversation with the current product team for GKE MCS and will defer to them, but reiterating my opinion from the prior discussions and with the perspective of a prior GKE MCS dev:

• the ips field in ServiceImport is not considered desired state since IPAM is (for GKE, and I think generally,) not user defined or hinted
• I prefer ServiceImport to be composed of fields in its root since I perceive the whole thing as basically a cross-cluster "extension" of a ServiceExport, like the ServiceExport's non-local status stanza

Ultimately I don't really feel very strongly as to the non-ips fields in ServiceImport beyond that general feeling. As a matter of implementation detail in GKE the ServiceImport copy of the data also does not act as desired state as it is forwarded through the same controller which is simultaneously responsible for constructing the dummy services -- said another way, unlike Cillium, there isn't a handoff between controllers at that data storage point for GKE MCS.

cc @bowei

[lauralorenz]

Cross-referencing here that there is simultaneously work to add a status.conditions field to ServiceImport in https://github.com/kubernetes/enhancements/pull/5439/files which, if it's intended to work anything like the "typical" status property "condition" (ref in the API conventions guide), standardized tooling may expect it to be in the status stanza (cc @bowei)

[lauralorenz]

Triage notes:

• we have the information from all the implementations we were most interested in regarding this question
• we are going to use our brain energy to finalize https://github.com/kubernetes/enhancements/pull/5439/files first
• then we need to interpret the information we have from this discussion to decide how to move forward with "the effort to update the current ServiceImport API before ultimately graduating MCS-API to v1beta1"

[lauralorenz]

Triage notes:

• SIG-MC as a community is more open to adding new things (like https://github.com/kubernetes/enhancements/pull/5439/files) than deprecating old things per the larger API conventions in kubernetes
• we still can't decide if this is worth it anyways
• in addition the variations available to us right now are
  • Merge to root
  • Move the IPs to status
  • Do nothing
• Work for implementers; work for sig-mc owned or adjacent releases like CoreDNS and Gateway API; changing things on users is bad until it's good and there's a bad time between that; tl;dr "churn"
• Open questions: potential implications for Karmada? Do they have a comment on the OP of this discussion specifically?
• Conversation about how minimizing the writes to the API server when there is not a status AND a spec to write could be a consideration. However based on the architectures we observe and expect this may be negligable.