Update HCP Packer build labels when re-running Packer on an incomplete build #11584

nywilken · 2022-02-22T21:20:21Z

Previously when running a partial build on multi-cloud build template it was found that build labels were only being applied at the creation for the partially executed build. Leaving all other completed builds with no HCP Packer build labels. This updates how incomplete builds are loaded from the registry and ensures that any defined hcp_packer_registry.build_labels are assigned to the build before starting an actual Packer build.

Related to: #11573

Previously when running a partial build on multi-cloud build template it was found that build labels were only being applied at the creation for the partially executed build. Leaving all other completed builds with no HCP Packer build labels. This updates how incomplete builds are loaded from the registry and ensure that any defined hcp_packer_registry.build_labels are assigned to the build before starting an actual Packer build. Related to: #11573

Before Change ``` WARNING: DATA RACE Write at 0x00c0005421b0 by goroutine 47: github.com/hashicorp/packer/internal/registry.(*MockPackerClientService).PackerServiceCreateBuild() /Users/scrubbed/Development/packer/internal/registry/mock_service.go:173 +0x2b6 github.com/hashicorp/packer/internal/registry.(*Client).CreateBuild() /Users/scrubbed/Development/packer/internal/registry/service.go:169 +0x592 github.com/hashicorp/packer/internal/registry.(*Bucket).CreateInitialBuildForIteration() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:107 +0x204 github.com/hashicorp/packer/internal/registry.(*Bucket).PopulateIteration.func1() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:368 +0x14e github.com/hashicorp/packer/internal/registry.(*Bucket).PopulateIteration·dwrap·1() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:376 +0x58 Previous write at 0x00c0005421b0 by goroutine 46: github.com/hashicorp/packer/internal/registry.(*MockPackerClientService).PackerServiceCreateBuild() /Users/scrubbed/Development/packer/internal/registry/mock_service.go:173 +0x2b6 github.com/hashicorp/packer/internal/registry.(*Client).CreateBuild() /Users/scrubbed/Development/packer/internal/registry/service.go:169 +0x592 github.com/hashicorp/packer/internal/registry.(*Bucket).CreateInitialBuildForIteration() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:107 +0x204 github.com/hashicorp/packer/internal/registry.(*Bucket).PopulateIteration.func1() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:368 +0x14e github.com/hashicorp/packer/internal/registry.(*Bucket).PopulateIteration·dwrap·1() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:376 +0x58 Goroutine 47 (running) created at: github.com/hashicorp/packer/internal/registry.(*Bucket).PopulateIteration() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:362 +0x5c7 github.com/hashicorp/packer/internal/registry.TestBucket_UpdateLabelsForBuild_withMultipleBuilds() /Users/scrubbed/Development/packer/internal/registry/types.bucket_test.go:179 +0xf7 testing.tRunner() /usr/local/go/src/testing/testing.go:1259 +0x22f testing.(*T).Run·dwrap·21() /usr/local/go/src/testing/testing.go:1306 +0x47 Goroutine 46 (finished) created at: github.com/hashicorp/packer/internal/registry.(*Bucket).PopulateIteration() /Users/scrubbed/Development/packer/internal/registry/types.bucket.go:362 +0x5c7 github.com/hashicorp/packer/internal/registry.TestBucket_UpdateLabelsForBuild_withMultipleBuilds() /Users/scrubbed/Development/packer/internal/registry/types.bucket_test.go:179 +0xf7 testing.tRunner() /usr/local/go/src/testing/testing.go:1259 +0x22f testing.(*T).Run·dwrap·21() /usr/local/go/src/testing/testing.go:1306 +0x47 ================== ```

internal/registry/mock_service.go

sylviamoss · 2022-02-23T14:12:38Z

internal/registry/types.bucket_test.go

+			noDiffExpected: false,
+		},
+		{
+			desc:      "existing incomplete build with extra previously set build label",


Let me see if I got it right, if I rerun an incomplete iteration, can I add more build labels the second time I try building it?

No wait, is it the other way around? If I'm trying to complete an existing iteration, I can't update labels?

It is actually the first one, but the example is a little confusing. Right now if a user sets one build label and the build fails for whatever reason the build label will get persisted. Now lets say when they rerun the failed build they decide to add a few extra build labels the new labels will get added but the old label will not get removed.

If the value for the previous label is modified the old persisted label will get updated to reflect the new value.

Ahh got it! I understood that aftewards. Makes sense!

sylviamoss · 2022-02-23T14:28:02Z

internal/registry/types.bucket.go

 				log.Printf("[TRACE] build %s already exists in Packer registry, continuing...", name)
 				return
 			}

 			errs = multierror.Append(errs, err)
 		}(buildName)
+		wg.Wait()


Why did this one move inside the for?

This was moved to fix a race condition that is introduced by calling CreateBuild on the MockService. By spawning off multiple routines for the CreateBuild the Mock Data is overwritten before it can be read resulting in a data race. This change makes it a bit more sequential. Maybe the correct change should be to leave it as it as and add a mutex to the Mock Client to prevent overwrites.

Yeah, now it will always wait for the goroutine before proceeding with the loop. I would either remove th goroutine here or fix the mutex. 🤔

So I updated the Mock service a little to be a bit more concurrent. It is a hack TBH but it works for the testing purposes. In doing so I found a race condition with the appending of errors so that it good.

