Skip to content
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

Fix replicated_regions not using shared_image_gallery_replica_count for build location when its specified in the list #438

Merged

Conversation

JenGoldstrich
Copy link
Contributor

The logic that handles the backwards compatibility for creating the new Target Regions block from the replicated regions list doesn't add the replica count to the default region. This causes the default region to always have a replica count of 1, this is currently a bit iffy to unit test and should probably be unwound into its own step still, but I don't think we should leave this functionality broken in the mean time

@JenGoldstrich JenGoldstrich requested a review from a team as a code owner August 22, 2024 19:05
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

I see what you are doing here. Maybe this changed with an upstream API change but when I first tested b.config.SharedGalleryImageVersionReplicaCount is used as the default replica count if a target region is left unset. But I see you are just reusing the value so that they match for the default build target.

@JenGoldstrich JenGoldstrich marked this pull request as draft August 22, 2024 20:56
@JenGoldstrich JenGoldstrich changed the title Fix replicated_regions not using shared_image_gallery_replica_count in build region Fix replicated_regions not using shared_image_gallery_replica_count when build region is included in list Aug 22, 2024
@JenGoldstrich JenGoldstrich marked this pull request as ready for review August 22, 2024 21:29
@JenGoldstrich JenGoldstrich changed the title Fix replicated_regions not using shared_image_gallery_replica_count when build region is included in list Fix replicated_regions not using shared_image_gallery_replica_count Aug 22, 2024
@JenGoldstrich JenGoldstrich changed the title Fix replicated_regions not using shared_image_gallery_replica_count Fix replicated_regions not using shared_image_gallery_replica_count for build location when its specified in the list Aug 22, 2024
@JenGoldstrich
Copy link
Contributor Author

So to clarify, this is only for the global replica case, since setting the replica_count field is not allowed when setting target_regions

We actually send this currently for the build region if its not specified in the replication_regions, the plugin then adds it to the replication regions here

// SIG requires that replication regions include the region in which the created image version resides
if foundMandatoryReplicationRegion == false {
normalizedRegions = append(normalizedRegions, TargetRegion{
Name: buildLocation,
DiskEncryptionSetId: b.config.DiskEncryptionSetId,
ReplicaCount: b.config.SharedGalleryImageVersionReplicaCount,
})
}
, so this is inconsistent based on when we specify the build location in that list or not, which is incorrect logic.

I think we should clean this up more, to just pass in the raw values the API expects, but I need to do more research and refactoring, so I am going to merge this for now to fix the bug

@JenGoldstrich JenGoldstrich merged commit b7642d6 into main Aug 22, 2024
12 checks passed
@JenGoldstrich JenGoldstrich deleted the fix_old_replica_count_not_applying_to_base_region branch August 22, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants