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

SIG: Avoid using path removed from path pool #1923

Merged
merged 2 commits into from
Oct 4, 2018

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Oct 4, 2018

Reset the path for the worker immediately if the current path is no longer part of the path set.


This change is Reviewable

Copy link
Contributor

@kormat kormat 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 r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)


go/sig/egress/session/sessmon.go, line 146 at r1 (raw file):

			currSessPath = sm.getNewPath(nil)
			sm.needUpdate = true
			// Traffic must no longer be sent on the old path.

I'd expand this comment a bit, and mention that this implies that encap traffic will be sent on an un-tested path, but that it will get handled later, etc.


go/sig/egress/worker/worker.go, line 216 at r1 (raw file):

			w.currPathEntry = remote.SessPath.PathEntry()
		} else {
			w.currPathEntry = nil

This could be simplified to:

w.currPathEntry = nil
if remote.SessPath != nil {
	w.currPathEntry = remote.SessPath.PathEntry()
}

Copy link
Contributor Author

@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.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @kormat)


go/sig/egress/session/sessmon.go, line 146 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'd expand this comment a bit, and mention that this implies that encap traffic will be sent on an un-tested path, but that it will get handled later, etc.

Done.


go/sig/egress/worker/worker.go, line 216 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This could be simplified to:

w.currPathEntry = nil
if remote.SessPath != nil {
	w.currPathEntry = remote.SessPath.PathEntry()
}

Done.

Set the path for the worker immediately if the current path
is no longer part of the path set.
Copy link
Contributor

@kormat kormat left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@oncilla oncilla merged commit 83a75ee into scionproto:master Oct 4, 2018
@oncilla oncilla deleted the sig-fix branch October 4, 2018 14:37
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