support workloads across multiple hosts using same /26 #8
Conversation
main.go
Outdated
| peerASN = *asn | ||
| } | ||
| if spec.IPv4Address != nil { | ||
| v4 = spec.IPv4Address.String() |
There was a problem hiding this comment.
We've been a little unusual here and modified our IPv4Address and IPv6Address fields to be CIDRs instead of just IPs. This way we can include the subnet information (and have updated our IPIP behavior to provide the option of only using IPIP if routing to a host that is off-subnet). You'll need to:
- Update this PR to use the new IPv4Address and IPv6Address formats.
- (Future work) Support the new IPIP mode field to only use IPIP off-subnet.
Also, I'd like to retire the use of the IP, IP6 and AS environments and just use the Node IPv4Address and IPv6Address fields. We currently are only maintaining them for the Go BGP daemon. Could you instead check if the node.Metadata.Name is equal to the NODENAME environment - that will be the equivalent to checking if the node is the local node.
main.go
Outdated
| type Server struct { | ||
| t tomb.Tomb | ||
| bgpServer *bgpserver.BgpServer | ||
| cli *calicocli.Client |
There was a problem hiding this comment.
It's a minor nit, but I find cli as a contraction for client confusing since cli is generally means command line interface. Would be nicer to expand to client.
main.go
Outdated
| } | ||
|
|
||
| // monitor routes from other BGP peers and update FIB | ||
| s.t.Go(s.monitorPath) |
There was a problem hiding this comment.
Does the difference between monitorX and watchX refer to something subtly different, or could they all be watchX?
|
A general comment: A lot of the functions/methods could really benefit from having useful doc strings describing what the function is doing - that will certainly be useful in the future when we need to update when we add watcher support to libcalico-go and backends other than etcd. |
main.go
Outdated
| return bgpServer.DeleteNeighbor(n) | ||
| case "set": | ||
| return s.bgpServer.DeleteNeighbor(n) | ||
| case "set", "create": |
There was a problem hiding this comment.
Also update and compareAndSwap for completeness?
|
One more thing - I thought I'd seen a calicoctl PR to test this - but I can't see it now - there is a placeholder comment in the ST code for testing this with GoBGP. |
|
@robbrockbank Hi thanks for your review. |
ishidawataru
force-pushed
the
dev
branch
2 times, most recently
from
9412893 to
3c61637
Compare
This commit tries to make calico-bgp-daemon works identical with BIRD backend.
When multiple hosts are sharing same /26, the prefix is only advertised
from the host which used the prefix first.
To get reachability, succeeding hosts needs to advertise /32 routes
which are taken from the /26 and assigned to each workloads.
The /32 routes can be learned by subscribing the netlink updates.
The first host which used the prefix needs to suppress advertising /32
routes and only advertise the /26 route.
Export policy('calico_aggr') is used to implement this behavior.
calico_aggr consist of two statements.
The first statement allows the /26 routes to be advertised, and the
second one rejects the /32 routes.
you can check the current policy by using gobgp command on calico/node
$ gobgp policy
Name calico_aggr:
StatementName calico_aggr_stmt0:
Conditions:
PrefixSet: any aggregated
Actions:
accept
StatementName calico_aggr_stmt1:
Conditions:
PrefixSet: any host
Actions:
reject
$ gobgp policy prefix
NAME PREFIX
aggregated 192.168.1.0/26 26..26
host 192.168.1.0/26 26..32
Signed-off-by: ISHIDA Wataru <[email protected]>
|
@robbrockbank Rebased on the master. PTAL! |
|
@robbrockbank ping. After this get merged, I'll work on this |
|
LGTM |
|
Hi @ishidawataru - I've merged this one! :-) |
The current implementation only advertises aggregated routes which are
stored in etcd. When workloads are deployed across multiple hosts using
same /26, we need to import /32 host routes from kernel and advertise them.
closes #5
closes https://github.com/projectcalico/calico-containers/issues/1362