Conversation
|
🤖 Created branch: z_pr3728/aswinsuryan/bump-ovn-ovsdb |
aswinsuryan
force-pushed
the
bump-ovn-ovsdb
branch
from
4c43dc4 to
8d4c20a
Compare
aswinsuryan
requested review from
Oats87,
dfarrell07,
maayanf24,
skitt,
sridhargaddam,
tpantelis,
vthapar and
yboaron
as code owners
aswinsuryan
force-pushed
the
bump-ovn-ovsdb
branch
3 times, most recently
from
3897d80 to
97582c4
Compare
Signed-off-by: Aswin Suryanarayanan <asuryan@redhat.com>
aswinsuryan
force-pushed
the
bump-ovn-ovsdb
branch
from
97582c4 to
228c57c
Compare
There was a problem hiding this comment.
@aswinsuryan Can you outline/describe the changes associated with the bump in the commit message? Eg why do we no longer call libovsdbops.CreateOrUpdateLogicalRouterPolicyWithPredicate, DeleteLogicalRouterStaticRoutesWithPredicate, etc?
|
|
||
| ops = append(ops, updateOps...) | ||
| } else { | ||
| lrp.UUID = "new_lrp" |
There was a problem hiding this comment.
This needs to be unique (same on L272). Internally OVSDB code uses buildNamedUUID tho we can't access that. We could duplicate the code but not ideal. I don't know how important it is to follow OVSDB rfc 7047 section 5.1. We can also just use NewUUID from k8s.io/apimachinery/pkg/util/uuid.
There was a problem hiding this comment.
I added commit 980b262 to adjust the relevant unit test so it now fails due to non-unique UUIDs. It didn't fail before due to timing and luck.
| return errors.Wrapf(err, "failed to find ovn cluster router %q", OVNClusterRouter) | ||
| } | ||
|
|
||
| if router.UUID == "" { |
There was a problem hiding this comment.
Is this just defensive or is it actually possible for this to be empty (and have you observed it)?
| defer c.client.mutex.Unlock() | ||
|
|
||
| uuid := getUUID(m) | ||
| if uuid != "" { |
There was a problem hiding this comment.
Perhaps we should fail if empty (ie Expect(uuid).NotTo(BeEmpty()))?
I assume the real implementation also looks up by UUID? Perhaps we should also modify Delete to do so.
Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
| } | ||
| } | ||
|
|
||
| c.client.models[reflect.TypeOf(m)] = append(c.client.models[reflect.TypeOf(m)], m) |
There was a problem hiding this comment.
Since this function is Update I don't think we should add if not present.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
🤖 Closed branches: [z_pr3728/aswinsuryan/bump-ovn-ovsdb] |
|
🤖 Created branch: z_pr3728/aswinsuryan/bump-ovn-ovsdb |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
5 participants
Bump OVN-K