fix(storage): correct direct connectivity check#11152
Conversation
| err = CheckDirectConnectivitySupported(ctx, newMRBucketName) | ||
| if err == nil { | ||
| t.Fatalf("CheckDirectConnectivitySupported: error expected, got nil") | ||
| } |
There was a problem hiding this comment.
So this covers the multi-regional case. Should we add a few more cases? I think we should cover:
- single region, same region as VM (expect success)
- single region, diff region from VM (fail)
Same thing with dual region and multiregion.
We could probably refactor this as a table driven test to make it clearer.
There was a problem hiding this comment.
make sense! i'll work that next. thanks
| } | ||
| if err != nil && !strings.Contains(err.Error(), "direct connectivity not detected") { | ||
| t.Fatalf("CheckDirectConnectivitySupported: failed on a different error %v", err) | ||
| if v, exists := attrs.Value("cloud.platform"); !exists || v.AsString() == "gcp_compute_engine" { |
There was a problem hiding this comment.
Does this actually run in Kokoro or trigger the skip?
There was a problem hiding this comment.
I'll trigger a job in kokoro; i was testing this in GCE first
| } | ||
| gceRegion := v.AsString() | ||
| var otherRegion string | ||
| for _, r := range []string{"us-west1", "us-east1", "us-central1"} { |
There was a problem hiding this comment.
Why does this need 3 regions if we're only picking one other?
Maybe just pick some non-US region that we never use for testing?
| }, | ||
| { | ||
| name: "dual-region bucket", | ||
| attrs: &BucketAttrs{Location: "NAM4"}, |
There was a problem hiding this comment.
Why is this case repeated?
| { | ||
| name: "dual-region bucket", | ||
| attrs: &BucketAttrs{Location: "NAM4"}, | ||
| expectDirectConnectivity: true, |
There was a problem hiding this comment.
I didn't expect NAM4 to return true.
|
Kokoro failed on |
| attrs: &BucketAttrs{Location: "us-west1"}, | ||
| expectDirectConnectivity: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Maybe have a TODO to add NAM4 with true as well?
There was a problem hiding this comment.
NAM4 wouldn't work in this case since it's a pairing of us-central1 and us-east1.
Need to create a new pairing with customPlacementConfig; I'll add a todo for customPlacement though.
Raised in: raj-prince/custom-go-client-benchmark#31
Condition didn't check for empty
{}which failed for multi-region and InterContinental GCE / bucket.Added an additional integration test to check for this case.