-
Notifications
You must be signed in to change notification settings - Fork 1
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
Removing hard dependency on SCION lab packages, SCION build config #3
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, great work!
While testing this with the example S02-scion-bgp-mixed I encountered the following issue:
Can you replicate this? On my machine this leads to the containers not starting in this example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some documentation on what the new build options are and what they do, e.g., as a longer Doxygen docstring.
It would also be good to give examples of the possible configuration options in the SCION example. I'd add a paragraph to the README in examples/S01_scion.
I was not able to replicate the issue @martenwallewein was facing, but I only tried the default build configuration.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @divinepaul and @martenwallewein)
seedemu/layers/ScionRouting.py
line 108 at r1 (raw file):
def __install_scion(self, emulator: Emulator, node: Node): """Install SCION packages on the node."""
Please change the comment as well, as the method is no longer installing SCION from the ETH packages.
seedemu/layers/Scion.py
line 130 at r1 (raw file):
try: result = urlparse(url) return result.scheme in ("http", "https") and bool(result.netloc)
Casting to bool looks kind of ugly to me. Maybe do a is not None'
or != ""
check instead. Whichever is actually intended.
seedemu/layers/Scion.py
line 141 at r1 (raw file):
# Ensure the URL ends with .git for Git repositories if not url.endswith(".git"): raise ValueError("URL does not look like a Git repository (missing .git)")
Requiring the URL to end in .git seems overly restrictive. Git repos don't have to end in .git. I'd remove the check.
seedemu/layers/Scion.py
line 182 at r1 (raw file):
def generateBuild(self) -> str : """ method to build all scion binaries and ouput to .scion_build_output based on the configuration mode
ouput -> output
seedemu/layers/Scion.py
line 246 at r1 (raw file):
self.__links = {} self.__ix_links = {} self.__default_build_config = ScionBuildConfig()
I'd pass the build config in as keyword args for the Scion layer constructor instead of using a separate method.
seedemu/layers/Scion.py
line 252 at r1 (raw file):
return "Scion" def getBuildConfiguration(self) -> ScionBuildConfig:
There is an asymmetry between setBuildConfiguration and getBuildConfiguration. The getter is returning a different type the the setter is setting.
Should getBuildConfiguration even by a public method? Only the ScionRouting layer is calling it, and only to call generateBuild() on the build config. Maybe make generateBuild a method on the SCION layer?
seedemu/layers/Scion.py
line 255 at r1 (raw file):
return self.__default_build_config def setBuildConfiguration(self, buildConfig: Dict[str,str]):
Keyword arguments would be more Pythonic than passing a dict. Make the signature setBuildConfiguration(**buildConfig)
. Or move to constructor as noted above.
I just noticed that if you log in to one of the SCION containers, the SCION binaries
|
I can also stumbled over this. Node::addBuildCommand( export PATH=xyz) gets translated to a RUN directive in the dockerfile, which sets the variable only in the shell it is run and the changes do not persist to after the image build. But from a design perspective it would be best to set all variables in the docker-compose.yml file where they are clearly visible and in one and the same place with all other configurations i.e. IP addresses. This also has the benefit that variables can be tweaked without having to rebuild any images. We should open an issue for 'Node custom ENV variables' and defer this one until it is merged.. so it can resort to this functionality |
Ability to set a build config on the scion layer which can,
.tar.gz
file containing the binariesThings to note,
All configurations write the binaries to a directory called
.scion_build_output
in the current working directory.the directory will contain sudirectories with the binaries named with a suffix to either the version or the checkout.
the build is not repeated for the same checkout or version.
any scion output will now have shared directories with docker volumes, so deleting the binaries or the shared folder will break the produced docker compose configuration.
I did not make another file for the
ScionBuildConfig
class since I was unsure where it could go in the project.And I need help nailing down how changing the build config for individual nodes could look like.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)