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

Do not attempt to join Windows agents to memberlist cluster #5434

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Aug 24, 2023

Windows Nodes should not join the memberlist cluster as all features relying on it is not only supported on Windows. Currently an agent only runs the memberlist module and listen to the memberlist port when required so attempts to join Windows agents are destined to fail, leading to misleading error logs.

This commit fixes it by ignoring Windows Nodes when joining agents.

Fixes #5431


Confirmed the error logs are gone in a mixed OS testbed.

Windows Nodes should not join the memberlist cluster as all features
relying on it is not only supported on Windows. Currently an agent only
runs the memberlist module and listen to the memberlist port when
required so attempts to join Windows agents are destined to fail,
leading to misleading error logs.

This commit fixes it by ignoring Windows Nodes when joining agents.

Signed-off-by: Quan Tian <[email protected]>
@tnqn tnqn requested review from antoninbas and xliuxu August 24, 2023 12:19
@tnqn tnqn assigned tnqn and unassigned tnqn Aug 24, 2023
@tnqn tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Aug 24, 2023
@tnqn
Copy link
Member Author

tnqn commented Aug 24, 2023

/test-all
/test-windows-all

@tnqn tnqn added the area/transit/egress Issues or PRs related to Egress (SNAT for traffic egressing the cluster). label Aug 24, 2023
func (c *Cluster) handleCreateNode(obj interface{}) {
node := obj.(*corev1.Node)
if !shouldJoinCluster(node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not worth it to have a filtered node informer based on the label, and avoid the calls to shouldJoinCluster? (or maybe not possible?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible but may be not worth as other modules need all Nodes so an unfiltered node informer is required anyway. If we create a filtered node informer, it will create another connection to kube-apiserver and store a copy of linux nodes:

// NewFilteredNodeInformer constructs a new informer for Node type.
// Always prefer using an informer factory to get a shared informer instead of getting an independent
// one. This reduces memory footprint and number of connections to the server.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

@tnqn tnqn merged commit 4cc6f63 into antrea-io:main Aug 25, 2023
44 of 46 checks passed
@tnqn tnqn deleted the clustering-ignore-windows branch August 25, 2023 06:35
@xliuxu xliuxu mentioned this pull request Aug 25, 2023
@tnqn tnqn added the kind/bug Categorizes issue or PR as related to a bug. label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. area/transit/egress Issues or PRs related to Egress (SNAT for traffic egressing the cluster). kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

antrea "Clustering" is not OS aware
2 participants