add: remote snapshotter dialer timeouts to service config #684

austinvazquez · 2022-06-22T22:12:49Z

Signed-off-by: Austin Vazquez [email protected]

Issue #, if available:
#673

Description of changes:
Expose all the dialer timeout knobs at the snapshotter service level. This enables tuning by users based on their host stability.

Buildkite instances used by the pipeline tend to be somewhat slower compared to developer instances, so this helps enable stabilizing remote snapshotter integration tests.

Additionally, remove some unneeded code from the remote snapshotter integration tests and make the tests run in parallel again to speed up the build.

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

Signed-off-by: Austin Vazquez <[email protected]>

austinvazquez · 2022-06-23T07:34:47Z

snapshotter/app/service.go

+				vsock.WithLogger(log.G(ctx)),
+				vsock.WithDialTimeout(dialerTimeouts.DialTimeout),
+				vsock.WithRetryTimeout(dialerTimeouts.RetryTimeout),
+				vsock.WithRetryInterval(dialerTimeouts.RetryInterval),


This is the value that really helped stabilize tests in the pipeline. By default, the SDK was retrying once every 100ms. I have dialed this frequency down to once every 5s. Yes the tests are now twice as slow as before but appearingly more stable on Buildkite.

100ms -> 5s is 50x change. Why do you specifically pick 5s? Does it make sense to reduce a bit, like 1s?

So I noticed the ticker was firing every 100ms and executing code that had up to 1s timeout. Although I assume separate goroutines handle the execution, I wanted to see if lowering the frequency increased stability. 5s is somewhat arbitrary but based on doing runs with up to 4s acknowledge message timeouts which still lead to timeouts.

austinvazquez · 2022-06-23T07:36:07Z

snapshotter/service_integ_test.go

