search

nmstate / kubernetes-nmstate

Declarative node network configuration driven through Kubernetes API.
GNU General Public License v2.0
184 stars 90 forks source link

Probes: choose the default gw from the main routing table #1153

Closed fedepaol closed 1 year ago

fedepaol commented 1 year ago

Is this a BUG FIX or a FEATURE ?:

Uncomment only one, leave it on its own line:

/kind bug /kind enhancement

What this PR does / why we need it:

The current logic relies on the first matching route entry with destination == 0.0.0.0/0, in order to find the default gateway to ping. When multiple routing table exist, we may have multiple entries with that destination, some of them may not be usable and even if they were, pinging that particular gateway doesn't assure the health of the node.

To overcome this, we narrow the filter to pick the next hop of the default route for the "main" routing table.

Special notes for your reviewer:

Release note:

Fix the probe check when configuring multiple routing tables and they contain a default gateway that is not accessible.
kubevirt-bot commented 1 year ago

Hi @fedepaol. Thanks for your PR.

I'm waiting for a nmstate member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
fedepaol commented 1 year ago

My understanding is, gjson does not allow to have multiple queries. See https://github.com/tidwall/gjson/issues/64

qinqon commented 1 year ago

/ok-to-test

fedepaol commented 1 year ago

/retest

fedepaol commented 1 year ago

Added, and also I added an e2e as requested offline

kubevirt-bot commented 1 year ago

@fedepaol: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-nmstate-e2e-handler-k8s-future 469a435a63f0bc511264ccb6c72c5a69abf7c4eb link false /test pull-kubernetes-nmstate-e2e-handler-k8s-future
Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
andreaskaris commented 1 year ago

omg, I can't believe that we ran into the same issue with 2 weeks difference and that I didn't find your PR first -_- I did find the syntax for defaultGwGjsonPath , though, so perhaps you can simplify your commit if your fancy: https://github.com/nmstate/kubernetes-nmstate/pull/1159 https://github.com/nmstate/kubernetes-nmstate/pull/1159/commits/d29117392c0a4522bff21b413cadf9a1fafc32df#diff-0c0168aef7c305629b107f3f2b5512406c3baa13146a6cce9463b244012634f1R147

gjson syntax that you were looking for:

a) that matches the first default route from table 254, table 0 or no table-id set (I dunno if table is always set):

defaultGwGjsonPath := fmt.Sprintf("[routes.running.#(table-id==~false)#,routes.running.#(table-id==%d)#].@flatten"+
        "|#(destination==\"0.0.0.0/0\").next-hop-address", mainRoutingTableID)

b) To match the first default route from table 254:

defaultGwGjsonPath := fmt.Sprintf("routes.running.#(table-id==%d)#" +
        "|#(destination==\"0.0.0.0/0\").next-hop-address", mainRoutingTableID)
andreaskaris commented 1 year ago

/retest-require

andreaskaris commented 1 year ago

/retest-required

qinqon commented 1 year ago

@fedepaol is this good to go ?

qinqon commented 1 year ago

omg, I can't believe that we ran into the same issue with 2 weeks difference and that I didn't find your PR first -_- I did find the syntax for defaultGwGjsonPath , though, so perhaps you can simplify your commit if your fancy: #1159 d291173#diff-0c0168aef7c305629b107f3f2b5512406c3baa13146a6cce9463b244012634f1R147

gjson syntax that you were looking for:

a) that matches the first default route from table 254, table 0 or no table-id set (I dunno if table is always set):

defaultGwGjsonPath := fmt.Sprintf("[routes.running.#(table-id==~false)#,routes.running.#(table-id==%d)#].@flatten"+
      "|#(destination==\"0.0.0.0/0\").next-hop-address", mainRoutingTableID)

b) To match the first default route from table 254:

defaultGwGjsonPath := fmt.Sprintf("routes.running.#(table-id==%d)#" +
      "|#(destination==\"0.0.0.0/0\").next-hop-address", mainRoutingTableID)

Better keep the loops so we don't depend on gjson syntax (we may ditch it in the future)

qinqon commented 1 year ago

/lgtm /approve

kubevirt-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/nmstate/kubernetes-nmstate/blob/main/OWNERS)~~ [qinqon] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment