-
Notifications
You must be signed in to change notification settings - Fork 292
CA-250748: MTU on pif does not always sync to xapi db #3007
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message should include what was changed and not just the error that is presumably fixed by the change. From what I understand, the change re-orders the sync after the network change. Why is that the right thing to do?
ocaml/xapi/nm.ml
Outdated
let mtu = Int64.of_int (Net.Interface.get_mtu dbg ~name:bridge) in | ||
Db.PIF.set_MTU ~__context ~self:pif ~value:mtu | ||
with _ -> | ||
debug "could not update MTU field on PIF %s" rc.API.pIF_uuid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have a priority higher than debug - at least info and I would argue warning is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line wasn't changed, just re-indented. But yes, let's fix it and make it a warn
now that we are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor changes.
ocaml/xapi/nm.ml
Outdated
(* sync MTU *) | ||
try | ||
let mtu = Int64.of_int (Net.Interface.get_mtu dbg ~name:bridge) in | ||
Db.PIF.set_MTU ~__context ~self:pif ~value:mtu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can optimise it a little and only do the DB set if the current MTU is different from the one in the DB (rc.API.pIF_MTU
). This is more important now that this is executed unconditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, better option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Fix when pif-plug is called, the mtu is not sync to xapi db. Signed-off-by: Taoyong Ding <taoyong.ding@citrix.com>
Signed-off-by: Taoyong Ding taoyong.ding@citrix.com