Add option to disable Gossip Router #1667

pruivo · 2022-08-10T11:01:29Z

Documentation missing.

ryanemerson · 2022-08-11T11:34:01Z

api/v1/infinispan_types.go

+	Type DiscoverySiteType `json:"type,omitempty"`
+	// Enables (default) or disables the Gossip Router pod and cross-site services
+	// +optional
+	LaunchGossipRouter *bool `json:"launchGossipRouter,omitempty"`


As DiscoverySiteSpec is a new struct, I think we can remove the pointer here.

The infinispan defaulting webhook, api/v1/infinispan_webhook, can add the DiscoverySiteSpec when spec.sites.local.discovery is nil for new CRs so that the default value of LaunchGossipRouter is true.

If upgrading from an old operator version, then spec.sites.local.discovery will remain nil and we can test for this in CrossSiteDiscoveryType and LaunchGossipRouterEnabled as you currently have.

what would happen if a user puts an empty element in the yaml? Like

spec: service: type: DataGrid sites: local: name: lon discovery: locations: -name: nyc

edit: assuming this is a valid yaml format
edit2: is this considered a nil structure?

kept the *bool but added the default in the webhook (and removed the nil checks)

I think the valid format would be to have discovery: {} or discovery: ~, but either way it's redundant as we have omitempty defined on the field, so any of these will result in discovery being nil in the struct.

kept the *bool but added the default in the webhook (and removed the nil checks)

I don't think this is necessary due to ^.

ryanemerson · 2022-08-11T11:35:19Z

api/v1/infinispan_types.go

+// +kubebuilder:validation:Enum=gossiprouter
+type DiscoverySiteType string
+
+// GossipRouterType is the only one supported but we may add other like submariner and/or skupper


Good thinking 👍

documentation/asciidoc/topics/proc_xsite_disable_gossip_router.adoc

ryanemerson · 2022-08-11T11:37:46Z

documentation/asciidoc/topics/proc_xsite_disable_gossip_router.adoc

+
+[role="_abstract"]
+Only a single Gossip Router is required to route the cross-site traffic however, the {brandname} operator starts a Gossip Route on each site.
+You can disable the Gossip Router in one of the sites to save resources.


What's the best practice if 3 sites are configured?

If you have a GossipRouter enabled on site 1, but not 2 & 3, does that mean that xsite will fail if Site 1 becomes unavailable?

the best practice for 2 or more sites is to let the operator start the Gossip Router on all sites.

This is a customer request that does not want to have a Gossip Router on one of the sites (2 sites total).

In your scenario, the connection will be broken and cross-site becomes available.

Should we add a note explaining this to the docs, maybe as a NOTE section?

+1, I'll leave this for the @infinispan/docs team :)

Cool. I'm looking into it and I'll be back on Monday

ryanemerson · 2022-08-11T11:40:37Z

@dvagnero @dfitzmau Can you review the doc contributions when you get a chance? Thanks

dfitzmau · 2022-08-11T11:53:19Z

@dvagnero @dfitzmau Can you review the doc contributions when you get a chance? Thanks

Hi @ryanemerson . Dominika beat me too it.

Just to let you know we created a members group for docs, so you can tag us both with @infinispan/docs

ryanemerson · 2022-08-11T14:40:36Z

