Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Network: Return ACL logs from syslogs when the OVN controller is deployed in MicroOVN #14327

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Network: Return ACL logs from syslogs when the OVN controller is deployed in MicroOVN #14327

wants to merge 4 commits into from

Conversation

gabrielmougard
Copy link
Contributor

@gabrielmougard gabrielmougard commented Oct 23, 2024 •
edited by tomponline
Loading

closes #12836

We need to remember to enable the log tests for microovn on canonical/lxd-ci#368 when this is merged

@gabrielmougard gabrielmougard changed the title lxd/network/acl: return ACL logs when the OVN controller is deployed in MicroOVN lxd/network/acl: return ACL logs from syslogs when the OVN controller is deployed in MicroOVN Oct 23, 2024
@gabrielmougard gabrielmougard mentioned this pull request Oct 23, 2024
Closed
@gabrielmougard gabrielmougard force-pushed the fix/microovn-logs branch 5 times, most recently from d4828be to 316aeed Compare October 25, 2024 18:24
@gabrielmougard gabrielmougard marked this pull request as ready for review October 25, 2024 18:25
@gabrielmougard
Copy link
Contributor Author

gabrielmougard commented Oct 25, 2024 •
edited
Loading

@tomponline this should be working. Regarding the documentation, as we discussed earlier, we probably need to add a note in the doc section Set up a LXD cluster on OVN to tell that a northbound_db_connection can also be added as a unix socket pointing toward an existing MicroOVN installation (unix:/var/snap/microovn/common/run/ovn/ovnnd_db.sock). However, as discovered here https://chat.canonical.com/canonical/pl/7fdhjesei3n9tcz1tkwbastozr, this seems to work only for a single node MicroOVN.. Do we really want to document that if it is not working for a multi node MicroOVN deployment?

@gabrielmougard gabrielmougard mentioned this pull request Oct 25, 2024
Closed
@gabrielmougard
Copy link
Contributor Author

Here is the lxd-ci PR: canonical/lxd-ci#330

@gabrielmougard gabrielmougard self-assigned this Oct 28, 2024
@gabrielmougard gabrielmougard force-pushed the fix/microovn-logs branch 2 times, most recently from ab7cce7 to 1794edb Compare October 30, 2024 08:18
@gabrielmougard
Copy link
Contributor Author

gabrielmougard commented Oct 30, 2024 •
edited
Loading

@tomponline the lxd-ci PR is ready at: https://github.com/canonical/lxd-ci/actions/runs/11599574607/job/32305555034?pr=330

Actually this lxd-ci PR completely enables MicroOVN as a 'backend' for the OVN tests so no need to add new acl show-logs tests.

@gabrielmougard gabrielmougard force-pushed the fix/microovn-logs branch 2 times, most recently from bc42c46 to fea89eb Compare October 31, 2024 11:00
@github-actions github-actions bot added the Documentation Documentation needs updating label Oct 31, 2024
@github-actions GitHub Actions
Copy link

Heads up @mionaalex - the "Documentation" label was applied to this issue.

simondeziel
simondeziel reviewed Nov 4, 2024
View reviewed changes
lxd/network/acl/acl_ovn.go Outdated Show resolved Hide resolved
lxd/network/acl/driver_common.go Outdated Show resolved Hide resolved
lxd/network/acl/driver_common.go Outdated Show resolved Hide resolved
@tomponline tomponline changed the title lxd/network/acl: return ACL logs from syslogs when the OVN controller is deployed in MicroOVN Network: Return ACL logs from syslogs when the OVN controller is deployed in MicroOVN Nov 5, 2024
minaelee
minaelee previously approved these changes Nov 5, 2024
View reviewed changes
@simondeziel simondeziel mentioned this pull request Nov 12, 2024
Merged
@gabrielmougard gabrielmougard force-pushed the fix/microovn-logs branch 2 times, most recently from ff7f5a4 to bebe6e1 Compare December 2, 2024 08:41
@gabrielmougard
lxd/network/acl: Allow 'ovnParseLogEntry' to read a log input using a…
f345349 
…n external timestamp

