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

unify path types in combinator, segutil, sciond #3893

Merged
merged 4 commits into from
Oct 14, 2020

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Oct 8, 2020

  • Combine mostly equivalent implementations of snet.Path
    segutil.path and sciond.Path, in a shared lib in go/lib/snet/path.

  • Adapt combinator to return a Path that is already almost identical
    with this path -- the only missing bit is the UnderlayNextHop (which is
    outside of the scope of the combinator, not having access to the local
    topology).
    In particular, the combinator now directly builds a raw forwarding path
    instead of returning a high level description of the same -- using the
    structs of the slayers for this actually allows to get rid of most of the
    intermediate helper types.
    This makes the places that call the combinator fairly trivial.

  • Side quest: strip down the public interface of the combinator to the
    essentials (combinator.Combine and combinator.Path).

  • Drop sciond.FwdPathMeta etc, use snet.Path directly
    in the sciond grpc.Daemon server. This is nice because the "input" on the
    server side and the output on the client side are now the same type.

  • Drop various unused sciond helper types

  • Remove sciond staticinfo "condenser" and seperate types. Instead of
    this, the staticinfo metadata will be added to snet.PathMetadata.
    This data will be prepared and returned in the identical format directly
    by the path combinator -- this is the motivation for this refactoring!

  • Remove unused sciond/fetcher/filter (instead of fixing the tests)

Planned next steps (future PRs):

  • add the "staticinfo" information to the snet.PathMetadata, adapt the existing (currently unused) code in the combinator to fill this information, adapt the sciond grpc proto to carry this extended metadata.
  • change snet.PathMetadata from an interface to a struct. Similar to the PathInterface, there simply seems no reason for this to be an interface.
    Depending on how exactly the extended "staticinfo" data is represented, this might be useful to allow defining higher-level accessor functions to aggregate the information but still giving applications the full information (e.g. storing latency for each hop, but allowing a simple TotalLatency() getter) -- note that having to deal with missing information may require the applications to have more insight than only the pre-condensed data.
    Perhaps move the Interfaces list into this Metadata struct as well, as it seems to belong in here.
  • perhaps, attempt to turn snet.Path into a struct as well -- it seems that the behaviour of all the different implementations (empty path, partial path, ...) can easily be combined.
  • remove the now pointless segutil.Router

Closes #3872


This change is Reviewable

@lukedirtwalker lukedirtwalker self-requested a review October 14, 2020 10:35
@lukedirtwalker lukedirtwalker self-assigned this Oct 14, 2020
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 27 of 27 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matzf)

a discussion (no related file):
nice work 💯



go/cs/segutil/router.go, line 26 at r1 (raw file):

// Router returns paths backed by the local path database.
// XXX(matzf): this doesn't do meaningful work anymore, drop it.

💯


go/lib/snet/path/path.go, line 39 at r1 (raw file):

	SPath   *spath.Path
	NextHop *net.UDPAddr
	IFaces  []snet.PathInterface

not super happy with this naming, but once we move it to PathMetadata we might be able to have a clean name, no method needed.


go/lib/snet/path/path.go, line 81 at r1 (raw file):

		return p.Dst
	}
	return p.IFaces[len(p.IFaces)-1].IA

don't we always set Dst correctly?


go/pkg/sciond/fetcher/fetcher.go, line 114 at r1 (raw file):

Bad

I know this was as before, but it should be lowercase bad as error messages should start with lower case. (https://github.com/golang/go/wiki/CodeReviewComments#error-strings )

Copy link
Contributor Author

@matzf matzf 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, 3 unresolved discussions (waiting on @lukedirtwalker)


go/lib/snet/path/path.go, line 39 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

not super happy with this naming, but once we move it to PathMetadata we might be able to have a clean name, no method needed.

Ack.


go/lib/snet/path/path.go, line 81 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

don't we always set Dst correctly?

Indeed! Done.


go/pkg/sciond/fetcher/fetcher.go, line 114 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
Bad

I know this was as before, but it should be lowercase bad as error messages should start with lower case. (https://github.com/golang/go/wiki/CodeReviewComments#error-strings )

Done.

@matzf matzf force-pushed the path-unification branch 2 times, most recently from 3b5c10a to 6e294b7 Compare October 14, 2020 12:42
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

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

@lukedirtwalker
Copy link
Collaborator

/rebase

matzf added 4 commits October 14, 2020 13:08
* Combine mostly equivalent implementations of `snet.Path`
segutil.path and sciond.Path, in a shared lib in `go/lib/snet/path`.

* Adapt combinator to return a Path that is already almost identical
with this path -- the only missing bit is the UnderlayNextHop (which is
outside of the scope of the combinator, not having access to the local
topology).
In particular, the combinator now directly builds a raw forwarding path
instead of returning a high level description of the same -- using the
structs of the slayers for this actually allows to get rid of most of the
intermediate helper types.
This makes the places that call the combinator fairly trivial.

* Side quest: strip down the public interface of the combinator to the
essentials (combinator.Combine and combinator.Path).

* Drop sciond.FwdPathMeta etc, use snet.Path directly
in the sciond grpc.Daemon server. This is nice because the "input" on the
server side and the output on the client side are now the same type.

* Drop various unused sciond helper types

* Remove sciond staticinfo "condenser" and seperate types. Instead of
this, the staticinfo metadata will be added to snet.PathMetadata.
This data will be prepared and returned in the identical format directly
by the path combinator -- this is the motivation for this refactoring!

* Remove unused sciond/fetcher/filter (instead of fixing the tests)
@lukedirtwalker lukedirtwalker merged commit f063423 into scionproto:master Oct 14, 2020
@matzf matzf deleted the path-unification branch November 12, 2020 13:38
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.

Unify path types
2 participants