On Thu, Sep 3, 2020 at 3:55 PM Dumitru Ceara <[email protected]> wrote:
> On 9/3/20 11:57 AM, Numan Siddique wrote: > > > > > > On Thu, Aug 27, 2020 at 7:32 PM Dumitru Ceara <[email protected] > > <mailto:[email protected]>> 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 <[email protected] > > <mailto:[email protected]>> > > > > > > > > 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 > > Oops. My bad. Thanks. I tested after applying both the patches. When I run this command - ovs-vsctl set open . external_ids:system-id=ch-2 I see below warning message in ovn-controller log ***** 2020-09-03T11:03:34.411Z|00014|ovsdb_idl|WARN|transaction error: {"details":"Transaction causes multiple rows in \"Chassis_Private\" table to have identical values (ch-2) for index on column \"name\". First row, with UUID 1cf765f0-774d-460f-b394-988e7a2bf8ee, was inserted by this transaction. Second row, with UUID 04b66cab-0705-4cbb-b11c-2414d076b09d, existed in the database before this transaction and was not modified by the transaction.","error":"constraint violation"} 2020-09-03T11:03:34.412Z|00015|ovsdb_idl|WARN|transaction error: {"details":"Transaction causes multiple rows in \"Chassis_Private\" table to have identical values (ch-2) for index on column \"name\". First row, with UUID c15a0c17-a56a-4ec2-be3a-ceff644aa908, was inserted by this transaction. Second row, with UUID 04b66cab-0705-4cbb-b11c-2414d076b09d, existed in the database before this transaction and was not modified by the transaction.","error":"constraint violation"} ******* Functionally it works as expected. i.e the previous chassis_private record is updated with the new name. I think should be fixed though. Also can you please update the commit message in patch 1 which states the issue clearly. It was not clear to me the issue the patch is trying to address. Thanks Numan > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
