From 31177c49ab2f10110da85495b17ff2911a0c745f Mon Sep 17 00:00:00 2001 From: Brendan Melville Date: Thu, 3 Dec 2015 14:04:03 -0500 Subject: [PATCH] Failures during config creation no longer leave deployments undeleteable Previously a failure would prematurely exit the creation flow without attaching the manifest to the deployment. This now always attaches the manifest, and then updates it in place with the outcome of resourcification. --- common/types.go | 24 -------- manager/manager/manager.go | 33 +++++------ manager/manager/manager_test.go | 22 ++++++++ manager/repository/repository.go | 80 +++++++++++++++++++-------- manager/repository/repository_test.go | 6 +- 5 files changed, 99 insertions(+), 66 deletions(-) diff --git a/common/types.go b/common/types.go index 40605bc70..13218dd31 100644 --- a/common/types.go +++ b/common/types.go @@ -28,30 +28,6 @@ type Schema struct { Imports []SchemaImport `json:"imports"` } -// Repository manages storage for all Deployment Manager entities, as well as -// the common operations to store, access and manage them. -type Repository interface { - // Deployments. - ListDeployments() ([]Deployment, error) - GetDeployment(name string) (*Deployment, error) - GetValidDeployment(name string) (*Deployment, error) - CreateDeployment(name string) (*Deployment, error) - DeleteDeployment(name string, forget bool) (*Deployment, error) - SetDeploymentStatus(name string, status DeploymentStatus) error - - // Manifests. - AddManifest(deploymentName string, manifest *Manifest) error - ListManifests(deploymentName string) (map[string]*Manifest, error) - GetManifest(deploymentName string, manifestName string) (*Manifest, error) - GetLatestManifest(deploymentName string) (*Manifest, error) - - // Types. - ListTypes() []string - GetTypeInstances(typeName string) []*TypeInstance - ClearTypeInstances(deploymentName string) - SetTypeInstances(deploymentName string, instances map[string][]*TypeInstance) -} - // Deployment defines a deployment that describes // the creation, modification and/or deletion of a set of resources. type Deployment struct { diff --git a/manager/manager/manager.go b/manager/manager/manager.go index a8aa08a7e..553c6736a 100644 --- a/manager/manager/manager.go +++ b/manager/manager/manager.go @@ -19,6 +19,7 @@ import ( "time" "github.com/kubernetes/deployment-manager/common" + "github.com/kubernetes/deployment-manager/manager/repository" ) // Manager manages a persistent set of Deployments. @@ -38,11 +39,11 @@ type Manager interface { type manager struct { expander Expander deployer Deployer - repository common.Repository + repository repository.Repository } // NewManager returns a new initialized Manager. -func NewManager(expander Expander, deployer Deployer, repository common.Repository) Manager { +func NewManager(expander Expander, deployer Deployer, repository repository.Repository) Manager { return &manager{expander, deployer, repository} } @@ -105,8 +106,14 @@ func (m *manager) CreateDeployment(t *common.Template) (*common.Deployment, erro return nil, err } - actualConfig, createErr := m.deployer.CreateConfiguration(manifest.ExpandedConfig) - if createErr != nil { + if err := m.repository.AddManifest(t.Name, manifest); err != nil { + log.Printf("AddManifest failed %v", err) + m.repository.SetDeploymentStatus(t.Name, common.FailedStatus) + return nil, err + } + + actualConfig, err := m.deployer.CreateConfiguration(manifest.ExpandedConfig) + if err != nil { // Deployment failed, mark as failed log.Printf("CreateConfiguration failed: %v", err) m.repository.SetDeploymentStatus(t.Name, common.FailedStatus) @@ -114,7 +121,7 @@ func (m *manager) CreateDeployment(t *common.Template) (*common.Deployment, erro // return the failure as such. Otherwise, we're going to add the manifest // and hence resource specific errors down below. if actualConfig == nil { - return nil, createErr + return nil, err } } else { m.repository.SetDeploymentStatus(t.Name, common.DeployedStatus) @@ -122,20 +129,10 @@ func (m *manager) CreateDeployment(t *common.Template) (*common.Deployment, erro // Update the manifest with the actual state of the reified resources manifest.ExpandedConfig = actualConfig - aErr := m.repository.AddManifest(t.Name, manifest) - if aErr != nil { - log.Printf("AddManifest failed %v", aErr) + if err := m.repository.SetManifest(t.Name, manifest); err != nil { + log.Printf("SetManifest failed %v", err) m.repository.SetDeploymentStatus(t.Name, common.FailedStatus) - // If there's an earlier error, return that instead since it contains - // more applicable error message. Adding manifest failure is more akin - // to a check fail (either deployment doesn't exist, or a manifest with the same - // name already exists). - // TODO(vaikas): Should we combine both errors and return a nicely formatted error for both? - if createErr != nil { - return nil, createErr - } else { - return nil, aErr - } + return nil, err } // Finally update the type instances for this deployment. diff --git a/manager/manager/manager_test.go b/manager/manager/manager_test.go index 67e1441c0..acfee73f9 100644 --- a/manager/manager/manager_test.go +++ b/manager/manager/manager_test.go @@ -127,6 +127,7 @@ type repositoryStub struct { FailListDeployments bool Created []string ManifestAdd map[string]*common.Manifest + ManifestSet map[string]*common.Manifest Deleted []string GetValid []string TypeInstances map[string][]string @@ -140,6 +141,7 @@ func (repository *repositoryStub) reset() { repository.FailListDeployments = false repository.Created = make([]string, 0) repository.ManifestAdd = make(map[string]*common.Manifest) + repository.ManifestSet = make(map[string]*common.Manifest) repository.Deleted = make([]string, 0) repository.GetValid = make([]string, 0) repository.TypeInstances = make(map[string][]string) @@ -194,6 +196,11 @@ func (repository *repositoryStub) AddManifest(d string, manifest *common.Manifes return nil } +func (repository *repositoryStub) SetManifest(d string, manifest *common.Manifest) error { + repository.ManifestSet[d] = manifest + return nil +} + func (repository *repositoryStub) GetLatestManifest(d string) (*common.Manifest, error) { if d == deploymentName { return repository.ManifestAdd[d], nil @@ -323,6 +330,11 @@ func TestCreateDeployment(t *testing.T) { "to begin with manifest-.", testRepository.ManifestAdd[template.Name].Name) } + if !strings.HasPrefix(testRepository.ManifestSet[template.Name].Name, "manifest-") { + t.Fatalf("Repository SetManifest was called with %s but expected manifest name"+ + "to begin with manifest-.", testRepository.ManifestSet[template.Name].Name) + } + if !reflect.DeepEqual(*testDeployer.Created[0], configuration) || err != nil { t.Fatalf("Deployer CreateConfiguration was called with %s but expected %s.", testDeployer.Created[0], configuration) @@ -396,6 +408,11 @@ func TestCreateDeploymentCreationResourceFailure(t *testing.T) { "to begin with manifest-.", testRepository.ManifestAdd[template.Name].Name) } + if !strings.HasPrefix(testRepository.ManifestSet[template.Name].Name, "manifest-") { + t.Fatalf("Repository SetManifest was called with %s but expected manifest name"+ + "to begin with manifest-.", testRepository.ManifestSet[template.Name].Name) + } + if err != nil || !reflect.DeepEqual(d, &deployment) { t.Fatalf("Expected a different set of response values from invoking CreateDeployment.\n"+ "Received: %v, %v. Expected: %v, %v.", d, err, &deployment, "nil") @@ -425,6 +442,11 @@ func TestDeleteDeploymentForget(t *testing.T) { "to begin with manifest-.", testRepository.ManifestAdd[template.Name].Name) } + if !strings.HasPrefix(testRepository.ManifestSet[template.Name].Name, "manifest-") { + t.Fatalf("Repository SetManifest was called with %s but expected manifest name"+ + "to begin with manifest-.", testRepository.ManifestSet[template.Name].Name) + } + if !reflect.DeepEqual(*testDeployer.Created[0], configuration) || err != nil { t.Fatalf("Deployer CreateConfiguration was called with %s but expected %s.", testDeployer.Created[0], configuration) diff --git a/manager/repository/repository.go b/manager/repository/repository.go index 6d8f98a4d..8abf6d84f 100644 --- a/manager/repository/repository.go +++ b/manager/repository/repository.go @@ -25,6 +25,31 @@ import ( "github.com/kubernetes/deployment-manager/common" ) +// Repository manages storage for all Deployment Manager entities, as well as +// the common operations to store, access and manage them. +type Repository interface { + // Deployments. + ListDeployments() ([]common.Deployment, error) + GetDeployment(name string) (*common.Deployment, error) + GetValidDeployment(name string) (*common.Deployment, error) + CreateDeployment(name string) (*common.Deployment, error) + DeleteDeployment(name string, forget bool) (*common.Deployment, error) + SetDeploymentStatus(name string, status common.DeploymentStatus) error + + // Manifests. + AddManifest(deploymentName string, manifest *common.Manifest) error + SetManifest(deploymentName string, manifest *common.Manifest) error + ListManifests(deploymentName string) (map[string]*common.Manifest, error) + GetManifest(deploymentName string, manifestName string) (*common.Manifest, error) + GetLatestManifest(deploymentName string) (*common.Manifest, error) + + // Types. + ListTypes() []string + GetTypeInstances(typeName string) []*common.TypeInstance + ClearTypeInstances(deploymentName string) + SetTypeInstances(deploymentName string, instances map[string][]*common.TypeInstance) +} + // deploymentTypeInstanceMap stores type instances mapped by deployment name. // This allows for simple updating and deleting of per-deployment instances // when deployments are created/updated/deleted. @@ -39,7 +64,7 @@ type mapBasedRepository struct { } // NewMapBasedRepository returns a new map based repository. -func NewMapBasedRepository() common.Repository { +func NewMapBasedRepository() Repository { return &mapBasedRepository{ deployments: make(map[string]common.Deployment, 0), manifests: make(map[string]map[string]*common.Manifest, 0), @@ -129,39 +154,48 @@ func (r *mapBasedRepository) CreateDeployment(name string) (*common.Deployment, return d, nil } +// AddManifest adds a manifest to the repository and repoints the latest +// manifest to it for the corresponding deployment. func (r *mapBasedRepository) AddManifest(deploymentName string, manifest *common.Manifest) error { - err := func() error { - r.Lock() - defer r.Unlock() + r.Lock() + defer r.Unlock() - d, err := r.GetValidDeployment(deploymentName) - if err != nil { - return err - } + l, err := r.listManifestsForDeployment(deploymentName) + if err != nil { + return err + } - l, err := r.listManifestsForDeployment(deploymentName) - if err != nil { - return err - } + // Make sure the manifest doesn't already exist, and if not, add the manifest to + // map of manifests this deployment has + if _, ok := l[manifest.Name]; ok { + return fmt.Errorf("Manifest %s already exists in deployment %s", manifest.Name, deploymentName) + } - // Make sure the manifest doesn't already exist, and if not, add the manifest to - // map of manifests this deployment has - if _, ok := l[manifest.Name]; ok { - return fmt.Errorf("Manifest %s already exists in deployment %s", manifest.Name, deploymentName) - } + d, err := r.GetValidDeployment(deploymentName) + if err != nil { + return err + } - l[manifest.Name] = manifest - d.LatestManifest = manifest.Name - r.deployments[deploymentName] = *d + l[manifest.Name] = manifest + d.LatestManifest = manifest.Name + r.deployments[deploymentName] = *d - return nil - }() + log.Printf("Added manifest %s to deployment: %s", manifest.Name, deploymentName) + return nil +} + +// SetManifest sets an existing manifest in the repository to provided +// manifest. +func (r *mapBasedRepository) SetManifest(deploymentName string, manifest *common.Manifest) error { + r.Lock() + defer r.Unlock() + l, err := r.listManifestsForDeployment(deploymentName) if err != nil { return err } - log.Printf("Added manifest %s to deployment: %s", manifest.Name, deploymentName) + l[manifest.Name] = manifest return nil } diff --git a/manager/repository/repository_test.go b/manager/repository/repository_test.go index 2f90fd4fe..4bb7067b6 100644 --- a/manager/repository/repository_test.go +++ b/manager/repository/repository_test.go @@ -82,11 +82,15 @@ func testCreateDeploymentWithManifests(t *testing.T, count int) { if err != nil { t.Fatalf("AddManifest failed: %v", err) } - _, err = r.GetDeployment(deploymentName) + d, err = r.GetDeployment(deploymentName) if err != nil { t.Fatalf("GetDeployment failed: %v", err) } + if d.LatestManifest != manifestName { + t.Fatalf("AddManifest did not update LatestManifest: %#v", d) + } + mListNew, err := r.ListManifests(deploymentName) if err != nil { t.Fatalf("ListManifests failed: %v", err)