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

Add multicast subcommand #2620

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Add multicast subcommand #2620

merged 1 commit into from
Jul 25, 2024

Conversation

yushoyamaguchi
Copy link
Contributor

Change

This change adds cilium multicast sub command to manage multicast groups and subscribers throughout the cluster

Following commands are added

# show list of multicast group
- cilium multicast list group

# show list of subscribers
- cilium multicast list subscriber --all
- cilium multicast list subscriber --group-ip <group-ip>

# make specified multicast group and add CiliumInternalIP of all cilium nodes to the group in each node
- cilium multicast add --group-ip <group-ip>

# Remove CiliumInternalIP of all cilium nodes from specified multicast group and delete the group in each node
- cilium multicast del --group-ip <group-ip>

This is follow up of #2574

Display example

cli-result

Additional

As for testing, we will proceed with the implementation of the connectivity test for multicast as described in this issue.(#2615)

Reference

Current way of setup multicast : https://github.com/cilium/cilium/blob/main/Documentation/network/multicast.rst

@yushoyamaguchi yushoyamaguchi requested a review from a team as a code owner June 21, 2024 15:01
@yushoyamaguchi yushoyamaguchi requested a review from asauber June 21, 2024 15:01
@harsimran-pabla harsimran-pabla self-requested a review June 21, 2024 16:00
@harsimran-pabla harsimran-pabla added the area/multicast Multicast related CLIs label Jun 21, 2024
Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Thank you for the changes; overall, it looks good. A couple of areas that need work are concurrency and error handling.

cli/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
multicast/multicast.go Outdated Show resolved Hide resolved
@yushoyamaguchi

This comment was marked as resolved.

@maintainer-s-little-helper
Copy link

Commit d70128e does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Copy link
Member

@asauber asauber left a comment

Choose a reason for hiding this comment

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

I will save a detailed review for after the current round of changes.

@asauber asauber self-requested a review June 25, 2024 16:39
@harsimran-pabla
Copy link
Contributor

Thanks @yushoyamaguchi for addressing these comments.

In other subcommands, variable name cmd is used. Can I ignore these warning messages, or should I change variable name to _ ?

It would be best if you fixed these comments ( these are static syntax checkers and have to make linters happy ).

Second, setting resolved tag for the place I fixed is my role or your role? (Currently, I only put 👍)
I'm sorry for ignorance about OSS development.

If you have addressed the comment; feel free to resolve it.

In addition, I replied to some your fix request comments which I cannot find the way to fix.

Please add a comment whichever comment is not clear, I will try my best to explain it further.

@harsimran-pabla
Copy link
Contributor

General commit comment.

  • Keep each commit about logical change instead of iterations on this PR.
  • Add more details about the change in the commit description.

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

@asauber
Thank you for your reviewing.
I'll fix all improvement point, then tell you.

Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Thanks @yushoyamaguchi, for bearing with me on this PR. There are a couple of things I would like to highlight: minor naming changes. And channel creations can be cleaned up. I'd suggest consolidating the channels, as shown in the example in one of the comments.

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

@harsimran-pabla

Thank you for kindly pointing out the places that needed improvement.
I'm happy because I could learn a lot.

I've fixed all the points you mentioned.

@harsimran-pabla
Copy link
Contributor

Thanks, @yushoyamaguchi, for the change; it looks good. Please squash both commits into a single one.

@yushoyamaguchi
Copy link
Contributor Author

@harsimran-pabla
Thank you so much for the detailed review
I've squashed.

This commit is to enable multicast status check and configuration from cilium-cli by adding multicast subcommand.
The list subcommand show the list of multicast groups or their subscribers on all nodes.
The add subcommand joins all nodes to the specified multicast group.
The del subcommand deletes the specified multicast group from all nodes.

This is follow up of #2574

Signed-off-by: Yusho Yamaguchi <[email protected]>
@yushoyamaguchi
Copy link
Contributor Author

yushoyamaguchi commented Jul 25, 2024

@harsimran-pabla @asauber
Thank you for the approval.
Should I do something else for merging?
As I said in this PR, I want to add test scenario using this multicast subcommand after merging...

@fujitatomoya -san 's approval is required?
He said maybe not required but I can ask him if needed, because he is a my colleague.

@harsimran-pabla harsimran-pabla removed the request for review from fujitatomoya July 25, 2024 15:12
@harsimran-pabla
Copy link
Contributor

Thanks, @yushoyamaguchi. I think this PR is ready to merge; I believe all concerns are addressed. We can move forward with e2e testing independently.

@harsimran-pabla
Copy link
Contributor

I'd suggest reaching out to @fujitatomoya to get his approval as well, since he reviewed this PR.

@yushoyamaguchi
Copy link
Contributor Author

Thank you.

Copy link

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@harsimran-pabla @asauber thanks for the reviews, im good to go.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 25, 2024
@michi-covalent michi-covalent merged commit 61f2b6d into cilium:main Jul 25, 2024
13 checks passed
@yushoyamaguchi
Copy link
Contributor Author

yushoyamaguchi commented Jul 26, 2024

@harsimran-pabla @asauber @michi-covalent @fujitatomoya
Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multicast Multicast related CLIs ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants