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

Automate calico ippools creation by routeagent #2576

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

yboaron
Copy link
Contributor

@yboaron yboaron commented Jun 28, 2023

To make calico compatible with Submariner, it needs to create calico IPPools for the subnets associated with the Pod and Service CIDRs of the remote clusters.

This PR adds a new handler (CalicoIPPool) for the routeagent, the CalicoIPPool handler will be responsible for creating calico IPPools for remote endpoints, running Calico API server is a
prerequisite for automating Calico IPPools creation.

Depends-On : submariner-io/submariner-operator#2713

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr2576/yboaron/calico_ippool
🚀 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.

@dfarrell07 dfarrell07 added the release-note-needed Should be mentioned in the release notes label Jun 28, 2023
pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
Comment on lines 51 to 50
remoteEndpoints map[string]*submV1.Endpoint
isGateway bool
Copy link
Contributor

Choose a reason for hiding this comment

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

We should protect these with a mutex as is done in other handlers (eg ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you still think we should use mutex here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed elsewhere it shouldn't be needed unless mutable fields can be accessed outside the parent's event loop. This appears to be the case with isGateway in the Stop method. To be completely safe, you can use an atomic bool for isGateway.

pkg/routeagent_driver/handlers/calico/ippool_handler.go Outdated Show resolved Hide resolved
@yboaron
Copy link
Contributor Author

yboaron commented Jul 6, 2023

Few observations after checking this PR on OCP with Calico clusters on IBM ROKS,

  • According to Calico recommendation we should use projectcalico.org/v3 to create IPPools and not crd.projectcalico.org/v1 (see [1] )

  • To enable projectcalico.org/v3 API the Calico API server should be installed in the cluster [2]

  • Calico API server not installed on OCP clusters on ROKS

  • After installing Calico API server by following the instructions from [2], Calico IPPools needed for Submariner created by this PR as expected.

I filed [3] issue on Calico project, let's wait for their reply to decide how to proceed with this PR

[1]
projectcalico/calico#6412
[2]
https://docs.tigera.io/calico/latest/operations/install-apiserver#how-to
[3]
projectcalico/calico#7845

@skitt skitt added the ready-to-test When a PR is ready for full E2E testing label Jul 7, 2023
Comment on lines 51 to 50
remoteEndpoints map[string]*submV1.Endpoint
isGateway bool
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed elsewhere it shouldn't be needed unless mutable fields can be accessed outside the parent's event loop. This appears to be the case with isGateway in the Stop method. To be completely safe, you can use an atomic bool for isGateway.

@yboaron
Copy link
Contributor Author

yboaron commented Jul 17, 2023

Few observations after checking this PR on OCP with Calico clusters on IBM ROKS,

  • According to Calico recommendation we should use projectcalico.org/v3 to create IPPools and not crd.projectcalico.org/v1 (see [1] )
  • To enable projectcalico.org/v3 API the Calico API server should be installed in the cluster [2]
  • Calico API server not installed on OCP clusters on ROKS
  • After installing Calico API server by following the instructions from [2], Calico IPPools needed for Submariner created by this PR as expected.

I filed [3] issue on Calico project, let's wait for their reply to decide how to proceed with this PR

[1] projectcalico/calico#6412 [2] https://docs.tigera.io/calico/latest/operations/install-apiserver#how-to [3] projectcalico/calico#7845

Calico folks plan to work on providing a new API to create IPPools that can be used regardless to the Calico API server , until this API will be available running Calico API server is a prerequisite for automating Calico IPPools creation.

To make calico compatible with Submariner, it needs to create calico
IPPools for the subnets associated with the Pod and Service CIDRs
of the remote clusters.

This PR adds a new handler (CalicoIPPool) for the routeagent,
the CalicoIPPool  handler will be responsible for creating calico
IPPools for remote endpoints.

Depends-On : submariner-io/submariner-operator#2713

Signed-off-by: Yossi Boaron <yboaron@redhat.com>
@sridhargaddam sridhargaddam enabled auto-merge (rebase) July 24, 2023 13:17
@sridhargaddam sridhargaddam merged commit d48ecae into submariner-io:devel Jul 25, 2023
37 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr2576/yboaron/calico_ippool]

tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Oct 17, 2023
Release note for submariner-io/submariner#2576

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Oct 18, 2023
Release note for submariner-io/submariner#2576

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis added a commit to submariner-io/submariner-website that referenced this pull request Oct 18, 2023
Release note for submariner-io/submariner#2576

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
dfarrell07 pushed a commit to dfarrell07/submariner-website that referenced this pull request Oct 18, 2023
Release note for submariner-io/submariner#2576

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
dfarrell07 pushed a commit to dfarrell07/submariner-website that referenced this pull request Oct 18, 2023
Release note for submariner-io/submariner#2576

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
dfarrell07 pushed a commit to dfarrell07/submariner-website that referenced this pull request Oct 19, 2023
Release note for submariner-io/submariner#2576

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis added a commit to submariner-io/submariner-website that referenced this pull request Oct 22, 2023
Release note for submariner-io/submariner#2576

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
@yboaron yboaron deleted the calico_ippool branch February 14, 2024 12:29
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 release-note-handled release-note-needed Should be mentioned in the release notes
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants