Traffic shaping for bitswap plan #352
Conversation
There was a problem hiding this comment.
I see that @Stebalien approved and merged while I was reviewing, but here's my feedback. (I was going to approve too).
|
|
||
| latency := time.Duration(runenv.IntParam("latency_ms")) * time.Millisecond | ||
| bandwidth := runenv.IntParam("bandwidth_mb") | ||
| writer.Write(sync.NetworkSubtree(hostname), &sync.NetworkConfig{ |
There was a problem hiding this comment.
At some point, we'll probably want some API sugar for the network operations, in the same vein as WaitNetworkInitialized().
| @@ -170,11 +185,12 @@ func Transfer(runenv *runtime.RunEnv) { | |||
| runenv.OK() | |||
| } | |||
|
|
|||
| func setupSeed(ctx context.Context, node *utils.Node, fileSize int) (cid.Cid, error) { | |||
| func setupSeed(ctx context.Context, node *utils.Node, fileSize int, runenv *runtime.RunEnv) (cid.Cid, error) { | |||
There was a problem hiding this comment.
Do we intend to use the runenv in this function going forward?
There was a problem hiding this comment.
Good catch, I'll remove in the next PR I do
| return err | ||
| } | ||
|
|
||
| // TODO: just put the hostname inside the runenv? |
There was a problem hiding this comment.
We can certainly do that with the local runners, but not sure how this would work with k8s. @nonsense?
There was a problem hiding this comment.
By the way I copied this from the DHT testplan, as with a bunch of the code in this utils package. It would be good to put it all in a central location somewhere
There was a problem hiding this comment.
This will be problematic in case we run concurrently the same testplan in multiple namespaces in the future. pod names are unique within a namespace, but we have sidecar running per host right now. os.Hostname returns the pod name in k8s by default (https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-hostname-and-subdomain-fields). I wouldn't worry too much about it now, but maybe change the comment to say unique testplan id, rather than hostname, because as far as I understand we just need a unique testplan identifier here, so that we have a unique subtree, and we use hostname as such.
Allows client to set latency and bandwidth as test parameters