Moves NetNS to Config from JailerConfig #155

xibz · 2019-11-04T20:17:09Z

This change moves the NetNS field from the JailerConfig to the Config.
This makes the most sense since net namespaces are not subject to only
jailers, but can be used generally as well.

Signed-off-by: xibz impactbchang@gmail.com

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

samuelkarp · 2019-11-04T23:49:18Z

machine.go

@@ -296,6 +291,12 @@ func NewMachine(ctx context.Context, cfg Config, opts ...Opt) (*Machine, error)
 	m.machineConfig = cfg.MachineCfg
 	m.Cfg = cfg

+	m.Handlers.FcInit = m.Handlers.FcInit.Swap(NewSetupNetworkHandler(m.netNSPath()))


This should probably only happen if m.netNSPath() != ""

Refactored. No longer uses handler to enter netns

samuelkarp · 2019-11-04T23:49:53Z

machine.go

@@ -296,6 +291,12 @@ func NewMachine(ctx context.Context, cfg Config, opts ...Opt) (*Machine, error)
 	m.machineConfig = cfg.MachineCfg
 	m.Cfg = cfg

+	m.Handlers.FcInit = m.Handlers.FcInit.Swap(NewSetupNetworkHandler(m.netNSPath()))
+
+	for _, opt := range opts {


Why'd you want to move the opts down here?

I moved them down because that's generally where they belong. They usually are towards the end of a function, but I found that we were adding things to the NewMachine function which could stomp over any option that was provided. So I moved it down for safety purposes to avoid stomping over user overrides.

Can you do that in a separate commit? It's unrelated to the subject of this commit (and PR).

Just removed altogether. I'll go ahead and create a separate PR for this.

sipsma · 2019-11-05T00:35:07Z

I'm not sure this addresses the original issue in firecracker-microvm/firecracker-containerd#305

Even if you override the handler to use a different netns path than what is returned by m.netNSPath(), if your machine has a CNI-configured interface then when you get to these lines m.netNSPath() will return the default path used when a path isn't specified via the go-sdk's JailerConfig.

So the only way this will actually work is if the handler gets overridden with the exact default value that is returned in m.netNSPath(). We could expose that default value in order to make it possible to do this, but honestly this just seems incredibly confusing. There's then 3 possible sources for the netns path can be set and 2 of them have some degree of correspondence required.

What's the downside of the other suggestion I mentioned in the other PR to just set the NetNS path of the machine's JailerCfg? That would leave the current logic for getting the netns path in the Go SDK the same and seems straightforward on the surface.

xibz · 2019-11-05T01:01:18Z

What's the downside of the other suggestion I mentioned in the other PR to just set the NetNS path of the machine's JailerCfg? That would leave the current logic for getting the netns path in the Go SDK the same and seems straightforward on the surface.

Because we use the nil and present value of jailer config to mean something. If we have a jailer config, that means we should enable jailing. So that means that logic would need to be rewritten and introduce a new flag to distinguish between running firecracker's jailer versus a custom one.

I'll have to think about this a little more. I didn't see that netNSPath was used in various locations and assumed it was only used once when setting up the network ns.

sipsma · 2019-11-05T01:19:32Z

Because we use the nil and present value of jailer config to mean something. If we have a jailer config, that means we should enable jailing. So that means that logic would need to be rewritten and introduce a new flag to distinguish between running firecracker's jailer versus a custom one.

Okay, I can see there being value in finding a way to unite the JailerCfg of the machine w/ firecracker-containerd's jailer instead of having firecracker-containerd mangle the handler setup of the machine entirely by itself, but I get that's kind of a monumental effort right now.

What I think matters for this change is that there is only one place for users of the SDK to externally specify the netns path (which keeps the complication for users to the possible minimum). What do you think about moving the NetNS field outside of JailerCfg into a field directly in the machine's Cfg? We can then internally pass the netns path to the jailer (as a private field) but also just use it elsewhere in the machine too. This is a breaking change, but I think it's pretty reasonable since the NetNS path is now a field that can apply to any machine, even one that is not jailed.

xibz · 2019-11-05T01:20:41Z

@sipsma - I messaged you a little bit ago with that same suggestion :p

sipsma · 2019-11-05T01:40:32Z

@sipsma - I messaged you a little bit ago with that same suggestion :p

Sorry, I'm on plane wifi right now; github pages load after a minute or two but Chime hangs indefinitely at "Connecting..." :-) This is also what I suggested as the fallback in the original comment on the other PR, so are there any concerns with it? If not then sounds good to me!

sipsma

We should also make a note of the breaking change (moving the NetNS field) in the release notes files

sipsma · 2019-11-05T17:22:31Z

machine.go

-	jailerProvidedNetNS := m.Cfg.JailerCfg.netNSPath() != ""
+	isDefaultNetNSPath := m.Cfg.NetNS == m.defaultNetNSPath()
 	startCmd := m.cmd.Start

 	var err error
-	if hasNetNS && !jailerProvidedNetNS {
+	if isDefaultNetNSPath {
 		// If the VM needs to be started in a netns but no jailer netns was configured,
 		// start the vmm child process in the netns directly here.


This check should result in us only spinning up the VMM process directly in the netns when the jailer is not going to do it for us, so something like

hasNetNS := m.netNSPath() != "" hasJailer := m.Cfg.JailerCfg != nil ... if hasNetNS && !hasJailer { ... }

I don't think just checking whether the path is the default works because it's actually very much in the realm of possibility that the user specifies a custom NetNS value that happens to be the same as the default (the path is /var/run/netns/<VMID> which is just the standard location for putting netns mounts + the VMID, which the user can also optionally specify in the machine Cfg).

kzys · 2019-11-05T18:33:31Z

machine.go

@@ -151,6 +155,7 @@ func (cfg *Config) Validate() error {
 	return nil
 }

+// ValidateNetwork .


Nit: please write the doc comment.

Removed. Shouldn't have added that :p

This change moves the NetNS field from the JailerConfig to the Config. This makes the most sense since net namespaces are not subject to only jailers, but can be used generally as well. Signed-off-by: xibz <impactbchang@gmail.com>

The current network namespace parameter nested in the JailerConfig parameter requires specifying the entire jailer configuration, which is not convenient and doesn't allow using the noop jailer with a network namespace. To overcome this limitation, add a new NetNS parameter to the CreateVM request. See also a similar change in firecracker-microvm/firecracker-go-sdk#155. Signed-off-by: Georgiy Lebedev <lebedev.gk@phystech.edu>

xibz requested review from kzys, nmeyerhans, samuelkarp, mxpv, IRCody and sipsma November 4, 2019 20:17

xibz force-pushed the netns-handler branch from 764a23e to 98d75b0 Compare November 4, 2019 20:27

xibz mentioned this pull request Nov 4, 2019

Adding netns to jailer firecracker-microvm/firecracker-containerd#305

Merged

kzys approved these changes Nov 4, 2019

View reviewed changes

samuelkarp reviewed Nov 4, 2019

View reviewed changes

xibz force-pushed the netns-handler branch from 12d3455 to 2972bd9 Compare November 5, 2019 00:01

xibz force-pushed the netns-handler branch 2 times, most recently from 2cc4ba3 to 4d0484b Compare November 5, 2019 01:52

xibz changed the title ~~Adds new constructor for setting up networks~~ Moves NetNS to Config from JailerConfig Nov 5, 2019

xibz force-pushed the netns-handler branch 4 times, most recently from a98f0e7 to 6c079fa Compare November 5, 2019 02:30

sipsma reviewed Nov 5, 2019

View reviewed changes

kzys suggested changes Nov 5, 2019

View reviewed changes

xibz force-pushed the netns-handler branch from 6c079fa to 6022d6d Compare November 7, 2019 22:17

Moves NetNS to Config from JailerConfig

f24bcda

This change moves the NetNS field from the JailerConfig to the Config. This makes the most sense since net namespaces are not subject to only jailers, but can be used generally as well. Signed-off-by: xibz <impactbchang@gmail.com>

xibz force-pushed the netns-handler branch from 6022d6d to f24bcda Compare November 7, 2019 22:19

sipsma approved these changes Nov 8, 2019

View reviewed changes

kzys approved these changes Nov 8, 2019

View reviewed changes

xibz merged commit 43d336c into firecracker-microvm:master Nov 8, 2019

xibz deleted the netns-handler branch November 8, 2019 19:54

CuriousGeorgiy mentioned this pull request Sep 8, 2023

Integrate firecracker-containerd with vHive vhive-serverless/firecracker-containerd#42

Merged

Moves NetNS to Config from JailerConfig #155

Moves NetNS to Config from JailerConfig #155

Uh oh!

Conversation

xibz commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipsma commented Nov 5, 2019

Uh oh!

xibz commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipsma commented Nov 5, 2019

Uh oh!

xibz commented Nov 5, 2019

Uh oh!

sipsma commented Nov 5, 2019

Uh oh!

sipsma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xibz commented Nov 4, 2019 •

edited

Loading

xibz commented Nov 5, 2019 •

edited

Loading