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

Automated backport of #1521: Add cluster name to aggregated SI status on local SI creation #1528

Conversation

tpantelis
Copy link
Contributor

Backport of #1521 on release-0.17.

#1521: Add cluster name to aggregated SI status on local SI creation

For details on the backport process, see the backport requests page.

...instead of when local EPS is synced to the broker. This can cause
inconsistency if another service instance is unexported on another
cluster simultaneously. The scenario is:

- a service is exported on C2
- the service is then exported on C1. The local SI is created and it's
  observed that the aggregated SI on the broker already exists.
- the service on C2 is unexported and the aggregated SI is deleted b/c
  its cluster status is now empty.
- the local EPS on C1 is synced to the broker. At this point, it tries
  to update the aggregated SI with the cluster info but it no longer exists.

There’s a couple ways to address it.

1) Do create-or-update when merging the local cluster info on EPS creation.
   The downside is that this wouldn’t do the service type conflict checking
   although the possibility that the SI was re-created by another cluster
   with a different service type in that window would be remote.

2) Add the cluster name to the aggregated SI cluster status when created on
   local SI creation. This would’ve prevented C1 from deleting the aggregated
   SI b/c C2's name would’ve been present in the cluster status. I didn’t do
   it this way for consistency so the cluster name and port info are added
   atomically and after the EPS has been successfully exported to ensure it’s
   ready for use if a consumer observes the cluster info present. But this
   isn’t a requirement.

The consensus is #2.

Signed-off-by: Tom Pantelis <[email protected]>
@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr1528/tpantelis/automated-backport-of-#1521-upstream-release-0.17
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@tpantelis tpantelis enabled auto-merge (rebase) March 28, 2024 17:22
@tpantelis tpantelis disabled auto-merge April 2, 2024 11:51
@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Apr 2, 2024
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Apr 2, 2024
@tpantelis tpantelis merged commit b8463dd into submariner-io:release-0.17 Apr 3, 2024
23 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr1528/tpantelis/automated-backport-of-#1521-upstream-release-0.17]

dfarrell07 pushed a commit to submariner-io/submariner-website that referenced this pull request Apr 3, 2024
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Apr 17, 2024
tpantelis added a commit to submariner-io/submariner-website that referenced this pull request Apr 18, 2024
@tpantelis tpantelis deleted the automated-backport-of-#1521-upstream-release-0.17 branch August 19, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants