docs for network light #2397

ashraffouda · 2024-08-14T11:40:29Z

Signed-off-by: Ashraf Fouda ashraf.m.fouda@gmail.com### Description

Describe the changes introduced by this PR and what does it affect

Changes

List of changes this PR includes

Related Issues

List of related issues

Checklist

Tests included
Build pass
Documentation
Code format and docstring

Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

muhamadazmy

I didn't cover all the PR because of its size. But i think I have left enough comments for the first iteration.

I think using the Light prefix might not be the best approach. Maybe use a version instead? so it's future proof

muhamadazmy · 2024-08-15T12:58:46Z

bootstrap/bootstrap/src/config.rs

+            .arg(
+                Arg::with_name("light")
+                    .short("l")
+                    .takes_value(false)
+                    .help("run in light mode, will use the bootstrap:development.flist"),
+            )


The light flag same as the runmode should be parsed from the kernel cmdline arguments not from the bootstrap command arguments.

muhamadazmy · 2024-08-15T13:01:28Z

cmds/modules/netlightd/nft.go

@@ -16,6 +16,7 @@ func ensureHostFw(ctx context.Context) error {
 nft 'add table inet filter'
 nft 'add table arp filter'
 nft 'add table bridge filter'
+nft 'add table nat'


I really think this entire file can completely replaced now with a .nft file basically rewrite this as a ruleset (similar to what we have under nft/rules.nft)

It's way cleaner than executing individual commands

muhamadazmy · 2024-08-15T13:12:31Z

cmds/modules/provisiond/reporter.go

-
-// getVmMetrics will collect network consumption every 5 min and store
-// it in the rrd database.
-func (r *Reporter) getGwMetrics(ctx context.Context, slot rrd.Slot) error {


This will probably be re-added

we decided to postpone gw and qsfs for later versions of zos light

muhamadazmy · 2024-08-15T13:14:40Z

etc/zinit/networkd.yaml

-test: zbusdebug --module network
+exec: netlightd --broker unix:///var/run/redis.sock --root /var/cache/modules/networkd
+
+# test: zbusdebug --module network


We still have to have something here to let other modules know that this networking is ready. I am sure netlightd also has a "zbus" service which means we can use the same test command but with different module name (if the module name was changed even)

muhamadazmy · 2024-08-15T13:15:16Z

pkg/environment/environment.go

@@ -321,6 +321,10 @@ func getEnvironmentFromParams(params kernel.Params) (Environment, error) {
 	if e := os.Getenv("ZOS_BIN_REPO"); e != "" {
 		env.BinRepo = e
 	}
+	env.Light = false


This of course is not needed, since the zero value for bool in Go is false.

muhamadazmy · 2024-08-15T13:17:02Z

pkg/gridtypes/zos/types.go

+	// NetworkLightType type
+	NetworkLightType gridtypes.WorkloadType = "network-light"
+	// ZDBLightType type
+	ZDBLightType gridtypes.WorkloadType = "zdb-light"


Why do we need zdb-light? is it because it doesn't have ygg IP?

I think we should only use the "-light" suffix if the data type for input or output changes (like vm and network) but I don't believe this applies to zdb

muhamadazmy · 2024-08-15T13:22:01Z

pkg/netlight/resource/resource.go

+}
+
+// myceliumIpSeed is 6 bytes
+func (r *Resource) AttachPrivate(id string, vmIp net.IP) (device pkg.TapDevice, err error) {


I can't figure out if this an idempotent function or not. But it's usually important that such functions to be idempotent so recreation doesn't happen if the service was restarted for example

muhamadazmy · 2024-08-15T13:23:25Z

pkg/network_light.go

+	Name    string
+	Mac     net.HardwareAddr
+	IP      *net.IPNet
+	Gateway *net.IPNet


I think this should be Routes instead so we can also have custom routes not only default GW. It's needed with mycelium for example and in other cases where custom routing is required

rename zdblight to zdb and do some hardening Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

muhamadazmy

Thanks @ashraffouda for the great work. I still have few comments in case you are interested

muhamadazmy · 2024-08-19T18:25:48Z

bootstrap/bootstrap/src/bootstrap.rs

@@ -26,15 +26,19 @@ fn bootstrap_zos(cfg: &config::Config) -> Result<()> {
    let flist = match &cfg.runmode {
        RunMode::Prod => match &cfg.version {
            Version::V3 => "zos:production-3:latest.flist",
+            _ => bail!("unsupported version in old style"),


it's safe to completely remove the old style from code since ALL environments have been updated to new style. We needed to keep the old style for a while because bootstrap is shared between all environments (since it's part of base image) because some envs used new style and others didn't.

Anyway, you can assume that old style does no longer exist

muhamadazmy · 2024-08-19T18:28:18Z

cmds/modules/netlightd/main.go

+	if err := nft.Apply(rules, ""); err != nil {
+		return fmt.Errorf("failed to apply host nft rules: %w", err)
+	}
+


close the rules file here

muhamadazmy · 2024-08-19T18:31:44Z

cmds/modules/netlightd/nft/rules.nft

@@ -0,0 +1,29 @@
+add table inet filter;


What I meant with this is to make it similar to other rules.nft (in the same PR here style. It doesn't have commands but instead the complete structure of the rule set

It contains simply the output of running nft list ruleset

If you don't get what I mean let me explain over DM

muhamadazmy · 2024-08-19T18:34:54Z

pkg/netlight/resource/resource.go

+	if err := nft.Apply(rules, nsName); err != nil {
+		return nil, fmt.Errorf("failed to apply nft rules for namespace '%s': %w", name, err)
+	}
+


close rules file here

muhamadazmy · 2024-08-19T18:35:45Z

pkg/network_light.go

+	Name   string
+	Mac    net.HardwareAddr
+	IP     *net.IPNet
+	Routes []Route


Where is Route defined

muhamadazmy · 2024-08-19T18:40:57Z

pkg/upgrade/upgrade.go

+	envName := mode.String()
+	if u.boot.Version().Major == 4 {
+		envName = fmt.Sprintf("%s-light", envName)
+	}


Since this is a fork and the entire fork knows that this is version 4. Hence you can always assume the envName to be "-light" without even the need to check for the major version

muhamadazmy · 2024-08-19T18:43:19Z

pkg/vm/ch.go

@@ -24,43 +23,6 @@ const (
 	cloudConsoleBin = "cloud-console"
 )

-// startCloudConsole Starts the cloud console for the vm on it's private network ip
-func (m *Machine) startCloudConsole(ctx context.Context, namespace string, networkAddr net.IPNet, machineIP net.IPNet, ptyPath string, logs string) (string, error) {


I think the cloud console can stay since the user network will always have a mycelium IP. the console can still work over the mycelium IP in case the VM is not reachable for any reason.

So the ip of the vm is the VM private ip (private range)

The console listen port is calculated the same way

The console IP itself is the mycelium IP assigned to this user mycelium instance.

muhamadazmy · 2024-08-19T18:46:56Z

pkg/vm/ch.go

@@ -131,12 +93,16 @@ func (m *Machine) Run(ctx context.Context, socket, logs string) (pkg.MachineInfo

 		for _, nic := range m.Interfaces {
 			var typ InterfaceType
-			typ, _, err = nic.getType()
+			typ, idx, err := nic.getType()


When you do Attach on the resource, you should create a tap device (not macvtap) which makes it easier to attach it to CH command line. The macvtap case was only used for public IPs specifically.

muhamadazmy · 2024-08-19T18:50:50Z

pkg/netlight/resource/resource.go

+	}
+	_, getLinkErr := netlink.LinkByName(tapName)
+	if getLinkErr != nil {
+		mtap, err := macvtap.CreateMACvTap(tapName, privateNetBr, hw)


We create TAP devices for VMs not Macvtap. this makes it un0necessary complex to pass this device to the VM when it can just be a tap device

muhamadazmy · 2024-08-19T18:53:20Z

pkg/netlight/resource/resource.go

+	myBr := fmt.Sprintf("m%s", r.name)
+	hw := ifaceutil.HardwareAddrFromInputBytes([]byte(tapName))
+
+	_, err = macvtap.CreateMACvTap(tapName, myBr, hw)


Same for mycelium this can perfectly be a tap device (not macvtap) hence it becomes simpler to pass to the cloud-hypervisor command line

macvtaps or macvlans are a disaster if they're more than one with a bridge interface as parent.
Somewhere since that dummy necessity version, macvlans have problems ... what they specifically are, I'm not sure yet, but I suspect seen that macvlans are also bridges in a lighter version, they don't cooperate well with regular bridges

I felt like since the https://bugzilla.kernel.org/show_bug.cgi?id=214631 bug, macvlans went worse and worse over time.
probably because of their infrequent use.
let's see how much work it is, but I guess

iwanbk · 2024-08-26T09:35:13Z

pkg/primitives/vm-light/vm.go

@@ -199,42 +187,14 @@ func (p *Manager) virtualMachineProvisionImpl(ctx context.Context, wl *gridtypes
 		if err != nil {


according to https://github.com/threefoldtech/zos/pull/2397/files#diff-21cf55cfe1ba697c2e7be4ae50cbe646bca4331061bba94ebc35fb5f7d05ca98R162-R165

we only have/use one private network, why still looping all net interfaces instead of using the only used one?

iwanbk · 2024-08-26T09:37:36Z

pkg/primitives/vm-light/vm.go

 		networkInfo.Ifaces = append(networkInfo.Ifaces, inf)
-		result.PlanetaryIP = inf.IPs[0].IP.String()
 	}

 	if config.Network.Mycelium != nil {


is it allowed to not have mycelium?

it is not allowed according to the specs, CMIIW

iwanbk · 2024-08-26T10:29:41Z

pkg/netlight/network.go

+
+var _ pkg.NetworkerLight = (*networker)(nil)
+
+func NewNetworker() (pkg.NetworkerLight, error) {


is it common to add er to the struct in zos?
because er suffix usually used for interface, not struct

iwanbk · 2024-08-26T10:53:37Z

pkg/netlight/resource/resource.go

+	var addrs []netlink.Addr
+
+	if err = netNs.Do(func(_ ns.NetNS) error {
+		link, err := netlink.LinkByName(infPrivate)
+		if err != nil {
+			return err
+		}
+		addrs, err = netlink.AddrList(link, netlink.FAMILY_V4)
+		if err != nil {
+			return err
+		}
+		return nil
+	}); err != nil {
+		return
+	}
+	if len(addrs) != 1 {
+		return device, fmt.Errorf("expect addresses on private interface to be 1 got %d", len(addrs))
+	}
+	gw := addrs[0].IPNet


for readability, i think we can move this block into separate func, maybe something like:
getAndValidateAddr()

And maybe including the code to get the namespace

nsName := fmt.Sprintf("n%s", r.name) netNs, err := namespace.GetByName(nsName)

i think the code below it can also be included

if !gw.Contains(vmIp) { return device, fmt.Errorf("ip not in range") }

iwanbk · 2024-08-26T11:03:03Z

pkg/netlight/resource/resource.go

+		Mask: gw.Mask,
+	}
+	_, getLinkErr := netlink.LinkByName(tapName)
+	if getLinkErr != nil {


so, is it OK if the tap already exists?

iwanbk · 2024-08-26T11:07:56Z

pkg/netlight/resource/mycelium.go

+}
+
+func inspectMycelium(seed []byte) (inspection MyceliumInspection, err error) {
+	// we check if the file exists before we do inspect because mycelium will create a random seed


we check if the file exists

can't find the check

iwanbk · 2024-08-26T11:08:42Z

pkg/netlight/resource/mycelium.go

+		return inspection, fmt.Errorf("failed to write seed: %w", err)
+	}
+
+	tmp.Close()


move to the above of the if block?

muhamadazmy and others added 17 commits July 10, 2024 14:50

docs for network light

fecd21a

WIP networkd light

6e9e369

Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

Merge branch 'netlight' into zos-light

f50626b

wip: testing initial setup

b6bb416

wip: first working network resource

6844783

finalize Resource creation

ae9081f

improve nft nat rulese table

ef6e2d9

update nft rulese for resource

87fae24

scratch board

eddc126

Add basic netlight zbus interface

2658cf7

Add vm-light and network-light workloads

fa153d7

Add zdb-light

d657225

Remove some dependencies on old network

e3f7135

replaces networkd with networkdlight

88ae10c

Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

change bootstrap to include booting nodes in light mode

259f9a0

Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

adds updates to bootstrap and update worker

87d16d4

Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

removes unsed stuff

70c90c4

Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

muhamadazmy reviewed Aug 15, 2024

View reviewed changes

make passing version=v4 instead of light

b25edf4

rename zdblight to zdb and do some hardening Signed-off-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>

muhamadazmy requested changes Aug 19, 2024

View reviewed changes

iwanbk reviewed Aug 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs for network light #2397

docs for network light #2397

ashraffouda commented Aug 14, 2024

muhamadazmy left a comment

muhamadazmy Aug 15, 2024

muhamadazmy Aug 15, 2024

muhamadazmy Aug 15, 2024

ashraffouda Aug 18, 2024

muhamadazmy Aug 15, 2024

muhamadazmy Aug 15, 2024

muhamadazmy Aug 15, 2024

muhamadazmy Aug 15, 2024

muhamadazmy Aug 15, 2024

muhamadazmy left a comment

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

muhamadazmy Aug 19, 2024

delandtj Aug 21, 2024

delandtj Aug 21, 2024

iwanbk Aug 26, 2024

iwanbk Aug 26, 2024

iwanbk Aug 26, 2024

iwanbk Aug 26, 2024

iwanbk Aug 26, 2024

iwanbk Aug 26, 2024

iwanbk Aug 26, 2024

iwanbk Aug 26, 2024 •

edited

Loading

iwanbk Aug 26, 2024

		@@ -199,42 +187,14 @@ func (p Manager) virtualMachineProvisionImpl(ctx context.Context, wl gridtypes
		if err != nil {


		var _ pkg.NetworkerLight = (*networker)(nil)

		func NewNetworker() (pkg.NetworkerLight, error) {

docs for network light #2397

Are you sure you want to change the base?

docs for network light #2397

Conversation

ashraffouda commented Aug 14, 2024

Changes

Related Issues

Checklist

muhamadazmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwanbk Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwanbk Aug 26, 2024 •

edited

Loading