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.

Sign up for GitHub

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

Jump to bottom

connectivity: Introduce Multicast connectivity test #2724

Closed
wants to merge 0 commits into from
Closed

connectivity: Introduce Multicast connectivity test #2724

wants to merge 0 commits into from

Conversation

yushoyamaguchi
Copy link
Contributor

@yushoyamaguchi yushoyamaguchi commented Jul 25, 2024 •
edited
Loading

Introduces Multicast connectivity test, working with netshoot container.
Using socat for IGMP and UDP communication test.
Receivers are running as Daemonset and a sender is running as a Deployment which has 1 replicaset.

Operation was confirmed on the kind and kubeadm clusters.

This if follow up of #2615 .

In addtion, this PR is related to #2620 .
I want to implement a test scenario using multicast-subcommand after this is merged.

related part of this command output (when 3 nodes) 
[=] [cilium-test-1] Test [multicast] [100/103]
  [.] Action [multicast/multicast/socat multicast]
  🐛 Executing command [timeout 1 socat STDIO UDP4-RECVFROM:6666,ip-add-membership=239.255.9.9:10.244.0.10]
.  [.] Action [multicast/multicast/socat multicast]
  🐛 Executing command [timeout 1 socat STDIO UDP4-RECVFROM:6666,ip-add-membership=239.255.9.9:10.244.2.142]
.  [.] Action [multicast/multicast/socat multicast]
  🐛 Executing command [timeout 1 socat STDIO UDP4-RECVFROM:6666,ip-add-membership=239.255.9.9:10.244.1.88]
.  🐛 Finalizing Test multicast
  🐛 Finalizing Test host-firewall-ingress
  🐛 Finalizing Test host-firewall-egress
  🐛 Finalizing Test check-log-errors

✅ [cilium-test-1] All 1 tests (3 actions) successful, 102 tests skipped, 0 scenarios skipped.

@yushoyamaguchi yushoyamaguchi requested review from a team as code owners July 25, 2024 14:26
@yushoyamaguchi
Copy link
Contributor Author

yushoyamaguchi commented Jul 25, 2024 •
edited
Loading

  • The current way is a reasonable way to realize multicast testing?
    • Currently, using socat client as sender and socat server as receiver. Judgment is made on the receiver side.
  • In current implementation, Receivers are running as Daemonset and a sender is running as a pod. Is it OK?
    • Should I put senders as Daemonset?
    • Alternatively, can I put a receiver as a pod?
  • Should I run the testing and cleanup procedures concurrently?
    • It seems that concurrent procedure is not implemented in many scenarios.
  • Should the user be able to set the value of the GroupAddress for testing? How can this be accomplished?
    • If user can select the GroupAddress for testing, user-inconvenient cilium state changes will not occur.
  • Is there a need for an action to make sure that packets are not delivered to a pod that is not a Local Endpoint?
  • Parameters are proper?
    • Receiver waiting time / Interval and number of sender iterations
    • How should I decide these?

@yushoyamaguchi

This comment was marked as resolved.

Sign in to view
@yushoyamaguchi yushoyamaguchi mentioned this pull request Jul 25, 2024
Merged
tommyp1ckles
tommyp1ckles requested changes Jul 25, 2024
View reviewed changes
connectivity/check/deployment.go Outdated Show resolved Hide resolved
@yushoyamaguchi
Copy link
Contributor Author

yushoyamaguchi commented Jul 27, 2024 •
edited
Loading

In addition, maybe we have to think about it : #2724
This multicast connectivity test mutate cilium eBPF map temporary. (After testing, it's going back to the original state.)

@yushoyamaguchi yushoyamaguchi changed the title connectivity: Introdue Multicast connectivity test connectivity: Introduce Multicast connectivity test Jul 30, 2024
@tommyp1ckles tommyp1ckles requested a review from ldelossa July 30, 2024 15:42
@tommyp1ckles
Copy link
Contributor

I'll do a general code review, however multicast isn't really my area of expertise. @ldelossa Could you take a look as well?

tommyp1ckles
tommyp1ckles reviewed Jul 30, 2024
View reviewed changes
Copy link
Contributor

@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

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

Changes make sense to me, but I'll defer to someone who knows a bit more about Cilium multicast.

cli/connectivity.go Outdated Show resolved Hide resolved
connectivity/check/deployment.go Outdated
@@ -1042,6 +1042,34 @@ func (ct *ConnectivityTest) deploy(ctx context.Context) error {
}
}

if ct.Features[features.Multicast].Enabled {
_, err = ct.clients.src.GetDeployment(ctx, ct.params.TestNamespace, netshootSocatClientDeploymentName, metav1.GetOptions{})
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check if this is not ErrNotExist?

connectivity/check/deployment.go Outdated
@@ -1200,6 +1228,8 @@ func (ct *ConnectivityTest) deploymentList() (srcList []string, dstList []string
srcList = append(srcList, lrpBackendDeploymentName)
}

// Q: How about multicast deployments?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this leftover?

connectivity/tests/multicast.go Outdated Show resolved Hide resolved
connectivity/tests/multicast.go Outdated Show resolved Hide resolved
connectivity/tests/multicast.go Outdated Show resolved Hide resolved
@yushoyamaguchi
Copy link
Contributor Author

yushoyamaguchi commented Aug 1, 2024 •
edited
Loading

@tommyp1ckles
Thank you for your review.
I've fixed many places which I can resolve.
I still have some questions for fix, so I'm waiting some opinions.
Remaining questions.

P.S.
Have this discussion finished? : #2726
I think whether this PR is acceptable or not depends on the outcome of that discussion in above issue.

@yushoyamaguchi yushoyamaguchi mentioned this pull request Aug 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi Awaiting requested review from christarazi christarazi was automatically assigned from cilium/ci-structure

@ldelossa ldelossa Awaiting requested review from ldelossa

@tommyp1ckles tommyp1ckles Awaiting requested review from tommyp1ckles

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yushoyamaguchi @tommyp1ckles