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

Add command brctl #2971

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add command brctl #2971

wants to merge 21 commits into from

Conversation

leongross
Copy link
Contributor

@leongross leongross commented Apr 25, 2024 •
edited

Add cmd/brctl which aims to be a (near) feature complete implementation of brctl.

TODOs

  • Enhance test coverage
    • add vmtest
  • minifiy str_to_jiffies
  • getBridge interface parsing
  • Setgcint command
  • add ioctl fallback methods
  • get interfaces from bridges
  • improve documentation

@10000TB 10000TB self-assigned this Apr 25, 2024
@10000TB 10000TB requested a review from a team April 25, 2024 16:48
@rminnich
Copy link
Member

nice! This is going to be very useful.

@rminnich rminnich added the Awaiting author Waiting for new changes or feedback for author. label Apr 25, 2024
hugelgupf
hugelgupf reviewed Apr 25, 2024
View reviewed changes
cmds/core/brctl/brctl.go Outdated Show resolved Hide resolved
@leongross leongross force-pushed the cmds/brctl branch 7 times, most recently from 4a4d9bf to b30fce5 Compare April 26, 2024 16:17
10000TB
10000TB reviewed Apr 30, 2024
View reviewed changes
cmds/core/brctl/brctl.go Outdated Show resolved Hide resolved
10000TB
10000TB reviewed Apr 30, 2024
View reviewed changes
pkg/brctl/brctl_integration_test.go
qemu.ArbitraryArgs("-nic", fmt.Sprintf("user,id=%s", BRCTL_TEST_IFACE_0)),
qemu.ArbitraryArgs("-nic", fmt.Sprintf("user,id=%s", BRCTL_TEST_IFACE_1)),
)),
)
Copy link
Member

Choose a reason for hiding this comment

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

Do we have steps to validate traffic are transferred properly ?

@10000TB
Copy link
Member

10000TB commented Apr 30, 2024 •
edited

Edit: NVM -- discussed offline, I mis-understood the github UI. I was told this PR was opened from a fork of u-root.

@leongross Hi Leon, looks like this Draft is pushed against upstream u-root

leongross wants to merge 13 commits into u-root:main from leongross:cmds/brctl

Could you push against a fork of u-root, and make this as a PR ?

This looks like review-worthy.

@leongross
Copy link
Contributor Author

leongross commented Apr 30, 2024 •
edited

@10000TB I'm not quite sure what kind of review process you suggest.
My PR goes from my private fork to the upstream u-root. What benefit is there to create a PR for another fork or even my own? There still needs to be a PR for upstream u-root after all, so why create another layer?
As I see it the procedure of this PR adheres to CONTRIBUTING.md guide. Maybe I misunderstood your suggestion, so I'd be glad if you could clarify.

@leongross leongross marked this pull request as ready for review April 30, 2024 09:41
@jensdrenhaus jensdrenhaus mentioned this pull request Apr 30, 2024
Open
@10000TB
Copy link
Member

10000TB commented May 2, 2024

@10000TB I'm not quite sure what kind of review process you suggest. My PR goes from my private fork to the upstream u-root. What benefit is there to create a PR for another fork or even my own? There still needs to be a PR for upstream u-root after all, so why create another layer? As I see it the procedure of this PR adheres to CONTRIBUTING.md guide. Maybe I misunderstood your suggestion, so I'd be glad if you could clarify.

you good. I mis-understood the github UI ;)

@leongross leongross force-pushed the cmds/brctl branch 4 times, most recently from 2a9acf3 to 032a98a Compare May 3, 2024 09:44
@leongross leongross force-pushed the cmds/brctl branch 4 times, most recently from 3d8137e to 3772a48 Compare May 15, 2024 18:43
@ChriMarMe
Copy link
Contributor

Main issue at the moment: executeIoctlStr: syscall.Syscall: package not installed, want nil

I suspect the kernel image doent have network bridging support built in, hence it needs an update.

@leongross
Copy link
Contributor Author

leongross commented Jun 6, 2024 •
edited

this solution is partly taken care of here https://github.com/leongross/vmtest/blob/main/images/kernel-amd64/config_linux.txt. I will clean the format of the kernel config and create a PR for the images used in the CI

@10000TB
Copy link
Member

10000TB commented Jun 6, 2024

Please also make sure the unit test coverage number reported by "Codecov Report" does not drop after this PR

@leongross leongross mentioned this pull request Jun 7, 2024
Open
@leongross
Copy link
Contributor Author

ci fixes incoming by #2995

leongross and others added 20 commits June 7, 2024 13:05
@leongross
add addbr, delbr
e8faaf0 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
update args
84a105b 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add preliminary show implementation
b9c07d1 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add prelim showmacs (parsing partially incorrect)
1f42498 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add subcommands
3191455 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add subcommands
023a987 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
extend usage, add cmd to template 'embedded'
81fa001 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
refctor brctl into pkg and cmd component
a1961ea 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add unit tests for jiffies, update jiffies
cc9a508 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
prelim: add qemu integration test for Addbr Delbr
4e1afcd 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
remove unused command 'Setgcint', refactor consts, rename helper came…
4c9096c 
…lCase

Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add tests
dd53b31 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
mod tidy
a9ef72b 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
remove command description comment
ab8e163 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add interface parsing for bridges
f351760 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
update tests
16fbdfa 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
make unix.Timval bitsize agnostic
414f0b6 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
add vmtest config with kernel that has bridge support, simplify jiffy
f7bb6e0 
conversion

Signed-off-by: leongross <leon.gross@9elements.com>
@ChriMarMe @leongross
Copium for CI
6acc9e7 
Signed-off-by: Christopher Meis <christopher.meis@9elements.com>
@leongross
add documentation for brctl
d81b88c 
Signed-off-by: leongross <leon.gross@9elements.com>
@leongross leongross force-pushed the cmds/brctl branch 3 times, most recently from 3dd1766 to 5040143 Compare June 7, 2024 11:27
@leongross
remove me: test ci kernel image
a1420fd 
Signed-off-by: leongross <leon.gross@9elements.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binjip978 binjip978 binjip978 left review comments

@hugelgupf hugelgupf hugelgupf left review comments

@10000TB 10000TB 10000TB left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@10000TB 10000TB

@leongross leongross

Labels
Awaiting author Waiting for new changes or feedback for author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@leongross @rminnich @10000TB @ChriMarMe @binjip978 @hugelgupf