diff --git a/cmd/create_env.go b/cmd/create_env.go index c3ed3a7bf..5b26497b0 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.SkipDrain) + return depPreparer.PrepareDeployment(stage, DeploymentOptions{ + Recreate: opts.Recreate, + RecreatePersistentDisks: opts.RecreatePersistentDisks, + FixStemcell: opts.FixStemcell, + SkipDrain: opts.SkipDrain, + }) } diff --git a/cmd/create_env_test.go b/cmd/create_env_test.go index 835d63afc..06e5ebfc0 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,25 @@ var _ = Describe("CreateEnvCmd", func() { Expect(err).NotTo(HaveOccurred()) Expect(mockDeployer.DeployCallCount()).To(Equal(1)) }) + + 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-stemcell` through to the stemcell upload", func() { + defaultCreateEnvOpts.FixStemcell = 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..a663407e9 100644 --- a/cmd/deployment_preparer.go +++ b/cmd/deployment_preparer.go @@ -100,7 +100,20 @@ type DeploymentPreparer struct { targetProvider biinstall.TargetProvider } -func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, recreate bool, recreatePersistentDisks bool, skipDrain bool) (err error) { +// DeploymentOptions carries the per-run switches for PrepareDeployment. +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() { @@ -183,7 +196,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 +217,7 @@ func (c *DeploymentPreparer) PrepareDeployment(stage biui.Stage, recreate bool, deploymentManifest, manifestSHA, skipDrain, + fix, stage, cloud, ) @@ -234,12 +248,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..b2bf5ec7a 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"` + 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 23a211524..9e083cd9b 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-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"`, + )) + }) }) Describe("CreateEnvArgs", func() { 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..e86dd3242 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,47 @@ func (r stemcellRepo) Save(name, version, cid string, apiVersion int) (StemcellR return stemcellRecord, err } +// 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{} + + 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..471f0a40c 100644 --- a/config/stemcell_repo_test.go +++ b/config/stemcell_repo_test.go @@ -29,6 +29,77 @@ 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()) + }) + + 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()) + 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/integration/create_env_test.go b/integration/create_env_test.go index 54fe5d969..c9f8ffc84 100644 --- a/integration/create_env_test.go +++ b/integration/create_env_test.go @@ -1139,6 +1139,84 @@ cloud_provider: Expect(mockCloud.CreateVMCallCount()).To(Equal(createVMCountBefore)) }) }) + + 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() { + 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 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, "") + fixOpts.FixStemcell = true + + err := newCreateEnvCmd().Run(fakeStage, fixOpts) + 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)) + }) + + 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.FixStemcell = 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() + + fixOpts := newDeployOpts(deploymentManifestPath, "") + fixOpts.FixStemcell = 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() { diff --git a/stemcell/manager.go b/stemcell/manager.go index 918a63146..b5fa0c88b 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,10 @@ 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) { +// +// 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) err = uploadStage.Perform(stageName, func() error { @@ -63,22 +66,38 @@ 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") } + // 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) } - stemcellRecord, err := m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) + // 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) + } else { + stemcellRecord, err = m.repo.Save(manifest.Name, manifest.Version, cid, manifest.ApiVersion) + } if err != nil { - // TODO: delete stemcell from cloud when saving fails + // 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()) + } + } return bosherr.WrapErrorf(err, "saving stemcell record in repo (cid=%s, stemcell=%s)", cid, extractedStemcell) } + // 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 }) @@ -136,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 0911c7848..c37a844d6 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" @@ -80,7 +81,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 +92,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 +109,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 +119,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 +130,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 +151,196 @@ 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.*")) }) + + 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()) + + 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")) + }) + + 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()) + + 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("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()) + + unused, err := manager.FindUnused() + Expect(err).ToNot(HaveOccurred()) + Expect(unused).To(BeEmpty(), "a blanked CurrentStemcellID would mark every stemcell unused") + }) + + It("leaves the replaced image in the cloud as the rollback target", 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 the upload fails partway, as a long transfer may", 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")) + }) + }) + }) + }) + + // 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 + 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 the returned CID is already tracked in the repo", func() { + BeforeEach(func() { + fakeRepo.AllReturns([]biconfig.StemcellRecord{{CID: "fake-stemcell-cid"}}, nil) + }) + + 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)) + }) + }) + }) + }) + + 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) {