Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

Don't create PV AMIs in regions that don't support them #810

Merged
merged 4 commits into from
Feb 9, 2018
Merged

Don't create PV AMIs in regions that don't support them #810

merged 4 commits into from
Feb 9, 2018

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Feb 6, 2018

Add eu-west-3 and cn-northwest-1, which don't, to plume. Add PV AKI ID for cn-north-1, which does.

Copy link
Contributor

@euank euank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks okay to me; had a couple readability comments.

The multiple disconnected pv-support checks seems a little messy, but I don't have any better suggestion.

@@ -439,6 +454,14 @@ func (a *API) CopyImage(sourceImageID string, regions []string) (map[string]stri
return nil, err
}

if *image.VirtualizationType == "paravirtual" {
for _, region := range regions {
if _, ok := akis[region]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well reuse the RegionSupportsPV helper for readability here.

@@ -439,6 +454,14 @@ func (a *API) CopyImage(sourceImageID string, regions []string) (map[string]stri
return nil, err
}

if *image.VirtualizationType == "paravirtual" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ec2.VirtualizationTypeParavirtual looks like the correct const to use here.

destRegions = append(destRegions, region)
if !pv || aws.RegionSupportsPV(region) {
destRegions = append(destRegions, region)
}
Copy link
Contributor

@euank euank Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should have an "else if pv" that logs to indicate each region pv is being skipped for?

We were creating PV images with no AKI ID.
CreatePVImage would previously create an AMI with no AKI ID, and
CopyImage would create an image that never exited pending state.
Assume that the BucketRegion for each AWS partition supports PV.
@bgilbert
Copy link
Contributor Author

bgilbert commented Feb 7, 2018

Updated.

@bgilbert bgilbert merged commit 5cb6ac8 into coreos:master Feb 9, 2018
@bgilbert bgilbert deleted the pv branch February 9, 2018 18:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants