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

Fix ClusterBootstrap using wrong fallback port value #925

Conversation

Arkatufus
Copy link
Contributor

@Arkatufus Arkatufus commented Oct 10, 2022

Fixes #924

The fix isn't obvious at first.
The failure happened at this filtering method, it is filtering out all contact points from the result:

internal static IImmutableList<ResolvedTarget> SelectHosts(
Lookup lookup,
int fallbackPort,
bool filterOnFallbackPort,
IImmutableList<ResolvedTarget> contactPoints)
{
// if the user has specified a port name in the search, don't do any filtering and assume it
// is handled in the service discovery mechanism
if (!string.IsNullOrEmpty(lookup.PortName) || !filterOnFallbackPort)
return contactPoints;
return contactPoints.GroupBy(t => t.Host).SelectMany(g =>
{
if (g.ToList().Count == 1)
return g;
if (g.Any(a => a.Port.HasValue))
return g.Where(a => a.Port != null && a.Port.Value == fallbackPort);
return g;
}).ToImmutableList();
}

There is nothing wrong with this method by itself, the problem is the fallback port passed into it.

If akka.management.cluster.bootstrap.contact-point.fallback-port was not overriden, the default value is null or undefined. By default, the cluster bootstrap settings will try to fill this information by reading the akka.management.http.port HOCON value.
This by itself is not a problem, if we're using pure HOCON configuration. The AkkaManagementSetup class though, is a different story.

AkkaManagementSetup is being applied directly to the AkkaManagementSettings class, changing the settings class without touching the HOCON configuration. By this time, there are two management HTTP port value, the correct one is held inside AkkaManagement.Settings property, the other is the stale value inside System.Settings.Config.

The bug happened because cluster bootstrap is using the stale value inside the HOCON config, and not the correct one in AkkaManagement.Settings.

@@ -151,8 +151,7 @@ public static ContactPointSettings Create(Config config)
var contactPointConfig = config.GetConfig("akka.management.cluster.bootstrap.contact-point");
var fallback = contactPointConfig.GetString("fallback-port");
var fallbackPort = string.IsNullOrWhiteSpace(fallback) || fallback == "<fallback-port>"
? config.GetInt("akka.management.http.port")
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 not depend on HOCON config values of another plugin. The HOCON config is not the single source of truth for configuration anymore, since the final settings value can be influenced by a Setup class.

@@ -164,6 +165,8 @@ public BootstrapCoordinator(ServiceDiscovery discovery, IJoinDecider joinDecider
_settings.ContactPointDiscovery.EffectiveName(Context.System),
_settings.ContactPointDiscovery.PortName,
_settings.ContactPointDiscovery.Protocol);

_fallbackPort = _settings.ContactPoint.FallbackPort ?? AkkaManagement.Get(Context.System).Settings.Http.Port;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We read the actual settings value directly from the plugin, not the plugin HOCON config

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb merged commit 29971dc into akkadotnet:dev Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forming of an Akka.Cluster is not working anymore after upgrading to 0.3.0-beta1 of Akka.Discovery.Azure
2 participants