From 59613d4c4f7dc7893b5b7a19a1f209236234422b Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Wed, 23 Sep 2026 11:37:31 -0700 Subject: [PATCH 01/12] Add --fix to create-env to force a stemcell re-upload The stemcell repo in the deployment state file records stemcells by name and version only. It has no notion of which IaaS -- or, for vSphere, which vCenter -- the image was actually materialized in. When a deployment is repointed at different infrastructure, the recorded CID names an image that does not exist there, but the name and version still match. Upload therefore skips, hands the stale CID to create_vm, and the CPI fails because it cannot find the stemcell. For create-env this is particularly unpleasant: the old VM has already been deleted by the time it surfaces, so the environment is left with nothing deployed. Until now the only way out was to bump the stemcell version, so that a version change misses the cache. Add --fix, mirroring `bosh upload-stemcell --fix`, which forces a fresh create_stemcell against whatever the CPI is currently pointed at. Two details worth recording: The stale record is deleted before the new image is created. StemcellRepo Save rejects a duplicate name/version pair, so without this the upload would succeed and then fail while saving, leaving an orphaned image. Only the record is removed, not the image. CloudStemcell#Delete would ask the CPI to delete the old CID, which against infrastructure that never had it is at best a no-op and at worst destroys the image the deployment can still be rolled back onto. --fix also bypasses the "no deployment, stemcell or release changes" short-circuit. Repointing at new infrastructure need not change the manifest, releases or stemcell version, so otherwise a fix run would be skipped before it reached the upload. Co-Authored-By: Claude Opus 5 --- cmd/create_env.go | 2 +- cmd/create_env_test.go | 24 +++++++++ cmd/deployment_preparer.go | 8 +-- cmd/opts/opts.go | 1 + cmd/opts/opts_test.go | 6 +++ stemcell/manager.go | 29 ++++++++-- stemcell/manager_test.go | 73 +++++++++++++++++++++++--- stemcell/stemcellfakes/fake_manager.go | 18 ++++--- 8 files changed, 138 insertions(+), 23 deletions(-) diff --git a/cmd/create_env.go b/cmd/create_env.go index c3ed3a7bf..025daf936 100644 --- a/cmd/create_env.go +++ b/cmd/create_env.go @@ -24,5 +24,5 @@ func (c *CreateEnvCmd) Run(stage boshui.Stage, opts CreateEnvOpts) error { depPreparer := c.envProvider(opts.Args.Manifest.Path, opts.StatePath, opts.VarFlags.AsVariables(), opts.OpsFlags.AsOp()) //nolint:staticcheck - return depPreparer.PrepareDeployment(stage, opts.Recreate, opts.RecreatePersistentDisks, opts.SkipDrain) + return depPreparer.PrepareDeployment(stage, opts.Recreate, opts.RecreatePersistentDisks, opts.Fix, opts.SkipDrain) } diff --git a/cmd/create_env_test.go b/cmd/create_env_test.go index 835d63afc..c207fdb36 100644 --- a/cmd/create_env_test.go +++ b/cmd/create_env_test.go @@ -585,6 +585,8 @@ var _ = Describe("CreateEnvCmd", func() { err := command.Run(fakeStage, defaultCreateEnvOpts) Expect(err).ToNot(HaveOccurred()) Expect(mockStemcellManager.UploadCallCount()).To(Equal(1)) + _, _, gotFix := mockStemcellManager.UploadArgsForCall(0) + Expect(gotFix).To(BeFalse()) }) It("adds a new 'deploying' event logger stage", func() { @@ -688,6 +690,28 @@ var _ = Describe("CreateEnvCmd", func() { Expect(err).NotTo(HaveOccurred()) Expect(mockDeployer.DeployCallCount()).To(Equal(1)) }) + + // Repointing at new infrastructure need not change the manifest, + // releases or stemcell version, so without this a fix run would be + // skipped before it ever reached the stemcell upload. + It("deploys if `fix` flag is specified", func() { + defaultCreateEnvOpts.Fix = true + + err := command.Run(fakeStage, defaultCreateEnvOpts) + Expect(err).NotTo(HaveOccurred()) + Expect(mockDeployer.DeployCallCount()).To(Equal(1)) + }) + + It("passes `fix` through to the stemcell upload", func() { + defaultCreateEnvOpts.Fix = true + + err := command.Run(fakeStage, defaultCreateEnvOpts) + Expect(err).NotTo(HaveOccurred()) + + Expect(mockStemcellManager.UploadCallCount()).To(Equal(1)) + _, _, gotFix := mockStemcellManager.UploadArgsForCall(0) + Expect(gotFix).To(BeTrue()) + }) }) Context("when parsing the cpi deployment manifest fails", func() { diff --git a/cmd/deployment_preparer.go b/cmd/deployment_preparer.go index a132b0b0f..4ceb86823 100644 --- a/cmd/deployment_preparer.go +++ b/cmd/deployment_preparer.go @@ -100,7 +100,7 @@ type DeploymentPreparer struct { targetProvider biinstall.TargetProvider } -func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, recreate bool, recreatePersistentDisks bool, skipDrain bool) (err error) { +func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, recreate bool, recreatePersistentDisks bool, fix bool, skipDrain bool) (err error) { c.ui.BeginLinef("Deployment state: '%s'\n", c.deploymentStateService.Path()) if !c.deploymentStateService.Exists() { @@ -183,7 +183,7 @@ func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, recreate bool, return bosherr.WrapError(err, "Checking if deployment has changed") } - if isDeployed && !recreate && !recreatePersistentDisks { + if isDeployed && !recreate && !recreatePersistentDisks && !fix { c.ui.BeginLinef("No deployment, stemcell or release changes. Skipping deploy.\n") return nil } @@ -204,6 +204,7 @@ func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, recreate bool, deploymentManifest, manifestSHA, skipDrain, + fix, stage, cloud, ) @@ -234,12 +235,13 @@ func (c *DeploymentPreparer) deploy( deploymentManifest bideplmanifest.Manifest, manifestSHA string, skipDrain bool, + fix bool, stage biui.Stage, cloud bicloud.Cloud, ) (err error) { stemcellManager := c.stemcellManagerFactory.NewManager(cloud) - cloudStemcell, err := stemcellManager.Upload(extractedStemcell, stage) + cloudStemcell, err := stemcellManager.Upload(extractedStemcell, stage, fix) if err != nil { return err } diff --git a/cmd/opts/opts.go b/cmd/opts/opts.go index 7123e7dfa..7c9cae10d 100644 --- a/cmd/opts/opts.go +++ b/cmd/opts/opts.go @@ -197,6 +197,7 @@ type CreateEnvOpts struct { StatePath string `long:"state" value-name:"PATH" description:"State file path"` Recreate bool `long:"recreate" description:"Recreate VM in deployment"` RecreatePersistentDisks bool `long:"recreate-persistent-disks" description:"Recreate persistent disks in the deployment"` + Fix bool `long:"fix" description:"Recreate the stemcell in the IaaS even if the state file already records one"` PackageDir string `long:"package-dir" value-name:"DIR" description:"Package cache location override"` cmd } diff --git a/cmd/opts/opts_test.go b/cmd/opts/opts_test.go index 23a211524..aff4a5eb3 100644 --- a/cmd/opts/opts_test.go +++ b/cmd/opts/opts_test.go @@ -868,6 +868,12 @@ var _ = Describe("Opts", func() { `long:"skip-drain" description:"Skip running drain and pre-stop scripts"`, )) }) + + It("has --fix", func() { + Expect(getStructTagForName("Fix", opts)).To(Equal( + `long:"fix" description:"Recreate the stemcell in the IaaS even if the state file already records one"`, + )) + }) }) Describe("CreateEnvArgs", func() { diff --git a/stemcell/manager.go b/stemcell/manager.go index 918a63146..a1709ed9c 100644 --- a/stemcell/manager.go +++ b/stemcell/manager.go @@ -18,7 +18,7 @@ import ( type Manager interface { FindCurrent() ([]CloudStemcell, error) - Upload(ExtractedStemcell, biui.Stage) (CloudStemcell, error) + Upload(ExtractedStemcell, biui.Stage, bool) (CloudStemcell, error) FindUnused() ([]CloudStemcell, error) DeleteUnused(biui.Stage) error } @@ -54,7 +54,15 @@ func (m *manager) FindCurrent() ([]CloudStemcell, error) { // Upload stemcell to an IAAS. It does the following steps: // 1) uploads the stemcell to the cloud (if needed), // 2) saves a record of the uploaded stemcell in the repo -func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.Stage) (cloudStemcell CloudStemcell, err error) { +// +// The repo records stemcells by name and version only -- it has no notion of +// which IaaS, or which vCenter, the image was actually materialized in. When +// the deployment is repointed at different infrastructure the recorded CID +// names an image that does not exist there, but the name and version still +// match, so the upload is skipped and the stale CID is handed to create_vm. +// Passing fix forces a fresh create_stemcell against whatever the CPI is now +// talking to. +func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.Stage, fix bool) (cloudStemcell CloudStemcell, err error) { manifest := extractedStemcell.Manifest() stageName := fmt.Sprintf("Uploading stemcell '%s/%s'", manifest.Name, manifest.Version) err = uploadStage.Perform(stageName, func() error { @@ -63,11 +71,26 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S return bosherr.WrapError(err, "Finding existing stemcell record in repo") } - if found { + if found && !fix { cloudStemcell = NewCloudStemcell(foundStemcellRecord, m.repo, m.cloud) return biui.NewSkipStageError(bosherr.Errorf("Found stemcell: %#v", foundStemcellRecord), "Stemcell already uploaded") } + if found { + // Drop the stale record before uploading: Save rejects a duplicate + // name/version pair, so it would fail after the new image had + // already been created. + // + // Only the record is removed, not the image. CloudStemcell.Delete + // would ask the CPI to delete the old CID, which at best is a + // no-op against infrastructure that never had it and at worst + // destroys the image the deployment can still be rolled back onto. + err = m.repo.Delete(foundStemcellRecord) + if err != nil { + return bosherr.WrapErrorf(err, "Deleting stale stemcell record (name=%s, version=%s, cid=%s)", foundStemcellRecord.Name, foundStemcellRecord.Version, foundStemcellRecord.CID) + } + } + cid, err := m.cloud.CreateStemcell(filepath.Join(extractedStemcell.GetExtractedPath(), "image"), manifest.CloudProperties) if err != nil { return bosherr.WrapErrorf(err, "creating stemcell (%s %s)", manifest.Name, manifest.Version) diff --git a/stemcell/manager_test.go b/stemcell/manager_test.go index 0911c7848..df40588fb 100644 --- a/stemcell/manager_test.go +++ b/stemcell/manager_test.go @@ -80,7 +80,7 @@ var _ = Describe("Manager", func() { }) It("uploads the stemcell to the infrastructure and returns the cid", func() { - cloudStemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage) + cloudStemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).ToNot(HaveOccurred()) Expect(cloudStemcell).To(Equal(expectedCloudStemcell)) @@ -91,7 +91,7 @@ var _ = Describe("Manager", func() { }) It("saves the stemcell record in the stemcellRepo", func() { - cloudStemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage) + cloudStemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).ToNot(HaveOccurred()) Expect(cloudStemcell).To(Equal(expectedCloudStemcell)) @@ -108,7 +108,7 @@ var _ = Describe("Manager", func() { }) It("prints uploading ui stage", func() { - _, err := manager.Upload(expectedExtractedStemcell, fakeStage) + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).ToNot(HaveOccurred()) Expect(fakeStage.PerformCalls).To(Equal([]*fakebiui.PerformCall{ @@ -118,7 +118,7 @@ var _ = Describe("Manager", func() { It("when the upload fails, prints failed uploading ui stage", func() { fakeCloud.CreateStemcellReturns("", errors.New("fake-create-error")) - _, err := manager.Upload(expectedExtractedStemcell, fakeStage) + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("fake-create-error")) @@ -129,7 +129,7 @@ var _ = Describe("Manager", func() { It("when the stemcellRepo save fails, logs uploading start and failure events to the eventLogger", func() { fs.WriteFileError = errors.New("fake-save-error") - _, err := manager.Upload(expectedExtractedStemcell, fakeStage) + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("fake-save-error")) @@ -150,26 +150,83 @@ var _ = Describe("Manager", func() { }) It("returns the existing cloud stemcell", func() { - stemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage) + stemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).ToNot(HaveOccurred()) foundStemcell := NewCloudStemcell(foundStemcellRecord, stemcellRepo, fakeCloud) Expect(stemcell).To(Equal(foundStemcell)) }) It("does not re-upload the stemcell to the infrastructure", func() { - _, err := manager.Upload(expectedExtractedStemcell, fakeStage) + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).ToNot(HaveOccurred()) Expect(fakeCloud.CreateStemcellCallCount()).To(Equal(0)) }) It("logs skipping uploading events to the eventLogger", func() { - _, err := manager.Upload(expectedExtractedStemcell, fakeStage) + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, false) Expect(err).ToNot(HaveOccurred()) Expect(fakeStage.PerformCalls[0].Name).To(Equal("Uploading stemcell 'fake-stemcell-name/fake-stemcell-version'")) Expect(fakeStage.PerformCalls[0].SkipError).To(HaveOccurred()) Expect(fakeStage.PerformCalls[0].SkipError.Error()).To(MatchRegexp("Stemcell already uploaded: Found stemcell: .*fake-existing-cid.*")) }) + + // The repo matches on name and version alone, so a record left over + // from different infrastructure still "matches" even though its CID + // names an image that does not exist where the CPI is now pointed. + Context("when fix is requested", func() { + It("re-uploads the stemcell to the infrastructure", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + + Expect(fakeCloud.CreateStemcellCallCount()).To(Equal(1)) + imagePath, cloudProperties := fakeCloud.CreateStemcellArgsForCall(0) + Expect(imagePath).To(Equal(filepath.Join(tempExtractionDir, "image"))) + Expect(cloudProperties).To(Equal(biproperty.Map{"fake-prop-key": "fake-prop-value"})) + }) + + It("returns the newly created stemcell, not the stale one", func() { + cloudStemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + Expect(cloudStemcell.CID()).To(Equal("fake-stemcell-cid")) + }) + + It("replaces the stale record rather than failing on a duplicate", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + + stemcellRecords, err := stemcellRepo.All() + Expect(err).ToNot(HaveOccurred()) + Expect(stemcellRecords).To(HaveLen(1)) + Expect(stemcellRecords[0].CID).To(Equal("fake-stemcell-cid")) + }) + + // Deleting it would ask the CPI to remove a CID it does not + // have, and would destroy the image the deployment can still be + // rolled back onto. + It("does not delete the old stemcell from the cloud", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + + Expect(fakeCloud.DeleteStemcellCallCount()).To(Equal(0)) + }) + + It("does not skip the upload stage", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + + Expect(fakeStage.PerformCalls[0].SkipError).ToNot(HaveOccurred()) + }) + }) + }) + + Context("when no stemcell record exists and fix is requested", func() { + It("uploads the stemcell as usual", func() { + cloudStemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + Expect(cloudStemcell).To(Equal(expectedCloudStemcell)) + Expect(fakeCloud.CreateStemcellCallCount()).To(Equal(1)) + }) }) }) diff --git a/stemcell/stemcellfakes/fake_manager.go b/stemcell/stemcellfakes/fake_manager.go index bb5856443..cdf59c7cc 100644 --- a/stemcell/stemcellfakes/fake_manager.go +++ b/stemcell/stemcellfakes/fake_manager.go @@ -44,11 +44,12 @@ type FakeManager struct { result1 []stemcell.CloudStemcell result2 error } - UploadStub func(stemcell.ExtractedStemcell, ui.Stage) (stemcell.CloudStemcell, error) + UploadStub func(stemcell.ExtractedStemcell, ui.Stage, bool) (stemcell.CloudStemcell, error) uploadMutex sync.RWMutex uploadArgsForCall []struct { arg1 stemcell.ExtractedStemcell arg2 ui.Stage + arg3 bool } uploadReturns struct { result1 stemcell.CloudStemcell @@ -235,19 +236,20 @@ func (fake *FakeManager) FindUnusedReturnsOnCall(i int, result1 []stemcell.Cloud }{result1, result2} } -func (fake *FakeManager) Upload(arg1 stemcell.ExtractedStemcell, arg2 ui.Stage) (stemcell.CloudStemcell, error) { +func (fake *FakeManager) Upload(arg1 stemcell.ExtractedStemcell, arg2 ui.Stage, arg3 bool) (stemcell.CloudStemcell, error) { fake.uploadMutex.Lock() ret, specificReturn := fake.uploadReturnsOnCall[len(fake.uploadArgsForCall)] fake.uploadArgsForCall = append(fake.uploadArgsForCall, struct { arg1 stemcell.ExtractedStemcell arg2 ui.Stage - }{arg1, arg2}) + arg3 bool + }{arg1, arg2, arg3}) stub := fake.UploadStub fakeReturns := fake.uploadReturns - fake.recordInvocation("Upload", []interface{}{arg1, arg2}) + fake.recordInvocation("Upload", []interface{}{arg1, arg2, arg3}) fake.uploadMutex.Unlock() if stub != nil { - return stub(arg1, arg2) + return stub(arg1, arg2, arg3) } if specificReturn { return ret.result1, ret.result2 @@ -261,17 +263,17 @@ func (fake *FakeManager) UploadCallCount() int { return len(fake.uploadArgsForCall) } -func (fake *FakeManager) UploadCalls(stub func(stemcell.ExtractedStemcell, ui.Stage) (stemcell.CloudStemcell, error)) { +func (fake *FakeManager) UploadCalls(stub func(stemcell.ExtractedStemcell, ui.Stage, bool) (stemcell.CloudStemcell, error)) { fake.uploadMutex.Lock() defer fake.uploadMutex.Unlock() fake.UploadStub = stub } -func (fake *FakeManager) UploadArgsForCall(i int) (stemcell.ExtractedStemcell, ui.Stage) { +func (fake *FakeManager) UploadArgsForCall(i int) (stemcell.ExtractedStemcell, ui.Stage, bool) { fake.uploadMutex.RLock() defer fake.uploadMutex.RUnlock() argsForCall := fake.uploadArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 } func (fake *FakeManager) UploadReturns(result1 stemcell.CloudStemcell, result2 error) { From 28bb647d70ca5f42b689073c9cd4c25be723bc83 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Wed, 23 Sep 2026 15:07:39 -0700 Subject: [PATCH 02/12] Delete the orphaned stemcell when saving its record fails If create_stemcell succeeds and the repo save then fails, the image exists in the IaaS with nothing recording it. Neither delete-env nor unused-stemcell cleanup can discover it, and a retry creates another. The code carried a TODO for this. --fix makes the window worse: the stale record is deleted before the upload, so a save failure now leaves no record at all alongside the orphan. Delete the new image on that path, reporting the original save error rather than the cleanup result, and mentioning the leak if the cleanup also fails. The ordering is deliberate: the stale record is removed before create_stemcell so that a failed delete cannot orphan an image. Creating first would trade one orphan path for two. The pre-existing "when the stemcellRepo save fails" example does not reach Save -- fs.WriteFileError breaks the fs-backed repo at Find, as its own assertion shows -- so the new examples use a fake repo to isolate the path. Co-Authored-By: Claude Opus 5 --- stemcell/manager.go | 8 +++++++- stemcell/manager_test.go | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/stemcell/manager.go b/stemcell/manager.go index a1709ed9c..48cb4d126 100644 --- a/stemcell/manager.go +++ b/stemcell/manager.go @@ -98,7 +98,13 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S stemcellRecord, err := m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) if err != nil { - // TODO: delete stemcell from cloud when saving fails + // The image now exists in the IaaS with nothing recording it, so + // neither delete-env nor unused-stemcell cleanup can ever find it, + // and a retry would create another one. Remove it, but report the + // original save failure rather than the cleanup result. + if deleteErr := m.cloud.DeleteStemcell(cid); deleteErr != nil { + return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s); the orphaned stemcell could not be deleted either: %s", cid, extractedStemcell, deleteErr.Error()) + } return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s)", cid, extractedStemcell) } diff --git a/stemcell/manager_test.go b/stemcell/manager_test.go index df40588fb..8f7751b32 100644 --- a/stemcell/manager_test.go +++ b/stemcell/manager_test.go @@ -13,6 +13,7 @@ import ( "github.com/cloudfoundry/bosh-cli/v7/cloud/cloudfakes" biconfig "github.com/cloudfoundry/bosh-cli/v7/config" + "github.com/cloudfoundry/bosh-cli/v7/config/configfakes" . "github.com/cloudfoundry/bosh-cli/v7/stemcell" fakebistemcell "github.com/cloudfoundry/bosh-cli/v7/stemcell/stemcellfakes" fakebiui "github.com/cloudfoundry/bosh-cli/v7/ui/fakes" @@ -220,6 +221,48 @@ var _ = Describe("Manager", func() { }) }) + // The fs-backed repo fails at Find when writes are broken, so Save + // cannot be made to fail independently through it. A fake repo isolates + // the path. + Context("when saving the stemcell record fails", func() { + var ( + fakeRepo *configfakes.FakeStemcellRepo + fakeManager Manager + ) + + BeforeEach(func() { + fakeRepo = &configfakes.FakeStemcellRepo{} + fakeRepo.FindReturns(biconfig.StemcellRecord{}, false, nil) + fakeRepo.SaveReturns(biconfig.StemcellRecord{}, errors.New("fake-save-error")) + fakeManager = NewManager(fakeRepo, fakeCloud) + }) + + It("deletes the orphaned stemcell from the cloud", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, false) + Expect(err).To(HaveOccurred()) + + Expect(fakeCloud.DeleteStemcellCallCount()).To(Equal(1)) + Expect(fakeCloud.DeleteStemcellArgsForCall(0)).To(Equal("fake-stemcell-cid")) + }) + + It("reports the save failure, not the cleanup result", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, false) + Expect(err.Error()).To(ContainSubstring("fake-save-error")) + }) + + Context("when deleting the orphaned stemcell also fails", func() { + BeforeEach(func() { + fakeCloud.DeleteStemcellReturns(errors.New("fake-delete-error")) + }) + + It("still reports the save failure, mentioning the leak", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, false) + Expect(err.Error()).To(ContainSubstring("fake-save-error")) + Expect(err.Error()).To(ContainSubstring("fake-delete-error")) + }) + }) + }) + Context("when no stemcell record exists and fix is requested", func() { It("uploads the stemcell as usual", func() { cloudStemcell, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) From 01d95ee8328f19b672536801fb33c14647f69a13 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Wed, 23 Sep 2026 15:15:45 -0700 Subject: [PATCH 03/12] Upload before mutating state; replace the record atomically Addresses review feedback on the first cut of create-env --fix, which deleted the existing stemcell record before starting the upload. Two real defects came out of that ordering. Crash consistency. repo.Delete commits a write to bosh-state.json immediately, while create_stemcell moves a multi-gigabyte image over the network and can run for many minutes. If the upload failed or was interrupted, the state file had already lost the record and its CID -- the only reference to the image the environment was still running on. The upload now happens first, so a failed upload leaves state untouched. Regression of #731. repo.Delete clears CurrentStemcellID when it removes the current record (config/stemcell_repo.go). An empty CurrentStemcellID makes FindUnused report *every* stemcell as unused, so delete-env deletes them all -- on AWS, deregistering live AMIs. That is the bug #731 describes and #737 is fixing, and the previous ordering re-opened it. It also made deployment_deleter silently fall back to CPI API version 1, since it resolves the version through CurrentStemcellID. StemcellRepo gains SaveOrUpdate, which replaces a record with the same name and version in a single state write and repoints CurrentStemcellID at the replacement, so it is never transiently empty. Upload uses it on the fix path and plain Save otherwise, leaving Save's duplicate rejection intact for every other caller. Also from the review: - --fix recreates the deployment VM, unlike upload-stemcell --fix which is non-destructive. The help text now says so. - PrepareDeployment took four consecutive booleans; they are now a DeploymentOptions struct. - New specs assert the state invariants that were missing: that a failed upload leaves both the record and CurrentStemcellID intact, that a successful --fix leaves CurrentStemcellID pointing at the replacement, and that FindUnused reports nothing afterwards. Restoring the old ordering fails them. Still not addressed: an image replaced by --fix against the *same* infrastructure is no longer tracked in state and will not be cleaned up by delete-env. Deleting it is not safe in general, because --fix exists precisely for the case where the recorded CID belongs to infrastructure the CPI can no longer reach. Co-Authored-By: Claude Opus 5 --- cmd/create_env.go | 7 +- cmd/deployment_preparer.go | 17 ++++- cmd/opts/opts.go | 2 +- cmd/opts/opts_test.go | 2 +- config/configfakes/fake_stemcell_repo.go | 83 ++++++++++++++++++++++++ config/stemcell_repo.go | 46 +++++++++++++ config/stemcell_repo_test.go | 73 +++++++++++++++++++++ stemcell/manager.go | 38 ++++++----- stemcell/manager_test.go | 56 ++++++++++++++++ 9 files changed, 304 insertions(+), 20 deletions(-) diff --git a/cmd/create_env.go b/cmd/create_env.go index 025daf936..69db7cde1 100644 --- a/cmd/create_env.go +++ b/cmd/create_env.go @@ -24,5 +24,10 @@ func (c *CreateEnvCmd) Run(stage boshui.Stage, opts CreateEnvOpts) error { depPreparer := c.envProvider(opts.Args.Manifest.Path, opts.StatePath, opts.VarFlags.AsVariables(), opts.OpsFlags.AsOp()) //nolint:staticcheck - return depPreparer.PrepareDeployment(stage, opts.Recreate, opts.RecreatePersistentDisks, opts.Fix, opts.SkipDrain) + return depPreparer.PrepareDeployment(stage, DeploymentOptions{ + Recreate: opts.Recreate, + RecreatePersistentDisks: opts.RecreatePersistentDisks, + FixStemcell: opts.Fix, + SkipDrain: opts.SkipDrain, + }) } diff --git a/cmd/deployment_preparer.go b/cmd/deployment_preparer.go index 4ceb86823..9ee00f4c2 100644 --- a/cmd/deployment_preparer.go +++ b/cmd/deployment_preparer.go @@ -100,7 +100,22 @@ type DeploymentPreparer struct { targetProvider biinstall.TargetProvider } -func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, recreate bool, recreatePersistentDisks bool, fix bool, skipDrain bool) (err error) { +// DeploymentOptions carries the per-run switches for PrepareDeployment. A +// struct rather than a run of consecutive booleans, which are easy to transpose +// at a call site. +type DeploymentOptions struct { + Recreate bool + RecreatePersistentDisks bool + FixStemcell bool + SkipDrain bool +} + +func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, opts DeploymentOptions) (err error) { + recreate := opts.Recreate + recreatePersistentDisks := opts.RecreatePersistentDisks + fix := opts.FixStemcell + skipDrain := opts.SkipDrain + c.ui.BeginLinef("Deployment state: '%s'\n", c.deploymentStateService.Path()) if !c.deploymentStateService.Exists() { diff --git a/cmd/opts/opts.go b/cmd/opts/opts.go index 7c9cae10d..8f4a0a12c 100644 --- a/cmd/opts/opts.go +++ b/cmd/opts/opts.go @@ -197,7 +197,7 @@ type CreateEnvOpts struct { StatePath string `long:"state" value-name:"PATH" description:"State file path"` Recreate bool `long:"recreate" description:"Recreate VM in deployment"` RecreatePersistentDisks bool `long:"recreate-persistent-disks" description:"Recreate persistent disks in the deployment"` - Fix bool `long:"fix" description:"Recreate the stemcell in the IaaS even if the state file already records one"` + Fix bool `long:"fix" description:"Re-upload the stemcell even if the state file already records one; also recreates the VM"` PackageDir string `long:"package-dir" value-name:"DIR" description:"Package cache location override"` cmd } diff --git a/cmd/opts/opts_test.go b/cmd/opts/opts_test.go index aff4a5eb3..ed069b6b0 100644 --- a/cmd/opts/opts_test.go +++ b/cmd/opts/opts_test.go @@ -871,7 +871,7 @@ var _ = Describe("Opts", func() { It("has --fix", func() { Expect(getStructTagForName("Fix", opts)).To(Equal( - `long:"fix" description:"Recreate the stemcell in the IaaS even if the state file already records one"`, + `long:"fix" description:"Re-upload the stemcell even if the state file already records one; also recreates the VM"`, )) }) }) diff --git a/config/configfakes/fake_stemcell_repo.go b/config/configfakes/fake_stemcell_repo.go index 7c96447cb..20a35f4ab 100644 --- a/config/configfakes/fake_stemcell_repo.go +++ b/config/configfakes/fake_stemcell_repo.go @@ -87,6 +87,22 @@ type FakeStemcellRepo struct { result1 config.StemcellRecord result2 error } + SaveOrUpdateStub func(string, string, string, int) (config.StemcellRecord, error) + saveOrUpdateMutex sync.RWMutex + saveOrUpdateArgsForCall []struct { + arg1 string + arg2 string + arg3 string + arg4 int + } + saveOrUpdateReturns struct { + result1 config.StemcellRecord + result2 error + } + saveOrUpdateReturnsOnCall map[int]struct { + result1 config.StemcellRecord + result2 error + } UpdateCurrentStub func(string) error updateCurrentMutex sync.RWMutex updateCurrentArgsForCall []struct { @@ -466,6 +482,73 @@ func (fake *FakeStemcellRepo) SaveReturnsOnCall(i int, result1 config.StemcellRe }{result1, result2} } +func (fake *FakeStemcellRepo) SaveOrUpdate(arg1 string, arg2 string, arg3 string, arg4 int) (config.StemcellRecord, error) { + fake.saveOrUpdateMutex.Lock() + ret, specificReturn := fake.saveOrUpdateReturnsOnCall[len(fake.saveOrUpdateArgsForCall)] + fake.saveOrUpdateArgsForCall = append(fake.saveOrUpdateArgsForCall, struct { + arg1 string + arg2 string + arg3 string + arg4 int + }{arg1, arg2, arg3, arg4}) + stub := fake.SaveOrUpdateStub + fakeReturns := fake.saveOrUpdateReturns + fake.recordInvocation("SaveOrUpdate", []interface{}{arg1, arg2, arg3, arg4}) + fake.saveOrUpdateMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeStemcellRepo) SaveOrUpdateCallCount() int { + fake.saveOrUpdateMutex.RLock() + defer fake.saveOrUpdateMutex.RUnlock() + return len(fake.saveOrUpdateArgsForCall) +} + +func (fake *FakeStemcellRepo) SaveOrUpdateCalls(stub func(string, string, string, int) (config.StemcellRecord, error)) { + fake.saveOrUpdateMutex.Lock() + defer fake.saveOrUpdateMutex.Unlock() + fake.SaveOrUpdateStub = stub +} + +func (fake *FakeStemcellRepo) SaveOrUpdateArgsForCall(i int) (string, string, string, int) { + fake.saveOrUpdateMutex.RLock() + defer fake.saveOrUpdateMutex.RUnlock() + argsForCall := fake.saveOrUpdateArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *FakeStemcellRepo) SaveOrUpdateReturns(result1 config.StemcellRecord, result2 error) { + fake.saveOrUpdateMutex.Lock() + defer fake.saveOrUpdateMutex.Unlock() + fake.SaveOrUpdateStub = nil + fake.saveOrUpdateReturns = struct { + result1 config.StemcellRecord + result2 error + }{result1, result2} +} + +func (fake *FakeStemcellRepo) SaveOrUpdateReturnsOnCall(i int, result1 config.StemcellRecord, result2 error) { + fake.saveOrUpdateMutex.Lock() + defer fake.saveOrUpdateMutex.Unlock() + fake.SaveOrUpdateStub = nil + if fake.saveOrUpdateReturnsOnCall == nil { + fake.saveOrUpdateReturnsOnCall = make(map[int]struct { + result1 config.StemcellRecord + result2 error + }) + } + fake.saveOrUpdateReturnsOnCall[i] = struct { + result1 config.StemcellRecord + result2 error + }{result1, result2} +} + func (fake *FakeStemcellRepo) UpdateCurrent(arg1 string) error { fake.updateCurrentMutex.Lock() ret, specificReturn := fake.updateCurrentReturnsOnCall[len(fake.updateCurrentArgsForCall)] diff --git a/config/stemcell_repo.go b/config/stemcell_repo.go index 75edc418a..4bb894f8b 100644 --- a/config/stemcell_repo.go +++ b/config/stemcell_repo.go @@ -13,6 +13,7 @@ type StemcellRepo interface // StemcellRepo persists stemcells metadata FindCurrent() (StemcellRecord, bool, error) ClearCurrent() error Save(name, version, cid string, apiVersion int) (StemcellRecord, error) + SaveOrUpdate(name, version, cid string, apiVersion int) (StemcellRecord, error) Find(name, version string) (StemcellRecord, bool, error) All() ([]StemcellRecord, error) Delete(StemcellRecord) error @@ -68,6 +69,51 @@ func (r stemcellRepo) Save(name, version, cid string, apiVersion int) (StemcellR return stemcellRecord, err } +// SaveOrUpdate records a stemcell, replacing any existing record with the same +// name and version rather than rejecting it as a duplicate. +// +// Replacement is done in a single state write, and CurrentStemcellID is +// repointed at the replacement record in the same operation. Deleting the old +// record and saving a new one separately would leave CurrentStemcellID empty in +// between, and an empty CurrentStemcellID makes FindUnused treat every stemcell +// as unused -- on AWS that deregisters live AMIs (see #731). +func (r stemcellRepo) SaveOrUpdate(name, version, cid string, apiVersion int) (StemcellRecord, error) { + stemcellRecord := StemcellRecord{} + + err := r.updateConfig(func(config *DeploymentState) error { + newRecord := StemcellRecord{ + Name: name, + Version: version, + CID: cid, + ApiVersion: apiVersion, + } + + var err error + newRecord.ID, err = r.uuidGenerator.Generate() + if err != nil { + return bosherr.WrapError(err, "Generating stemcell id") + } + + keptRecords := []StemcellRecord{} + for _, oldRecord := range config.Stemcells { + if oldRecord.Name == name && oldRecord.Version == version { + if config.CurrentStemcellID == oldRecord.ID { + config.CurrentStemcellID = newRecord.ID + } + continue + } + keptRecords = append(keptRecords, oldRecord) + } + + config.Stemcells = append(keptRecords, newRecord) + stemcellRecord = newRecord + + return nil + }) + + return stemcellRecord, err +} + func (r stemcellRepo) Find(name, version string) (StemcellRecord, bool, error) { _, records, err := r.load() if err != nil { diff --git a/config/stemcell_repo_test.go b/config/stemcell_repo_test.go index 2c7b5a998..048fa5927 100644 --- a/config/stemcell_repo_test.go +++ b/config/stemcell_repo_test.go @@ -29,6 +29,79 @@ var _ = Describe("StemcellRepo", func() { repo = NewStemcellRepo(deploymentStateService, fakeUUIDGenerator) }) + Describe("SaveOrUpdate", func() { + It("saves a new record when none matches", func() { + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-1" + record, err := repo.SaveOrUpdate("fake-name", "fake-version", "fake-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + Expect(record.CID).To(Equal("fake-cid")) + + records, err := repo.All() + Expect(err).ToNot(HaveOccurred()) + Expect(records).To(HaveLen(1)) + }) + + It("replaces an existing record with the same name and version", func() { + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-1" + _, err := repo.SaveOrUpdate("fake-name", "fake-version", "old-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-2" + _, err = repo.SaveOrUpdate("fake-name", "fake-version", "new-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + + records, err := repo.All() + Expect(err).ToNot(HaveOccurred()) + Expect(records).To(HaveLen(1)) + Expect(records[0].CID).To(Equal("new-cid")) + }) + + It("does not reject a duplicate name and version the way Save does", func() { + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-1" + _, err := repo.Save("fake-name", "fake-version", "old-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-2" + _, err = repo.SaveOrUpdate("fake-name", "fake-version", "new-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + }) + + // An empty CurrentStemcellID makes FindUnused treat every stemcell as + // unused, which on AWS deregisters live AMIs (#731). + It("repoints CurrentStemcellID at the replacement record", func() { + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-1" + oldRecord, err := repo.SaveOrUpdate("fake-name", "fake-version", "old-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + Expect(repo.UpdateCurrent(oldRecord.ID)).To(Succeed()) + + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-2" + newRecord, err := repo.SaveOrUpdate("fake-name", "fake-version", "new-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + + current, found, err := repo.FindCurrent() + Expect(err).ToNot(HaveOccurred()) + Expect(found).To(BeTrue(), "CurrentStemcellID must never be left empty") + Expect(current.ID).To(Equal(newRecord.ID)) + Expect(current.CID).To(Equal("new-cid")) + }) + + It("leaves CurrentStemcellID alone when it points at an unrelated record", func() { + fakeUUIDGenerator.GeneratedUUID = "other-uuid" + otherRecord, err := repo.Save("other-name", "other-version", "other-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + Expect(repo.UpdateCurrent(otherRecord.ID)).To(Succeed()) + + fakeUUIDGenerator.GeneratedUUID = "fake-uuid-1" + _, err = repo.SaveOrUpdate("fake-name", "fake-version", "new-cid", apiVersion) + Expect(err).ToNot(HaveOccurred()) + + current, found, err := repo.FindCurrent() + Expect(err).ToNot(HaveOccurred()) + Expect(found).To(BeTrue()) + Expect(current.ID).To(Equal(otherRecord.ID)) + }) + }) + Describe("Save", func() { It("saves the stemcell record using the config service", func() { _, err := repo.Save("fake-name", "fake-version", "fake-cid", apiVersion) diff --git a/stemcell/manager.go b/stemcell/manager.go index 48cb4d126..e8ce586f4 100644 --- a/stemcell/manager.go +++ b/stemcell/manager.go @@ -76,27 +76,26 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S return biui.NewSkipStageError(bosherr.Errorf("Found stemcell: %#v", foundStemcellRecord), "Stemcell already uploaded") } - if found { - // Drop the stale record before uploading: Save rejects a duplicate - // name/version pair, so it would fail after the new image had - // already been created. - // - // Only the record is removed, not the image. CloudStemcell.Delete - // would ask the CPI to delete the old CID, which at best is a - // no-op against infrastructure that never had it and at worst - // destroys the image the deployment can still be rolled back onto. - err = m.repo.Delete(foundStemcellRecord) - if err != nil { - return bosherr.WrapErrorf(err, "Deleting stale stemcell record (name=%s, version=%s, cid=%s)", foundStemcellRecord.Name, foundStemcellRecord.Version, foundStemcellRecord.CID) - } - } - + // Upload before touching the state file. create_stemcell moves a + // multi-gigabyte image across the network and can fail or be + // interrupted; mutating state first would discard the existing record, + // and with it the only reference to the image still in use. cid, err := m.cloud.CreateStemcell(filepath.Join(extractedStemcell.GetExtractedPath(), "image"), manifest.CloudProperties) if err != nil { return bosherr.WrapErrorf(err, "creating stemcell (%s %s)", manifest.Name, manifest.Version) } - stemcellRecord, err := m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) + // Replacing in a single write keeps CurrentStemcellID pointed at a real + // record throughout. Deleting the old record first would blank it, and + // an empty CurrentStemcellID makes FindUnused report every stemcell as + // unused -- on AWS that deregisters live AMIs (#731) -- and makes + // delete-env silently fall back to CPI API version 1. + var stemcellRecord biconfig.StemcellRecord + if found { + stemcellRecord, err = m.repo.SaveOrUpdate(manifest.Name, manifest.Version, cid, manifest.ApiVersion) + } else { + stemcellRecord, err = m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) + } if err != nil { // The image now exists in the IaaS with nothing recording it, so // neither delete-env nor unused-stemcell cleanup can ever find it, @@ -108,6 +107,13 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s)", cid, extractedStemcell) } + // NOTE: the replaced image is deliberately not deleted. It may live on + // infrastructure the CPI is no longer pointed at, where the delete + // would fail or target the wrong thing, and it is the rollback target + // if the new deployment does not come up. It is no longer tracked in + // state, so re-running --fix against the same infrastructure can leave + // images behind that need manual cleanup. + cloudStemcell = NewCloudStemcell(stemcellRecord, m.repo, m.cloud) return nil }) diff --git a/stemcell/manager_test.go b/stemcell/manager_test.go index 8f7751b32..b812f7fbc 100644 --- a/stemcell/manager_test.go +++ b/stemcell/manager_test.go @@ -176,6 +176,11 @@ var _ = Describe("Manager", func() { // from different infrastructure still "matches" even though its CID // names an image that does not exist where the CPI is now pointed. Context("when fix is requested", func() { + BeforeEach(func() { + err := stemcellRepo.UpdateCurrent(foundStemcellRecord.ID) + Expect(err).ToNot(HaveOccurred()) + }) + It("re-uploads the stemcell to the infrastructure", func() { _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) Expect(err).ToNot(HaveOccurred()) @@ -202,6 +207,28 @@ var _ = Describe("Manager", func() { Expect(stemcellRecords[0].CID).To(Equal("fake-stemcell-cid")) }) + // An empty CurrentStemcellID makes FindUnused report every + // stemcell as unused, which on AWS deregisters live AMIs (#731), + // and makes delete-env fall back to CPI API version 1. + It("keeps CurrentStemcellID pointing at the replacement record", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + + currentRecord, found, err := stemcellRepo.FindCurrent() + Expect(err).ToNot(HaveOccurred()) + Expect(found).To(BeTrue(), "CurrentStemcellID must never be left empty") + Expect(currentRecord.CID).To(Equal("fake-stemcell-cid")) + }) + + It("reports no unused stemcells afterwards", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).ToNot(HaveOccurred()) + + unused, err := manager.FindUnused() + Expect(err).ToNot(HaveOccurred()) + Expect(unused).To(BeEmpty(), "a blanked CurrentStemcellID would mark every stemcell unused") + }) + // Deleting it would ask the CPI to remove a CID it does not // have, and would destroy the image the deployment can still be // rolled back onto. @@ -218,6 +245,35 @@ var _ = Describe("Manager", func() { Expect(fakeStage.PerformCalls[0].SkipError).ToNot(HaveOccurred()) }) + + // create_stemcell moves gigabytes over the network and is the + // most likely thing to fail or be interrupted. State must be + // untouched when it does. + Context("when the upload fails", func() { + BeforeEach(func() { + fakeCloud.CreateStemcellReturns("", errors.New("fake-create-error")) + }) + + It("leaves the existing record intact", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).To(HaveOccurred()) + + records, err := stemcellRepo.All() + Expect(err).ToNot(HaveOccurred()) + Expect(records).To(HaveLen(1)) + Expect(records[0].CID).To(Equal("fake-existing-cid")) + }) + + It("leaves CurrentStemcellID intact", func() { + _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).To(HaveOccurred()) + + currentRecord, found, err := stemcellRepo.FindCurrent() + Expect(err).ToNot(HaveOccurred()) + Expect(found).To(BeTrue(), "a failed upload must not blank CurrentStemcellID") + Expect(currentRecord.CID).To(Equal("fake-existing-cid")) + }) + }) }) }) From 9b97f038a0ab8af282d632ab332ea992d0f8cbbc Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Wed, 23 Sep 2026 15:35:25 -0700 Subject: [PATCH 04/12] Add integration coverage for create-env --fix Closes the last gap from review. The integration suite runs in-process against fakes -- no CPI or infrastructure -- so this needed no new harness, contrary to what I said earlier on the PR. Two examples alongside the existing "same deployment attempted again" case: that --fix re-uploads rather than taking the no-changes short-circuit, and that CurrentStemcellID still resolves to a real record afterwards. Removing the short-circuit bypass fails the first. A second deploy within one spec draws a fresh agent ID, which the shared CreateVM stub installed by expectDeployFlow asserts against a fixed value. These examples are about stemcell handling, so they relax that stub rather than weaken the assertion for everyone. Co-Authored-By: Claude Opus 5 --- integration/create_env_test.go | 49 ++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/integration/create_env_test.go b/integration/create_env_test.go index 54fe5d969..d0b8f4e38 100644 --- a/integration/create_env_test.go +++ b/integration/create_env_test.go @@ -1139,6 +1139,55 @@ cloud_provider: Expect(mockCloud.CreateVMCallCount()).To(Equal(createVMCountBefore)) }) }) + + // --fix exists for the case where the recorded stemcell CID names + // an image on infrastructure the CPI is no longer pointed at. It + // must force a re-upload even though nothing else changed, and it + // must not leave CurrentStemcellID empty: an empty value makes + // FindUnused report every stemcell as unused, which on AWS + // deregisters live AMIs (#731). + Context("and the same deployment is attempted again with --fix", func() { + // A second deploy in the same spec draws a fresh agent ID from + // the fake generator, which the shared CreateVM stub (installed + // by expectDeployFlow) asserts against a fixed value. These + // examples are about stemcell handling, so relax that stub. + relaxCreateVM := func() { + mockCloud.CreateVMStub = func(_, _ string, _ biproperty.Map, _ []string, _ map[string]biproperty.Map, _ biproperty.Map) (string, error) { + return "fake-vm-cid-1", nil + } + } + + It("re-uploads the stemcell instead of skipping", func() { + relaxCreateVM() + createStemcellCountBefore := mockCloud.CreateStemcellCallCount() + + fixOpts := newDeployOpts(deploymentManifestPath, "") + fixOpts.Fix = true + + err := newCreateEnvCmd().Run(fakeStage, fixOpts) + Expect(err).ToNot(HaveOccurred()) + + Expect(mockCloud.CreateStemcellCallCount()).To(Equal(createStemcellCountBefore + 1)) + }) + + It("leaves CurrentStemcellID pointing at a real record", func() { + relaxCreateVM() + + fixOpts := newDeployOpts(deploymentManifestPath, "") + fixOpts.Fix = true + + err := newCreateEnvCmd().Run(fakeStage, fixOpts) + Expect(err).ToNot(HaveOccurred()) + + currentRecord, found, err := stemcellRepo.FindCurrent() + Expect(err).ToNot(HaveOccurred()) + Expect(found).To(BeTrue(), "CurrentStemcellID must never be left empty") + + records, err := stemcellRepo.All() + Expect(err).ToNot(HaveOccurred()) + Expect(records).To(ContainElement(currentRecord)) + }) + }) }) Context("when the stemcell supports api_version 2", func() { From 4b18d0ff2210aef1b5bb278d60c99eea250a8b26 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Wed, 23 Sep 2026 17:08:04 -0700 Subject: [PATCH 05/12] Trim comments; move the reasoning into spec names The comments were carrying explanation that belongs either in a spec name or nowhere. Production comments are cut to the non-obvious point only -- why the upload precedes the state write, why SaveOrUpdate rather than Save, why the replaced image is left in place -- and the rest is dropped. In the specs the rationale is now in the descriptions, so a failure reports why the behaviour matters rather than just what broke: never leaves CurrentStemcellID empty, which would strand delete-env on CPI api version 1 does not report live stemcells as unused, which on AWS would deregister the AMI leaves the replaced image in the cloud as the rollback target when the upload fails partway, as a long transfer may deploys if `fix` flag is specified, even with no manifest or release changes Two short comments are kept where the reason is about the harness and has nowhere else to live: that a fake repo is needed because the fs-backed one fails at Find before Save is reached, and that a second deploy in one spec draws a fresh agent ID. No behaviour change. Co-Authored-By: Claude Opus 5 --- cmd/create_env_test.go | 5 +---- cmd/deployment_preparer.go | 4 +--- config/stemcell_repo.go | 12 ++++-------- config/stemcell_repo_test.go | 4 +--- integration/create_env_test.go | 14 +++----------- stemcell/manager.go | 34 ++++++++-------------------------- stemcell/manager_test.go | 24 +++++------------------- 7 files changed, 23 insertions(+), 74 deletions(-) diff --git a/cmd/create_env_test.go b/cmd/create_env_test.go index c207fdb36..96fb3c956 100644 --- a/cmd/create_env_test.go +++ b/cmd/create_env_test.go @@ -691,10 +691,7 @@ var _ = Describe("CreateEnvCmd", func() { Expect(mockDeployer.DeployCallCount()).To(Equal(1)) }) - // Repointing at new infrastructure need not change the manifest, - // releases or stemcell version, so without this a fix run would be - // skipped before it ever reached the stemcell upload. - It("deploys if `fix` flag is specified", func() { + It("deploys if `fix` flag is specified, even with no manifest or release changes", func() { defaultCreateEnvOpts.Fix = true err := command.Run(fakeStage, defaultCreateEnvOpts) diff --git a/cmd/deployment_preparer.go b/cmd/deployment_preparer.go index 9ee00f4c2..a663407e9 100644 --- a/cmd/deployment_preparer.go +++ b/cmd/deployment_preparer.go @@ -100,9 +100,7 @@ type DeploymentPreparer struct { targetProvider biinstall.TargetProvider } -// DeploymentOptions carries the per-run switches for PrepareDeployment. A -// struct rather than a run of consecutive booleans, which are easy to transpose -// at a call site. +// DeploymentOptions carries the per-run switches for PrepareDeployment. type DeploymentOptions struct { Recreate bool RecreatePersistentDisks bool diff --git a/config/stemcell_repo.go b/config/stemcell_repo.go index 4bb894f8b..e86dd3242 100644 --- a/config/stemcell_repo.go +++ b/config/stemcell_repo.go @@ -69,14 +69,10 @@ func (r stemcellRepo) Save(name, version, cid string, apiVersion int) (StemcellR return stemcellRecord, err } -// SaveOrUpdate records a stemcell, replacing any existing record with the same -// name and version rather than rejecting it as a duplicate. -// -// Replacement is done in a single state write, and CurrentStemcellID is -// repointed at the replacement record in the same operation. Deleting the old -// record and saving a new one separately would leave CurrentStemcellID empty in -// between, and an empty CurrentStemcellID makes FindUnused treat every stemcell -// as unused -- on AWS that deregisters live AMIs (see #731). +// SaveOrUpdate replaces any record with the same name and version instead of +// rejecting it as a duplicate, repointing CurrentStemcellID at the replacement +// in the same write. An empty CurrentStemcellID makes FindUnused treat every +// stemcell as unused, which on AWS deregisters live AMIs (#731). func (r stemcellRepo) SaveOrUpdate(name, version, cid string, apiVersion int) (StemcellRecord, error) { stemcellRecord := StemcellRecord{} diff --git a/config/stemcell_repo_test.go b/config/stemcell_repo_test.go index 048fa5927..471f0a40c 100644 --- a/config/stemcell_repo_test.go +++ b/config/stemcell_repo_test.go @@ -66,9 +66,7 @@ var _ = Describe("StemcellRepo", func() { Expect(err).ToNot(HaveOccurred()) }) - // An empty CurrentStemcellID makes FindUnused treat every stemcell as - // unused, which on AWS deregisters live AMIs (#731). - It("repoints CurrentStemcellID at the replacement record", func() { + It("repoints CurrentStemcellID at the replacement rather than leaving it empty", func() { fakeUUIDGenerator.GeneratedUUID = "fake-uuid-1" oldRecord, err := repo.SaveOrUpdate("fake-name", "fake-version", "old-cid", apiVersion) Expect(err).ToNot(HaveOccurred()) diff --git a/integration/create_env_test.go b/integration/create_env_test.go index d0b8f4e38..327241bd5 100644 --- a/integration/create_env_test.go +++ b/integration/create_env_test.go @@ -1140,17 +1140,9 @@ cloud_provider: }) }) - // --fix exists for the case where the recorded stemcell CID names - // an image on infrastructure the CPI is no longer pointed at. It - // must force a re-upload even though nothing else changed, and it - // must not leave CurrentStemcellID empty: an empty value makes - // FindUnused report every stemcell as unused, which on AWS - // deregisters live AMIs (#731). Context("and the same deployment is attempted again with --fix", func() { - // A second deploy in the same spec draws a fresh agent ID from - // the fake generator, which the shared CreateVM stub (installed - // by expectDeployFlow) asserts against a fixed value. These - // examples are about stemcell handling, so relax that stub. + // A second deploy draws a fresh agent ID, which the shared stub + // asserts against a fixed value. relaxCreateVM := func() { mockCloud.CreateVMStub = func(_, _ string, _ biproperty.Map, _ []string, _ map[string]biproperty.Map, _ biproperty.Map) (string, error) { return "fake-vm-cid-1", nil @@ -1170,7 +1162,7 @@ cloud_provider: Expect(mockCloud.CreateStemcellCallCount()).To(Equal(createStemcellCountBefore + 1)) }) - It("leaves CurrentStemcellID pointing at a real record", func() { + It("leaves CurrentStemcellID resolving to a real record", func() { relaxCreateVM() fixOpts := newDeployOpts(deploymentManifestPath, "") diff --git a/stemcell/manager.go b/stemcell/manager.go index e8ce586f4..c2967077a 100644 --- a/stemcell/manager.go +++ b/stemcell/manager.go @@ -55,13 +55,8 @@ func (m *manager) FindCurrent() ([]CloudStemcell, error) { // 1) uploads the stemcell to the cloud (if needed), // 2) saves a record of the uploaded stemcell in the repo // -// The repo records stemcells by name and version only -- it has no notion of -// which IaaS, or which vCenter, the image was actually materialized in. When -// the deployment is repointed at different infrastructure the recorded CID -// names an image that does not exist there, but the name and version still -// match, so the upload is skipped and the stale CID is handed to create_vm. -// Passing fix forces a fresh create_stemcell against whatever the CPI is now -// talking to. +// Records are keyed on name and version alone, so a record left over from other +// infrastructure still matches. fix forces a fresh create_stemcell. func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.Stage, fix bool) (cloudStemcell CloudStemcell, err error) { manifest := extractedStemcell.Manifest() stageName := fmt.Sprintf("Uploading stemcell '%s/%s'", manifest.Name, manifest.Version) @@ -76,20 +71,13 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S return biui.NewSkipStageError(bosherr.Errorf("Found stemcell: %#v", foundStemcellRecord), "Stemcell already uploaded") } - // Upload before touching the state file. create_stemcell moves a - // multi-gigabyte image across the network and can fail or be - // interrupted; mutating state first would discard the existing record, - // and with it the only reference to the image still in use. + // Upload first: a failed create_stemcell must leave state untouched. cid, err := m.cloud.CreateStemcell(filepath.Join(extractedStemcell.GetExtractedPath(), "image"), manifest.CloudProperties) if err != nil { return bosherr.WrapErrorf(err, "creating stemcell (%s %s)", manifest.Name, manifest.Version) } - // Replacing in a single write keeps CurrentStemcellID pointed at a real - // record throughout. Deleting the old record first would blank it, and - // an empty CurrentStemcellID makes FindUnused report every stemcell as - // unused -- on AWS that deregisters live AMIs (#731) -- and makes - // delete-env silently fall back to CPI API version 1. + // SaveOrUpdate replaces the record without blanking CurrentStemcellID. var stemcellRecord biconfig.StemcellRecord if found { stemcellRecord, err = m.repo.SaveOrUpdate(manifest.Name, manifest.Version, cid, manifest.ApiVersion) @@ -97,22 +85,16 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S stemcellRecord, err = m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) } if err != nil { - // The image now exists in the IaaS with nothing recording it, so - // neither delete-env nor unused-stemcell cleanup can ever find it, - // and a retry would create another one. Remove it, but report the - // original save failure rather than the cleanup result. + // Nothing records this image, so no cleanup could ever find it. if deleteErr := m.cloud.DeleteStemcell(cid); deleteErr != nil { return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s); the orphaned stemcell could not be deleted either: %s", cid, extractedStemcell, deleteErr.Error()) } return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s)", cid, extractedStemcell) } - // NOTE: the replaced image is deliberately not deleted. It may live on - // infrastructure the CPI is no longer pointed at, where the delete - // would fail or target the wrong thing, and it is the rollback target - // if the new deployment does not come up. It is no longer tracked in - // state, so re-running --fix against the same infrastructure can leave - // images behind that need manual cleanup. + // The replaced image is left alone: it may be on infrastructure the CPI + // can no longer reach, and it is the rollback target. It is untracked + // from here on and may need manual cleanup. cloudStemcell = NewCloudStemcell(stemcellRecord, m.repo, m.cloud) return nil diff --git a/stemcell/manager_test.go b/stemcell/manager_test.go index b812f7fbc..9146cea5c 100644 --- a/stemcell/manager_test.go +++ b/stemcell/manager_test.go @@ -172,9 +172,6 @@ var _ = Describe("Manager", func() { Expect(fakeStage.PerformCalls[0].SkipError.Error()).To(MatchRegexp("Stemcell already uploaded: Found stemcell: .*fake-existing-cid.*")) }) - // The repo matches on name and version alone, so a record left over - // from different infrastructure still "matches" even though its CID - // names an image that does not exist where the CPI is now pointed. Context("when fix is requested", func() { BeforeEach(func() { err := stemcellRepo.UpdateCurrent(foundStemcellRecord.ID) @@ -207,10 +204,7 @@ var _ = Describe("Manager", func() { Expect(stemcellRecords[0].CID).To(Equal("fake-stemcell-cid")) }) - // An empty CurrentStemcellID makes FindUnused report every - // stemcell as unused, which on AWS deregisters live AMIs (#731), - // and makes delete-env fall back to CPI API version 1. - It("keeps CurrentStemcellID pointing at the replacement record", func() { + It("never leaves CurrentStemcellID empty, which would strand delete-env on CPI api version 1", func() { _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) Expect(err).ToNot(HaveOccurred()) @@ -220,7 +214,7 @@ var _ = Describe("Manager", func() { Expect(currentRecord.CID).To(Equal("fake-stemcell-cid")) }) - It("reports no unused stemcells afterwards", func() { + It("does not report live stemcells as unused, which on AWS would deregister the AMI", func() { _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) Expect(err).ToNot(HaveOccurred()) @@ -229,10 +223,7 @@ var _ = Describe("Manager", func() { Expect(unused).To(BeEmpty(), "a blanked CurrentStemcellID would mark every stemcell unused") }) - // Deleting it would ask the CPI to remove a CID it does not - // have, and would destroy the image the deployment can still be - // rolled back onto. - It("does not delete the old stemcell from the cloud", func() { + It("leaves the replaced image in the cloud as the rollback target", func() { _, err := manager.Upload(expectedExtractedStemcell, fakeStage, true) Expect(err).ToNot(HaveOccurred()) @@ -246,10 +237,7 @@ var _ = Describe("Manager", func() { Expect(fakeStage.PerformCalls[0].SkipError).ToNot(HaveOccurred()) }) - // create_stemcell moves gigabytes over the network and is the - // most likely thing to fail or be interrupted. State must be - // untouched when it does. - Context("when the upload fails", func() { + Context("when the upload fails partway, as a long transfer may", func() { BeforeEach(func() { fakeCloud.CreateStemcellReturns("", errors.New("fake-create-error")) }) @@ -277,9 +265,7 @@ var _ = Describe("Manager", func() { }) }) - // The fs-backed repo fails at Find when writes are broken, so Save - // cannot be made to fail independently through it. A fake repo isolates - // the path. + // A fake repo: the fs-backed one fails at Find before Save is reached. Context("when saving the stemcell record fails", func() { var ( fakeRepo *configfakes.FakeStemcellRepo From aff51a8d0d3dd092deedf69a2fc94f2584427eda Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Thu, 24 Sep 2026 09:12:25 -0700 Subject: [PATCH 06/12] improve description of create-env --fix flag Clarify the message to emphasize that this is a forced re-upload that will leave any existing image untracked on the infrastructure. --- cmd/opts/opts.go | 2 +- cmd/opts/opts_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/opts/opts.go b/cmd/opts/opts.go index 8f4a0a12c..d0cf369d1 100644 --- a/cmd/opts/opts.go +++ b/cmd/opts/opts.go @@ -197,7 +197,7 @@ type CreateEnvOpts struct { StatePath string `long:"state" value-name:"PATH" description:"State file path"` Recreate bool `long:"recreate" description:"Recreate VM in deployment"` RecreatePersistentDisks bool `long:"recreate-persistent-disks" description:"Recreate persistent disks in the deployment"` - Fix bool `long:"fix" description:"Re-upload the stemcell even if the state file already records one; also recreates the VM"` + Fix bool `long:"fix" description:"Forces re-upload of the stemcell; any existing stemcell image is orphaned and the VM is recreated"` PackageDir string `long:"package-dir" value-name:"DIR" description:"Package cache location override"` cmd } diff --git a/cmd/opts/opts_test.go b/cmd/opts/opts_test.go index ed069b6b0..9fc75b5af 100644 --- a/cmd/opts/opts_test.go +++ b/cmd/opts/opts_test.go @@ -871,7 +871,7 @@ var _ = Describe("Opts", func() { It("has --fix", func() { Expect(getStructTagForName("Fix", opts)).To(Equal( - `long:"fix" description:"Re-upload the stemcell even if the state file already records one; also recreates the VM"`, + `long:"fix" description:"Forces re-upload of the stemcell; any existing stemcell image is orphaned and the VM is recreated"`, )) }) }) From 4462b1177d874acf49631c8d202903e3c390a6b8 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Thu, 24 Sep 2026 09:44:29 -0700 Subject: [PATCH 07/12] Cover the #731 scenario now that #737 has landed #737 stopped VM delete from clearing CurrentStemcellID; this branch stops the stemcell upload from clearing it. Neither alone survives a replacement VM that fails before PromoteAsCurrent, which is the failure #731 describes. Asserts the composed invariant: after a --fix run whose replacement VM fails, CurrentStemcellID still resolves to a record in the repo, so delete-env neither treats live images as unused nor falls back to CPI api version 1. Reverting this branch's half to delete-then-Save fails the example. Co-Authored-By: Claude Opus 5 --- integration/create_env_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/integration/create_env_test.go b/integration/create_env_test.go index 327241bd5..70de494a7 100644 --- a/integration/create_env_test.go +++ b/integration/create_env_test.go @@ -1162,6 +1162,29 @@ cloud_provider: Expect(mockCloud.CreateStemcellCallCount()).To(Equal(createStemcellCountBefore + 1)) }) + // The #731 scenario: #737 stopped VM delete from clearing the + // pointer, this PR stops the upload from clearing it. Neither + // alone survives a replacement VM that fails before promotion. + It("leaves CurrentStemcellID resolving to a real record when the replacement VM fails", func() { + mockCloud.CreateVMStub = func(_, _ string, _ biproperty.Map, _ []string, _ map[string]biproperty.Map, _ biproperty.Map) (string, error) { + return "", bosherr.Error("fake-create-vm-error") + } + + fixOpts := newDeployOpts(deploymentManifestPath, "") + fixOpts.Fix = true + + err := newCreateEnvCmd().Run(fakeStage, fixOpts) + Expect(err).To(HaveOccurred()) + + currentRecord, found, err := stemcellRepo.FindCurrent() + Expect(err).ToNot(HaveOccurred()) + Expect(found).To(BeTrue(), "an empty CurrentStemcellID would make delete-env deregister live images") + + records, err := stemcellRepo.All() + Expect(err).ToNot(HaveOccurred()) + Expect(records).To(ContainElement(currentRecord)) + }) + It("leaves CurrentStemcellID resolving to a real record", func() { relaxCreateVM() From 855d9cd45e2d0d94932a6e82aece1a7c92e1471e Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Thu, 24 Sep 2026 11:03:07 -0700 Subject: [PATCH 08/12] Assert the replacement CID reaches create_vm The previous example only counted create_stemcell calls, so it passed even if the upload happened and the stale record was still handed to create_vm -- which is the original failure this flag exists to prevent. The second upload now returns a distinct CID and the example asserts create_vm receives it, and that the repo's current record points at it. Returning the stale record while still uploading fails the example; the call count alone does not catch it. Co-Authored-By: Claude Opus 5 --- integration/create_env_test.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/integration/create_env_test.go b/integration/create_env_test.go index 70de494a7..92c98b4c0 100644 --- a/integration/create_env_test.go +++ b/integration/create_env_test.go @@ -1149,8 +1149,19 @@ cloud_provider: } } - It("re-uploads the stemcell instead of skipping", func() { - relaxCreateVM() + It("re-uploads the stemcell and recreates the VM from the replacement CID, not the stale one", func() { + const replacementCID = "fake-replacement-stemcell-cid" + + mockCloud.CreateStemcellStub = func(_ string, _ biproperty.Map) (string, error) { + return replacementCID, nil + } + + var createVMStemcellCID string + mockCloud.CreateVMStub = func(_, gotStemcellCID string, _ biproperty.Map, _ []string, _ map[string]biproperty.Map, _ biproperty.Map) (string, error) { + createVMStemcellCID = gotStemcellCID + return "fake-vm-cid-1", nil + } + createStemcellCountBefore := mockCloud.CreateStemcellCallCount() fixOpts := newDeployOpts(deploymentManifestPath, "") @@ -1160,6 +1171,12 @@ cloud_provider: Expect(err).ToNot(HaveOccurred()) Expect(mockCloud.CreateStemcellCallCount()).To(Equal(createStemcellCountBefore + 1)) + Expect(createVMStemcellCID).To(Equal(replacementCID)) + + currentRecord, found, err := stemcellRepo.FindCurrent() + Expect(err).ToNot(HaveOccurred()) + Expect(found).To(BeTrue()) + Expect(currentRecord.CID).To(Equal(replacementCID)) }) // The #731 scenario: #737 stopped VM delete from clearing the From 6fd2d607a67fe5feaa45d4f64524034f7bbd8c84 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Thu, 24 Sep 2026 12:15:00 -0700 Subject: [PATCH 09/12] Don't delete previously tracked stemcell in error case Light stemcells will generally re-use the CID of the actual IaaS image. Re-upload of a light stemcell will typically return the same CID. We shouldn't attempt to delete the stemcell in this scenario --- stemcell/manager.go | 8 +++++--- stemcell/manager_test.go | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/stemcell/manager.go b/stemcell/manager.go index c2967077a..5bd934961 100644 --- a/stemcell/manager.go +++ b/stemcell/manager.go @@ -85,9 +85,11 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S stemcellRecord, err = m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) } if err != nil { - // Nothing records this image, so no cleanup could ever find it. - if deleteErr := m.cloud.DeleteStemcell(cid); deleteErr != nil { - return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s); the orphaned stemcell could not be deleted either: %s", cid, extractedStemcell, deleteErr.Error()) + // Only delete from cloud if this CID was newly created + if !found || foundStemcellRecord.CID != cid { + if deleteErr := m.cloud.DeleteStemcell(cid); deleteErr != nil { + return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s); the orphaned stemcell could not be deleted either: %s", cid, extractedStemcell, deleteErr.Error()) + } } return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s)", cid, extractedStemcell) } diff --git a/stemcell/manager_test.go b/stemcell/manager_test.go index 9146cea5c..f727a90ab 100644 --- a/stemcell/manager_test.go +++ b/stemcell/manager_test.go @@ -303,6 +303,20 @@ var _ = Describe("Manager", func() { Expect(err.Error()).To(ContainSubstring("fake-delete-error")) }) }) + + Context("when the returned CID was already tracked in the repo", func() { + BeforeEach(func() { + fakeRepo.FindReturns(biconfig.StemcellRecord{CID: "fake-stemcell-cid"}, true, nil) + fakeRepo.SaveOrUpdateReturns(biconfig.StemcellRecord{}, errors.New("fake-save-error")) + }) + + It("does not delete the pre-existing stemcell from the cloud", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("fake-save-error")) + Expect(fakeCloud.DeleteStemcellCallCount()).To(Equal(0)) + }) + }) }) Context("when no stemcell record exists and fix is requested", func() { From 19d1d1eb4464e908596625e52f2a24ffde350474 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Thu, 24 Sep 2026 14:35:21 -0700 Subject: [PATCH 10/12] Really don't delete previously tracked stemcell in error case Check all records to ensure that the CID is not tracked before we delete it --- stemcell/manager.go | 17 +++++++++++++++-- stemcell/manager_test.go | 31 +++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/stemcell/manager.go b/stemcell/manager.go index 5bd934961..b5fa0c88b 100644 --- a/stemcell/manager.go +++ b/stemcell/manager.go @@ -85,8 +85,8 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S stemcellRecord, err = m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) } if err != nil { - // Only delete from cloud if this CID was newly created - if !found || foundStemcellRecord.CID != cid { + // Only delete from cloud if this CID is not tracked by any record in state + if !m.isCIDTracked(cid) { if deleteErr := m.cloud.DeleteStemcell(cid); deleteErr != nil { return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s); the orphaned stemcell could not be deleted either: %s", cid, extractedStemcell, deleteErr.Error()) } @@ -155,3 +155,16 @@ func (m *manager) DeleteUnused(deleteStage biui.Stage) error { return nil } + +func (m *manager) isCIDTracked(cid string) bool { + records, err := m.repo.All() + if err != nil { + return true // Defensively assume tracked if repo lookup fails + } + for _, record := range records { + if record.CID == cid { + return true + } + } + return false +} diff --git a/stemcell/manager_test.go b/stemcell/manager_test.go index f727a90ab..c37a844d6 100644 --- a/stemcell/manager_test.go +++ b/stemcell/manager_test.go @@ -304,17 +304,32 @@ var _ = Describe("Manager", func() { }) }) - Context("when the returned CID was already tracked in the repo", func() { + Context("when the returned CID is already tracked in the repo", func() { BeforeEach(func() { - fakeRepo.FindReturns(biconfig.StemcellRecord{CID: "fake-stemcell-cid"}, true, nil) - fakeRepo.SaveOrUpdateReturns(biconfig.StemcellRecord{}, errors.New("fake-save-error")) + fakeRepo.AllReturns([]biconfig.StemcellRecord{{CID: "fake-stemcell-cid"}}, nil) }) - It("does not delete the pre-existing stemcell from the cloud", func() { - _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, true) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("fake-save-error")) - Expect(fakeCloud.DeleteStemcellCallCount()).To(Equal(0)) + Context("by the record being uploaded", func() { + BeforeEach(func() { + fakeRepo.FindReturns(biconfig.StemcellRecord{CID: "fake-stemcell-cid"}, true, nil) + fakeRepo.SaveOrUpdateReturns(biconfig.StemcellRecord{}, errors.New("fake-save-error")) + }) + + It("does not delete the pre-existing stemcell from the cloud", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("fake-save-error")) + Expect(fakeCloud.DeleteStemcellCallCount()).To(Equal(0)) + }) + }) + + Context("by a different record", func() { + It("does not delete the stemcell from the cloud", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, true) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("fake-save-error")) + Expect(fakeCloud.DeleteStemcellCallCount()).To(Equal(0)) + }) }) }) }) From 95fb744995addb09f0e20f421061b365d749f4f6 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Thu, 24 Sep 2026 14:51:42 -0700 Subject: [PATCH 11/12] review feedback omit historical note about PRs in test comments. rename --fix to --fix-stemcell to make it clear what we're fixing. --- cmd/create_env.go | 2 +- cmd/create_env_test.go | 8 ++++---- cmd/opts/opts.go | 2 +- cmd/opts/opts_test.go | 6 +++--- integration/create_env_test.go | 11 ++++------- 5 files changed, 13 insertions(+), 16 deletions(-) diff --git a/cmd/create_env.go b/cmd/create_env.go index 69db7cde1..5b26497b0 100644 --- a/cmd/create_env.go +++ b/cmd/create_env.go @@ -27,7 +27,7 @@ func (c *CreateEnvCmd) Run(stage boshui.Stage, opts CreateEnvOpts) error { return depPreparer.PrepareDeployment(stage, DeploymentOptions{ Recreate: opts.Recreate, RecreatePersistentDisks: opts.RecreatePersistentDisks, - FixStemcell: opts.Fix, + FixStemcell: opts.FixStemcell, SkipDrain: opts.SkipDrain, }) } diff --git a/cmd/create_env_test.go b/cmd/create_env_test.go index 96fb3c956..06e5ebfc0 100644 --- a/cmd/create_env_test.go +++ b/cmd/create_env_test.go @@ -691,16 +691,16 @@ var _ = Describe("CreateEnvCmd", func() { Expect(mockDeployer.DeployCallCount()).To(Equal(1)) }) - It("deploys if `fix` flag is specified, even with no manifest or release changes", func() { - defaultCreateEnvOpts.Fix = true + It("deploys if `fix-stemcell` flag is specified, even with no manifest or release changes", func() { + defaultCreateEnvOpts.FixStemcell = true err := command.Run(fakeStage, defaultCreateEnvOpts) Expect(err).NotTo(HaveOccurred()) Expect(mockDeployer.DeployCallCount()).To(Equal(1)) }) - It("passes `fix` through to the stemcell upload", func() { - defaultCreateEnvOpts.Fix = true + It("passes `fix-stemcell` through to the stemcell upload", func() { + defaultCreateEnvOpts.FixStemcell = true err := command.Run(fakeStage, defaultCreateEnvOpts) Expect(err).NotTo(HaveOccurred()) diff --git a/cmd/opts/opts.go b/cmd/opts/opts.go index d0cf369d1..b2bf5ec7a 100644 --- a/cmd/opts/opts.go +++ b/cmd/opts/opts.go @@ -197,7 +197,7 @@ type CreateEnvOpts struct { StatePath string `long:"state" value-name:"PATH" description:"State file path"` Recreate bool `long:"recreate" description:"Recreate VM in deployment"` RecreatePersistentDisks bool `long:"recreate-persistent-disks" description:"Recreate persistent disks in the deployment"` - Fix bool `long:"fix" description:"Forces re-upload of the stemcell; any existing stemcell image is orphaned and the VM is recreated"` + FixStemcell bool `long:"fix-stemcell" description:"Forces re-upload of the stemcell; any existing stemcell image is orphaned and the VM is recreated"` PackageDir string `long:"package-dir" value-name:"DIR" description:"Package cache location override"` cmd } diff --git a/cmd/opts/opts_test.go b/cmd/opts/opts_test.go index 9fc75b5af..9e083cd9b 100644 --- a/cmd/opts/opts_test.go +++ b/cmd/opts/opts_test.go @@ -869,9 +869,9 @@ var _ = Describe("Opts", func() { )) }) - It("has --fix", func() { - Expect(getStructTagForName("Fix", opts)).To(Equal( - `long:"fix" description:"Forces re-upload of the stemcell; any existing stemcell image is orphaned and the VM is recreated"`, + It("has --fix-stemcell", func() { + Expect(getStructTagForName("FixStemcell", opts)).To(Equal( + `long:"fix-stemcell" description:"Forces re-upload of the stemcell; any existing stemcell image is orphaned and the VM is recreated"`, )) }) }) diff --git a/integration/create_env_test.go b/integration/create_env_test.go index 92c98b4c0..c9f8ffc84 100644 --- a/integration/create_env_test.go +++ b/integration/create_env_test.go @@ -1140,7 +1140,7 @@ cloud_provider: }) }) - Context("and the same deployment is attempted again with --fix", func() { + Context("and the same deployment is attempted again with --fix-stemcell", func() { // A second deploy draws a fresh agent ID, which the shared stub // asserts against a fixed value. relaxCreateVM := func() { @@ -1165,7 +1165,7 @@ cloud_provider: createStemcellCountBefore := mockCloud.CreateStemcellCallCount() fixOpts := newDeployOpts(deploymentManifestPath, "") - fixOpts.Fix = true + fixOpts.FixStemcell = true err := newCreateEnvCmd().Run(fakeStage, fixOpts) Expect(err).ToNot(HaveOccurred()) @@ -1179,16 +1179,13 @@ cloud_provider: Expect(currentRecord.CID).To(Equal(replacementCID)) }) - // The #731 scenario: #737 stopped VM delete from clearing the - // pointer, this PR stops the upload from clearing it. Neither - // alone survives a replacement VM that fails before promotion. It("leaves CurrentStemcellID resolving to a real record when the replacement VM fails", func() { mockCloud.CreateVMStub = func(_, _ string, _ biproperty.Map, _ []string, _ map[string]biproperty.Map, _ biproperty.Map) (string, error) { return "", bosherr.Error("fake-create-vm-error") } fixOpts := newDeployOpts(deploymentManifestPath, "") - fixOpts.Fix = true + fixOpts.FixStemcell = true err := newCreateEnvCmd().Run(fakeStage, fixOpts) Expect(err).To(HaveOccurred()) @@ -1206,7 +1203,7 @@ cloud_provider: relaxCreateVM() fixOpts := newDeployOpts(deploymentManifestPath, "") - fixOpts.Fix = true + fixOpts.FixStemcell = true err := newCreateEnvCmd().Run(fakeStage, fixOpts) Expect(err).ToNot(HaveOccurred()) From e00a7a83a846b4aae03ebb17503a65b007b64bc2 Mon Sep 17 00:00:00 2001 From: Julian Hjortshoj Date: Thu, 24 Sep 2026 15:09:21 -0700 Subject: [PATCH 12/12] Report tracking lookup failures when stemcell save fails If saving the stemcell record fails and the repo lookup used to decide whether the CID is tracked also fails, keep the stemcell but say so in the error, since it may now be orphaned. Co-Authored-By: Claude Opus 5.5 --- stemcell/manager.go | 14 +++++++++----- stemcell/manager_test.go | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/stemcell/manager.go b/stemcell/manager.go index b5fa0c88b..acc5bb854 100644 --- a/stemcell/manager.go +++ b/stemcell/manager.go @@ -86,7 +86,11 @@ func (m *manager) Upload(extractedStemcell ExtractedStemcell, uploadStage biui.S } if err != nil { // Only delete from cloud if this CID is not tracked by any record in state - if !m.isCIDTracked(cid) { + tracked, lookupErr := m.isCIDTracked(cid) + if lookupErr != nil { + return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s); could not determine whether the CID is tracked, so the stemcell may be orphaned: %s", cid, extractedStemcell, lookupErr.Error()) + } + if !tracked { if deleteErr := m.cloud.DeleteStemcell(cid); deleteErr != nil { return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s); the orphaned stemcell could not be deleted either: %s", cid, extractedStemcell, deleteErr.Error()) } @@ -156,15 +160,15 @@ func (m *manager) DeleteUnused(deleteStage biui.Stage) error { return nil } -func (m *manager) isCIDTracked(cid string) bool { +func (m *manager) isCIDTracked(cid string) (bool, error) { records, err := m.repo.All() if err != nil { - return true // Defensively assume tracked if repo lookup fails + return true, err // Defensively assume tracked if repo lookup fails } for _, record := range records { if record.CID == cid { - return true + return true, nil } } - return false + return false, nil } diff --git a/stemcell/manager_test.go b/stemcell/manager_test.go index c37a844d6..3368ff669 100644 --- a/stemcell/manager_test.go +++ b/stemcell/manager_test.go @@ -304,6 +304,25 @@ var _ = Describe("Manager", func() { }) }) + Context("when checking whether the CID is tracked also fails", func() { + BeforeEach(func() { + fakeRepo.AllReturns(nil, errors.New("fake-all-error")) + }) + + It("does not delete the stemcell from the cloud", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, false) + Expect(err).To(HaveOccurred()) + Expect(fakeCloud.DeleteStemcellCallCount()).To(Equal(0)) + }) + + It("still reports the save failure, mentioning the lookup failure and possible orphan", func() { + _, err := fakeManager.Upload(expectedExtractedStemcell, fakeStage, false) + Expect(err.Error()).To(ContainSubstring("fake-save-error")) + Expect(err.Error()).To(ContainSubstring("fake-all-error")) + Expect(err.Error()).To(ContainSubstring("may be orphaned")) + }) + }) + Context("when the returned CID is already tracked in the repo", func() { BeforeEach(func() { fakeRepo.AllReturns([]biconfig.StemcellRecord{{CID: "fake-stemcell-cid"}}, nil)