-
Notifications
You must be signed in to change notification settings - Fork 54
ASMap v2 #217
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
ASMap v2 #217
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
m3dwards
left a comment
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.
Applying this to k8s will be slightly trickier as we are not assigning IP addresses up front like we are with compose.
Might be nice to include a bit of type hinting, especially on the tank.autonomous_system and warnet.a_systems which I think are just ints?
I would like to see k8s support before merging to prevent us from having a design that only works on compose unless there is a pressing need to get this into compose quickly.
| @property | ||
| def autonomous_system(self): | ||
| if self._a_system is None: | ||
| self._a_system = generate_as(self.warnet) | ||
| return self._a_system | ||
|
|
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.
Personally, I'm not a huge fan of passing the warnet object in to tanks, especially in this case as it's having it's a_systems property updated by the tank.
IMHO if the warnet object needs to keep track of what's been assigned then the logic belongs with the warnet object to assign the tanks a autonomous_system number.
|
Thanks, I'll try address this this evening. Agree it's not ready yet. It's very annoying (with asmap) the workflow requires us to start the pods, wait for ip addresses, and then pass those ip addresses back to bitcoind before it starts :S |
Lifted from Sipa's ASMap builder: https://github.com/sipa/asmap/tree/nextgen Don't ruff format to pull upstream changes more easily.
|
OK I updated those nits, but it's still not going to be any good to merge... Need to brainstorm how it would be possible to build an ASMap on k8s. The only "sane" way I can think of doing it, which might work on both systems, is to override the default command ( This might have it's own problems though, for example if bitcoind crashes the container won't notify as the "main" process is now Any other less insane ideas? |
|
In k8s there is a concept of init containers which I think might be worth considering for this. Init containers are part of the same pod, they run to completion before the main workload container starts. How about:
The reason I think we will need a single volume that all pods mount is that I don't think it's possible to mount the volume after pod creation. As the RPC pod is long running it would have started before any tanks and their respective volumes were created. |
josibake
left a comment
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.
Did a quick glance, mostly to familiarize myself, but agree we should focus on getting k8s in before merging / more serious review. Happy to start reviewing/testing if you have a WIP k8s support branch?
|
|
||
| def generate_as(): | ||
| while True: | ||
| as_number = random.randint(1, 64496) # I think these are not "reserved" |
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.
magic number alert! do we use this value anywhere else? even if not, might be good to set it as a global const with a nice comment explaining what the number means
| with open(self.as_map_txt_path, "w") as f: | ||
| for tank in self.tanks: | ||
| f.write(f"{tank.ipv4}/32 AS{tank.autonomous_system}\n") | ||
|
|
||
| # Yes, read back into a string... | ||
| with open(self.as_map_txt_path, "r") as f: | ||
| file_content = f.read() | ||
|
|
||
| buffer = io.StringIO(file_content) |
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.
why the write / read? are we using the as_map_txt file anywhere else?
|
Going to close this for now. As we are looking like we are going to fallback to orchestrator-assigned ip addresses, either with @pinheadmz nftables approach, or patched images a-la #261, ASMap will not work easily in either case. We can revisit if needed. |
Another go at using ASMap.
This time do not rely on copying in asmap.dat when we build the container, instead (in Dockerland) mount it as a volume into each container.
Missing: