Skip to content
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

Changes to make lighthouse MCS API complaint #230

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

vthapar
Copy link
Contributor

@vthapar vthapar commented Aug 27, 2024

Currently Lighthouse implementation of MCS API differs on 2 key points:

  1. There is no VIP allocated for a MultiCluster Service.
  2. EndpointSlices don't carry PodIPs for ClusterIP services.

This Enhancement proposes design changes to support above two in a user configurable way without impacting existing deployments.

@submariner-bot
Copy link
Collaborator

🤖 Created branch: z_pr230/vthapar/mcs-compliance

@vthapar vthapar requested review from aswinsuryan and yboaron August 27, 2024 06:37
@vthapar vthapar force-pushed the mcs-compliance branch 3 times, most recently from 4e09002 to a99f141 Compare August 27, 2024 10:38
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
lighthouse/mcs-compliance.md Outdated Show resolved Hide resolved
Comment on lines 18 to 19
When creating `ServiceExport`, users will have option to add annotation for enabling VIP for that Service and another
to use PodIPs in `EndpointSlices`. These annotations will also be added to the aggregated `ServiceImport`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When creating `ServiceExport`, users will have option to add annotation for enabling VIP for that Service and another
to use PodIPs in `EndpointSlices`. These annotations will also be added to the aggregated `ServiceImport`.
When creating `ServiceExport`, users will have the option to add an annotation for enabling VIP for that Service. This annotation will also be added to the aggregated `ServiceImport`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 21 to 23
Configuration flags will also be added to `Submariner` and `ServiceDiscovery` CRDs, and `subctl` to set these options at
global level. This is for deployments where user only wants to use one specific option and doesn't want to set the
annotation explicitly for each `ServiceExport`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Configuration flags will also be added to `Submariner` and `ServiceDiscovery` CRDs, and `subctl` to set these options at
global level. This is for deployments where user only wants to use one specific option and doesn't want to set the
annotation explicitly for each `ServiceExport`.
A configuration flag will also be added to the `Submariner` and `ServiceDiscovery` CRDs, and `subctl` to set the options at
the global level. This is for deployments where the user wants the option set for all exported Services and doesn't want to set the
annotation explicitly for each `ServiceExport`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 67 to 70
4. If `ServiceImport` exists, compare the combination of annotations on `ServiceExport` and global flags against
annotations in `ServiceImport` for any conflicts.
5. In case of conflict, set `ServiceExportConflict` condition on the `ServiceExport` and attempt conflict resolution.
6. In case conflict can't be resolved, don't export the service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
4. If `ServiceImport` exists, compare the combination of annotations on `ServiceExport` and global flags against
annotations in `ServiceImport` for any conflicts.
5. In case of conflict, set `ServiceExportConflict` condition on the `ServiceExport` and attempt conflict resolution.
6. In case conflict can't be resolved, don't export the service.
4. If a `ServiceImport` exists, compare the annotation on the `ServiceExport` and global flag against
the annotation in `ServiceImport` for any conflicts.
5. In case of conflict, set `ServiceExportConflict` condition on the `ServiceExport` and continue with export.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Currently Lighthouse implementation of MCS API differs on 2 key points:

1. There is no VIP allocated for a MultiCluster Service.
2. EndpointSlices don't carry PodIPs for ClusterIP services.

This Enhancement proposes design changes to support above two in a user
configurable way without impacting existing deployments.

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to vthapar/submariner-operator that referenced this pull request Sep 2, 2024
Add fields to CRDs to support ClustersetIPs

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to vthapar/submariner-operator that referenced this pull request Sep 2, 2024
Add fields to CRDs to support ClustersetIPs

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
4. When Service is exported on subsequent clusters, no VIP allocation is done if VIP already present on `ServiceImport`.
5. VIP is deallocated only when `ServiceImport` is deleted i.e. `Service` or `ServiceExport` are no longer present on any
of the clusters.
6. If Submariner is uninstalled on the cluster that allocated a given VIP, VIP is not changed on `ServiceImport`.
Copy link
Contributor

@yboaron yboaron Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some corner case Qs:

Regarding
If Submariner is uninstalled on the cluster that allocated a given VIP, VIP is not changed on ServiceImport
Don't we delete all serviceimport CRs when submariner is uninstalled ?

