revise mycelium wiring #22
Conversation
Omarabdul3ziz
requested review from
Eslam-Nawara,
ashraffouda,
muhamadazmy,
rawdaGastan and
xmonader
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR revises the mycelium wiring to set mycelium public addresses as peers for private instances, export NDZM constant values for external use, and initialize a common utility package between net and netlight.
- Replaces custom mycelium peer list logic with dynamic retrieval using public interface IPs.
- Refactors NDZM namespace and interface constants to improve naming consistency.
- Introduces a new utility package (netbase/ifaceutil) to centralize common network interface functions.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/network/nr/net_resource.go | Updated mycelium peer setup to use dynamically retrieved IPs. |
| pkg/network/ndmz/dualstack.go | Renamed NDZM namespace and public interface constants for uniformity. |
| pkg/netlight/resource/mycelium.go | Modified mycelium service setup to use dynamic HostPeer URLs. |
| pkg/netbase/ifaceutil/ifaceutil.go | Added new utility functions for retrieving IPs and building peer URLs. |
Comments suppressed due to low confidence (1)
pkg/network/ndmz/dualstack.go:46
- For consistency, consider renaming the IPv6 public interface constant (currently 'dmzPub6') to 'DmzPub6', matching the 'DmzPub4' naming convention.
// DmzPub4 ipv4 public interface
|
|
||
| // global peers list | ||
| args = append(args, peers.Mycelium.Peers...) | ||
| // set mycelium public addresses are the private peers |
There was a problem hiding this comment.
[nitpick] Consider rephrasing the comment for clarity, for example: 'Set mycelium public addresses as private peers'.
| // set mycelium public addresses are the private peers | |
| // Set mycelium public addresses as private peers |
pkg/netbase/ifaceutil/ifaceutil.go
Outdated
| // GetIPsForIFace retrieves the IP addresses for a given interface name in a specified network namespace. | ||
| // If the namespace name is empty, it retrieves the IP addresses from the host. | ||
| func GetIPsForIFace(iface, nsName string) ([]net.IPNet, error) { | ||
| getIPs := func(_ ns.NetNS) ([]net.IPNet, error) { |
There was a problem hiding this comment.
why passing netns as a params it is not used inside this func
There was a problem hiding this comment.
it is the definition of the NetNs.Do method
type NetNS interface {
Do(toRun func(NetNS) error) error
i don't need access to the namespace inside. so i just skip and pass nil
There was a problem hiding this comment.
so no need to make the function take this param
pkg/netbase/ifaceutil/ifaceutil.go
Outdated
| var results []net.IPNet | ||
| err = netns.Do(func(_ ns.NetNS) error { | ||
| var getErr error | ||
| results, getErr = getIPs(nil) |
There was a problem hiding this comment.
u always pass nil to get ips in both cases if feel here u want to pass netns to get ips and remove netns.Do?
pkg/netbase/ifaceutil/ifaceutil.go
Outdated
| // GetIPsForIFace retrieves the IP addresses for a given interface name in a specified network namespace. | ||
| // If the namespace name is empty, it retrieves the IP addresses from the host. | ||
| func GetIPsForIFace(iface, nsName string) ([]net.IPNet, error) { | ||
| getIPs := func(_ ns.NetNS) ([]net.IPNet, error) { |
There was a problem hiding this comment.
so no need to make the function take this param
- set mycelium public addresses as peers for private instance, to reduce unnecessary traffic for public nodes in internal communications - export ndmz constant values for external use - init netbase/ifaceutil pkg for common utils between net/netlight
Omarabdul3ziz
force-pushed
the
main_PrivateMyceliumPeers
branch
from
faa6fb0 to
634dd95
Compare
description
verification
listing peers in the private network namespace
in zos
in zoslight
i couldn't found any differences in the output of
traceroutebefore/after this change as mentioned here threefoldtech/zos#2443 (comment) but maybe due to traceroute itself is not sufficient enough