Add public IPs validation task #2070

AbdelrahmanElawady · 2023-10-03T15:23:41Z

Description

Add public IPs validation task while ensuring only runs on one node from the farm.

Steps

On node startup create a test namespace and macvlan once for the task to use. Do not create them if they already exist.
Decide if the node should run the task or another one in the farm based on the node ID. The node with the least ID and with power target as up should run it. The other will log why they shouldn't run the task and exit with no errors.
Get public IPs set on the farm.
Inside the test namespace and using the macvlan link, assign each IP and its routes to the link and check reliable source for the real public IP. If they match then the public IP is valid.
Delete all IPs and routes between each IP validation and before running the task to ensure they are available to be used and no problems happen between task runs.

Issues

#2069

…e running

xmonader · 2023-10-17T10:54:58Z

Blocked on docs

muhamadazmy · 2023-10-19T08:27:51Z

pkg/network/public/public.go

+	err = ensureTestNamespace(br)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create test namespace: %w", err)
+	}
+


Suggested change

err = ensureTestNamespace(br)

if err != nil {

return nil, fmt.Errorf("failed to create test namespace: %w", err)

}

if err := ensureTestNamespace(br); err != nil {

return nil, errors.Wrap(err, "failed to create test namespace")

}

pkg/network/public/public.go

muhamadazmy · 2023-10-19T08:33:35Z

pkg/perf/pubip_task.go

+				log.Err(err).Send()
+				continue
+			}
+			err = macvlan.Install(mv, nil, ipNet, routes, netNS)


I don't believe you need to pass netNS because u are already inside the namespace. it's then better to pass nil to avoid unpredeceted errors

muhamadazmy · 2023-10-19T08:34:34Z

pkg/perf/pubip_task.go

+	}
+	unusedIPs := make(map[string]bool)
+	err = netNS.Do(func(nn ns.NetNS) error {
+		mv, err := macvlan.GetByName(testMacvlan)


This is too much code in the Do closure, i prefer u create another function that has the logic then do netNS.Do(p.logic) this way the indentation of the code won't also be too much

muhamadazmy · 2023-10-19T08:38:18Z

pkg/perf/pubip_task.go

+	for {
+		nodeID, err = registrar.NodeID(ctx)
+		if err == nil {
+			break
+		}
+		log.Err(err).Msg("failed to get node id")
+		time.Sleep(10 * time.Second)
+	}


There is a package backoff that we use you should use here instead of this.

muhamadazmy · 2023-10-19T08:41:09Z

pkg/perf/pubip_task.go

+	return string(body), nil
+}
+
+func deleteIPAndRoutes(publicIP substrate.PublicIP, routes []*netlink.Route, macvlan netlink.Link) error {


I really think you should just delee all Ips and routes blindly, so u start by listing the ips on this link and ALL routes inside the namespace and delete them.

This function should also be called before the test to make sure there are no left over ips on the device because of a crash

AbdelrahmanElawady · 2023-10-22T09:41:09Z

Also, I think the directory of the tasks should not be called perf as it is concerned with running tasks and monitoring them. Maybe tasks is better? or moving tasks not related to "performance" to another directory?

muhamadazmy

Please also create a sub pkg under perf to include all your files related to this task for organization purposes

muhamadazmy · 2023-10-24T07:24:48Z

cmds/modules/noded/main.go

+	pubIPTask := perf.NewPublicIPValidationTask()
+	perfMon.AddTask(pubIPTask)


Why do u need the intermediate variable pubIPTask ?

Suggested change

pubIPTask := perf.NewPublicIPValidationTask()

perfMon.AddTask(pubIPTask)

perfMon.AddTask(perf.NewPublicIPValidationTask())

muhamadazmy · 2023-10-24T07:26:00Z

pkg/network/public/public.go

@@ -26,6 +26,8 @@ import (
 const (
 	toZosVeth                   = "tozos" // veth pair from br-pub to zos
 	publicNsMACDerivationSuffix = "-public"
+	testMacvlan                 = "pubtestmacvlan"


weird name, this can just be pub or something. it's inside a namespace anyway so no chances of name collision.

muhamadazmy · 2023-10-24T07:29:55Z

pkg/network/public/public.go

@@ -349,6 +355,24 @@ func EnsurePublicSetup(nodeID pkg.Identifier, inf *pkg.PublicConfig) (*netlink.B
 	return br, netlink.LinkSetUp(br)
 }

+func ensureTestNamespace(publicBrdige *netlink.Bridge) error {
+	netNS, err := namespace.GetByName(testNamespace)
+	if err != nil {


if you check GetByName implementation you will see that it returns os.ErrNotExist if the namespace does not exist. So that's the only valid case you should try to create the namespace, then any other error type should be returned

netNS, err := GetByName(name) if os.IsNotExist(err) { err = Create(name) } if err != nil { return err }

muhamadazmy · 2023-10-24T07:42:59Z

pkg/perf/pubip_task.go

+		return "", err
+	}
+	defer req.Body.Close()
+


You need to check the returned status from the req in case it's not 200

muhamadazmy · 2023-10-24T14:12:20Z

pkg/perf/publicip/publicip_task.go

+		realIP, err := getRealPublicIP()
+		if err != nil {
+			p.farmIPsReport[publicIP.IP] = IPReport{
+				State:  SkippedState,
+				Reason: FetchRealIPFailed,
+			}
+			log.Err(err).Msg("failed to get node real IP")
+			continue
+		}


This is not always correct, if you failed to get the real ip this can duo to different reasons. and they all need to be handled differently:

http.Get fail, this can be duo to name look failure, or failed to establish connection this is hugely mean that the IP is invalid (is not routable to the internet)

http.Get succeeded but the response is not with success status (!= 200) this means that u have internet but the check service is donw, in that case we are not sure about the IP, we can mark it as skipped, for now.

http.Get succeed but the the returned value does not match the actual IP we are testing means those IPs are natted, so they are also invalid.

the best way to do this is that getRealPublicIP need to return (wrapped) errors with more details on what happened exactly, and then based on that choose the proper state

muhamadazmy · 2023-10-24T14:13:49Z

pkg/network/public/public.go

+	}
+	if err != nil {


this is probably what u want. since u already check the error of the create

Suggested change

}

if err != nil {

} else if err != nil {

AbdelrahmanElawady added 2 commits October 17, 2023 11:24

Add public IPs validation task

b8d3598

Separate creating namespace form the task and check other nodes befor…

2502823

…e running

AbdelrahmanElawady force-pushed the pub-ip-task branch from f46491b to 2502823 Compare October 17, 2023 10:20

AbdelrahmanElawady marked this pull request as ready for review October 17, 2023 10:23

AbdelrahmanElawady requested review from muhamadazmy, delandtj, MarioBassem, ashraffouda, rawdaGastan and xmonader as code owners October 17, 2023 10:23

muhamadazmy requested changes Oct 19, 2023

View reviewed changes

muhamadazmy reviewed Oct 19, 2023

View reviewed changes

AbdelrahmanElawady added 3 commits October 22, 2023 12:15

Check the namespace and macvlan before creating them

d7ad1df

Delete all IPs and routes between every IP check and use Backoff

1325dd1

Remove redundant string

d069fd0

Add docs

7ee67fb

muhamadazmy requested changes Oct 24, 2023

View reviewed changes

Check request status code and skip already validated IPs

9fb69e0

AbdelrahmanElawady requested a review from muhamadazmy October 24, 2023 12:21

AbdelrahmanElawady and others added 2 commits October 24, 2023 15:25

Merge branch 'main' into pub-ip-task

9e1b7da

Remove public IPs from task struct

1aa23d7

muhamadazmy requested changes Oct 24, 2023

View reviewed changes

AbdelrahmanElawady added 3 commits October 24, 2023 17:52

Add separate check when the the request itself failed

29e27da

Wrap error for http.Get case

0bbe4ae

Remove continue to ensure cleanup

06c15d2

muhamadazmy approved these changes Oct 31, 2023

View reviewed changes

muhamadazmy merged commit 61cc487 into main Oct 31, 2023
2 checks passed

muhamadazmy deleted the pub-ip-task branch October 31, 2023 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add public IPs validation task #2070

Add public IPs validation task #2070

AbdelrahmanElawady commented Oct 3, 2023 •

edited

Loading

xmonader commented Oct 17, 2023

muhamadazmy Oct 19, 2023

muhamadazmy Oct 19, 2023

muhamadazmy Oct 19, 2023

muhamadazmy Oct 19, 2023

muhamadazmy Oct 19, 2023

AbdelrahmanElawady commented Oct 22, 2023

muhamadazmy left a comment

muhamadazmy Oct 24, 2023

muhamadazmy Oct 24, 2023

muhamadazmy Oct 24, 2023

muhamadazmy Oct 24, 2023

muhamadazmy Oct 24, 2023

muhamadazmy Oct 24, 2023

		pubIPTask := perf.NewPublicIPValidationTask()
		perfMon.AddTask(pubIPTask)

	pubIPTask := perf.NewPublicIPValidationTask()
	perfMon.AddTask(pubIPTask)
	perfMon.AddTask(perf.NewPublicIPValidationTask())

Add public IPs validation task #2070

Add public IPs validation task #2070

Conversation

AbdelrahmanElawady commented Oct 3, 2023 • edited Loading

Description

Steps

Issues

xmonader commented Oct 17, 2023

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

AbdelrahmanElawady commented Oct 22, 2023

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

AbdelrahmanElawady commented Oct 3, 2023 •

edited

Loading