api: add ovn bridge mappings to node network state #1237

maiqueb · 2024-04-29T15:11:34Z

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:
This PR exposes the node's ovn bridge mappings (already provided on nmstatectl show) via the NNS object.

Special notes for your reviewer:

Release note:

Display OVN bridge mappings on the NodeNetworkState object

kubevirt-bot · 2024-04-29T15:11:45Z

Hi @maiqueb. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

maiqueb · 2024-04-29T15:11:47Z

/cc @qinqon

qinqon

Let's generalize this PR and add the missing top level field with a transparent data type json.RawMessage.

qinqon · 2024-04-30T06:30:18Z

pkg/state/type.go

+
+type bridgeMappings struct {
+	PhysicalNetworkMappings []PhysicalNetworks `json:"bridge-mappings,omitempty" yaml:"bridge-mappings,omitempty"`
+}
+
+type PhysicalNetworks struct {
+	Name   string `json:"localnet" yaml:"localnet"`
+	Bridge string `json:"bridge" yaml:"bridge"`
+	State  string `json:"state,omitempty" yaml:"state,omitempty"`
+}


This is not really needed since we don't need to filter it out, we just need an "ovn" field at desiredState level of type json.RawMessage

It's not only filtering - it makes testing a lot easier, since I can use these structs to assert against.

I prefer to

Expect(nodeBridgeMappings(node)).To( ConsistOf(state.PhysicalNetworks{Name: networkName, Bridge: bridgeName}))

instead of comparing against a string.

But I won't insist.

We will consume a generated golang api in the furure so I am ok with this.

qinqon · 2024-04-30T06:31:14Z

pkg/state/type.go