When reading a log file, the logtime (timestamp of the log entry) is part of the entry but when we read a syslog, the logtime is not contained
in the log entry fields and need to be parsed and converted.

We added two local functions to cope with these two usages:
* 'parseLogTimeFromFields'
* 'parseLogTimeFromTimestamp' (used when the OVN deployment is part of MicroOVN and we need to read the syslog)

Signed-off-by: Gabriel Mougard <[email protected]>
@gabrielmougard
lxd/network/acl: Read OVN logs from systemd journal
62f251d 
In the case of an OVN controller being deployed as part of a MicroOVN deployment,
the OVN controller logs are stored in MicroOVN's snap syslog. The LXD snap should have root access,
which means that it should be authorized (this is being tested) to read the OVN controller logs.

Signed-off-by: Gabriel Mougard <[email protected]>
@gabrielmougard
acl: Pass request ctx to acl.GetLog function
3990d6f 
Passing the request ctx to the GetLog function allows to stop the syslog stream (in case of OVN being deployed with MicroOVN)
in case of request interruption

Signed-off-by: Gabriel Mougard <[email protected]>
@gabrielmougard
doc: Describe how to manually connect the MicroOVN northbound connect…
29c825d 
…ion to LXD

When a user decides to deploy OVN through a MicroOVN deployment, it is not clearly explained how to connect
LXD and MicroOVN. The only missing info was how to correctly setup the northbound connection:

`lxc config set network.ovn.northbound_connection <ovn-northd-nb-db>`

With `<ovn-northd-nb-db>` that MUST use the `ssl:<microovn_node_ip>:6641`. Passing a UNIX socket
targeting `ovnnb_db.sock` inside the MicroOVN snap will produce certificate errors so we MUSN'T use the UNIX notation.

Signed-off-by: Gabriel Mougard <[email protected]>
@tomponline
Copy link
Member

doc tests failing

@simondeziel
Copy link
Member

doc tests failing

The fix is a rebase away AFAICT.

simondeziel added a commit to simondeziel/lxd-ci that referenced this pull request Dec 12, 2024
@simondeziel
tests/network-ovn: ignore empty ACL logs if not using deb
99c626f 
canonical/lxd#14327 is needed to get access to MicroOVN
logs.

Signed-off-by: Simon Deziel <[email protected]>
simondeziel added a commit to simondeziel/lxd-ci that referenced this pull request Dec 12, 2024
@simondeziel
tests/network-ovn: ignore empty ACL logs if not using deb
1625850 
canonical/lxd#14327 is needed to get access to MicroOVN
logs.

Signed-off-by: Simon Deziel <[email protected]>
simondeziel added a commit to simondeziel/lxd-ci that referenced this pull request Dec 12, 2024
@simondeziel
tests/network-ovn: ignore empty ACL logs if not using deb
5b89e3f 
canonical/lxd#14327 is needed to get access to MicroOVN
logs.

Signed-off-by: Simon Deziel <[email protected]>
tomponline
tomponline reviewed Dec 13, 2024
View reviewed changes
lxd/network/acl/driver_common.go
prefix := fmt.Sprintf("lxd_acl%d-", d.id)
logEntries, err = ovnParseLogEntriesFromJournald(context.TODO(), "snap.microovn.chassis.service", prefix)
if err != nil {
return "", fmt.Errorf("Failed to get OVN log entries from syslog: %v\n", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldnt use \n in returned errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use %w when wrapping errors

tomponline
tomponline reviewed Dec 13, 2024
View reviewed changes
lxd/network/acl/driver_common.go

// First, check if we can resolve the /run/openvswitch symlink to determine if OVN is in use.
// This is used in case of a MicroOVN deployment is interfaced with LXD.
targetPath, err := os.Readlink("/run/openvswitch")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break out the logic to detect if microovn is being used into a different function in network_utils.go

tomponline
tomponline requested changes Dec 13, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor refactoring to do please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline requested changes

@minaelee minaelee minaelee left review comments

@simondeziel simondeziel simondeziel left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@gabrielmougard gabrielmougard

Labels
Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ACL logs with MicroOVN do not work because of hard coded file path
4 participants
@gabrielmougard @tomponline @simondeziel @minaelee