Colibri store backend #3778

juagargi · 2020-06-08T13:24:21Z

Introduces the storage interface.

Interface
Rebase after COLIBRI application level types #3776 . First real commit here is 8c91bd7 "DB interface for storage"

This change is

oncilla

Reviewed 10 of 12 files at r1.
Reviewable status: 10 of 12 files reviewed, 16 unresolved discussions (waiting on @juagargi)

go/cs/reservation/reservationdbsqlite/db.go, line 15 at r1 (raw file):

// limitations under the License.

package reservationdbsqlite

we try do drop this pattern. Just call the package sqliteif the caller has a conflict with another sqlite package, they should resolve it with a named import themself

go/cs/reservation/reservationdbsqlite/db.go, line 268 at r1 (raw file):

// newSuffix finds a segment reservation ID suffix not being used at the moment. Should be called
// inside a transaction so the suffix is not used in the meantime, or fail.
func newSuffix(ctx context.Context, x db.Sqler, ASID addr.AS) (uint32, error) {

I wonder if choosing a random number and attempting to insert would be easier?

go/cs/reservation/reservationdbsqlite/schema.go, line 29 at r1 (raw file):

		id_as	INTEGER NOT NULL,
		id_suffix	INTEGER NOT NULL,
		inout_ingress	INTEGER NOT NULL,

just ingress or ingress_id or ingress_interface?

go/cs/reservation/reservationdbsqlite/schema.go, line 30 at r1 (raw file):

		id_suffix	INTEGER NOT NULL,
		inout_ingress	INTEGER NOT NULL,
		inout_egress	INTEGER NOT NULL,

ditto

go/cs/reservation/reservationdbtest/reservationdbtest.go, line 35 at r1 (raw file):

func TestDB(t *testing.T, db TestableDB) {
	tests := map[string]func(*testing.T, backend.DB){

I like it 💯

go/cs/reservation/reservationdbtest/reservationdbtest.go, line 53 at r1 (raw file):

	r.Path = &p
	r.ID.ASID = xtest.MustParseAS("ff00:0:1")
	ctx := context.Background()

take a context in the arguments.

go/cs/reservation/segment/path.go, line 32 at r1 (raw file):

// NewPathFromRaw constructs a new Path from the byte representation.
func NewPathFromRaw(buff []byte) Path {

This feels unsafe.
I think it is safe against out-of-bound access, but you can still have malformed input.
e.g. non-multiple of PathStepWithIALen, and parse it successfully.

go/cs/reservation/segment/path.go, line 75 at r1 (raw file):

// source AS of the reservation and has no access to the Path of the reservation).
// If the Path is not nil, it assumes is valid, i.e. it has at least length 2.
func (p *Path) GetSrcIA() addr.IA {

This should not be defined on the pointer, as Path is a slice type:

see: https://play.golang.org/p/OwBkG_zdbZw

go/cs/reservation/segment/path.go, line 77 at r1 (raw file):

func (p *Path) GetSrcIA() addr.IA {
	if p == nil {
		return addr.IA{I: 0, A: 0}

addr.IA{}

go/cs/reservation/segment/path.go, line 85 at r1 (raw file):

// source AS of the reservation and has no access to the Path of the reservation).
// If the Path is not nil, it assumes is valid, i.e. it has at least length 2.
func (p *Path) GetDstIA() addr.IA {

should not be defined on the pointer

go/cs/reservation/segment/path.go, line 87 at r1 (raw file):

func (p *Path) GetDstIA() addr.IA {
	if p == nil {
		return addr.IA{I: 0, A: 0}

addr.IA{}

go/cs/reservation/segment/path.go, line 93 at r1 (raw file):

// Len returns the length of this Path in bytes, when serialized.
func (p *Path) Len() int {

ditto

go/cs/reservation/segment/path.go, line 100 at r1 (raw file):

}

func (p *Path) Read(buff []byte) (int, error) {

ditto

go/cs/reservation/segment/path.go, line 117 at r1 (raw file):

// ToRaw returns a buffer representing this Path.
func (p *Path) ToRaw() []byte {

When do we want to have the path in this format?
I don't really see the use case atm

go/cs/reservation/segment/path.go, line 154 at r1 (raw file):

		return true
	}
	return s.PathStep.Equal(&o.PathStep) && s.IA == o.IA

s.IA.Equal(o.IA)

go/cs/reservation/segment/reservation.go, line 31 at r1 (raw file):

	Indices      Indices                  // existing indices in this reservation
	activeIndex  int                      // -1 <= activeIndex < len(Indices)
	IngressIFID  common.IFIDType          // igress interface ID: reservation packets enter

Just Ingress?

go/cs/reservationstorage/backend/db.go, line 72 at r1 (raw file):

	DeleteSegmentIndex(ctx context.Context, rsv *segment.Reservation,
		idx reservation.IndexNumber) error
	// DeleteExpiredIndices removes the segment reservation. Used in teardown.

comment does not match method

go/cs/reservationstorage/backend/db.go, line 78 at r1 (raw file):

	// without any index after removing the expired ones, it will also be removed.
	// Used on schedule.
	DeleteExpiredIndices(ctx context.Context) (int, error)

For testing it is sometimes very useful to pass the "currentTime" as an argument.

(Non-blocking comment)

juagargi

Reviewable status: 10 of 12 files reviewed, 16 unresolved discussions (waiting on @juagargi and @oncilla)

go/cs/reservation/reservationdbsqlite/db.go, line 15 at r1 (raw file):

Previously, Oncilla wrote…

we try do drop this pattern. Just call the package sqliteif the caller has a conflict with another sqlite package, they should resolve it with a named import themself

Done.

go/cs/reservation/reservationdbsqlite/db.go, line 268 at r1 (raw file):

Previously, Oncilla wrote…

I wonder if choosing a random number and attempting to insert would be easier?

I have not measured it, although I think that as the number of already used suffixes increases, obtaining a free suffix will become increasingly difficult, e.g. the case for 1 free suffix needs 19779055340 iterations to have a probability of failure of 0.01 ( log 0.01 / log p, where p = 1-1/2^32 ) . Just opted for the "classical" solution, but we can always change it if we see it's not good enough.

go/cs/reservation/reservationdbsqlite/schema.go, line 29 at r1 (raw file):

Previously, Oncilla wrote…

just ingress or ingress_id or ingress_interface?

Done.

go/cs/reservation/reservationdbsqlite/schema.go, line 30 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.

go/cs/reservation/reservationdbtest/reservationdbtest.go, line 53 at r1 (raw file):

Previously, Oncilla wrote…

take a context in the arguments.

Done.

go/cs/reservation/segment/path.go, line 32 at r1 (raw file):

Previously, Oncilla wrote…

This feels unsafe.
I think it is safe against out-of-bound access, but you can still have malformed input.
e.g. non-multiple of PathStepWithIALen, and parse it successfully.

Done. Extended test for this.

go/cs/reservation/segment/path.go, line 75 at r1 (raw file):

Previously, Oncilla wrote…

This should not be defined on the pointer, as Path is a slice type:

see: https://play.golang.org/p/OwBkG_zdbZw

In segment.Reservation we have a *Path, that can be nil (if reservation is not in the originating AS), non-nil but empty (wrong path, fails validation) or with some bytes. I wanted to have the two first cases differentiated clearly, that's why the pointer-to-slice member and also the receiver.
In https://play.golang.org/p/GDukaW4za_i I show how I intended to use the pointer for e.g. Validate
Also, from the path_test this is assumed:

	p = nil
	require.Equal(t, xtest.MustParseIA("0-0"), p.GetSrcIA())

But if you think a pointer to a slice should not happen, tell me and I'll replace the field in segment.Reservation

go/cs/reservation/segment/path.go, line 77 at r1 (raw file):

Previously, Oncilla wrote…

addr.IA{}

Done.

go/cs/reservation/segment/path.go, line 87 at r1 (raw file):

Previously, Oncilla wrote…

addr.IA{}

Done.

go/cs/reservation/segment/path.go, line 117 at r1 (raw file):

Previously, Oncilla wrote…

When do we want to have the path in this format?
I don't really see the use case atm

ToRaw is used to serialize the path to the DB. There is GetSegmentRsvFromPath in the ReserverOnly interface that needs the path as a field.

go/cs/reservation/segment/path.go, line 154 at r1 (raw file):

Previously, Oncilla wrote…

s.IA.Equal(o.IA)

I am a bit confused about the existence of IA.Equal. I understand that structs in go are comparable without an Equal method, e.g. https://play.golang.org/p/cTtKQ4EQxw0
I found the specs in https://golang.org/ref/spec#Comparison_operators
This made me realize that the only Equal operator we'd need here would be for the Path, as it is a slice 🎉

go/cs/reservation/segment/reservation.go, line 31 at r1 (raw file):

Previously, Oncilla wrote…

Just Ingress?

Done. I've modified also segment.Request to be consistent.

go/cs/reservationstorage/backend/db.go, line 72 at r1 (raw file):

Previously, Oncilla wrote…

comment does not match method

Done.

oncilla

Reviewed 11 of 11 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @juagargi)

go/cs/reservation/reservationdbsqlite/db.go, line 15 at r1 (raw file):