@@ -28,6 +28,7 @@ type rootState struct {
 	Interfaces  []interfaceState `json:"interfaces"             yaml:"interfaces"`
 	Routes      *routes          `json:"routes,omitempty"       yaml:"routes,omitempty"`
 	DNSResolver *dnsResolver     `json:"dns-resolver,omitempty" yaml:"dns-resolver,omitempty"`
+	Ovn         *bridgeMappings  `json:"ovn,omitempty"          yaml:"ovn,omitempty"`


Suggested change

Ovn *bridgeMappings `json:"ovn,omitempty" yaml:"ovn,omitempty"`

Ovn json.RawMessage `json:"ovn,omitempty" yaml:"ovn,omitempty"`

Now that we are a it maybe we can add the missing ones looking at https://docs.rs/nmstate/latest/nmstate/struct.NetworkState.html

We are missing:

hostname

route-rules

ovs-db

So we can do the following

type rootState struct { Interfaces []interfaceState `json:"interfaces" yaml:"interfaces"` Routes *routes `json:"routes,omitempty" yaml:"routes,omitempty"` DNSResolver *dnsResolver `json:"dns-resolver,omitempty" yaml:"dns-resolver,omitempty"` Hostname json.RawMessage `json:"hostname,omitempty" yaml:"ovn,omitempty"` RouteRules json.RawMessage `json:"route-rules,omitempty" yaml:"ovn,omitempty"` OVSDb json.RawMessage `json:"ovs-db,omitempty" yaml:"ovs-db,omitempty"` OVN json.RawMessage `json:"ovn,omitempty" yaml:"ovn,omitempty"` }

An alternative is a custom encoder that will just bypass whatever we don't need to filter

I'd rather focus exclusively on the attributes that I need to add. And test only those attributes.

qinqon · 2024-04-30T12:42:13Z

/lgtm
/approve

qinqon · 2024-04-30T13:38:39Z

/lgtm
/ok-to-test

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

qinqon

/lgtm
/approve

kubevirt-bot · 2024-05-06T09:05:29Z

[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~~ [qinqon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

maiqueb · 2024-05-06T09:41:03Z

/test pull-kubernetes-nmstate-e2e-operator-k8s

kubevirt-bot · 2024-05-06T09:41:19Z

@maiqueb: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-kubernetes-nmstate-e2e-operator-k8s

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

qinqon · 2024-05-06T09:50:52Z

/ok-to-test

maiqueb · 2024-05-06T09:52:46Z

/test pull-kubernetes-nmstate-e2e-operator-k8s

kubevirt-bot · 2024-05-06T09:53:02Z

@maiqueb: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-kubernetes-nmstate-e2e-operator-k8s

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

maiqueb · 2024-05-06T10:41:15Z

/test pull-kubernetes-nmstate-e2e-operator-k8s

kubevirt-bot · 2024-05-06T11:27:48Z

@maiqueb: 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	`caf1f23`	link	false	`/test pull-kubernetes-nmstate-e2e-handler-k8s-future`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

maiqueb · 2024-05-06T14:13:45Z

hm, I'm checking this locally and it passes.

Could it be we need to somehow integrate OVS in CI for this to work ?

maiqueb · 2024-05-06T14:18:50Z

/test pull-kubernetes-nmstate-e2e-handler-k8s

maiqueb · 2024-05-06T15:54:38Z

@qinqon ok, so this got merged.

When / how will it become available downstream ? What's the flow here ? ... Will it be automatically cherry-picked downstream by some automation, or do I need to do something further ?

Thanks for your help and time.

mkowalski · 2024-05-06T18:12:06Z

It will land in downstream automatically after a few days. Unless you have a critical case and need it in downstream immediately, there is nothing to do at all, just sit and watch

kubevirt-bot requested a review from qinqon April 29, 2024 15:11

kubevirt-bot added the size/L label Apr 29, 2024

kubevirt-bot requested a review from yboaron April 29, 2024 15:12

maiqueb force-pushed the expose-ovn-bridge-mappings-nns branch from c2c4714 to b99fcf4 Compare April 29, 2024 15:24

maiqueb marked this pull request as ready for review April 29, 2024 15:28

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2024

kubevirt-bot requested review from mkowalski and rhrazdil April 29, 2024 15:28

qinqon requested changes Apr 30, 2024

View reviewed changes

kubevirt-bot assigned qinqon Apr 30, 2024

kubevirt-bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 30, 2024

maiqueb force-pushed the expose-ovn-bridge-mappings-nns branch from b99fcf4 to 874deca Compare April 30, 2024 13:24

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024

api: add ovn bridge mappings to node network state

caf1f23

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>

maiqueb force-pushed the expose-ovn-bridge-mappings-nns branch from 874deca to caf1f23 Compare April 30, 2024 14:33

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024

qinqon approved these changes May 6, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2024

qinqon added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 6, 2024

kubevirt-bot merged commit 84ca0cb into nmstate:main May 6, 2024
7 of 8 checks passed

maiqueb deleted the expose-ovn-bridge-mappings-nns branch May 6, 2024 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: add ovn bridge mappings to node network state #1237

api: add ovn bridge mappings to node network state #1237

maiqueb commented Apr 29, 2024

kubevirt-bot commented Apr 29, 2024

maiqueb commented Apr 29, 2024

qinqon left a comment

qinqon Apr 30, 2024

maiqueb Apr 30, 2024

qinqon Apr 30, 2024

qinqon Apr 30, 2024

qinqon Apr 30, 2024

qinqon Apr 30, 2024

maiqueb Apr 30, 2024

qinqon commented Apr 30, 2024

qinqon commented Apr 30, 2024

qinqon left a comment

kubevirt-bot commented May 6, 2024

maiqueb commented May 6, 2024

kubevirt-bot commented May 6, 2024

qinqon commented May 6, 2024

maiqueb commented May 6, 2024

kubevirt-bot commented May 6, 2024

maiqueb commented May 6, 2024

kubevirt-bot commented May 6, 2024 •

edited

maiqueb commented May 6, 2024

maiqueb commented May 6, 2024

maiqueb commented May 6, 2024

mkowalski commented May 6, 2024

	Ovn *bridgeMappings `json:"ovn,omitempty" yaml:"ovn,omitempty"`
	Ovn json.RawMessage `json:"ovn,omitempty" yaml:"ovn,omitempty"`

api: add ovn bridge mappings to node network state #1237

api: add ovn bridge mappings to node network state #1237

Conversation

maiqueb commented Apr 29, 2024

kubevirt-bot commented Apr 29, 2024

maiqueb commented Apr 29, 2024

qinqon left a comment

Choose a reason for hiding this comment

qinqon Apr 30, 2024

Choose a reason for hiding this comment

maiqueb Apr 30, 2024

Choose a reason for hiding this comment

qinqon Apr 30, 2024

Choose a reason for hiding this comment

qinqon Apr 30, 2024

Choose a reason for hiding this comment

qinqon Apr 30, 2024

Choose a reason for hiding this comment

qinqon Apr 30, 2024

Choose a reason for hiding this comment

maiqueb Apr 30, 2024

Choose a reason for hiding this comment

qinqon commented Apr 30, 2024

qinqon commented Apr 30, 2024

qinqon left a comment

Choose a reason for hiding this comment

kubevirt-bot commented May 6, 2024

maiqueb commented May 6, 2024

kubevirt-bot commented May 6, 2024

qinqon commented May 6, 2024

maiqueb commented May 6, 2024

kubevirt-bot commented May 6, 2024

maiqueb commented May 6, 2024

kubevirt-bot commented May 6, 2024 • edited

maiqueb commented May 6, 2024

maiqueb commented May 6, 2024

maiqueb commented May 6, 2024

mkowalski commented May 6, 2024

kubevirt-bot commented May 6, 2024 •

edited