From 73a549a4d08c9849bb7dd8ea4310397980a23dc6 Mon Sep 17 00:00:00 2001 From: Konstantina Chremmou Date: Wed, 17 Aug 2016 22:35:01 +0100 Subject: [PATCH] Improvements made while investigating CA-218953: - when the cd drive or the tools iso cannot be found, the action should fail. - if the tools SR is broken, do not show the error on a pop-up because the UX is bad when installation is attempted on multiple VMS. - added one more entry to the list of possible tools iso names. - small performance improvement. Signed-off-by: Konstantina Chremmou --- XenAdmin/Commands/InstallToolsCommand.cs | 16 ++-------- XenModel/Actions/VM/InstallPVToolsAction.cs | 34 +++++++++------------ XenModel/Network/XenConnection.cs | 5 +-- XenModel/XenAPI-Extensions/VDI.cs | 13 +++----- 4 files changed, 22 insertions(+), 46 deletions(-) diff --git a/XenAdmin/Commands/InstallToolsCommand.cs b/XenAdmin/Commands/InstallToolsCommand.cs index d6c6ddd5a..fe8ab6c2e 100644 --- a/XenAdmin/Commands/InstallToolsCommand.cs +++ b/XenAdmin/Commands/InstallToolsCommand.cs @@ -150,7 +150,7 @@ namespace XenAdmin.Commands DialogResult dr = new InstallToolsWarningDialog(vm.Connection).ShowDialog(Parent); if (dr == DialogResult.Yes) { - InstallPVToolsAction installToolsAction = new InstallPVToolsAction( vm, XSToolsSRNotFound, Properties.Settings.Default.ShowHiddenVMs); + var installToolsAction = new InstallPVToolsAction(vm, Properties.Settings.Default.ShowHiddenVMs); installToolsAction.Completed += InstallToolsActionCompleted; installToolsAction.RunAsync(); @@ -160,18 +160,6 @@ namespace XenAdmin.Commands return null; } - private static void XSToolsSRNotFound() - { - Program.Invoke(Program.MainWindow, delegate - { - using (var dlg = new ThreeButtonDialog( - new ThreeButtonDialog.Details(SystemIcons.Error, Messages.XS_TOOLS_SR_NOT_FOUND, Messages.XENCENTER))) - { - dlg.ShowDialog(Program.MainWindow); - } - }); - } - /// /// Attempts to install tools on several VMs /// @@ -225,7 +213,7 @@ namespace XenAdmin.Commands { foreach (VM vm in vms) { - InstallPVToolsAction installToolsAction = new InstallPVToolsAction(vm, XSToolsSRNotFound, Properties.Settings.Default.ShowHiddenVMs); + var installToolsAction = new InstallPVToolsAction(vm, Properties.Settings.Default.ShowHiddenVMs); if (vms.IndexOf(vm) == 0) { diff --git a/XenModel/Actions/VM/InstallPVToolsAction.cs b/XenModel/Actions/VM/InstallPVToolsAction.cs index 96f489fca..50eaff40c 100644 --- a/XenModel/Actions/VM/InstallPVToolsAction.cs +++ b/XenModel/Actions/VM/InstallPVToolsAction.cs @@ -30,6 +30,7 @@ */ using System; +using System.Linq; using XenAPI; @@ -41,16 +42,15 @@ namespace XenAdmin.Actions public class InstallPVToolsAction : AsyncAction { private static readonly log4net.ILog log = log4net.LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType); - private readonly Action _xsToolsNotFound; private readonly bool _searchHiddenIsOs; - public InstallPVToolsAction(VM vm, Action xsToolsNotfound, bool searchHiddenISOs) + public InstallPVToolsAction(VM vm, bool searchHiddenISOs) : base(vm.Connection, string.Format(Messages.INSTALLTOOLS_TITLE, vm.Name)) { VM = vm; - _xsToolsNotFound = xsToolsNotfound; _searchHiddenIsOs = searchHiddenISOs; -#region RBAC Dependencies + + #region RBAC Dependencies foreach (SR sr in VM.Connection.Cache.SRs) { if (sr.IsToolsSR && sr.IsBroken()) @@ -63,7 +63,7 @@ namespace XenAdmin.Actions ApiMethodsToRoleCheck.Add("vbd.eject"); ApiMethodsToRoleCheck.Add("vbd.insert"); ApiMethodsToRoleCheck.AddRange(Role.CommonSessionApiList); -#endregion + #endregion } protected override void Run() @@ -75,12 +75,12 @@ namespace XenAdmin.Actions { try { - SrRepairAction action = new SrRepairAction(sr.Connection, sr,false); + SrRepairAction action = new SrRepairAction(sr.Connection, sr, false); action.RunExternal(Session); } - catch (Failure) + catch (Failure f) { - _xsToolsNotFound(); + throw new Failure(Messages.XS_TOOLS_SR_NOT_FOUND, f); } } @@ -98,17 +98,14 @@ namespace XenAdmin.Actions XenAPI.VBD cdrom = VM.FindVMCDROM(); if (cdrom == null) { - Description = Messages.INSTALLTOOLS_COULDNOTFIND_CD; - return; + throw new Failure(Messages.INSTALLTOOLS_COULDNOTFIND_CD); } // Find the tools ISO... XenAPI.VDI winIso = findWinISO(_searchHiddenIsOs); if (winIso == null) { - // Could not find the windows PV drivers ISO. - Description = Messages.INSTALLTOOLS_COULDNOTFIND_WIN; - return; + throw new Failure(Messages.INSTALLTOOLS_COULDNOTFIND_WIN); } Description = Messages.INSTALLTOOLS_STARTING; @@ -122,7 +119,7 @@ namespace XenAdmin.Actions // Insert the tools ISO... XenAPI.VBD.insert(Session, cdrom.opaque_ref, winIso.opaque_ref); - // done(ish)... + // done here; installation continues on the VM Description = Messages.INSTALLTOOLS_DONE; } @@ -134,11 +131,10 @@ namespace XenAdmin.Actions { if (XenAPI.SR.SRTypes.iso.ToString() == sr.content_type) { - foreach (VDI vdi in Connection.ResolveAllShownXenModelObjects(sr.VDIs, searchHiddenISOs)) - { - if (vdi.IsToolsIso) - return vdi; - } + var vdis = Connection.ResolveAllShownXenModelObjects(sr.VDIs, searchHiddenISOs); + var vdi = vdis.FirstOrDefault(v => v.IsToolsIso); + if (vdi != null) + return vdi; } } diff --git a/XenModel/Network/XenConnection.cs b/XenModel/Network/XenConnection.cs index 780447142..c70105cb5 100644 --- a/XenModel/Network/XenConnection.cs +++ b/XenModel/Network/XenConnection.cs @@ -1932,10 +1932,7 @@ namespace XenAdmin.Network public List ResolveAllShownXenModelObjects(List> xenRefs, bool showHiddenObjects) { List result = ResolveAll(xenRefs); - result.RemoveAll(delegate(VDI vdi) - { - return !vdi.Show(showHiddenObjects); - }); + result.RemoveAll(vdi => !vdi.Show(showHiddenObjects)); return result; } diff --git a/XenModel/XenAPI-Extensions/VDI.cs b/XenModel/XenAPI-Extensions/VDI.cs index b3e63559b..3ece308e4 100644 --- a/XenModel/XenAPI-Extensions/VDI.cs +++ b/XenModel/XenAPI-Extensions/VDI.cs @@ -302,20 +302,15 @@ namespace XenAPI } /// - /// Whether this is a Tools ISO. Finds the old method (by name) as well as - /// the new method (field on the VDI). + /// Whether this is a Tools ISO. + /// The new method is to check the is_tools_iso flag, the old one to check the name_label. /// public bool IsToolsIso { get { - const string ISONameOld = "xswindrivers.iso"; - const string ISONameNew = "xs-tools.iso"; - - return - is_tools_iso || - ISONameOld.Equals(name_label) || - ISONameNew.Equals(name_label); + string[] toolIsoNames = {"xswindrivers.iso", "xs-tools.iso", "guest-tools.iso"}; + return is_tools_iso || toolIsoNames.Contains(name_label); } }