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

Add OVS to OVN migration tests #365

Merged
merged 8 commits into from Sep 11, 2020
Merged

Add OVS to OVN migration tests #365

merged 8 commits into from Sep 11, 2020

Conversation

fnordahl
Copy link
Contributor

No description provided.

@fnordahl fnordahl force-pushed the ovn-charm branch 9 times, most recently from 589c373 to 023461f Compare July 17, 2020 12:01
@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2020

Codecov Report

Merging #365 into master will increase coverage by 1.58%.
The diff coverage is 13.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
+ Coverage   18.57%   20.16%   +1.58%     
==========================================
  Files         154      155       +1     
  Lines        8389     9270     +881     
==========================================
+ Hits         1558     1869     +311     
- Misses       6831     7401     +570     
Impacted Files Coverage Δ
zaza/openstack/charm_tests/neutron/tests.py 0.00% <0.00%> (ø)
zaza/openstack/charm_tests/ovn/setup.py 0.00% <0.00%> (ø)
zaza/openstack/charm_tests/ovn/tests.py 0.00% <0.00%> (ø)
zaza/openstack/utilities/openstack.py 58.11% <50.00%> (+0.96%) ⬆️
zaza/openstack/charm_tests/test_utils.py 29.33% <81.25%> (+2.11%) ⬆️
zaza/openstack/charm_tests/ceph/tests.py 0.00% <0.00%> (ø)
zaza/openstack/charm_tests/trilio/tests.py 0.00% <0.00%> (ø)
zaza/openstack/charm_tests/hacluster/tests.py 0.00% <0.00%> (ø)
zaza/openstack/charm_tests/neutron_arista/setup.py 0.00% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66d95d6...eca298c. Read the comment docs.

@fnordahl fnordahl force-pushed the ovn-charm branch 5 times, most recently from 4e79462 to 6a84fa1 Compare July 23, 2020 11:20
@fnordahl fnordahl marked this pull request as ready for review September 4, 2020 09:28
Copy link
Contributor

@lourot lourot left a comment

Choose a reason for hiding this comment

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

The second commit says "Make tearDown a class instance method." but it doesn't seem to be doing that anymore?

@fnordahl
Copy link
Contributor Author

fnordahl commented Sep 4, 2020

The second commit says "Make tearDown a class instance method." but it doesn't seem to be doing that anymore?

That part got merged already, I'll update the commit message.

# this must be a class instance method otherwise descentents will not be
# able to influence if cleanup should be run.
def tearDown(self):
"""Run teardown for test class."""
if self.run_resource_cleanup:
logging.info('Running resource cleanup')
self.resource_cleanup()

Copy link
Contributor

@lourot lourot left a comment

Choose a reason for hiding this comment

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

Not done reviewing yet, but getting there :)

zaza/openstack/charm_tests/ovn/setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@lourot lourot left a comment

Choose a reason for hiding this comment

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

Apart from a typo, this all looks good to me, thanks!

on instances created prior to the migration.
"""
# The setUp method of this test class will perform the migration steps.
# The tests.yaml is programmed to to further validation after the
Copy link
Contributor

Choose a reason for hiding this comment

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

s/to to/to do/

This is done by building a prefix for options key from dot-notated
absolute path to calling method or function.
Adjust NeutronNetworkingTest to optionally run tearDown and
re-use existing instances on subsequent runs.

tearDown is controlled through a key under the `tests_options`
dictionary in tests.yaml.

Useful for morphing a deployment and then validating connectivity
for existing instances afterwards.
For compatibility with existing scenario tests the
`configure_gateway_ext_port` helper currently make use of
`juju_wait` when configuring the deployed cloud.

This does not work well if the model you are testing has
applications with non-standard workload status messaging.

Allow to override the behaviour through config step options.
To support OVS to OVN migration checks we want the basic overcloud
configure job to set up N-OVS and/or N-GW when present and the
OVN pre migration configure job will copy the configuration for us.
Copy link
Contributor

@thedac thedac left a comment

Choose a reason for hiding this comment

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

My apologies for taking so long to review. But as you might imaging I have several requests.

There are a handful of missing docstrings.

I would also like to get confirmation that regular OVS testing has been run against this PR as some of the common tests are affected.

I would like to get a discussion on the use of test_options.

Thank you for all the comments which made reading the code much easier and made a complex task understandable.

# The reason for using the `config_change` helper for this
# is that it already deals with all the permutations of
# config already being set etc and does not get into
# trouble if the test bundle already have the values we try
Copy link
Contributor

Choose a reason for hiding this comment

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

s/have/has

zaza/openstack/charm_tests/ovn/setup.py Show resolved Hide resolved
def setUp(self):
"""Perform migration steps prior to validation."""
super(OVSOVNMigrationTest, self).setUp()
# These steps here due to them having to be executed once and in a
Copy link
Contributor

Choose a reason for hiding this comment

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

These steps are here

zaza/openstack/charm_tests/ovn/tests.py Show resolved Hide resolved
zaza/openstack/charm_tests/ovn/tests.py Show resolved Hide resolved
zaza/openstack/charm_tests/ovn/tests.py Show resolved Hide resolved
zaza/openstack/charm_tests/test_utils.py Show resolved Hide resolved
@fnordahl
Copy link
Contributor Author

fnordahl commented Sep 9, 2020

I would also like to get confirmation that regular OVS testing has been run against this PR as some of the common tests are affected.

Yes, the neutron-gateway, neutron-openvswitch and neutron-api-plugin-ovn have or will be tested with this PR both for regular and OVN migration jobs.

@fnordahl fnordahl marked this pull request as draft September 9, 2020 15:42
@fnordahl fnordahl marked this pull request as ready for review September 10, 2020 09:01
Merge two similar config set patterns into a common method.

Add missing docstrings.

Fix typos.
Copy link
Contributor

@thedac thedac left a comment

Choose a reason for hiding this comment

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

Landing.

@thedac thedac merged commit 23b24a5 into master Sep 11, 2020
openstack-mirroring pushed a commit to openstack/charm-neutron-openvswitch that referenced this pull request Sep 11, 2020
Add OVS to OVN migration at the end of the regular gate test. This
adds only 5-10 minutes to each job and we want to confirm this
works from focal-ussuri and onwards as this is the point where we
recomend our end users to migrate from OVS to OVN.

Do ch-sync.

Merge after juju/charm-helpers#511

Func-Test-Pr: openstack-charmers/zaza-openstack-tests#365
Depends-On: Ifa99988612eaaeb9d60a0d99db172f97e27cfc93
Change-Id: Ia4b1d3a9e642b540d1e04adc0363f9b3e11f37cd
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Sep 11, 2020
* Update charm-neutron-openvswitch from branch 'master'
  - Add cleanup action and OVS to OVN migration test
    
    Add OVS to OVN migration at the end of the regular gate test. This
    adds only 5-10 minutes to each job and we want to confirm this
    works from focal-ussuri and onwards as this is the point where we
    recomend our end users to migrate from OVS to OVN.
    
    Do ch-sync.
    
    Merge after juju/charm-helpers#511
    
    Func-Test-Pr: openstack-charmers/zaza-openstack-tests#365
    Depends-On: Ifa99988612eaaeb9d60a0d99db172f97e27cfc93
    Change-Id: Ia4b1d3a9e642b540d1e04adc0363f9b3e11f37cd
@ajkavanagh ajkavanagh deleted the ovn-charm branch July 13, 2021 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants