CP-10531: Update vm_guest_metrics to implement PV drivers update #2160
Conversation
|
@xen-git Retest this please |
|
|
||
| field ~qualifier:DynamicRO ~ty:Bool ~in_oss_since:None | ||
| ~lifecycle:[ | ||
| Removed, rel_dundee, "Disabled in favour of the PV drivers auto-update" |
There was a problem hiding this comment.
This should be marked as Deprecated, not Removed. Suggested text:
"Deprecated in favour of network_paths_optimized and storage_paths_optimized, and redefined in terms of them."
|
At present some of the English is rather foreign: please would you change "networks_path" to "network_paths" in identifiers such as "network_paths_optimized", and the equivalent for "storage_paths"? (Note: I've not finished reviewing this yet.) |
|
I don't think we should have a commit in the history with message: I see the main body includes more useful information. It would be better if the title had a description of the change and (if absolutely necessary) you reference the REQ in the body of the message. This page has some good thoughts on commit messages: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html |
|
Thanks Thomas and Simon for the comments. |
huizh
changed the title
| @@ -233,6 +281,25 @@ let all (lookup: string -> string option) (list: string -> string list) ~__conte | |||
| Db.VM_guest_metrics.set_other ~__context ~self:gm ~value:other; | |||
| Helpers.call_api_functions ~__context (fun rpc session_id -> Client.Client.VM.update_allowed_operations rpc session_id self); | |||
| end; | |||
| if(vif_state_cached <> vif_state) then begin | |||
There was a problem hiding this comment.
Is there any reason not to just cache network_paths_optimized instead of building up vif_state and caching that?
Same for VBDs.
There was a problem hiding this comment.
Thanks Thomas.
To get network_paths_optimzied, the following steps must be run:
- get
vif_statewhich contains the state of all VIFs; - execute logical AND of the states.
That means under eachvm_guest_metricsrefresh, to comparenetwork_paths_optimizedandnetwork_paths_optimized_cached,
network_paths_optimizedmust be calculated with both step 1 and 2 even ifvif_statehas no change.
If we just cache vif_state, step 1 is mandatory with vm_guest_metrics refresh.
But only when vif_state doesn't equal to vif_state_cached, step 2 will be executed.
Considering vif_state is normally immutable, step 2 seldom needs to run.
For VBD, that's similar.
Could you please let me know if you have any concern?
Thanks.
There was a problem hiding this comment.
Comparing vif_state with vif_state_cached is more costly than doing a logical AND across one of them.
But as I said, I see no need to build up vif_state in the first place: we can do logical AND as we go along and cache the result.
There was a problem hiding this comment.
OK. I see.
Just update the pull request, could you please help review it?
Thanks.
|
Looks good to me except that in xapi_guest_agent I think we should calculate and cache |
1. Introduce networks_path_optimized and storages_path_optimized flags to vm_guest_metrics. 2. Update the implementation of PV_drivers_up_to_date. Signed-off-by: Hui Zhang <hui.zhang@citrix.com>
|
Hi Thomas Thanks for your suggestion that "we can do logical AND as we go along and cache the result." Thanks. |
|
Looks good now. |
|
Thanks Thomas for the review. |
|
Hi Thomas, |
Signed-off-by: Hui Zhang hui.zhang@citrix.com