On 9/3/20 11:57 AM, Numan Siddique wrote:
>
>
> On Thu, Aug 27, 2020 at 7:32 PM Dumitru Ceara <dce...@redhat.com
> <mailto:dce...@redhat.com>> wrote:
>
> ovn-controller always stores the last configured system-id/chassis-id in
> memory regardless if the connection to the SB is up or down. This is OK
> as long as the change can be committed successfully when the SB DB
> connection comes back up.
>
> This commit changes the way we search for a "stale" chassis record
> in the
> SB to cover the above mentioned case. Also, in such cases there's no
> need
> to recreate the Encaps, it's sufficient to update the chassis_name
> field.
>
> Fixes: 5344f24ecb1a ("ovn-controller: Refactor chassis.c to abstract
> the string parsing")
> Signed-off-by: Dumitru Ceara <dce...@redhat.com
> <mailto:dce...@redhat.com>>
>
>
>
> Hi Dumitru,
>
> Thanks for the patch. Below are few observations when I tested with this
> patch and without
>
Hi Numan,
Thanks for trying it out.
> 1. In a sandbox environment, when I change the
> external_ids:system-id, the chassis row for the
> ovn-controller never gets updated. I observed with this patch and
> without this patch. I see the below error
> message
>
> ******
> 2020-09-03T09:19:53.654Z|00032|ovsdb_idl|WARN|transaction error:
> {"details":"RBAC rules for client \"chassis-1\" role \"ovn-controller\"
> prohibit row insertion into table
> \"Chassis_Private\".","error":"permission error"}
> 2020-09-03T09:19:53.654Z|00033|ovsdb_idl|WARN|transaction error:
> {"details":"RBAC rules for client \"chassis-1\" role \"ovn-controller\"
> prohibit row insertion into table
> \"Chassis_Private\".","error":"permission error"}
> *****
> When I use a unix socket for SB DB connection, I don't see this
> issue. May be its to do with the RBAC permissions. Its not related to
> your patch. But thought of listing it here.
>
Right, this is not addressed by this series. We should probably follow
up with a patch for this issue too.
> 2. When I change the external_ids:system-id name, the chassis_private
> row is leaked. Again this is not related to your patch. But something we
> should fix ?
>
Is this with both patches applied?
https://patchwork.ozlabs.org/project/openvswitch/list/?series=198050
> 3. with this patch, I ran the below commands
> - ovs-vsctl set open . external_ids:system-id=ch-1 -- > the
> chassis row is recreated with ch-1
> - ovs-vsctl set open . external_ids:system-id=ch-2 -- > the
> chassis row is recreated with ch-2
> - ovs-vsctl set open . external_ids:system-id=ch-1 -- > the
> chassis row is still ch-2.
> - ovs-vsctl set open . external_ids:system-id=ch-3 -- > the
> chassis row is recreated with ch-3
> - ovs-vsctl set open . external_ids:system-id=ch-2 -- > the chassis
> row is still ch-3.
>
> Something is not correct when the old system-id is restored.
Same question here: are both patches of the series applied?
Because I did:
$ SANDBOXFLAGS=--no-ovn-rbac make sandbox
$ ovn-sbctl list chassis_private | grep '^name'
name : chassis-1
$ ovn-sbctl list chassis | grep '^name'
name : chassis-1
$ ovs-vsctl set open . external_ids:system-id=ch-1
$ ovn-sbctl list chassis_private | grep '^name'
name : ch-1
$ ovn-sbctl list chassis | grep '^name'
name : ch-1
$ ovs-vsctl set open . external_ids:system-id=ch-2
$ ovn-sbctl list chassis | grep '^name'
name : ch-2
$ ovn-sbctl list chassis_private | grep '^name'
name : ch-2
$ ovs-vsctl set open . external_ids:system-id=ch-1
$ ovn-sbctl list chassis | grep '^name'
name : ch-1
$ ovn-sbctl list chassis_private | grep '^name'
name : ch-1
$ ovs-vsctl set open . external_ids:system-id=ch-3
$ ovn-sbctl list chassis | grep '^name'
name : ch-3
$ ovn-sbctl list chassis_private | grep '^name'
name : ch-3
$ ovs-vsctl set open . external_ids:system-id=ch-2
$ ovn-sbctl list chassis | grep '^name'
name : ch-2
$ ovn-sbctl list chassis_private | grep '^name'
name : ch-2
So with both patches applied it seems to me that the system-id update is
working fine.
The first patch of the series doesn't apply cleanly on branch-20.03 but
if I cherry pick it on branch-20.06:
$ SANDBOXFLAGS=--no-ovn-rbac make sandbox
$ ovn-sbctl list chassis | grep '^name'
name : chassis-1
$ ovs-vsctl set open . external_ids:system-id=ch-1
$ ovn-sbctl list chassis | grep '^name'
name : ch-1
$ ovs-vsctl set open . external_ids:system-id=ch-2
$ ovn-sbctl list chassis | grep '^name'
name : ch-2
$ ovs-vsctl set open . external_ids:system-id=ch-1
$ ovn-sbctl list chassis | grep '^name'
name : ch-1
$ ovs-vsctl set open . external_ids:system-id=ch-3
$ ovn-sbctl list chassis | grep '^name'
name : ch-3
$ ovs-vsctl set open . external_ids:system-id=ch-2
$ ovn-sbctl list chassis | grep '^name'
name : ch-2
Regards,
Dumitru
>
> I think it's better to address (3) in this patch. I think (1) and (2)
> can be addressed in the same patch series or as a separate series.
>
> What do you think ?
>
> Thanks
> Numan
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