Regarding
VIP allocation is done by the first cluster to export the Service.
do we need to implement some lock mechanism to resolve race condition in case two clusters export service at the same time ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - that’s handled by using K8s retry on conflict functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some corner case Qs:

Regarding If Submariner is uninstalled on the cluster that allocated a given VIP, VIP is not changed on ServiceImport Don't we delete all serviceimport CRs when submariner is uninstalled ?
Aggregated ServiceImport will still exist if service is exported on other clusters. Deletion will only happen if this was the last cluster to export service, in which case it will be deleted. Point here is that if the cluster allocating VIP is no longer there [deleted or submariner removed] we don't try to change the VIP as it will impact existing clients.

Do we expect that cluster will get the same VIPs cidr subset after reinstall ?
No. We don't make any assumptions and it is also possible that some other cluster gets that CIDR. We can see this happen with Globalnet.

Regarding VIP allocation is done by the first cluster to export the Service. do we need to implement some lock mechanism to resolve race condition in case two clusters export service at the same time ?
Not needed. As both try to create Aggregated ServiceImport, only one will be able to do so. Other will get conflict with Resource already exists error.

vthapar added a commit to vthapar/submariner-operator that referenced this pull request Sep 3, 2024
Add fields to CRDs to support ClustersetIPs

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
@yboaron yboaron merged commit cafb1e2 into submariner-io:devel Sep 3, 2024
6 checks passed
@submariner-bot
Copy link
Collaborator

🤖 Closed branches: [z_pr230/vthapar/mcs-compliance]

vthapar added a commit to vthapar/submariner-operator that referenced this pull request Sep 4, 2024
Add fields to CRDs to support ClustersetIPs

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to vthapar/submariner-operator that referenced this pull request Sep 6, 2024
Add fields to CRDs to support ClustersetIPs

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to submariner-io/submariner-operator that referenced this pull request Sep 6, 2024
Add fields to CRDs to support ClustersetIPs

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to vthapar/subctl that referenced this pull request Sep 24, 2024
1. Add ClustersetIP flags to deploy-broker and join.
2. Add ClustersetIP CIDRs to config CRs.
3. Show clustersetupCIDR in `show networks` output.
4. Add use-clusterset-ip flag to `service export`

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to vthapar/subctl that referenced this pull request Sep 25, 2024
1. Add ClustersetIP flags to deploy-broker and join.
2. Add ClustersetIP CIDRs to config CRs.
3. Show clustersetupCIDR in `show networks` output.
4. Add use-clusterset-ip flag to `service export`

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to vthapar/subctl that referenced this pull request Sep 25, 2024
1. Add ClustersetIP flags to deploy-broker and join.
2. Add ClustersetIP CIDRs to config CRs.
3. Show clustersetupCIDR in `show networks` output.
4. Add use-clusterset-ip flag to `service export`

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
vthapar added a commit to vthapar/subctl that referenced this pull request Sep 25, 2024
1. Add ClustersetIP flags to deploy-broker and join.
2. Add ClustersetIP CIDRs to config CRs.
3. Show clustersetupCIDR in `show networks` output.
4. Add use-clusterset-ip flag to `service export`

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
tpantelis pushed a commit to vthapar/subctl that referenced this pull request Sep 25, 2024
1. Add ClustersetIP flags to deploy-broker and join.
2. Add ClustersetIP CIDRs to config CRs.
3. Show clustersetupCIDR in `show networks` output.
4. Add use-clusterset-ip flag to `service export`

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
tpantelis pushed a commit to submariner-io/subctl that referenced this pull request Sep 25, 2024
1. Add ClustersetIP flags to deploy-broker and join.
2. Add ClustersetIP CIDRs to config CRs.
3. Show clustersetupCIDR in `show networks` output.
4. Add use-clusterset-ip flag to `service export`

Refer: submariner-io/enhancements#230

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Sep 26, 2024
Enhancement submariner-io/enhancements#230

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Sep 26, 2024
Enhancement submariner-io/enhancements#230

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Oct 7, 2024
Enhancement submariner-io/enhancements#230

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis added a commit to submariner-io/submariner-website that referenced this pull request Oct 7, 2024
Enhancement submariner-io/enhancements#230

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Oct 25, 2024
Enhancement submariner-io/enhancements#230

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
dfarrell07 pushed a commit to submariner-io/submariner-website that referenced this pull request Oct 25, 2024
Enhancement submariner-io/enhancements#230

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants