Skip to content

Commit

Permalink
Remove plan dao methods (openshift#766)
Browse files Browse the repository at this point in the history 
* remove plans names -> id mapping from storage.

* Currently we save a mapping between names and id and have the data in
the spec already.
* This was a convince method to quickly get that data, but when moving
to CRDs I don't think we should create a resource for a single value
mapping

* clean up copied comment.
  • Loading branch information
@shawn-hurley @jmrodri
shawn-hurley authored and jmrodri committed Feb 16, 2018
1 parent b09c52b commit 1526cd8
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 95 deletions.
23 changes: 17 additions & 6 deletions pkg/apb/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,26 @@ type Spec struct {
Plans []Plan `json:"plans"`
}

// GetPlan - retrieves a reference to a plan from a spec by name. Will return
// nil if the requested plan does not exist.
func (s *Spec) GetPlan(name string) *Plan {
for i, plan := range s.Plans {
// GetPlan - retrieves a plan from a spec by name. Will return
// empty plan and false if the requested plan does not exist.
func (s *Spec) GetPlan(name string) (Plan, bool) {
for _, plan := range s.Plans {
if plan.Name == name {
return &s.Plans[i]
return plan, true
}
}
return nil
return Plan{}, false
}

// GetPlanFromID - retrieves a plan from a spec by id. Will return
// empty plan and false if the requested plan does not exist.
func (s *Spec) GetPlanFromID(id string) (Plan, bool) {
for _, plan := range s.Plans {
if plan.ID == id {
return plan, true
}
}
return Plan{}, false
}

// Context - Determines the context in which the service is running
Expand Down
88 changes: 32 additions & 56 deletions pkg/broker/broker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,6 @@ func (a AnsibleBroker) Bootstrap() (*BootstrapResponse, error) {
return nil, err
}

// save off the plan names as well
if err = a.dao.BatchSetPlanNames(planNameManifest); err != nil {
return nil, err
}

apb.AddSecrets(specs)

return &BootstrapResponse{SpecCount: len(specs), ImageCount: imageCount}, nil
Expand Down Expand Up @@ -542,7 +537,6 @@ func (a AnsibleBroker) Provision(instanceUUID uuid.UUID, req *ProvisionRequest,
*/
var spec *apb.Spec
var err error
var planName string

// Retrieve requested spec
specID := req.ServiceID
Expand All @@ -568,20 +562,15 @@ func (a AnsibleBroker) Provision(instanceUUID uuid.UUID, req *ProvisionRequest,
return nil, errors.New(errMsg)
}

planName, err = a.dao.GetPlanName(req.PlanID)
if err != nil {
// etcd return not found i.e. code 100
if client.IsKeyNotFound(err) {
return nil, ErrorNotFound
}
// otherwise unknown error bubble it up
return nil, err
plan, ok := spec.GetPlanFromID(req.PlanID)
if !ok {
return nil, ErrorNotFound
}

log.Debugf(
"Injecting PlanID as parameter: { %s: %s }",
planParameterKey, planName)
parameters[planParameterKey] = planName
planParameterKey, plan.Name)
parameters[planParameterKey] = plan.Name
log.Debugf("Injecting ServiceClassID as parameter: { %s: %s }",
serviceClassIDKey, req.ServiceID)
parameters[serviceClassIDKey] = req.ServiceID
Expand Down Expand Up @@ -814,23 +803,17 @@ func (a AnsibleBroker) Bind(instance apb.ServiceInstance, bindingUUID uuid.UUID,
"Bind requests must specify PlanIDs"
return nil, false, errors.New(errMsg)
}

planName, err := a.dao.GetPlanName(req.PlanID)
if err != nil {
// etcd return not found i.e. code 100
if client.IsKeyNotFound(err) {
log.Debug("Plan not found")
return nil, false, ErrorNotFound
}
// otherwise unknown error bubble it up
return nil, false, err
plan, ok := instance.Spec.GetPlanFromID(req.PlanID)
if !ok {
log.Debug("Plan not found")
return nil, false, ErrorNotFound
}

log.Debugf(
"Injecting PlanID as parameter: { %s: %s }",
planParameterKey, planName)
planParameterKey, plan.Name)

params[planParameterKey] = planName
params[planParameterKey] = plan.Name
log.Debugf("Injecting ServiceClassID as parameter: { %s: %s }",
serviceClassIDKey, req.ServiceID)

Expand Down Expand Up @@ -1149,8 +1132,8 @@ func (a AnsibleBroker) Update(instanceUUID uuid.UUID, req *UpdateRequest, async
*/

var err error
var fromPlanName, toPlanName string
var fromPlan, toPlan *apb.Plan
var fromPlanName string
var fromPlan, toPlan apb.Plan

si, err := a.dao.GetServiceInstance(instanceUUID.String())
if err != nil {
Expand Down Expand Up @@ -1208,46 +1191,43 @@ func (a AnsibleBroker) Update(instanceUUID uuid.UUID, req *UpdateRequest, async
return nil, errors.New(emsg)
}

fromPlan, ok = spec.GetPlan(fromPlanName)
if !ok {
log.Error("The plan %s, specified for updating from on instance %s, does not exist.", fromPlanName, si.ID)
return nil, ErrorPlanNotFound
}

log.Debugf("Update received the following Request.PlanID: [%s]", req.PlanID)

if req.PlanID == "" {
// Lock to currentPlan if no plan passed in request
// No need to decode from FQPlanID -> ServiceClass scoped plan name, since
// `fromPlanName` in this case is already decoded. Ex: "prod" instead of the md5 hash
toPlanName = fromPlanName
toPlan = fromPlan
} else {
// The catalog only identifies plans via their md5(FQPlanID), and will request
// and update using that hash. If a PlanID is submitted, we'll need to look up
// the ServiceClass scoped plan name via the passed in hash so the APB
// will understand what to do with it, since APBs do not understand plan hashes.
toPlanName, err = a.dao.GetPlanName(req.PlanID)
if err != nil {

toPlan, ok = spec.GetPlanFromID(req.PlanID)
if !ok {
log.Error("Could not find requested PlanID %s in plan name lookup table", req.PlanID)
return nil, ErrorPlanNotFound
}
}

// Retrieve from/to plans by name, else respond with appropriate error
if fromPlan = spec.GetPlan(fromPlanName); fromPlan == nil {
log.Error("The plan %s, specified for updating from on instance %s, does not exist.", fromPlanName, si.ID)
return nil, ErrorPlanNotFound
}
if toPlan = spec.GetPlan(toPlanName); toPlan == nil {
log.Error("The plan %s, specified for updating to on instance %s, does not exist.", toPlanName, si.ID)
return nil, ErrorPlanNotFound
}

// If a plan transition has been requested, validate it is possible and then
// update the service instance with the desired next plan
if fromPlanName != toPlanName {
log.Debug("Validating plan transition from: %s, to: %s", fromPlanName, toPlanName)
if ok := a.isValidPlanTransition(fromPlan, toPlanName); !ok {
log.Error("The current plan, %s, cannot be updated to the requested plan, %s.", fromPlanName, toPlanName)
if fromPlan.Name != toPlan.Name {
log.Debug("Validating plan transition from: %s, to: %s", fromPlan.Name, toPlan.Name)
if ok := a.isValidPlanTransition(fromPlan, toPlan.Name); !ok {
log.Error("The current plan, %s, cannot be updated to the requested plan, %s.", fromPlan.Name, toPlan.Name)
return nil, ErrorPlanUpdateNotPossible
}

log.Debug("Plan transition valid!")
(*si.Parameters)[planParameterKey] = toPlanName
(*si.Parameters)[planParameterKey] = toPlan.Name
} else {
log.Debug("Plan transition NOT requested as part of update")
}
Expand All @@ -1257,7 +1237,7 @@ func (a AnsibleBroker) Update(instanceUUID uuid.UUID, req *UpdateRequest, async
return nil, err
}

if fromPlanName == toPlanName && len(req.Parameters) == 0 {
if fromPlan.Name == toPlan.Name && len(req.Parameters) == 0 {
log.Warningf("Returning without running the APB. No changes were actually requested")

return &UpdateResponse{}, ErrorNoUpdateRequested
Expand All @@ -1277,7 +1257,7 @@ func (a AnsibleBroker) Update(instanceUUID uuid.UUID, req *UpdateRequest, async

log.Debug("Initiating update with the inputs:")
log.Debugf("fromPlanName: [%s]", fromPlanName)
log.Debugf("toPlanName: [%s]", toPlanName)
log.Debugf("toPlanName: [%s]", toPlan.Name)
log.Debugf("PreviousValues: [ %+v ]", req.PreviousValues)
log.Debugf("ServiceInstance Parameters: [%v]", *si.Parameters)

Expand Down Expand Up @@ -1308,7 +1288,7 @@ func (a AnsibleBroker) Update(instanceUUID uuid.UUID, req *UpdateRequest, async
return &UpdateResponse{Operation: token}, nil
}

func (a AnsibleBroker) isValidPlanTransition(fromPlan *apb.Plan, toPlanName string) bool {
func (a AnsibleBroker) isValidPlanTransition(fromPlan apb.Plan, toPlanName string) bool {
// Make sure that we can find the plan we're updating from.
// This should probably never fail, but cover our tail.
for _, validToPlanName := range fromPlan.UpdatesTo {
Expand All @@ -1321,7 +1301,7 @@ func (a AnsibleBroker) isValidPlanTransition(fromPlan *apb.Plan, toPlanName stri

func (a AnsibleBroker) validateRequestedUpdateParams(
reqParams map[string]string,
toPlan *apb.Plan,
toPlan apb.Plan,
prevParams map[string]interface{},
si *apb.ServiceInstance,
) (map[string]string, error) {
Expand Down Expand Up @@ -1422,10 +1402,6 @@ func (a AnsibleBroker) AddSpec(spec apb.Spec) (*CatalogResponse, error) {
spec.Image = spec.FQName
addNameAndIDForSpec([]*apb.Spec{&spec}, apbPushRegName)
log.Debugf("Generated name for pushed APB: [%s], ID: [%s]", spec.FQName, spec.ID)
for _, p := range spec.Plans {
a.dao.SetPlanName(p.ID, p.Name)
}

if err := a.dao.SetSpec(spec.ID, &spec); err != nil {
return nil, err
}
Expand Down
9 changes: 0 additions & 9 deletions pkg/dao/dao.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,4 @@ type Dao interface {

// GetStateByKey - Retrieve a job state from the kvp API for a job key
GetStateByKey(key string) (apb.JobState, error)

// BatchSetPlanNames - set plannames based on PlanNameManifest in the kvp API.
BatchSetPlanNames(map[string]string) error

// SetPlanName - Set the Plan name in the kvp API for the given id.
SetPlanName(string, string) error

// GetPlanName - Retrieve the plan name associated with the ID
GetPlanName(string) (string, error)
}
24 changes: 0 additions & 24 deletions pkg/dao/etcd/dao.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,30 +353,6 @@ func (d *Dao) GetStateByKey(key string) (apb.JobState, error) {
return state, nil
}

// BatchSetPlanNames - set plannames based on PlanNameManifest in the kvp API.
func (d *Dao) BatchSetPlanNames(planNames map[string]string) error {
for id, planName := range planNames {
err := d.SetPlanName(id, planName)
if err != nil {
return err
}
}

return nil
}

// SetPlanName - Set the Plan name in the kvp API for the given id.
func (d *Dao) SetPlanName(id string, name string) error {
// it's just a string so no other work is needed
return d.SetRaw(planNameKey(id), name)
}

// GetPlanName - Retrieve the plan name associated with the ID
func (d *Dao) GetPlanName(id string) (string, error) {
// it's just a string so no other work is needed
return d.GetRaw(planNameKey(id))
}

func (d *Dao) getObject(key string, data interface{}) error {
raw, err := d.GetRaw(key)
if err != nil {
Expand Down

0 comments on commit 1526cd8

Please sign in to comment.