New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use SDK's InstanceReady waiter #5773
Conversation
|
||
func waitUntilInstanceReady(ec2conn *ec2.EC2, instanceID, instanceType string) error { | ||
statusOKMap := make(map[string]struct{}) | ||
statusOKMap["c5"] = struct{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a comment explaining why this convoluted extra code for c5. something like // c5 instances need us to use the time-sucking WaitUntilStatusOK feature instead of just waiting for SSH to become available
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure the new m5 instance type is also backed by the new hypervisor. Ought to be easy to test if the issue is limited to c5, or if a more general approach is needed.
Apart from future-proofing the code with a small "why" comment, I think this is fine. I know you weren't a fan of special casing it, but it's probably the best way to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to check if the virtualization type is the new KVM based type. I'm assuming all new instance types on that will be affected by this.
@@ -115,7 +115,6 @@ Packer to work: | |||
"ec2:DeregisterImage", | |||
"ec2:DescribeImageAttribute", | |||
"ec2:DescribeImages", | |||
"ec2:DescribeInstanceStatus", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this still needed for C5's?
Why does the KVM hypervisor make the status check necessary? Why isn't SSH access enough? |
@patrickf55places because the IP isn't available until some time after the
will investigate if that's feasible, but it sounds way better |
I've taken this in an odd direction. This replaces our home grown instance ready waiter with the one from the SDK. This brings build times back to where they were in the previous release, but without the errors. I cannot for the life of me reproduce the error with this patch, but since the error isn't falsifiable, it's going to be hard to tell if this fixes it completely. I'll keep running builds in a loop and see if I see any problems, but I think we'll probably be good with this patch |
I very occasionally get ssh timeout errors, but I don't think these are related to discovering the public IP
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way better and simpler. Verified it works with both c5 instance types and older instance types.
👍 had same issue with a t2.nano build, was missing "ec2:DescribeInstanceStatus" permission on packer IAM user |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Closes #5705
This replaces our home grown instance ready waiter with the one from the SDK. This brings build times back to where they were in the previous release, but without the errors.
I cannot for the life of me reproduce the error with this patch, but since the error isn't falsifiable, it's going to be hard to tell if this fixes it completely. I'll keep running builds in a loop and see if I see any problems, but I think we'll probably be good with this patch