Add `use_shallow_replication` field for Compute Gallery/SIG builds, fix replica count type #337

JenGoldstrich · 2023-08-28T16:51:05Z

Adds flag to allow SIG builds to use shallow replication https://learn.microsoft.com/en-us/azure/virtual-machines/shared-image-galleries?tabs=azure-cli#shallow-replication

Fix a bug where ReplicaCount was cast as int32 then an int64 leading to the wrong value being sent to the API

Closes #313
Closes #341

maxilampert · 2023-09-11T10:27:26Z

Maybe a config check should be added that source image/vm and image gallery replication destination region must be the same otherwise publishing will fail with an error?

JenGoldstrich · 2023-09-29T22:24:45Z

builder/azure/arm/config.go

@@ -215,7 +217,7 @@ type Config struct {
 	// The number of replicas of the Image Version to be created per region. This
 	// property would take effect for a region when regionalReplicaCount is not specified.
 	// Replica count must be between 1 and 100, but 50 replicas should be sufficient for most use cases.
-	SharedGalleryImageVersionReplicaCount int32 `mapstructure:"shared_image_gallery_replica_count" required:"false"`
+	SharedGalleryImageVersionReplicaCount int64 `mapstructure:"shared_image_gallery_replica_count" required:"false"`


In the new SDK this value is an int64, because this was being parsed as an int32, stored in the state bag, and then in step_publish_to_shared_image_gallery it was retrieved as an int64, this int would be incorrectly registered, leading to #341

JenGoldstrich · 2023-09-29T23:05:57Z

builder/azure/arm/testdata/arm_linux_specialized.pkr.hcl

@@ -4,25 +4,26 @@ locals { timestamp = regex_replace(timestamp(), "[- TZ:]", "") }



Ran packer fmt on these two templates after changing fields

builder/azure/arm/step_publish_to_shared_image_gallery.go

JenGoldstrich · 2023-09-29T23:14:36Z

Maybe a config check should be added that source image/vm and image gallery replication destination region must be the same otherwise publishing will fail with an error?

@maxilampert I agree that sounds like a good change, I think we should add it in a different PR though as this already changes a few things.

nywilken · 2023-10-02T18:55:58Z

Maybe a config check should be added that source image/vm and image gallery replication destination region must be the same otherwise publishing will fail with an error?

@JenGoldstrich @maxilampert does this mean that in order for shallow replication to work the replication_regions attribute can not contain a region that is not the same as the source image? If so, what is the difference between using shallow replication mode and not specifying a replication at all?

nywilken

This is looking good. I left a few suggestions and questions about the feature. I know there is a small bug fix rolled into this change which we can break out if needed.

builder/azure/arm/step_publish_to_shared_image_gallery.go

docs-partials/builder/azure/arm/SharedImageGalleryDestination-not-required.mdx

maxilampert · 2023-10-03T15:34:52Z

@JenGoldstrich @maxilampert does this mean that in order for shallow replication to work the replication_regions attribute can not contain a region that is not the same as the source image? If so, what is the difference between using shallow replication mode and not specifying a replication at all?

@JenGoldstrich @nywilken Yes correct source and destination must be the same and there can be no additional regions.
There is one difference between using shallow or full is that in case of shallow the replica count can only be 1 otherwise it fails. I don't know if there is any speed(or other) differences if you would set all those options with the full replication mode or use shallow.

…plica count at int64 to fix it being parsed as the wrong number Fix sig replica count - use shallow replication in acc test - fix units Make generate Add tests Update docs-partials/builder/azure/arm/SharedImageGalleryDestination-not-required.mdx Co-authored-by: Wilken Rivera <dev@wilkenrivera.com> Update builder/azure/arm/step_publish_to_shared_image_gallery.go Co-authored-by: Wilken Rivera <dev@wilkenrivera.com> Implement the rest of Wilken's suggested changes Add validation, fix acceptance tests with new name for shallow replication field make generate Fix make generate Fix make generate after rebasing on main Add comments and tests

JenGoldstrich · 2023-10-16T19:23:14Z

@maxilampert so I don't think we'll need to validate that specifically because the resulting image version is determined by the build location, if the source image and the build location differ the build will fail during VM publishing

As to the difference between Shallow replication & full replication to 1 region, the main difference is that images using shallow replication can not have their replication information updated after the fact, whereas full replicated images can always go update their replica counts/regions

nywilken

I have a few suggestions on the documentation and a few small nits but otherwise looks good. Approving in advance to unblock the merge when you've applied the suggestions.

builder/azure/arm/config.go

.web-docs/components/builder/arm/README.md

builder/azure/arm/builder.go

builder/azure/arm/config.go

builder/azure/arm/config_test.go

builder/azure/arm/step_publish_to_shared_image_gallery.go

Co-authored-by: Wilken Rivera <dev@wilkenrivera.com>

…tion

…bit simpler, be stricter about requiring replica count = 1 rather than > 1 for shallow replication

JenGoldstrich commented Sep 29, 2023

View reviewed changes

JenGoldstrich force-pushed the shallow_replication branch from da111e7 to 7cf0c40 Compare September 29, 2023 22:26

JenGoldstrich changed the title ~~Add Support for Shallow Replication to Compute (Shared Image) Gallery~~ Add Support for Shallow Replication for Compute Galleries, Fix Sig Replication Count Type Conversion Sep 29, 2023

JenGoldstrich changed the title ~~Add Support for Shallow Replication for Compute Galleries, Fix Sig Replication Count Type Conversion~~ Add Support for Shallow Replication for Compute Galleries, Fix Replication Count Type Conversion Sep 29, 2023

JenGoldstrich changed the title ~~Add Support for Shallow Replication for Compute Galleries, Fix Replication Count Type Conversion~~ Add Support for Shallow Replication for Compute Galleries, Fix Replication Count not being used correctly Sep 29, 2023

JenGoldstrich commented Sep 29, 2023

View reviewed changes

builder/azure/arm/step_publish_to_shared_image_gallery.go Outdated Show resolved Hide resolved

JenGoldstrich marked this pull request as ready for review September 29, 2023 23:07

JenGoldstrich requested a review from a team as a code owner September 29, 2023 23:07

nywilken suggested changes Oct 2, 2023

View reviewed changes

JenGoldstrich force-pushed the shallow_replication branch 2 times, most recently from a073050 to ee73d8e Compare October 13, 2023 23:59

JenGoldstrich force-pushed the shallow_replication branch from ff7d7b6 to c5fdc78 Compare October 16, 2023 19:19

JenGoldstrich requested a review from nywilken October 16, 2023 19:21

JenGoldstrich changed the title ~~Add Support for Shallow Replication for Compute Galleries, Fix Replication Count not being used correctly~~ Add Shallow Replication field for Compute Galleries, fix replica count type Oct 17, 2023

nywilken approved these changes Oct 17, 2023

View reviewed changes

nywilken added bug enhancement automated and removed automated labels Oct 17, 2023

JenGoldstrich and others added 4 commits October 17, 2023 11:03

Apply suggestions from code review

a5e0c5c

Co-authored-by: Wilken Rivera <dev@wilkenrivera.com>

Update builder/azure/arm/step_publish_to_shared_image_gallery.go

e413c14

Co-authored-by: Wilken Rivera <dev@wilkenrivera.com>

Fix brackets after code review, gofmt config file, and make generate

2d3f35a

Fix replica count check should only happen when using shallow replica…

4044fa3

…tion

Shallow Replication region check didn't work so I changed it to be a …

9a58704

…bit simpler, be stricter about requiring replica count = 1 rather than > 1 for shallow replication

JenGoldstrich changed the title ~~Add Shallow Replication field for Compute Galleries, fix replica count type~~ Add use_shallow_replication field for Compute Gallery/SIG builds, fix replica count type Oct 17, 2023

JenGoldstrich merged commit d042ec9 into main Oct 17, 2023
12 checks passed

JenGoldstrich deleted the shallow_replication branch October 17, 2023 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `use_shallow_replication` field for Compute Gallery/SIG builds, fix replica count type #337

Add `use_shallow_replication` field for Compute Gallery/SIG builds, fix replica count type #337

JenGoldstrich commented Aug 28, 2023 •

edited

Loading

maxilampert commented Sep 11, 2023

JenGoldstrich Sep 29, 2023

JenGoldstrich Sep 29, 2023

JenGoldstrich commented Sep 29, 2023

nywilken commented Oct 2, 2023

nywilken left a comment

maxilampert commented Oct 3, 2023

JenGoldstrich commented Oct 16, 2023

nywilken left a comment

		@@ -4,25 +4,26 @@ locals { timestamp = regex_replace(timestamp(), "[- TZ:]", "") }

Add use_shallow_replication field for Compute Gallery/SIG builds, fix replica count type #337

Add use_shallow_replication field for Compute Gallery/SIG builds, fix replica count type #337

Conversation

JenGoldstrich commented Aug 28, 2023 • edited Loading

maxilampert commented Sep 11, 2023

JenGoldstrich Sep 29, 2023

Choose a reason for hiding this comment

JenGoldstrich Sep 29, 2023

Choose a reason for hiding this comment

JenGoldstrich commented Sep 29, 2023

nywilken commented Oct 2, 2023

nywilken left a comment

Choose a reason for hiding this comment

maxilampert commented Oct 3, 2023

JenGoldstrich commented Oct 16, 2023

nywilken left a comment

Choose a reason for hiding this comment

Add `use_shallow_replication` field for Compute Gallery/SIG builds, fix replica count type #337

Add `use_shallow_replication` field for Compute Gallery/SIG builds, fix replica count type #337

JenGoldstrich commented Aug 28, 2023 •

edited

Loading