@@ -49,15 +49,15 @@ const (
 )

 func TestLaunchContainerWithRemoteSnapshotter_Isolated(t *testing.T) {
-	integtest.Prepare(t, integtest.WithDefaultNetwork())


Not needed. All CreateVM calls specify network interface.

Let's have this one in either a different commit and/or PR.

Question regarding this: have you tried making just this change and observed any different behavior on the BuildKite tests?

Guilty, yes. WithDefaultNetwork configures the fcnet network as the default network for CreateVM calls who do not specify a network configuration. So it is technically redundant for this and the below network configuration in the create call. Just a simplification of the test.

austinvazquez · 2022-06-23T07:36:28Z

snapshotter/service_integ_test.go

 	_, err = fcClient.CreateVM(ctx, &proto.CreateVMRequest{
 		VMID:       vmID,
-		KernelArgs: kernelArgs,
+		KernelArgs: integtest.DefaultRuntimeConfig.KernelArgs,


Default kernel args should be enough.

austinvazquez · 2022-06-23T07:38:12Z

snapshotter/service_integ_test.go

@@ -104,15 +100,14 @@ func launchContainerWithRemoteSnapshotterInVM(ctx context.Context, vmID string)
 				AllowMMDS: true,
 				CNIConfig: &proto.CNIConfiguration{
 					NetworkName:   "fcnet",
-					InterfaceName: "veth0",
+					InterfaceName: fmt.Sprintf("veth%s", vmID),


While I don't believe having each microVM on the same interface causes any issue, there also isn't a reason to have them on the same interface so just place each VM on it's own interface.

This is just the name of the interface as seen inside the VM's netns, isn't it? There shouldn't be any sharing by using the same name.

Oh yes, you are correct. I misread the CNI documentation when it stated "traffic would be mirrored on the external veth0 device". The name outside the VM appears dynamic as well.

austinvazquez · 2022-06-23T07:39:27Z

snapshotter/config/config.go

-	AckMsgTimeoutInSeconds int `toml:"ack_msg_timeout_in_seconds" default:"1"`
+	DialTimeout       string `toml:"dial_timeout" default:"100ms"`
+	RetryTimeout      string `toml:"retry_timeout" default:"10s"`
+	RetryInterval     string `toml:"retry_interval" default:"5s"`


This might be too slow but we have the knobs now and can tune based on observations.

Regarding the timeouts, if we don't have them, would it wait indefinitely without clear errors? I wonder whether we could have a single timeout, instead of asking end-users to budget all of them specifically.

I have not done any experiments here to prove without the timeouts. I agree reducing complexity would be better as long as the solution becomes stable.

kzys · 2022-06-23T15:07:49Z

snapshotter/app/service.go

@@ -156,6 +156,44 @@ func initResolver(config config.Config) (proxyaddress.Resolver, error) {
 	}
 }

+type dialerTimeouts struct {


I don't like the direction it is heading to.

firecracker-containerd itself is using vsock but we don't have these configuration values. What is the difference between firecracker-containerd and demux snapshotter?

Exposing all knobs to end users basically moving the responsibility from ourselves to them, which isn't good.

That's a fair comment. My reasoning for lift and shifting the responsibility was, and based purely on observations, the dial performance seemed to be greatly impacted by host load. I think we could consider mechanisms like exponential backoff to help reduce the number of knobs and loss of performance.

Let's pick stable values for our BuildKite first. While we don't have much customers today, I feel these values are too tied to the implementation and hard to tune.

kzys · 2022-06-23T15:13:20Z

snapshotter/app/service.go

+				vsock.WithLogger(log.G(ctx)),
+				vsock.WithDialTimeout(dialerTimeouts.DialTimeout),
+				vsock.WithRetryTimeout(dialerTimeouts.RetryTimeout),
+				vsock.WithRetryInterval(dialerTimeouts.RetryInterval),


100ms -> 5s is 50x change. Why do you specifically pick 5s? Does it make sense to reduce a bit, like 1s?

kzys · 2022-06-23T15:15:43Z

snapshotter/config/config.go

-	AckMsgTimeoutInSeconds int `toml:"ack_msg_timeout_in_seconds" default:"1"`
+	DialTimeout       string `toml:"dial_timeout" default:"100ms"`
+	RetryTimeout      string `toml:"retry_timeout" default:"10s"`
+	RetryInterval     string `toml:"retry_interval" default:"5s"`


Regarding the timeouts, if we don't have them, would it wait indefinitely without clear errors? I wonder whether we could have a single timeout, instead of asking end-users to budget all of them specifically.

kzys · 2022-06-23T15:17:01Z

snapshotter/service_integ_test.go

@@ -49,15 +49,15 @@ const (
 )

 func TestLaunchContainerWithRemoteSnapshotter_Isolated(t *testing.T) {
-	integtest.Prepare(t, integtest.WithDefaultNetwork())


Let's have this one in either a different commit and/or PR.

austinvazquez · 2022-06-27T18:56:58Z

Superseded by #689, #690

austinvazquez changed the title ~~fix: unique interface name for multi VM test~~ add: remote snapshotter dialer timeouts to service config Jun 23, 2022

add: remote snapshotter dialer timeouts to service config

bb81a14

Signed-off-by: Austin Vazquez <[email protected]>

austinvazquez commented Jun 23, 2022

View reviewed changes

austinvazquez marked this pull request as ready for review June 23, 2022 15:16

austinvazquez requested a review from a team as a code owner June 23, 2022 15:16

kzys suggested changes Jun 23, 2022

View reviewed changes

austinvazquez closed this Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: remote snapshotter dialer timeouts to service config #684

add: remote snapshotter dialer timeouts to service config #684

austinvazquez commented Jun 22, 2022 •

edited

Loading

austinvazquez Jun 23, 2022

kzys Jun 23, 2022

austinvazquez Jun 23, 2022

austinvazquez Jun 23, 2022

kzys Jun 23, 2022

ginglis13 Jun 23, 2022 •

edited

Loading

austinvazquez Jun 23, 2022

austinvazquez Jun 23, 2022

austinvazquez Jun 23, 2022

Kern-- Jun 23, 2022

austinvazquez Jun 23, 2022 •

edited

Loading

austinvazquez Jun 23, 2022

kzys Jun 23, 2022

austinvazquez Jun 23, 2022

kzys Jun 23, 2022 •

edited

Loading

austinvazquez Jun 23, 2022

kzys Jun 23, 2022

kzys Jun 23, 2022

kzys Jun 23, 2022

kzys Jun 23, 2022

austinvazquez commented Jun 27, 2022

add: remote snapshotter dialer timeouts to service config #684

add: remote snapshotter dialer timeouts to service config #684

Conversation

austinvazquez commented Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginglis13 Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvazquez Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzys Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvazquez commented Jun 27, 2022

austinvazquez commented Jun 22, 2022 •

edited

Loading

ginglis13 Jun 23, 2022 •

edited

Loading

austinvazquez Jun 23, 2022 •

edited

Loading

kzys Jun 23, 2022 •

edited

Loading