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)