Skip to content

Commit

Permalink
mapper: make Mapper use mapper.Info as input
Browse files Browse the repository at this point in the history 
All around the codebase we're creating mapper.Info structures and then
calling Mapper functions that create Info structures again.

Given that mapper.Info already defines all the metadata that Mapper
needs to do the native-to-ovs transations, it makes sense to use Info
structures as input to all functions.

That simplifies the code inside the mapper module. Also, I'd expect some
performance improvement since we were creating multiple Info structs
unnecessarily in the host path.

It's true that, for now, it makes it sligthly more cumbersone to call
mapper functions, since the Info struct has to be created first and it
now requires an additional argument (the table name). However, this
can be improved later on by having the database model build the
Info structs for us.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
  • Loading branch information
@amorenoz
amorenoz committed Oct 13, 2021
1 parent cf54b19 commit 3ff43c5
Show file tree
Hide file tree
Showing 13 changed files with 228 additions and 236 deletions.
29 changes: 14 additions & 15 deletions cache/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func (r *RowCache) RowByModel(m model.Model) model.Model {
if reflect.TypeOf(m) != r.dataType {
return nil
}
info, _ := mapper.NewInfo(&r.schema, m)
info, _ := mapper.NewInfo(r.name, &r.schema, m)
uuid, err := info.FieldByColumn("_uuid")
if err != nil {
return nil
Expand Down Expand Up @@ -120,7 +120,7 @@ func (r *RowCache) Create(uuid string, m model.Model, checkIndexes bool) error {
if reflect.TypeOf(m) != r.dataType {
return fmt.Errorf("expected data of type %s, but got %s", r.dataType.String(), reflect.TypeOf(m).String())
}
info, err := mapper.NewInfo(&r.schema, m)
info, err := mapper.NewInfo(r.name, &r.schema, m)
if err != nil {
return err
}
Expand Down Expand Up @@ -156,11 +156,11 @@ func (r *RowCache) Update(uuid string, m model.Model, checkIndexes bool) error {
return fmt.Errorf("row %s does not exist", uuid)
}
oldRow := model.Clone(r.cache[uuid])
oldInfo, err := mapper.NewInfo(&r.schema, oldRow)
oldInfo, err := mapper.NewInfo(r.name, &r.schema, oldRow)
if err != nil {
return err
}
newInfo, err := mapper.NewInfo(&r.schema, m)
newInfo, err := mapper.NewInfo(r.name, &r.schema, m)
if err != nil {
return err
}
Expand Down Expand Up @@ -218,7 +218,7 @@ func (r *RowCache) Update(uuid string, m model.Model, checkIndexes bool) error {
}

func (r *RowCache) IndexExists(row model.Model) error {
info, err := mapper.NewInfo(&r.schema, row)
info, err := mapper.NewInfo(r.name, &r.schema, row)
if err != nil {
return err
}
Expand Down Expand Up @@ -252,7 +252,7 @@ func (r *RowCache) Delete(uuid string) error {
return fmt.Errorf("row %s does not exist", uuid)
}
oldRow := r.cache[uuid]
oldInfo, err := mapper.NewInfo(&r.schema, oldRow)
oldInfo, err := mapper.NewInfo(r.name, &r.schema, oldRow)
if err != nil {
return err
}
Expand Down Expand Up @@ -325,7 +325,7 @@ func (r *RowCache) RowsByCondition(conditions []ovsdb.Condition) ([]model.Model,
} else {
for _, uuid := range r.Rows() {
row := r.Row(uuid)
info, err := mapper.NewInfo(&r.schema, row)
info, err := mapper.NewInfo(r.name, &r.schema, row)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -761,18 +761,17 @@ func (t *TableCache) CreateModel(tableName string, row *ovsdb.Row, uuid string)
if err != nil {
return nil, err
}

err = t.dbModel.Mapper().GetRowData(tableName, row, model)
info, err := mapper.NewInfo(tableName, table, model)
if err != nil {
return nil, err
}
err = t.dbModel.Mapper().GetRowData(row, info)
if err != nil {
return nil, err
}

if uuid != "" {
mapperInfo, err := mapper.NewInfo(table, model)
if err != nil {
return nil, err
}
if err := mapperInfo.SetField("_uuid", uuid); err != nil {
if err := info.SetField("_uuid", uuid); err != nil {
return nil, err
}
}
Expand All @@ -791,7 +790,7 @@ func (t *TableCache) ApplyModifications(tableName string, base model.Model, upda
if schema == nil {
return fmt.Errorf("no schema for table %s", tableName)
}
info, err := mapper.NewInfo(schema, base)
info, err := mapper.NewInfo(tableName, schema, base)
if err != nil {
return err
}
Expand Down
12 changes: 6 additions & 6 deletions cache/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func TestRowCacheCreateMultiIndex(t *testing.T) {
}
} else {
assert.Nil(t, err)
mapperInfo, err := mapper.NewInfo(tSchema, tt.model)
mapperInfo, err := mapper.NewInfo("Open_vSwitch", tSchema, tt.model)
require.Nil(t, err)
h, err := valueFromIndex(mapperInfo, newIndex("foo", "bar"))
require.Nil(t, err)
Expand Down Expand Up @@ -419,7 +419,7 @@ func TestRowCacheUpdateMultiIndex(t *testing.T) {
assert.Error(t, err)
} else {
assert.Nil(t, err)
mapperInfo, err := mapper.NewInfo(tSchema, tt.model)
mapperInfo, err := mapper.NewInfo("Open_vSwitch", tSchema, tt.model)
require.Nil(t, err)
h, err := valueFromIndex(mapperInfo, newIndex("foo", "bar"))
require.Nil(t, err)
Expand Down Expand Up @@ -982,7 +982,7 @@ func TestIndex(t *testing.T) {
t.Run("Index by single column", func(t *testing.T) {
idx, err := table.Index("foo")
assert.Nil(t, err)
info, err := mapper.NewInfo(schema.Table("Open_vSwitch"), obj)
info, err := mapper.NewInfo("Open_vSwitch", schema.Table("Open_vSwitch"), obj)
assert.Nil(t, err)
v, err := valueFromIndex(info, newIndex("foo"))
assert.Nil(t, err)
Expand All @@ -994,7 +994,7 @@ func TestIndex(t *testing.T) {
obj2 := obj
obj2.Foo = "wrong"
assert.Nil(t, err)
info, err := mapper.NewInfo(schema.Table("Open_vSwitch"), obj2)
info, err := mapper.NewInfo("Open_vSwitch", schema.Table("Open_vSwitch"), obj2)
assert.Nil(t, err)
v, err := valueFromIndex(info, newIndex("foo"))
assert.Nil(t, err)
Expand All @@ -1012,7 +1012,7 @@ func TestIndex(t *testing.T) {
t.Run("Index by multi-column", func(t *testing.T) {
idx, err := table.Index("bar", "baz")
assert.Nil(t, err)
info, err := mapper.NewInfo(schema.Table("Open_vSwitch"), obj)
info, err := mapper.NewInfo("Open_vSwitch", schema.Table("Open_vSwitch"), obj)
assert.Nil(t, err)
v, err := valueFromIndex(info, newIndex("bar", "baz"))
assert.Nil(t, err)
Expand All @@ -1023,7 +1023,7 @@ func TestIndex(t *testing.T) {
assert.Nil(t, err)
obj2 := obj
obj2.Baz++
info, err := mapper.NewInfo(schema.Table("Open_vSwitch"), obj)
info, err := mapper.NewInfo("Open_vSwitch", schema.Table("Open_vSwitch"), obj)
assert.Nil(t, err)
v, err := valueFromIndex(info, newIndex("bar", "baz"))
assert.Nil(t, err)
Expand Down
18 changes: 9 additions & 9 deletions client/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ func (a api) Create(models ...model.Model) ([]ovsdb.Operation, error) {
table := a.cache.Mapper().Schema.Table(tableName)

// Read _uuid field, and use it as named-uuid
info, err := mapper.NewInfo(table, model)
info, err := mapper.NewInfo(tableName, table, model)
if err != nil {
return nil, err
}
Expand All @@ -256,7 +256,7 @@ func (a api) Create(models ...model.Model) ([]ovsdb.Operation, error) {
return nil, err
}

row, err := a.cache.Mapper().NewRow(tableName, model)
row, err := a.cache.Mapper().NewRow(info)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -294,7 +294,7 @@ func (a api) Mutate(model model.Model, mutationObjs ...model.Mutation) ([]ovsdb.
return nil, err
}

info, err := mapper.NewInfo(table, model)
info, err := mapper.NewInfo(tableName, table, model)
if err != nil {
return nil, err
}
Expand All @@ -305,7 +305,7 @@ func (a api) Mutate(model model.Model, mutationObjs ...model.Mutation) ([]ovsdb.
return nil, err
}

mutation, err := a.cache.Mapper().NewMutation(tableName, model, col, mobj.Mutator, mobj.Value)
mutation, err := a.cache.Mapper().NewMutation(info, col, mobj.Mutator, mobj.Value)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -335,12 +335,12 @@ func (a api) Update(model model.Model, fields ...interface{}) ([]ovsdb.Operation
return nil, err
}
tableSchema := a.cache.Mapper().Schema.Table(table)
info, err := mapper.NewInfo(table, tableSchema, model)
if err != nil {
return nil, err
}

if len(fields) > 0 {
info, err := mapper.NewInfo(tableSchema, model)
if err != nil {
return nil, err
}
for _, f := range fields {
colName, err := info.ColumnByPtr(f)
if err != nil {
Expand All @@ -357,7 +357,7 @@ func (a api) Update(model model.Model, fields ...interface{}) ([]ovsdb.Operation
return nil, err
}

row, err := a.cache.Mapper().NewRow(table, model, fields...)
row, err := a.cache.Mapper().NewRow(info, fields...)
if err != nil {
return nil, err
}
Expand Down
14 changes: 10 additions & 4 deletions client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/cenkalti/rpc2"
"github.com/cenkalti/rpc2/jsonrpc"
"github.com/ovn-org/libovsdb/cache"
"github.com/ovn-org/libovsdb/mapper"
"github.com/ovn-org/libovsdb/model"
"github.com/ovn-org/libovsdb/ovsdb"
"github.com/ovn-org/libovsdb/ovsdb/serverdb"
Expand Down Expand Up @@ -134,7 +135,7 @@ func newOVSDBClient(databaseModelRequest *model.DatabaseModelRequest, opts ...Op
monitors: make(map[string]*Monitor),
}
}
ovs.metrics.init(databaseModel.Name())
ovs.metrics.init(databaseModelRequest.Name())

return ovs, nil
}
Expand All @@ -146,7 +147,7 @@ func newOVSDBClient(databaseModelRequest *model.DatabaseModelRequest, opts ...Op
func (o *ovsdbClient) Connect(ctx context.Context) error {
// add the "model" value to the structured logger
// to make it easier to tell between different DBs (e.g. ovn nbdb vs. sbdb)
l := o.options.logger.WithValues("model", o.primaryDB().model.Name())
l := o.options.logger.WithValues("model", o.primaryDB().model.Request().Name())
o.options.logger = &l
o.registerMetrics()

Expand Down Expand Up @@ -701,7 +702,8 @@ func (o *ovsdbClient) monitor(ctx context.Context, cookie MonitorCookie, reconne
dbName := cookie.DatabaseName
db := o.databases[dbName]
db.schemaMutex.RLock()
mapper := db.model.Mapper()
mmapper := db.model.Mapper()
schema := db.model.Schema()
db.schemaMutex.RUnlock()
typeMap := o.databases[dbName].model.Types()
requests := make(map[string]ovsdb.MonitorRequest)
Expand All @@ -710,7 +712,11 @@ func (o *ovsdbClient) monitor(ctx context.Context, cookie MonitorCookie, reconne
if !ok {
return fmt.Errorf("type for table %s does not exist in model", o.Table)
}
request, err := mapper.NewMonitorRequest(o.Table, m, o.Fields)
info, err := mapper.NewInfo(o.Table, schema.Table(o.Table), m)
if err != nil {
return err
}
request, err := mmapper.NewMonitorRequest(info, o.Fields)
if err != nil {
return err
}
Expand Down
40 changes: 28 additions & 12 deletions client/condition.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,16 @@ type Conditional interface {
type equalityConditional struct {
mapper *mapper.Mapper
tableName string
model model.Model
info *mapper.Info
singleOp bool
}

func (c *equalityConditional) Matches(m model.Model) (bool, error) {
return c.mapper.EqualFields(c.tableName, c.model, m)
info, err := mapper.NewInfo(c.tableName, c.mapper.Schema.Table(c.tableName), m)
if err != nil {
return false, err
}
return c.mapper.EqualFields(c.info, info)
}

func (c *equalityConditional) Table() string {
Expand All @@ -44,7 +48,7 @@ func (c *equalityConditional) Table() string {
func (c *equalityConditional) Generate() ([][]ovsdb.Condition, error) {
var result [][]ovsdb.Condition

conds, err := c.mapper.NewEqualityCondition(c.tableName, c.model)
conds, err := c.mapper.NewEqualityCondition(c.info)
if err != nil {
return nil, err
}
Expand All @@ -59,11 +63,15 @@ func (c *equalityConditional) Generate() ([][]ovsdb.Condition, error) {
}

// NewEqualityCondition creates a new equalityConditional
func newEqualityConditional(mapper *mapper.Mapper, table string, all bool, model model.Model, fields ...interface{}) (Conditional, error) {
func newEqualityConditional(m *mapper.Mapper, table string, all bool, model model.Model, fields ...interface{}) (Conditional, error) {
info, err := mapper.NewInfo(table, m.Schema.Table(table), model)
if err != nil {
return nil, err
}
return &equalityConditional{
mapper: mapper,
mapper: m,
tableName: table,
model: model,
info: info,
singleOp: all,
}, nil
}
Expand All @@ -72,7 +80,7 @@ func newEqualityConditional(mapper *mapper.Mapper, table string, all bool, model
type explicitConditional struct {
mapper *mapper.Mapper
tableName string
model model.Model
info *mapper.Info
conditions []model.Condition
singleOp bool
}
Expand All @@ -91,7 +99,7 @@ func (c *explicitConditional) Generate() ([][]ovsdb.Condition, error) {
var conds []ovsdb.Condition

for _, cond := range c.conditions {
ovsdbCond, err := c.mapper.NewCondition(c.tableName, c.model, cond.Field, cond.Function, cond.Value)
ovsdbCond, err := c.mapper.NewCondition(c.info, cond.Field, cond.Function, cond.Value)
if err != nil {
return nil, err
}
Expand All @@ -109,11 +117,15 @@ func (c *explicitConditional) Generate() ([][]ovsdb.Condition, error) {
}

// newIndexCondition creates a new equalityConditional
func newExplicitConditional(mapper *mapper.Mapper, table string, all bool, model model.Model, cond ...model.Condition) (Conditional, error) {
func newExplicitConditional(m *mapper.Mapper, table string, all bool, model model.Model, cond ...model.Condition) (Conditional, error) {
info, err := mapper.NewInfo(table, m.Schema.Table(table), model)
if err != nil {
return nil, err
}
return &explicitConditional{
mapper: mapper,
mapper: m,
tableName: table,
model: model,
info: info,
conditions: cond,
singleOp: all,
}, nil
Expand Down Expand Up @@ -153,7 +165,11 @@ func (c *predicateConditional) Generate() ([][]ovsdb.Condition, error) {
return nil, err
}
if match {
elemCond, err := c.cache.Mapper().NewEqualityCondition(c.tableName, elem)
info, err := mapper.NewInfo(c.tableName, c.cache.Mapper().Schema.Table(c.tableName), elem)
if err != nil {
return nil, err
}
elemCond, err := c.cache.Mapper().NewEqualityCondition(info)
if err != nil {
return nil, err
}
Expand Down
Loading

0 comments on commit 3ff43c5

Please sign in to comment.