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

[launch] add support for --bridged network #2074

Merged
merged 3 commits into from
May 7, 2021
Merged

[launch] add support for --bridged network #2074

merged 3 commits into from
May 7, 2021

Conversation

Saviq
Copy link
Collaborator

@Saviq Saviq commented Apr 26, 2021


For context: the other options we were considering (--public, --exposed) would not be intuitive to anyone. Even if "bridging" isn't strictly correct, e.g. on Hyper-V, that is the term other hypervisors use and people that know what it is will likely know from the get go what it's about. Even ourselves we refer to the feature as "bridging", because that's the prevalent term.

I decided against validating on multipass set local.bridged-interface as the given interface may not exist at that point in time for whatever reason, and on launch we'll communicate well what the problem is, if any.

@Saviq Saviq force-pushed the add-launch-bridged branch from b4a5a39 to 0f727ec Compare April 26, 2021 09:49
@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #2074 (3e98060) into main (9f18346) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2074      +/-   ##
==========================================
+ Coverage   81.45%   81.48%   +0.02%     
==========================================
  Files         184      184              
  Lines        9442     9456      +14     
==========================================
+ Hits         7691     7705      +14     
  Misses       1751     1751              
Impacted Files Coverage Δ
src/client/cli/cmd/launch.cpp 75.78% <100.00%> (+0.33%) ⬆️
src/daemon/daemon.cpp 54.41% <100.00%> (+0.35%) ⬆️
src/utils/settings.cpp 22.09% <100.00%> (+0.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f18346...3e98060. Read the comment docs.

@Saviq Saviq force-pushed the add-launch-bridged branch from 0f727ec to f695d08 Compare April 27, 2021 07:28
@luis4a0 luis4a0 self-requested a review May 3, 2021 13:38
@luis4a0
Copy link
Contributor

luis4a0 commented May 3, 2021

Thanks for this @Saviq! The name --bridged sounds the best option to me.

The code looks good, I only have a minor concern with respect to the functionality: what happens if the host has an interface named 'bridged'? I think we should provide a way to bridge it. I know this is unlikely, but it's still possible.

One possibility I see to address this corner case is to allow --network id=bridged. WDYT?

@Saviq
Copy link
Collaborator Author

Saviq commented May 3, 2021

One possibility I see to address this corner case is to allow --network id=bridged. WDYT?

That would require us to change the RPC and CLI (--bridged and --network bridged would no longer be shortcuts for id=bridged) for tiny gain. IMO not worth it, but let me know if you think otherwise.

@luis4a0
Copy link
Contributor

luis4a0 commented May 3, 2021

Well, I don't think this is strictly necessary at that cost. It would be nice to have if easier to implement. Let's leave it like it is now and if some user complains at some point, we can think again about it.

@Saviq Saviq force-pushed the add-launch-bridged branch from f695d08 to 8f49e0a Compare May 4, 2021 19:44
@Saviq
Copy link
Collaborator Author

Saviq commented May 4, 2021

@LuisP I added the tweak to only restart in the daemon if the driver changed. Unfortunately I was unable to add a reliable test, but didn't want to block on it.

@Saviq Saviq force-pushed the add-launch-bridged branch 4 times, most recently from 3e6759e to 1e5ff65 Compare May 5, 2021 06:30
@luis4a0
Copy link
Contributor

luis4a0 commented May 5, 2021

Looks good, thanks!

@Saviq Saviq force-pushed the add-launch-bridged branch 4 times, most recently from 4c17e28 to 47ebbdb Compare May 6, 2021 06:02
@Saviq
Copy link
Collaborator Author

Saviq commented May 6, 2021

@luis4a0 I reverted the recent changes, went back to just adding the setting and --bridged option. I'll work on preventing the unnecessary restart in a separate PR.

@luis4a0
Copy link
Contributor

luis4a0 commented May 6, 2021

bors r+

bors bot added a commit that referenced this pull request May 6, 2021
2074: [launch] add support for `--bridged` network r=luis4a0 a=Saviq



Co-authored-by: Michał Sawicz <michal@sawicz.net>
@bors
Copy link
Contributor

bors bot commented May 6, 2021

Build failed:

@Saviq
Copy link
Collaborator Author

Saviq commented May 6, 2021

bors retry

@Saviq Saviq force-pushed the add-launch-bridged branch from 47ebbdb to 3e98060 Compare May 6, 2021 18:01
@bors
Copy link
Contributor

bors bot commented May 6, 2021

Canceled.

@Saviq Saviq changed the base branch from main to ask-user-bridging May 6, 2021 18:05
@Saviq
Copy link
Collaborator Author

Saviq commented May 6, 2021

bors r=luis4a0,Saviq

bors bot added a commit that referenced this pull request May 6, 2021
2074: [launch] add support for `--bridged` network r=luis4a0,Saviq a=Saviq



Co-authored-by: Michał Sawicz <michal@sawicz.net>
@Saviq
Copy link
Collaborator Author

Saviq commented May 6, 2021

bors cancel

@bors
Copy link
Contributor

bors bot commented May 6, 2021

Canceled.

@bors bors bot changed the base branch from ask-user-bridging to main May 6, 2021 21:52
@Saviq
Copy link
Collaborator Author

Saviq commented May 6, 2021

bors retry

@Saviq
Copy link
Collaborator Author

Saviq commented May 7, 2021

bors r=luis4a0,Saviq

bors bot added a commit that referenced this pull request May 7, 2021
2074: [launch] add support for `--bridged` network r=luis4a0,Saviq a=Saviq



Co-authored-by: Michał Sawicz <michal@sawicz.net>
@bors
Copy link
Contributor

bors bot commented May 7, 2021

Build failed:

@Saviq
Copy link
Collaborator Author

Saviq commented May 7, 2021

bors retry

bors bot added a commit that referenced this pull request May 7, 2021
2074: [launch] add support for `--bridged` network r=luis4a0,Saviq a=Saviq



Co-authored-by: Michał Sawicz <michal@sawicz.net>
@bors
Copy link
Contributor

bors bot commented May 7, 2021

Build failed:

@Saviq
Copy link
Collaborator Author

Saviq commented May 7, 2021

One last time.

bors retry

@bors
Copy link
Contributor

bors bot commented May 7, 2021

@bors bors bot merged commit 1394819 into main May 7, 2021
@bors bors bot deleted the add-launch-bridged branch May 7, 2021 10:14
@Saviq Saviq added this to the v1.7.0 milestone Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants