Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

colibri: e2e admission #3863

Merged
merged 15 commits into from
Sep 28, 2020
Merged

colibri: e2e admission #3863

merged 15 commits into from
Sep 28, 2020

Conversation

juagargi
Copy link
Contributor

@juagargi juagargi commented Sep 2, 2020

Do the end to end admission.
This PR also introduces some small changes to the app messages and capnp for e2e setup messages.


This change is Reviewable

@juagargi juagargi marked this pull request as ready for review September 4, 2020 09:34
@oncilla oncilla self-requested a review September 16, 2020 07:00
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 23 of 24 files at r1.
Reviewable status: 23 of 24 files reviewed, 5 unresolved discussions (waiting on @juagargi and @oncilla)


go/cs/reservation/request.go, line 44 at r1 (raw file):

// IsHopByHop returns true. If a subtype needs not to be hop by hop, it can override the function.
func (m *RequestMetadata) IsHopByHop() bool {

What requests are planned to not be hop by hop?


go/cs/reservation/e2e/request.go, line 57 at r1 (raw file):

// SetupRequest represents all possible e2e setup requests.
type SetupRequest interface {
	IsThisASTheSrc() bool

The names are not that great.

Also, this could just be Location() PathLocation

with

type PathLocation int
const (
    UnknownPathLocation PathLocation = iota
    Source
    Transit
    Transfer
    Destination
)

(or similar)


go/cs/reservation/e2e/request.go, line 71 at r1 (raw file):

	RequestedBW              reservation.BWCls
	AllocationTrail          []reservation.BWCls
	totalAScount             int

totalASCount (could also be a function that operates on SegmentResvASCount)


go/cs/reservationstore/store.go, line 410 at r1 (raw file):

	// admitted so far
	// TODO(juagargi) update token here

Will you address that TODO as part of this PR?


go/cs/reservationstore/store.go, line 422 at r1 (raw file):

	var msg base.MessageWithPath
	if req.IsThisASTheDst() {
		asAResponse := failedResponse.(*e2e.ResponseSetupFailure)

Why is this done?

Copy link
Contributor Author

@juagargi juagargi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 23 of 24 files reviewed, 5 unresolved discussions (waiting on @oncilla)


go/cs/reservation/request.go, line 44 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

What requests are planned to not be hop by hop?

Function removed: if the new header is approved, all control messages will be hopping from COLIBRI service to the next.


go/cs/reservation/e2e/request.go, line 57 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

The names are not that great.

Also, this could just be Location() PathLocation

with

type PathLocation int
const (
    UnknownPathLocation PathLocation = iota
    Source
    Transit
    Transfer
    Destination
)

(or similar)

Done. The enum is slightly different since Transfer is not a location, and a transit node may be also a transfer one. The function for checking if it's a transfer node is now called Transfer()


go/cs/reservation/e2e/request.go, line 71 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

totalASCount (could also be a function that operates on SegmentResvASCount)

Done. The value is pre-computed on construction to allow cheap calls to get it.


go/cs/reservationstore/store.go, line 410 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

Will you address that TODO as part of this PR?

The token needs to be updated also in the segment admission. I want to soon open a PR with the changes to handle the MACs and token, and take care of both TODOs.


go/cs/reservationstore/store.go, line 422 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

Why is this done?

Done.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 24 files at r1, 4 of 5 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @juagargi)


go/cs/reservation/e2e/request.go, line 71 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done. The value is pre-computed on construction to allow cheap calls to get it.

right. Although there is much more optimization potential elsewhere :)


go/cs/reservation/e2e/request.go, line 122 at r2 (raw file):

}

type PathLocation int

consider adding a string method


go/cs/reservation/e2e/request.go, line 125 at r2 (raw file):

const (
	Source PathLocation = iota

It would be best to have the zero value be unknown, such that bugs are easier to detect.
Like this, when you forget to set the PathLocation somewhere, it looks valid.


go/cs/reservationstore/store.go, line 422 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done.

No, I don't understand the intuition behind it. Why do we use the response for failure here?

Copy link
Contributor Author

@juagargi juagargi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)


go/cs/reservation/e2e/request.go, line 125 at r2 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

It would be best to have the zero value be unknown, such that bugs are easier to detect.
Like this, when you forget to set the PathLocation somewhere, it looks valid.

I though about it, but the location is a derived value, it only depends on len(r.AllocationTrail) and it always has a value, i.e. it can never be unknown.


go/cs/reservationstore/store.go, line 422 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

No, I don't understand the intuition behind it. Why do we use the response for failure here?

You are right: the failed response has to be transformed to a success response.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@oncilla
Copy link
Contributor

oncilla commented Sep 23, 2020

/rebase

@oncilla
Copy link
Contributor

oncilla commented Sep 25, 2020

/rebase

@oncilla
Copy link
Contributor

oncilla commented Sep 28, 2020

/rebase

@oncilla oncilla changed the title COLIBRI e2e admission colibri: e2e admission Sep 28, 2020
@oncilla oncilla merged commit fb4a0f7 into scionproto:master Sep 28, 2020
@juagargi juagargi deleted the colibri.store3 branch September 30, 2020 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants