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

feat: specify an ovn underlay network #245

Closed
wants to merge 11 commits into from
Closed

feat: specify an ovn underlay network #245

wants to merge 11 commits into from

Conversation

gabrielmougard
Copy link
Contributor

@gabrielmougard gabrielmougard commented Dec 7, 2023 •
edited
Loading

closes #239
related to: canonical/microovn#113
require: canonical/microcluster#168

WIP

  • Add interactive + preseed tests (validation that a custom Geneve tunnel has been established can be done with microovn.ovn-sbctl --columns=ip,type find Encap type=geneve)

@gabrielmougard gabrielmougard mentioned this pull request Dec 7, 2023
Merged
@gabrielmougard gabrielmougard mentioned this pull request Jan 2, 2024
Closed
This was referenced Feb 12, 2024
Merged
Closed
@markylaing
Copy link
Contributor

Blocked by canonical/microcluster#94 and canonical/microcluster#86

@gabrielmougard gabrielmougard force-pushed the feat/specify-ovn-east-west-v2 branch 2 times, most recently from 899cd9c to 776a81c Compare July 5, 2024 13:37
@gabrielmougard gabrielmougard marked this pull request as ready for review July 5, 2024 15:30
@masnax
Copy link
Contributor

masnax commented Jul 5, 2024

@gabrielmougard Looks like there's some doc rebases to do before the tests will run.

masnax
masnax requested changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@masnax masnax left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple questions

api/services.go Outdated Show resolved Hide resolved
cmd/microcloud/main_init.go Outdated
}

if !hasFeature {
fmt.Println("Underlay network setup for MicroOVN was not applied because your MicroOVN memmbers do not support custom encapsulation IP for the Geneve tunnel, skipping")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we checking this after joining nodes together? The list of API extensions is stored in memory during on first star, so why can't we check this earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, true. I faced the issue of the DB not initialized at first (for the joining members), that's why I put the check at the end. But I didn't remember that it was also in memory.... let's do that then. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@masnax Then, I'm wondering, do we need this function for checking the database if we can check it in memory before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe what would work best here is extending the Service interface with a new SupportsFeature function that checks the in-memory API extensions of the other snaps.

Since we're going to introduce enforced versioning of the snaps, I think for the purposes of this PR, it's safe to assume that if your local MicroOVN supports the feature, and you were actually able to find systems that are eligible to cluster with you, then you don't have to check those systems since they must already be at the same version as you.

Right now that validation isn't in place but it will be before the LTS so I think it's fine here to just check the local snaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will need this then canonical/microcluster#168

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@masnax updated :)

@gabrielmougard gabrielmougard force-pushed the feat/specify-ovn-east-west-v2 branch 2 times, most recently from a868eec to 6db31d2 Compare July 8, 2024 12:28
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 10, 2024
View reviewed changes
test/includes/microcloud.sh Fixed Show fixed Hide fixed
test/includes/microcloud.sh Fixed Show fixed Hide fixed
test/includes/microcloud.sh Fixed Show fixed Hide fixed
test/includes/microcloud.sh Fixed Show fixed Hide fixed
@gabrielmougard gabrielmougard force-pushed the feat/specify-ovn-east-west-v2 branch 3 times, most recently from 7985d56 to fc1fb5b Compare July 10, 2024 13:03
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 10, 2024
View reviewed changes
test/includes/microcloud.sh Fixed Show fixed Hide fixed
test/includes/microcloud.sh Fixed Show fixed Hide fixed
@masnax
Copy link
Contributor

masnax commented Jul 10, 2024

Looks like the validation for the underlay network is incorrect: Error: Invalid IP address: 10.19.33.184/24

@gabrielmougard gabrielmougard force-pushed the feat/specify-ovn-east-west-v2 branch 2 times, most recently from 4cda623 to 0a18928 Compare July 11, 2024 15:10
@gabrielmougard
refactor: Update MicroOVN dep
f0d47fd 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
refactor: pass an initConfig map[string]string to different services
4cc71da 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
cmd/microcloud: Add OVNGeneveAddr system parameter
503af2d 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard gabrielmougard force-pushed the feat/specify-ovn-east-west-v2 branch 8 times, most recently from 14d6dbd to 37c8e6b Compare July 12, 2024 14:33
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 12, 2024
View reviewed changes
test/suites/basic.sh Outdated
export OVN_UNDERLAY_NETWORK="yes"
export OVN_UNDERLAY_FILTER="${ovn_underlay_subnet_prefix}"
exit 1
microcloud_interactive | lxc exec micro01 -- sh -c "microcloud init 2>&1 tee out"

Check warning

Code scanning / shellcheck

SC2317 Warning test

Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
test/suites/basic.sh
exit 1
microcloud_interactive | lxc exec micro01 -- sh -c "microcloud init 2>&1 tee out"

lxc exec micro01 -- tail -1 out | grep "MicroCloud is ready" -q

Check warning

Code scanning / shellcheck

SC2317 Warning test

Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
test/suites/basic.sh Outdated
Comment on lines 232 to 244
for m in micro01 micro02 micro03 ; do
validate_system_lxd "${m}" 3 disk1 3 1 "${OVN_FILTER}" "${IPV4_SUBNET}" "${IPV4_START}"-"${IPV4_END}" "${IPV6_SUBNET}"
validate_system_microceph "${m}" 1 disk2
validate_system_microovn "${m}" "${ovn_underlay_subnet_prefix}"
done

Check warning

Code scanning / shellcheck

SC2317 Warning test

Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
test/suites/basic.sh

lxc exec micro01 -- tail -1 out | grep "MicroCloud is ready" -q
for m in micro01 micro02 micro03 ; do
validate_system_lxd "${m}" 3 disk1 3 1 "${OVN_FILTER}" "${IPV4_SUBNET}" "${IPV4_START}"-"${IPV4_END}" "${IPV6_SUBNET}"

Check warning

Code scanning / shellcheck

SC2317 Warning test

Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
test/suites/basic.sh Outdated
lxc exec micro01 -- tail -1 out | grep "MicroCloud is ready" -q
for m in micro01 micro02 micro03 ; do
validate_system_lxd "${m}" 3 disk1 3 1 "${OVN_FILTER}" "${IPV4_SUBNET}" "${IPV4_START}"-"${IPV4_END}" "${IPV6_SUBNET}"
validate_system_microceph "${m}" 1 disk2

Check warning

Code scanning / shellcheck

SC2317 Warning test

Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
test/suites/basic.sh
for m in micro01 micro02 micro03 ; do
validate_system_lxd "${m}" 3 disk1 3 1 "${OVN_FILTER}" "${IPV4_SUBNET}" "${IPV4_START}"-"${IPV4_END}" "${IPV6_SUBNET}"
validate_system_microceph "${m}" 1 disk2
validate_system_microovn "${m}" "${ovn_underlay_subnet_prefix}"

Check warning

Code scanning / shellcheck

SC2317 Warning test

Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
@gabrielmougard gabrielmougard force-pushed the feat/specify-ovn-east-west-v2 branch 5 times, most recently from 884a1c9 to 5aba2ba Compare July 15, 2024 07:55
@gabrielmougard
service/lxd: Introduce GetUnderlayInterfaces
9311922 
This refactored method will be used to fetch all the interfaces, both for uplink as before but also for underlay
(if asked) that should have an IP address.

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
service: Implement new SupportsFeature function
30653aa 
This will be needed to check the API extensions of different services from MicroCloud

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
service: Add OVNConfig to the JoinConfig
d944c13 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/ask: Interactively configure a custom underlay network
deead89 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/preseed: Allow the creation of an underlay network in pres…
c08b105 
…eed mode

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/cmd/microcloud: apply config for when a custom encaps Gene…
351b92d 
…ve IP is needed

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
doc: Add documentation for OVN underlay configuration
72528b9 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard gabrielmougard force-pushed the feat/specify-ovn-east-west-v2 branch 5 times, most recently from f54d149 to 9f2ef76 Compare July 15, 2024 18:47
@gabrielmougard
tests: Add OVN underlay tests
daf03bf 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline Awaiting requested review from tomponline

@roosterfish roosterfish Awaiting requested review from roosterfish

@markylaing markylaing Awaiting requested review from markylaing

@masnax masnax Awaiting requested review from masnax

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: add option for specifying the network to use for OVN east-west (underlay) traffic
3 participants
@gabrielmougard @markylaing @masnax