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

Implement ServiceImport aggregation #1144

Merged
merged 4 commits into from
Mar 27, 2023

Conversation

tpantelis
Copy link
Contributor

@tpantelis tpantelis commented Mar 15, 2023

This PR contains two main commits: the first to implement ServiceImport aggregation in the agent controller and the second to use the aggregated ServiceImports in the CoreDNS plugin. See commits for details.

There's still remaining work described as TODOs in the commits that will be completed in subsequent PRs.

Related to #214
Depends on submariner-io/admiral#571

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr1144/tpantelis/si_aggregate
🚀 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 force-pushed the si_aggregate branch 3 times, most recently from 33358c2 to bf4673f Compare March 22, 2023 19:35
@vthapar
Copy link
Contributor

vthapar commented Mar 23, 2023

Doesn't this depend on submariner-io/admiral#572 too?

@skitt skitt added the ready-to-test When a PR is ready for full E2E testing label Mar 23, 2023
@tpantelis
Copy link
Contributor Author

Doesn't this depend on submariner-io/admiral#572 too?

It didn't really need to but now that's it's been merged I can remove pending from the test.

- A local ServiceImport is still created in the submariner NS
  from the ServiceExport as before to decouple it from the
  broker syncing.

- The local service information is merged into an aggregated ServiceImport
  on the broker in two stages. First, a resource syncer that watches for
  local ServiceImports creates the aggregated ServiceImport with no
  service info using a custom Federator. This will also check for service type
  conflict if the aggregated ServiceImport already exists. On success, the
  Endpoints controller is started.

  The local service information is merged into the aggregated ServiceImport
  after the local EndpointSlice is created and synced to the broker. This is
  done b/c the aggregated port information is determined from the constituent
  clusters' EndpointSlices, thus each cluster must have a consistent view of
  all the EndpointSlices in order for the aggregated port information to be
  eventually consistent.

- The aggregated ServiceImports from the broker are synced to the local
  service NS.

- The local EndpointSlice for non-headless now creates a single Endpoint
  with the service IP. It's Ready condition is set to based on the
  availability of the backend pod IPs.

TODO:

- Merging ports in the aggregated ServiceImport from the constituent
  clusters for non-headless.

- Service type and port conflict detection.

- Migration of legacy per-cluster ServiceImports.

Signed-off-by: Tom Pantelis <[email protected]>
The aggregated ServiceImport is only used to obtain the
headlesness of the service. The rest of the per-cluster info,
ie service IP and ports, is obtained from the EndpointSlices.

For migration, it still processes legacy cluster ServiceImports
and EndpointSlices to support non-upgraded clusters during a
rolling upgrade. For the former, the service IP is used as before
and the latter is only used to determine the health of the endpoints.

Signed-off-by: Tom Pantelis <[email protected]>
@github-actions
Copy link

This PR/issue depends on:

Copy link
Contributor

@vthapar vthapar left a comment

Choose a reason for hiding this comment

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

Looks good, tested locally.

@vthapar vthapar merged commit 48f0861 into submariner-io:devel Mar 27, 2023
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr1144/tpantelis/si_aggregate]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants