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

Adds test for Router Interface idempotence #176

Merged
merged 1 commit into from May 13, 2020

Conversation

rbrady
Copy link
Contributor

@rbrady rbrady commented Apr 23, 2020

This patch adds a test to ensure router interface idempotence.

@rbrady rbrady force-pushed the router-interface-idempotence branch from 7328dd8 to 41bd287 Compare April 23, 2020 18:12
@rbrady rbrady added the WIP Work in progress label Apr 23, 2020
@rbrady rbrady force-pushed the router-interface-idempotence branch from 41bd287 to ca8e0e6 Compare April 23, 2020 22:30
@rbrady
Copy link
Contributor Author

rbrady commented Apr 23, 2020

This latest push is just the progress so far. It's currently throwing openstack.exceptions.ConflictException when running through the import in the idempotence test. I'll address this exception in a change tomorrow.

@rbrady rbrady force-pushed the router-interface-idempotence branch from ca8e0e6 to 6b3ea19 Compare April 24, 2020 02:50
@rbrady
Copy link
Contributor Author

rbrady commented Apr 24, 2020

The create_or_update method has been modified to handle the exception, but the test isn't likely checking the right data to ensure idempotency (it's looking at routers.updated_at)

conn.network.add_interface_to_router(
refs['device_id'], port_id=port['id'])
changed = True
except openstack.exceptions.ConflictException:
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be any conflict exception here:

  • we're looking up the port by IPs, if it exists, we don't create it

  • we're looking if it's assigned to the router already, if yes, we don't try to assign it again

The method should be a no-op, something's not working. I think we should figure out what's being executed that shouldn't be executed, rather than catching the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ccamacho ccamacho added e2e and removed e2e labels Apr 25, 2020
@rbrady rbrady force-pushed the router-interface-idempotence branch from 6b3ea19 to 6e9fb74 Compare May 13, 2020 12:21
@rbrady rbrady requested a review from jistr May 13, 2020 12:21
This patch adds a test to ensure router interface idempotence and
updates the router interface resource class to handle the case
when a router interface already exists when attempting to create
an identical one.

This patch also fixes a small bug in the router interface where
it was using the "subnet_name" key where it should have been
using a "subnet_id" key.  This was causing the unintended behavior
of never finding a matching port when checking for ports that may
have already been created, so it always tried to create the port
leading to conflicts.
@rbrady rbrady force-pushed the router-interface-idempotence branch from 6e9fb74 to bd0c25c Compare May 13, 2020 13:18
@rbrady
Copy link
Contributor Author

rbrady commented May 13, 2020

closes #21

@@ -151,7 +150,7 @@ def _refs_from_ser(self, conn):
for fixed_ip in params['fixed_ips_names']:
refs['fixed_ips'].append({
'ip_address': fixed_ip['ip_address'],
'subnet_name': reference.subnet_id(conn, fixed_ip['subnet_name']),
'subnet_id': reference.subnet_id(conn, fixed_ip['subnet_name']),
Copy link
Contributor

Choose a reason for hiding this comment

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

Damn :). Thanks for the fix.

@rbrady rbrady merged commit c307fd5 into os-migrate:master May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants