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

Get multiport-eswitch mode from metalnet rundir #270

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

PlagueCZ
Copy link
Contributor

@PlagueCZ PlagueCZ commented Oct 2, 2024

In OSC it is beneficial to use the same metalnet deployment for both versions of dpservice - legacy (single-port) and multiport-eswitch ones.

Thus using the command-line argument is not the way to go, instead we simply echo -n "eswitch" > /var/run/metalnet/mode and metalnet then reads this information, just like it gets other stuff from virtlet(?) about VFs.

The original implementation in 78efc7c is tested in our clusters, I simplified the code without being able to run it in a cluster, though I did verify the output locally.

@PlagueCZ PlagueCZ requested a review from a team as a code owner October 2, 2024 16:00
@github-actions github-actions bot added enhancement New feature or request size/XS labels Oct 2, 2024
@PlagueCZ PlagueCZ requested a review from guvenc October 2, 2024 16:04
Copy link
Contributor

@byteocean byteocean left a comment

Choose a reason for hiding this comment

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

Looks no issue to me. But it would be nice to have one line comment like: run touch /var/run/metalnet/mode && echo -n "eswitch" > /var/run/metalnet/mode to enable multiport eswitch mode, as this approach of configuration is not so crystal clear it is a manual step to setup the flag.

@PlagueCZ PlagueCZ force-pushed the feature/multiport_mode_detection branch from e7c6708 to 4463703 Compare October 7, 2024 13:51
@github-actions github-actions bot added size/S documentation Improvements or additions to documentation and removed size/XS labels Oct 7, 2024
@PlagueCZ
Copy link
Contributor Author

PlagueCZ commented Oct 7, 2024

I added a comment and also some documentation in README.md covering multiport-eswitch and pf1-proxy

@PlagueCZ PlagueCZ force-pushed the feature/multiport_mode_detection branch from 4463703 to e193110 Compare October 14, 2024 21:13
@github-actions github-actions bot added size/M and removed size/S labels Oct 14, 2024
@PlagueCZ
Copy link
Contributor Author

I have reverted the removal of --multiport-eswitch and mentioned the automatic detection in help and markdown.

main.go Outdated
@@ -104,7 +104,7 @@ func main() {
flag.BoolVar(&enableIPv6Support, "enable-ipv6", false, "Enable IPv6 support")
flag.IntVar(&publicVNI, "public-vni", 100, "Virtual network identifier used for public routing announcements.")
flag.IPVar(&routerAddress, "router-address", net.IP{}, "The address of the next router.")
flag.BoolVar(&multiportEswitchMode, "multiport-eswitch", false, "Enable multiport eswitch support")
flag.BoolVar(&multiportEswitchMode, "multiport-eswitch", false, "Enable multiport eswitch support (unless specified in metalnet-dir)")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe 'Enale multiport eswitch support explicitly, unless specified in metalnet-dir' is a bit more understandable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well arguments are already explicit, but then we would say "explicit unless" which is strange, so how about changing the parentheses to "(file in metalnet-dir can override this)"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reworded the help and also changed the handling so it should work normally when the metalnet-dir file is not present

@PlagueCZ PlagueCZ force-pushed the feature/multiport_mode_detection branch from e193110 to c5ebb2c Compare October 16, 2024 15:37
Copy link
Collaborator

@guvenc guvenc left a comment

Choose a reason for hiding this comment

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

LGTM

@guvenc guvenc merged commit c5ebb2c into main Oct 23, 2024
7 checks passed
@guvenc guvenc deleted the feature/multiport_mode_detection branch October 23, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants