Egress IP health monitoring over GRPC #3100

flavio-fernandes · 2022-07-28T18:20:58Z

This PR includes a new functionality to egress ip, so it can
use gRPC in order to determine the health of its egress nodes.

Info on the protobuf definition used for the probing is here:
https://github.com/grpc/grpc/blob/master/doc/health-checking.md

The requirement in order to use this method is that both
ovnkube-master and ovnkube-node pods are started with a
new flag: --egressip-node-healthcheck-port <TCP_PORT>.

The original behavior remains as a viable method, should
ovnkube-master pod get started without the new flag.
For sake of completeness, the original method is to
have the ovnkube-master dial to discard (TCP port 9) and
declare node up upon receiving a TCP reject answer.

Signed-off-by: Flavio Fernandes flaviof@redhat.com
Reported-at: https://issues.redhat.com/browse/SDN-3156

bpickard22 · 2022-07-28T18:43:13Z

/assign @bpickard22

flavio-fernandes · 2022-07-28T20:39:45Z

/retest

andreaskaris · 2022-07-29T13:33:35Z

/retest-failed

flavio-fernandes · 2022-07-29T14:10:15Z

go-controller/pkg/ovn/egressip.go

+	}
+}
+
+func checkEgressNodesReachabilityIterate(oc *Controller) {


Note: best to view these changes with ignoring white-spacing option

flavio-fernandes · 2022-07-29T19:44:14Z

/retest-failed

andreaskaris · 2022-08-01T10:33:04Z

/retest-failed

coveralls · 2022-08-01T11:55:23Z

Coverage increased (+0.1%) to 52.504% when pulling 77ceb00 on flavio-fernandes:eip_health into 9906372 on ovn-org:master.

andreaskaris

Run this against a linter and address all issues:

[root@ovnkubernetes healthcheck]# golint
egressip_healthcheck.go:15:2: const ServiceEgressIpNode should be ServiceEgressIPNode
egressip_healthcheck.go:15:2: exported const ServiceEgressIpNode should have comment (or a comment on this block) or be unexported
egressip_healthcheck.go:36:2: don't use underscores in Go names; struct field node_mgmt_ip should be nodeMgmtIP
egressip_healthcheck.go:39:2: don't use underscores in Go names; struct field health_check_port should be healthCheckPort
egressip_healthcheck.go:42:1: exported function NewEgressIPHealthServer should have comment or be unexported
egressip_healthcheck.go:42:30: don't use underscores in Go names; func parameter node_mgmt_ip should be nodeMgmtIP
egressip_healthcheck.go:42:51: don't use underscores in Go names; func parameter health_check_port should be healthCheckPort
egressip_healthcheck.go:42:75: exported func NewEgressIPHealthServer returns unexported type *healthcheck.egressIPHealthServer, which can be annoying to use
egressip_healthcheck.go:81:6: exported type EgressIPHealthClient should have comment or be unexported
egressip_healthcheck.go:83:10: don't use underscores in Go names; interface method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:83:54: don't use underscores in Go names; interface method parameter health_check_port should be healthCheckPort
egressip_healthcheck.go:85:8: don't use underscores in Go names; interface method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:95:2: don't use underscores in Go names; struct field probe_failed should be probeFailed
egressip_healthcheck.go:98:1: exported function NewEgressIPHealthClient should have comment or be unexported
egressip_healthcheck.go:106:42: don't use underscores in Go names; method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:106:86: don't use underscores in Go names; method parameter health_check_port should be healthCheckPort
egressip_healthcheck.go:109:6: don't use underscores in Go names; var node_addr should be nodeAddr
egressip_healthcheck.go:112:9: don't use underscores in Go names; range var node_mgmt_ip should be nodeMgmtIP
egressip_healthcheck.go:143:40: don't use underscores in Go names; method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:158:3: don't use underscores in Go names; var prev_probe_failed should be prevProbeFailed

contrib/kind.sh

go-controller/pkg/ovn/egressip.go

go-controller/pkg/config/config.go

go-controller/pkg/node/node.go

go-controller/pkg/ovn/healthcheck/egressip_healthcheck.go

flavio-fernandes · 2022-08-01T18:33:17Z

Run this against a linter and address all issues:

[root@ovnkubernetes healthcheck]# golint
egressip_healthcheck.go:15:2: const ServiceEgressIpNode should be ServiceEgressIPNode
egressip_healthcheck.go:15:2: exported const ServiceEgressIpNode should have comment (or a comment on this block) or be unexported
egressip_healthcheck.go:36:2: don't use underscores in Go names; struct field node_mgmt_ip should be nodeMgmtIP
egressip_healthcheck.go:39:2: don't use underscores in Go names; struct field health_check_port should be healthCheckPort
egressip_healthcheck.go:42:1: exported function NewEgressIPHealthServer should have comment or be unexported
egressip_healthcheck.go:42:30: don't use underscores in Go names; func parameter node_mgmt_ip should be nodeMgmtIP
egressip_healthcheck.go:42:51: don't use underscores in Go names; func parameter health_check_port should be healthCheckPort
egressip_healthcheck.go:42:75: exported func NewEgressIPHealthServer returns unexported type *healthcheck.egressIPHealthServer, which can be annoying to use
egressip_healthcheck.go:81:6: exported type EgressIPHealthClient should have comment or be unexported
egressip_healthcheck.go:83:10: don't use underscores in Go names; interface method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:83:54: don't use underscores in Go names; interface method parameter health_check_port should be healthCheckPort
egressip_healthcheck.go:85:8: don't use underscores in Go names; interface method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:95:2: don't use underscores in Go names; struct field probe_failed should be probeFailed
egressip_healthcheck.go:98:1: exported function NewEgressIPHealthClient should have comment or be unexported
egressip_healthcheck.go:106:42: don't use underscores in Go names; method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:106:86: don't use underscores in Go names; method parameter health_check_port should be healthCheckPort
egressip_healthcheck.go:109:6: don't use underscores in Go names; var node_addr should be nodeAddr
egressip_healthcheck.go:112:9: don't use underscores in Go names; range var node_mgmt_ip should be nodeMgmtIP
egressip_healthcheck.go:143:40: don't use underscores in Go names; method parameter dial_ctx should be dialCtx
egressip_healthcheck.go:158:3: don't use underscores in Go names; var prev_probe_failed should be prevProbeFailed

❯ make lint 2>&1 | tee ~/x
level=info msg="[config_reader] Config search paths: [./ /app / /root]"
level=info msg="[config_reader] Used config file .golangci.yml"
level=info msg="[lintersdb] Active 13 linters: [deadcode errcheck exportloopref gofmt gosimple govet ineffassign nosprintfhostport staticcheck structcheck typecheck unused varcheck]"
level=info msg="[loader] Go packages loading at mode 575 (types_sizes|deps|exports_file|files|imports|name|compiled_files) took 1m31.31143454s"
level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 237.327084ms"
level=info msg="[linters context/goanalysis] analyzers took 5m53.600139829s with top 10 stages: buildir: 3m53.207123522s, gofmt: 10.513114941s, nilness: 9.511269896s, S1038: 5.990555809s, inspect: 4.666516648s, printf: 3.177458915s, fact_deprecated: 3.01233256s, ctrlflow: 2.702656853s, SA4030: 2.596352305s, directives: 2.588695656s"
level=info msg="[runner] Issues before processing: 725, after processing: 0"
level=info msg="[runner] Processors filtering stat (out/in): filename_unadjuster: 725/725, identifier_marker: 21/21, exclude-rules: 3/21, skip_files: 21/725, exclude: 21/21, path_prettifier: 725/725, autogenerated_exclude: 21/21, nolint: 0/3, cgo: 725/725, skip_dirs: 21/21"
level=info msg="[runner] processing took 44.703253ms with stages: autogenerated_exclude: 17.36304ms, nolint: 12.297915ms, path_prettifier: 12.184903ms, skip_files: 1.160597ms, identifier_marker: 836.52µs, exclude-rules: 614.246µs, skip_dirs: 128.188µs, cgo: 66.865µs, filename_unadjuster: 45.66µs, max_same_issues: 1.736µs, uniq_by_line: 737ns, source_code: 574ns, exclude: 425ns, max_from_linter: 378ns, diff: 374ns, severity-rules: 235ns, sort_results: 233ns, max_per_file_from_linter: 224ns, path_shortener: 221ns, path_prefixer: 182ns"
level=info msg="[runner] linters took 1m4.886636201s with stages: goanalysis_metalinter: 1m4.841802035s"
level=info msg="File cache stats: 0 entries of total size 0B"
level=info msg="Memory: 1445 samples, avg is 391.2MB, max is 1731.6MB"
level=info msg="Execution took 2m36.447970116s"
lint OK!

Lint is playing tricks on me! :P

flavio-fernandes · 2022-08-01T19:55:13Z

@andreaskaris I made the changes you pointed out, including lint.
I may need you help in learning how to lint outside makefile, tho.
PTAL. Thanks!!

flavio-fernandes · 2022-08-01T20:17:27Z

/retest-failed

andreaskaris · 2022-08-02T12:11:05Z

/retest-failed

andreaskaris · 2022-08-03T11:20:25Z

@andreaskaris I made the changes you pointed out, including lint. I may need you help in learning how to lint outside makefile, tho. PTAL. Thanks!!

I have this here in my bashrc:

lint ()
{ 
    golint -min_confidence 0.8
}

flavio-fernandes · 2022-08-03T12:06:55Z

I have this here in my bashrc:

TY. I'm addressing the last issues I found soon

❯ ~/go/bin/golint -min_confidence 0.8
egressip_healthcheck.go:34:1: comment on exported type EgressIPHealthServer should be of the form "EgressIPHealthServer ..." (with optional leading article)
egressip_healthcheck.go:46:1: comment on exported function NewEgressIPHealthServer should be of the form "NewEgressIPHealthServer ..."
egressip_healthcheck.go:86:1: comment on exported type EgressIPHealthClient should be of the form "EgressIPHealthClient ..." (with optional leading article)
egressip_healthcheck.go:104:1: comment on exported function NewEgressIPHealthClient should be of the form "NewEgressIPHealthClient ..."

andreaskaris

Sorry, some more linter stuff .. feel free to correct what you see fit but at least fix the python underscore syntax to golang camelcase

[akaris@linux ovn ((afbe09999...))]$ cd healthcheck/
[akaris@linux healthcheck ((afbe09999...))]$ lint
egressip_healthcheck.go:37:1: comment on exported type EgressIPHealthServer should be of the form "EgressIPHealthServer ..." (with optional leading article)
egressip_healthcheck.go:49:1: comment on exported function NewEgressIPHealthServer should be of the form "NewEgressIPHealthServer ..."
egressip_healthcheck.go:93:1: comment on exported type EgressIPHealthClient should be of the form "EgressIPHealthClient ..." (with optional leading article)
egressip_healthcheck.go:111:1: comment on exported function NewEgressIPHealthClient should be of the form "NewEgressIPHealthClient ..."

[akaris@linux ovn ((afbe09999...))]$ lint | grep egressip.go
egressip.go:1393:6: exported type EgressIPPatchStatus should have comment or be unexported
egressip.go:1982:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
egressip.go:2241:60: don't use underscores in Go names; method parameter health_check_port should be healthCheckPort
egressip.go:2252:2: don't use underscores in Go names; var dial_ctx should be dialCtx
egressip.go:2252:12: don't use underscores in Go names; var dial_cancel should be dialCancel

[akaris@linux node ((afbe09999...))]$ lint | grep node.go
node.go:586:2: don't use underscores in Go names; var health_check_port should be healthCheckPort
node.go:589:7: don't use underscores in Go names; var node_mgmt_ip should be nodeMgmtIP
node.go:602:3: don't use underscores in Go names; var health_server should be healthServer
node.go:619:1: exported method OvnNode.WatchEndpoints should have comment or be unexported
node.go:742:1: exported method OvnNode.WatchNamespaces should have comment or be unexported

go-controller/pkg/node/node.go

go-controller/pkg/config/config_test.go

go-controller/pkg/ovn/egressip.go

go-controller/pkg/ovn/healthcheck/egressip_healthcheck.go

trozet

looks pretty good except for some test cases and swapping the default config behavior

go-controller/pkg/ovn/healthcheck/health.proto

go-controller/pkg/node/node.go

go-controller/pkg/ovn/egressip.go

go-controller/pkg/ovn/egressip_test.go

flavio-fernandes · 2022-08-31T19:52:44Z

@flavio-fernandes Do you have any good docs to introduce me to how you created the auto generated grpc code?

@martinkennelly below are the places I used to come up to speed on using gRPC. Please take a look and let me know if that is not good for you:

https://grpc.io/docs/languages/go/quickstart/
- probably the only link you need :)
https://tutorialedge.net/golang/go-grpc-beginners-tutorial/
- good hello world example
https://github.com/grpc/grpc-go/tree/master/examples/features/encryption
- good reference code for making tls work over gRPC
https://github.com/flavio-fernandes/ovn-kubernetes/tree/eip_health.wip.3/go-controller/junkyard
- https://github.com/flavio-fernandes/ovn-kubernetes/blob/eip_health.wip.3/go-controller/junkyard/notes.txt
- this is how I initially tested the gRPC code locally

contrib/kind.sh

Copied proto definition from https://github.com/grpc/grpc/blob/master/doc/health-checking.md and generated the associated code. Steps taken: $ export PATH="$PATH:$(go env GOPATH)/bin" $ go version go version go1.18.3 linux/amd64 $ protoc --version libprotoc 3.14.0 $ cd go-controller/pkg/ovn/healthcheck $ protoc --go_out=. --go_opt=paths=source_relative \ --go-grpc_out=. --go-grpc_opt=paths=source_relative health.proto Signed-off-by: Flavio Fernandes <flaviof@redhat.com>

Break checkEgressNodesReachability into checkEgressNodesReachabilityIterate calls so implementation can honor oc.stopChan events and know when egressip should stop probing its egress nodes. Signed-off-by: Flavio Fernandes <flaviof@redhat.com>

flavio-fernandes · 2022-09-01T00:47:30Z

/retest

trozet · 2022-09-01T13:30:51Z

@flavio-fernandes one of the jobs failed with ovnkbue node crashing 9x:

2022-08-31T22:41:24.067948114Z stderr F F0831 22:41:24.067877   12131 egressip_healthcheck.go:63] Health checking listen failed: listen tcp [fd00:10:244:1::2]:9107: bind: cannot assign requested address

https://github.com/ovn-org/ovn-kubernetes/runs/8123914416?check_suite_focus=true

flavio-fernandes · 2022-09-01T13:49:06Z

@flavio-fernandes one of the jobs failed with ovnkbue node crashing 9x:
2022-08-31T22:41:24.067948114Z stderr F F0831 22:41:24.067877   12131 egressip_healthcheck.go:63] Health checking listen failed: listen tcp [fd00:10:244:1::2]:9107: bind: cannot assign requested address
https://github.com/ovn-org/ovn-kubernetes/runs/8123914416?check_suite_focus=true

my changes broke ipv6. Fixing it now

When ovnkube container, in both master and node pods, is started with the newly introduced flag 'egressip-node-healthcheck-port', egressip implementation will now use gRPC with that parameter. Signed-off-by: Flavio Fernandes <flaviof@redhat.com>

Extend kind and dist to support a new flag added to ovn-kube: egress ip health check This flag is used by ovnkube-master and ovnkube-node templates, so eip probe uses gRPC to that port instead of dialing discard port (9). NOTE: With this change, kind will use port 9107 by default. Signed-off-by: Flavio Fernandes <flaviof@redhat.com>

flavio-fernandes · 2022-09-01T21:04:21Z

@flavio-fernandes one of the jobs failed with ovnkbue node crashing 9x:
2022-08-31T22:41:24.067948114Z stderr F F0831 22:41:24.067877   12131 egressip_healthcheck.go:63] Health checking listen failed: listen tcp [fd00:10:244:1::2]:9107: bind: cannot assign requested address
https://github.com/ovn-org/ovn-kubernetes/runs/8123914416?check_suite_focus=true

Added settling change to ensure it is safe to use ipv6. TY @jcaamano and @trozet !!!

diff --git a/go-controller/pkg/node/node.go b/go-controller/pkg/node/node.go
index 5fdd03eb2..fd5905123 100644
--- a/go-controller/pkg/node/node.go
+++ b/go-controller/pkg/node/node.go
@@ -20,6 +20,7 @@ import (
        "k8s.io/klog/v2"
        utilnet "k8s.io/utils/net"

+       "github.com/containernetworking/plugins/pkg/ip"
        honode "github.com/ovn-org/ovn-kubernetes/go-controller/hybrid-overlay/pkg/controller"
        "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/cni"
        "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
@@ -593,6 +594,11 @@ func (n *OvnNode) startEgressIPHealthCheckingServer(wg *sync.WaitGroup, mgmtPort
                nodeMgmtIP = mgmtPortConfig.ipv4.ifAddr.IP
        } else if mgmtPortConfig.ipv6 != nil {
                nodeMgmtIP = mgmtPortConfig.ipv6.ifAddr.IP
+               // Wait for address to become usable.
+               if err := ip.SettleAddresses(mgmtPortConfig.ifName, 10); err != nil {
+                       return fmt.Errorf("failed start health checking server due to unsettled IPv6: %w", err)
+               }
        } else {
                return fmt.Errorf("unable to start health checking server: no mgmt ip")
        }

go-controller/pkg/node/node.go

trozet

/lgtm

kyrtapz · 2022-09-02T15:25:44Z

/lgtm

martinkennelly · 2022-09-02T16:18:11Z

/lgtm

Thanks for the info flavio.

flavio-fernandes requested a review from trozet July 28, 2022 18:21

flavio-fernandes force-pushed the eip_health branch from 5ddd7a3 to 3d690f5 Compare July 28, 2022 18:41

flavio-fernandes force-pushed the eip_health branch from 3d690f5 to 2773b7f Compare July 28, 2022 19:26

andreaskaris self-assigned this Jul 29, 2022

flavio-fernandes commented Jul 29, 2022

View reviewed changes

flavio-fernandes changed the title ~~WIP: Egress IP health monitoring over GRPC~~ [WIP] Egress IP health monitoring over GRPC Jul 29, 2022

andreaskaris suggested changes Aug 1, 2022

View reviewed changes

flavio-fernandes requested a review from andreaskaris August 1, 2022 19:51

flavio-fernandes force-pushed the eip_health branch 2 times, most recently from 24fedd7 to afbe099 Compare August 1, 2022 19:59

andreaskaris reviewed Aug 3, 2022

View reviewed changes

flavio-fernandes force-pushed the eip_health branch from afbe099 to 0ed7237 Compare August 3, 2022 14:07

andreaskaris reviewed Aug 3, 2022

View reviewed changes

go-controller/pkg/ovn/healthcheck/egressip_healthcheck.go Show resolved Hide resolved

flavio-fernandes force-pushed the eip_health branch 4 times, most recently from a9f8ac0 to 48a4bff Compare August 4, 2022 20:02

trozet reviewed Aug 5, 2022

View reviewed changes

go-controller/pkg/ovn/healthcheck/health.proto Show resolved Hide resolved

go-controller/pkg/node/node.go Outdated Show resolved Hide resolved

go-controller/pkg/ovn/egressip.go Outdated Show resolved Hide resolved

go-controller/pkg/ovn/egressip.go Outdated Show resolved Hide resolved

trozet reviewed Aug 31, 2022

View reviewed changes

go-controller/pkg/ovn/egressip.go Show resolved Hide resolved

go-controller/pkg/ovn/egressip_test.go Show resolved Hide resolved

go-controller/pkg/ovn/egressip_test.go Show resolved Hide resolved

go-controller/pkg/ovn/egressip_test.go Show resolved Hide resolved

trozet reviewed Aug 31, 2022

View reviewed changes

contrib/kind.sh Outdated Show resolved Hide resolved

flavio-fernandes force-pushed the eip_health branch from d614401 to 602542f Compare August 31, 2022 21:10

flavio-fernandes added 2 commits August 31, 2022 21:13

flavio-fernandes force-pushed the eip_health branch from 602542f to e2c0b0d Compare August 31, 2022 21:51

flavio-fernandes mentioned this pull request Aug 31, 2022

Enable egress_ip health check port openshift/cluster-network-operator#1520

Closed

oribon mentioned this pull request Sep 1, 2022

Implement Egress Services #3135

Merged

flavio-fernandes force-pushed the eip_health branch from de57394 to d93e072 Compare September 1, 2022 19:24

flavio-fernandes added 2 commits September 1, 2022 21:00

flavio-fernandes force-pushed the eip_health branch from d93e072 to 77ceb00 Compare September 1, 2022 21:01

flavio-fernandes requested review from trozet, kyrtapz and martinkennelly and removed request for kyrtapz September 1, 2022 21:04

flavio-fernandes commented Sep 2, 2022

View reviewed changes

go-controller/pkg/node/node.go Show resolved Hide resolved

trozet approved these changes Sep 2, 2022

View reviewed changes

trozet merged commit 5c4213d into ovn-org:master Sep 2, 2022

flavio-fernandes deleted the eip_health branch September 2, 2022 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Egress IP health monitoring over GRPC #3100

Egress IP health monitoring over GRPC #3100

flavio-fernandes commented Jul 28, 2022 •

edited

bpickard22 commented Jul 28, 2022 •

edited

flavio-fernandes commented Jul 28, 2022

andreaskaris commented Jul 29, 2022

flavio-fernandes Jul 29, 2022

flavio-fernandes commented Jul 29, 2022

andreaskaris commented Aug 1, 2022

coveralls commented Aug 1, 2022 •

edited

andreaskaris left a comment

flavio-fernandes commented Aug 1, 2022

flavio-fernandes commented Aug 1, 2022

flavio-fernandes commented Aug 1, 2022

andreaskaris commented Aug 2, 2022

andreaskaris commented Aug 3, 2022

flavio-fernandes commented Aug 3, 2022

andreaskaris left a comment

trozet left a comment

flavio-fernandes commented Aug 31, 2022

flavio-fernandes commented Sep 1, 2022

trozet commented Sep 1, 2022

flavio-fernandes commented Sep 1, 2022

flavio-fernandes commented Sep 1, 2022

trozet left a comment

kyrtapz commented Sep 2, 2022

martinkennelly commented Sep 2, 2022

Egress IP health monitoring over GRPC #3100

Egress IP health monitoring over GRPC #3100

Conversation

flavio-fernandes commented Jul 28, 2022 • edited

bpickard22 commented Jul 28, 2022 • edited

flavio-fernandes commented Jul 28, 2022

andreaskaris commented Jul 29, 2022

flavio-fernandes Jul 29, 2022

Choose a reason for hiding this comment

flavio-fernandes commented Jul 29, 2022

andreaskaris commented Aug 1, 2022

coveralls commented Aug 1, 2022 • edited

andreaskaris left a comment

Choose a reason for hiding this comment

flavio-fernandes commented Aug 1, 2022

flavio-fernandes commented Aug 1, 2022

flavio-fernandes commented Aug 1, 2022

andreaskaris commented Aug 2, 2022

andreaskaris commented Aug 3, 2022

flavio-fernandes commented Aug 3, 2022

andreaskaris left a comment

Choose a reason for hiding this comment

trozet left a comment

Choose a reason for hiding this comment

flavio-fernandes commented Aug 31, 2022

flavio-fernandes commented Sep 1, 2022

trozet commented Sep 1, 2022

flavio-fernandes commented Sep 1, 2022

flavio-fernandes commented Sep 1, 2022

trozet left a comment

Choose a reason for hiding this comment

kyrtapz commented Sep 2, 2022

martinkennelly commented Sep 2, 2022

flavio-fernandes commented Jul 28, 2022 •

edited

bpickard22 commented Jul 28, 2022 •

edited

coveralls commented Aug 1, 2022 •

edited