sylviamoss · 2022-02-23T14:30:29Z

internal/registry/types.builds.go

+func (b *Build) IsNotDone() bool {
+	hasBuildID := b.ID != ""
+	hasImages := len(b.Images) == 0
+	isNotDone := b.Status != models.HashicorpCloudPackerBuildStatusDONE


Would this validation be enough?

I believe so. Do you think we should validate something else?

The rationale is as follows:

A build without and Id has not been registered on the registry therefore it doesn't exist and not worth considering.

A build that exists on the registry with no images is by the service specs not done.

An image can only be set to the done status if it has at least one image.

Sorry, I confused you. I was saying that this validation isNotDone := b.Status != models.HashicorpCloudPackerBuildStatusDONE I expected to be enough 🤔
I'm wanted to know if you had any other use cases that this wasn't enough.

Ah got it. This might be artifact of an edge case earlier on where we may have created or put a build in a done status without having images.But I can't recall immediately. I believe the API does not allow a build to be put in the DONE status without images so that check might not be needed

I have to review previous commits and notes before looking to remove the other conditions 😄

It is a simple set of changes to make it work for the current testing use cases. If we need to we can move the Called fields to counters or re-architect the mock. This change also fixes a race condition when appending to the Slice of errs when calling PopulateIteration.

internal/registry/types.bucket.go

Only append to errs if err is not nil Co-authored-by: Sylvia Moss <moss@hashicorp.com>

sylviamoss

🎉

azr · 2022-02-28T14:39:53Z

Nice one !

github-actions · 2022-03-31T02:18:06Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

nywilken added 2 commits February 22, 2022 15:40

Add test case for overwriting build labels

87ef0ac

nywilken requested a review from a team as a code owner February 22, 2022 21:20

nywilken changed the title ~~wilken/fix build labels for partial builds~~ Update HCP Packer build labels when re-running Packer on an incomplete build Feb 22, 2022

Update tests to call CreateInitialBuild for non-existing builds

92581c3

vercel bot temporarily deployed to Preview February 22, 2022 21:35 Inactive

Rename test case to TestBucket_PopulateIteration

e75fb06

vercel bot temporarily deployed to Preview February 22, 2022 21:50 Inactive

vercel bot temporarily deployed to Preview February 23, 2022 00:06 Inactive

vercel bot temporarily deployed to Preview February 23, 2022 02:16 Inactive

Add methods for managing builds on an Iteration

c80d9e0

nywilken force-pushed the wilken/fix-build-labels-for-partial-builds branch from df8ae13 to c80d9e0 Compare February 23, 2022 11:06

vercel bot temporarily deployed to Preview February 23, 2022 11:06 Inactive

nywilken requested a review from sylviamoss February 23, 2022 11:07

sylviamoss reviewed Feb 23, 2022

View reviewed changes

internal/registry/mock_service.go Show resolved Hide resolved

sylviamoss reviewed Feb 23, 2022

View reviewed changes

vercel bot temporarily deployed to Preview February 23, 2022 18:35 Inactive

nywilken force-pushed the wilken/fix-build-labels-for-partial-builds branch from 112f368 to a3898f6 Compare February 23, 2022 18:53

vercel bot temporarily deployed to Preview February 23, 2022 18:54 Inactive

Update test case descriptions

8f82e5a

vercel bot temporarily deployed to Preview February 24, 2022 05:51 Inactive

sylviamoss reviewed Feb 25, 2022

View reviewed changes

internal/registry/types.bucket.go Outdated Show resolved Hide resolved

Apply suggestions from code review

2addd53

Only append to errs if err is not nil Co-authored-by: Sylvia Moss <moss@hashicorp.com>

vercel bot temporarily deployed to Preview February 25, 2022 15:13 Inactive

sylviamoss approved these changes Feb 25, 2022

View reviewed changes

Fix linting issues

76ca3b3

vercel bot temporarily deployed to Preview February 25, 2022 20:47 Inactive

nywilken merged commit dd525fb into master Feb 25, 2022

nywilken deleted the wilken/fix-build-labels-for-partial-builds branch February 25, 2022 20:50

github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update HCP Packer build labels when re-running Packer on an incomplete build #11584

Update HCP Packer build labels when re-running Packer on an incomplete build #11584

nywilken commented Feb 22, 2022 •

edited

Loading

sylviamoss Feb 23, 2022

sylviamoss Feb 23, 2022

nywilken Feb 23, 2022

sylviamoss Feb 23, 2022

sylviamoss Feb 23, 2022

nywilken Feb 23, 2022

sylviamoss Feb 23, 2022

nywilken Feb 23, 2022

sylviamoss Feb 23, 2022

nywilken Feb 23, 2022

sylviamoss Feb 25, 2022

nywilken Feb 25, 2022

sylviamoss left a comment

azr commented Feb 28, 2022

github-actions bot commented Mar 31, 2022

Update HCP Packer build labels when re-running Packer on an incomplete build #11584

Update HCP Packer build labels when re-running Packer on an incomplete build #11584

Conversation

nywilken commented Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylviamoss left a comment

Choose a reason for hiding this comment

azr commented Feb 28, 2022

github-actions bot commented Mar 31, 2022

nywilken commented Feb 22, 2022 •

edited

Loading