test/e2e/utils/kubernetes.go

 		if err != nil {
+			fmt.Println(err)


Adding a comment so we don't forget to remove these before merging.

already gone!

ryanemerson · 2022-08-11T14:43:26Z

api/v1/infinispan_webhook.go

+		}
+		if i.Spec.Service.Sites.Local.Discovery.LaunchGossipRouter == nil {
+			i.Spec.Service.Sites.Local.Discovery.LaunchGossipRouter = new(bool)
+			*i.Spec.Service.Sites.Local.Discovery.LaunchGossipRouter = true


FYI you can do this as a one-liner with i.Spec.Service.Sites.Local.Discovery.LaunchGossipRouter = pointer.BoolPtr(true)

domiborges · 2022-08-16T17:02:02Z

Hey, I added a commit with some docs suggestions. Please let me know what you think

ryanemerson · 2022-08-17T09:02:32Z

documentation/asciidoc/topics/proc_disabling_gossip_router_cross_site.adoc

+
+.Prerequisites
+
+* Have multiple working Gossip routers.


I don't multiple Gossip Routers is a prerequisite. Rather, it's necessary for a Gossip Router to be enabled on at least one site.

ryanemerson · 2022-08-17T09:03:57Z

documentation/asciidoc/topics/proc_disabling_gossip_router_cross_site.adoc

+
+* Have multiple working Gossip routers.
+
+You can have an external Gossip router that is not managed by the {ispn_operator}.


This isn't correct. The Gossip Router will be managed by the Operator, however it will only be enabled on one of the sites, therefore it's only managed by the Operator on the enabled site.

documentation/asciidoc/topics/proc_disabling_gossip_router_cross_site.adoc

ryanemerson · 2022-08-24T16:44:09Z

documentation/asciidoc/topics/proc_disabling_gossip_router_cross_site.adoc

+. For the *LON* cluster, set `false` as the value for the `spec.service.sites.local.discovery.launchGossipRouter`.
+. For the *LON* cluster, specify the `url` with the `spec.service.sites.locations.url` to connect to the *NYC*.
+
+In the *NYC* configuration, do not specify `spec.service.sites.locations.url`.


Should this be part of the procedure steps?

pruivo · 2022-08-29T11:10:02Z

just rebased on top of main branch.
The docs look good to me. Thanks @dvagnero !
@ryanemerson is there anything that I miss?

ryanemerson · 2022-08-31T11:21:51Z

@pruivo LGTM. Can you rebase on latest main to fix the webhook tests

Closes infinispan#1665

ryanemerson · 2022-09-01T15:41:21Z

Thanks @pruivo @dvagnero

pruivo force-pushed the t_1665_gr_disable branch from 79dda49 to b51cba1 Compare August 11, 2022 08:47

ryanemerson reviewed Aug 11, 2022

View reviewed changes

pruivo force-pushed the t_1665_gr_disable branch 2 times, most recently from 024b761 to 733f955 Compare August 11, 2022 15:39

ryanemerson requested changes Aug 17, 2022

View reviewed changes

domiborges requested a review from ryanemerson August 23, 2022 16:05

ryanemerson reviewed Aug 24, 2022

View reviewed changes

documentation/asciidoc/topics/proc_disabling_gossip_router_cross_site.adoc Outdated Show resolved Hide resolved

ryanemerson reviewed Aug 24, 2022

View reviewed changes

documentation/asciidoc/topics/proc_disabling_gossip_router_cross_site.adoc Outdated Show resolved Hide resolved

ryanemerson reviewed Aug 24, 2022

View reviewed changes

pruivo force-pushed the t_1665_gr_disable branch from 4b525af to d14dd1b Compare August 29, 2022 11:08

pruivo and others added 5 commits August 31, 2022 14:23

Add option to disable Gossip Router

e2ea630

Closes infinispan#1665

Docs suggestions for infinispan#1665

0c2913b

Docs follow-up for infinispan#1665

00f1bd8

Docs update for infinispan#1665

2d4118c

Docs for infinispan#1665

4757dad

pruivo force-pushed the t_1665_gr_disable branch from d14dd1b to 4757dad Compare August 31, 2022 13:23

ryanemerson merged commit 3360f34 into infinispan:main Sep 1, 2022

pruivo deleted the t_1665_gr_disable branch September 1, 2022 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to disable Gossip Router #1667

Add option to disable Gossip Router #1667

pruivo commented Aug 10, 2022

ryanemerson Aug 11, 2022

pruivo Aug 11, 2022 •

edited

pruivo Aug 11, 2022

ryanemerson Aug 11, 2022

ryanemerson Aug 11, 2022

ryanemerson Aug 11, 2022

pruivo Aug 11, 2022

ryanemerson Aug 11, 2022

pruivo Aug 11, 2022

domiborges Aug 11, 2022

ryanemerson commented Aug 11, 2022

dfitzmau commented Aug 11, 2022

ryanemerson Aug 11, 2022

pruivo Aug 11, 2022

ryanemerson Aug 11, 2022

domiborges commented Aug 16, 2022

ryanemerson Aug 17, 2022

ryanemerson Aug 17, 2022

ryanemerson Aug 24, 2022

pruivo commented Aug 29, 2022

ryanemerson commented Aug 31, 2022

ryanemerson commented Sep 1, 2022

Add option to disable Gossip Router #1667

Add option to disable Gossip Router #1667

Conversation

pruivo commented Aug 10, 2022

Choose a reason for hiding this comment

pruivo Aug 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanemerson commented Aug 11, 2022

dfitzmau commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domiborges commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pruivo commented Aug 29, 2022

ryanemerson commented Aug 31, 2022

ryanemerson commented Sep 1, 2022

pruivo Aug 11, 2022 •

edited