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.

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

OVN external addresses on lxc network list-allocations #14210

Merged
merged 13 commits into from
Oct 17, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Oct 4, 2024

This also adds a network field to each network allocation, indicating to which network each allocated address belongs.
Closes #13412

Example:

$ lxc network list-allocations          
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
|       USED BY        |                  ADDRESS                   | NETWORK |   TYPE   | NAT  | HARDWARE ADDRESS  |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/networks/lxdbr0 | 10.26.240.1/24                             | lxdbr0  | network  | true |                   |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/networks/lxdbr0 | fd42:c38d:53ad:cf4f::1/64                  | lxdbr0  | network  | true |                   |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/networks/ovn    | 10.26.240.160/32                           | lxdbr0  | uplink   | true |                   |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/networks/ovn    | fd42:c38d:53ad:cf4f:216:3eff:fe9d:3aa8/128 | lxdbr0  | uplink   | true |                   |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/instances/env   | fd42:c38d:53ad:cf4f:216:3eff:fecd:31c5/128 | lxdbr0  | instance | true | 00:16:3e:cd:31:c5 |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/instances/vm1   | fd42:c38d:53ad:cf4f:216:3eff:fe92:5d6f/128 | lxdbr0  | instance | true | 00:16:3e:92:5d:6f |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/instances/env   | 10.26.240.157/32                           | lxdbr0  | instance | true | 00:16:3e:cd:31:c5 |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/networks/ovn    | 10.14.146.1/24                             | ovn     | network  | true |                   |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+
| /1.0/networks/ovn    | fd42:70da:489e:43ac::1/64                  | ovn     | network  | true |                   |
+----------------------+--------------------------------------------+---------+----------+------+-------------------+

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Oct 4, 2024
Copy link

github-actions bot commented Oct 4, 2024

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

@tomponline
Copy link
Member

there's a spelling error (either needs quoting or specified as OVN)

@hamistao hamistao marked this pull request as ready for review October 7, 2024 10:15
@hamistao
Copy link
Contributor Author

hamistao commented Oct 7, 2024

@tomponline Sorry, could you please point out where the spelling error is?
Also, this is ready for a full review.
Tests were included in canonical/lxd-ci#308

@tomponline
Copy link
Member

@hamistao hamistao force-pushed the list-ovn-routers branch 4 times, most recently from 96a7b1e to ebd8e3a Compare October 8, 2024 17:10
@hamistao
Copy link
Contributor Author

hamistao commented Oct 8, 2024

@tomponline All green here, ready for a review!

Edit: Tests went back to failing :(
Still pretty sure they are failing for reasons outside the scope of this PR, so feel free to take a look here when you can

@hamistao hamistao force-pushed the list-ovn-routers branch 2 times, most recently from fd0798c to 3d5c988 Compare October 9, 2024 02:27
@tomponline
Copy link
Member

please can you rebase

@hamistao hamistao force-pushed the list-ovn-routers branch 5 times, most recently from 41082b4 to 4e4612e Compare October 9, 2024 23:01
@hamistao
Copy link
Contributor Author

hamistao commented Oct 10, 2024

@tomponline Now rebased and all green, ready for a review when you have the time
As a reminedr, tests are included in canonical/lxd-ci#308

@@ -2501,3 +2501,9 @@ Expands APIs under `/1.0/auth` to include:
The caller must provide a base64 encoded x509 certificate in the `certificate` field of the request body.
Fine-grained TLS identities may update their own certificate.
To update the certificate of another identity, the caller must have `can_edit` on the identity.

## `list_ovn_uplink_allocations`
Copy link
Member

Choose a reason for hiding this comment

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

Should be called something like network_allocations_ovn_uplink so it aligns with "thing_feature" style and with the earlier "network_allocations" extension.

@@ -3483,7 +3483,7 @@ func (n *bridge) Leases(projectName string, clientType request.ClientType) ([]ap
v := network.Config[k]
if v != "" {
leases = append(leases, api.NetworkLease{
Hostname: fmt.Sprintf("%s-%s.uplink", projectName, network.Name),
Hostname: fmt.Sprintf("%s/%s.uplink", projectName, network.Name),
Copy link
Member

Choose a reason for hiding this comment

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

/ is not valid character to include in hostname values.

Also its an API break.

@@ -3127,24 +3127,28 @@ definitions:
e.g, instance, network forward, load-balancer, network...
properties:
addresses:
description: The network address of the allocation (in CIDR format)
description: The network address of the allocation (in CIDR format).
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the existing rest-api.yaml the vast majority of description entries do not end with fullstop.

I tend to check what is the most consistent existing format before changing something like this.

It looks like we should be removing all full stops from end of these descriptions rather than adding them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, I just followed the first other example I found instead of looking at many instances

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 changes to be made, and i think we should discuss the project issue together.

Please can you flag any API breaks in the future just in case I dont notice them. thanks!

@hamistao hamistao force-pushed the list-ovn-routers branch 3 times, most recently from 79bafa2 to 5545914 Compare October 15, 2024 12:53
@hamistao hamistao marked this pull request as draft October 16, 2024 05:01
@hamistao hamistao force-pushed the list-ovn-routers branch 7 times, most recently from 213603a to 1fdf2d6 Compare October 17, 2024 05:16
@tomponline
Copy link
Member

How are you doing with this @hamistao ?

@hamistao
Copy link
Contributor Author

@tomponline Tests passing and ready for a review, sorry for the delay

This populates the new `HostProject` field accordingly and also supports calling `Leases` with an empty project name, which returns leases from all projects.

Signed-off-by: hamistao <[email protected]>
This populates the new `HostProject` field accordingly and also supports calling `Leases` with an empty project name, which returns leases from all projects.

Signed-off-by: hamistao <[email protected]>
This is needed to also show leases for instances that are in project other than the network's project.

Signed-off-by: hamistao <[email protected]>
This also uses the correct project for instance leases, instead of
assuming the instance is always on the same project as the network.

Signed-off-by: hamistao <[email protected]>
Also puts a full stop in comments

Signed-off-by: hamistao <[email protected]>
@tomponline tomponline marked this pull request as ready for review October 17, 2024 12:11
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.

Ta!

@tomponline tomponline merged commit 2199e24 into canonical:main Oct 17, 2024
27 checks passed
tomponline added a commit to canonical/lxd-ci that referenced this pull request Nov 5, 2024
@hamistao hamistao deleted the list-ovn-routers branch November 22, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OVN virtual router external addresses should show in lxc network list-allocations on the uplink network
2 participants