Merge pull request #143 from bmelville/refs

Failures during config creation no longer leave deployments undeleteable

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)