MAAS

Merge lp://qastaging/~blake-rouse/maas/vlan-relay into lp://qastaging/~maas-committers/maas/trunk

  1. vlan-relay
  2. Merge into trunk
Proposed by Blake Rouse
Status: Merged
Approved by: Blake Rouse
Approved revision: no longer in the source branch.
Merged at revision: 5585
Proposed branch: lp://qastaging/~blake-rouse/maas/vlan-relay
Merge into: lp://qastaging/~maas-committers/maas/trunk
Diff against target: 1628 lines (+770/-208)
22 files modified
src/maasserver/api/tests/test_vlans.py (+40/-0)
src/maasserver/api/vlans.py (+8/-1)
src/maasserver/dhcp.py (+41/-61)
src/maasserver/exceptions.py (+0/-4)
src/maasserver/forms_vlan.py (+25/-0)
src/maasserver/migrations/builtin/maasserver/0056_add_description_to_fabric_and_space.py (+1/-1)
src/maasserver/migrations/builtin/maasserver/0095_vlan_relay_vlan.py (+23/-0)
src/maasserver/migrations/builtin/maasserver/0096_set_default_vlan_field.py (+24/-0)
src/maasserver/models/tests/test_vlan.py (+8/-0)
src/maasserver/models/vlan.py (+5/-0)
src/maasserver/static/js/angular/controllers/tests/test_vlan_details.js (+56/-3)
src/maasserver/static/js/angular/controllers/vlan_details.js (+126/-18)
src/maasserver/static/js/angular/factories/tests/test_vlans.js (+6/-3)
src/maasserver/static/js/angular/factories/vlans.js (+12/-7)
src/maasserver/static/partials/vlan-details.html (+97/-22)
src/maasserver/testing/factory.py (+3/-2)
src/maasserver/tests/test_dhcp.py (+27/-80)
src/maasserver/tests/test_forms_vlan.py (+70/-0)
src/maasserver/triggers/system.py (+56/-0)
src/maasserver/triggers/tests/test_system_listener.py (+114/-0)
src/maasserver/websockets/handlers/tests/test_vlan.py (+15/-0)
src/maasserver/websockets/handlers/vlan.py (+13/-6)
To merge this branch: bzr merge lp://qastaging/~blake-rouse/maas/vlan-relay
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Approve
Review via email: mp+312165@code.qastaging.launchpad.net

Commit message

Support the ability for a VLAN to act as a relay for another VLAN.

To post a comment you must log in.
Revision history for this message
Blake Rouse (blake-rouse) wrote :

I have tested this and the dhcpd.conf is getting written correctly. Still waiting on review from design before landing.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Looks good; I have a few questions and comments below before this lands.

Don't we also need a validation to ensure you cannot enable DHCP on a VLAN with a relay?

review: Needs Information
Revision history for this message
Mike Pontillo (mpontillo) wrote :

Also, can you link bug #1602412 to this branch with --fixes?

Revision history for this message
Blake Rouse (blake-rouse) wrote :

> Looks good; I have a few questions and comments below before this lands.
>
> Don't we also need a validation to ensure you cannot enable DHCP on a VLAN
> with a relay?

I did not want to block this. So if they enable DHCP it clears the relay_vlan and shows a warning in the UI when they go to enable.

Revision history for this message
Blake Rouse (blake-rouse) :
Revision history for this message
Mike Pontillo (mpontillo) :
Revision history for this message
Blake Rouse (blake-rouse) wrote :

I am not making that change in my branch. That is not even related to the
change I am making. File a bug if its causing an issue.

On Wed, Nov 30, 2016 at 4:14 PM, Mike Pontillo <email address hidden>
wrote:

>
>
> Diff comments:
>
> >
> > === added file 'src/maasserver/migrations/builtin/maasserver/0095_set_
> default_vlan_field.py'
> > --- src/maasserver/migrations/builtin/maasserver/0095_set_default_vlan_field.py
> 1970-01-01 00:00:00 +0000
> > +++ src/maasserver/migrations/builtin/maasserver/0095_set_default_vlan_field.py
> 2016-11-30 16:44:24 +0000
> > @@ -0,0 +1,24 @@
> > +# -*- coding: utf-8 -*-
> > +from __future__ import unicode_literals
> > +
> > +from django.db import (
> > + migrations,
> > + models,
> > +)
> > +import django.db.models.deletion
> > +import maasserver.models.subnet
> > +
> > +
> > +class Migration(migrations.Migration):
> > +
> > + dependencies = [
> > + ('maasserver', '0094_vlan_relay_vlan'),
> > + ]
> > +
> > + operations = [
> > + migrations.AlterField(
> > + model_name='subnet',
> > + name='vlan',
> > + field=models.ForeignKey(to='maasserver.VLAN',
> default=maasserver.models.subnet.get_default_vlan,
> on_delete=django.db.models.deletion.PROTECT),
>
> Ah, my mistake. But I think there might still be an issue here, just not
> exactly where I pointed it out.
>
> According to the Django docs, CASCADE is the default when no `on_delete`
> option is set. Wouldn't that cause severe unintended consequences in the
> case of the `relay_vlan` field? I think we should use `on_delete=SET_NULL`
> on the `relay_vlan` field.
>
> > + ),
> > + ]
>
>
> --
> https://code.launchpad.net/~blake-rouse/maas/vlan-relay/+merge/312165
> You are the owner of lp:~blake-rouse/maas/vlan-relay.
>

Revision history for this message
Mike Pontillo (mpontillo) :
Revision history for this message
Blake Rouse (blake-rouse) wrote :

Ah thanks for clarifying. I have fixed the issue and included a test to check.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Thanks for the fixes. I found one other issue below; please at least fix the text. I'll leave it up to you what action to take in that scenario.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.