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

@@ -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 {

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.

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)

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
 }
 

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)