Adding retry for eni plugin to acquire mac address #62
Conversation
| // express or implied. See the License for the specific language governing | ||
| // permissions and limitations under the License. | ||
|
|
||
| package utils |
There was a problem hiding this comment.
I wonder if now might be the right time to split this code out into a standalone library. Copy & Paste between projects is not so good.
There was a problem hiding this comment.
yeah, I don't like copy and paste either, but it will require additional work which probably won't happen within this pr, I will create an issue to follow up.
plugins/eni/commands/commands.go
Outdated
| @@ -152,6 +163,63 @@ func add(args *skel.CmdArgs, engine engine.Engine, dhclient engine.DHClient) err | |||
| return cnitypes.PrintResult(result, conf.CNIVersion) | |||
| } | |||
|
|
|||
| // getMACAddressOfENIAndVerify acquires the mac address of eni and verify the ip address information | |||
| func getMACAddressOfENIAndVerify(conf *types.NetConf, engine engine.Engine, timeout time.Duration) (string, error) { | |||
There was a problem hiding this comment.
I don't really like the name getMACAddressOfENIAndVerify. It feels unnecessarily verbose and isn't descriptive of the fact that we're also verifying IP info (not just MAC addr). I'd name this getENIMetadata and leave the "verify" part as implicit. Or getAndVerifyENIMetadata maybe, if you really want "verify" to be part of the function name.
plugins/eni/commands/commands.go
Outdated
| return nil | ||
| } | ||
|
|
||
| _, ok := err.(*engine.UnmappedMACAddressError) |
There was a problem hiding this comment.
I realize that we don't actually have the issue here, but this style of type assertion gives me flashbacks to aws/amazon-ecs-agent#910. Interface conversions are a less error prone way to perform type assertions, IMO.
There was a problem hiding this comment.
Can you address @nmeyerhans 's comment here as well?
plugins/eni/commands/commands.go
Outdated
| ec2InstanceMetadataBackoffMax = 1 * time.Second | ||
| ec2InstanceMetadataBackoffMultiple = 2 | ||
| ec2InstanceMetadataBackoffJitter = 0.2 | ||
| ec2InstanceMetadataTimeout = 5 * time.Second |
There was a problem hiding this comment.
How did you arrive at a 5 second timeout?
There was a problem hiding this comment.
@aaithal and I did some tests, the worst case I saw is 1.5s. So I put 5s here to be safe, but it's changeable.
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() |
There was a problem hiding this comment.
nice. we need to fix this in the agent repo as well.
plugins/eni/commands/commands.go
Outdated
| @@ -27,6 +30,14 @@ import ( | |||
| "github.com/pkg/errors" | |||
| ) | |||
|
|
|||
| const ( | |||
| ec2InstanceMetadataBackoffMin = 500 * time.Millisecond | |||
There was a problem hiding this comment.
can you decrease this to 100ms? half a second seems really long to wait here.
plugins/eni/commands/commands.go
Outdated
| return nil | ||
| } | ||
|
|
||
| _, ok := err.(*engine.UnmappedMACAddressError) |
There was a problem hiding this comment.
Can you address @nmeyerhans 's comment here as well?
This PR adds backoff retry logic for retrieving mac address of the eni from ec2 instance metadata.