CP-44171: XenCenter: Add SNMP page #3254

YizhouChen1998 · 2023-11-08T02:42:39Z

No description provided.

DeliZhangX · 2023-11-08T03:39:09Z

As we discussed, remember to change UI

Authentication Passphrase --> Authentication Password
Privacy Passphrase--> Privacy Password
Security Name --> User Name

See details from CA-384958.

YizhouChen1998 · 2023-11-13T09:02:09Z

As we discussed, remember to change UI

Authentication Passphrase --> Authentication Password

Privacy Passphrase--> Privacy Password

Security Name --> User Name

See details from CA-384958.

finished and committed

BengangY · 2023-11-13T08:27:42Z

XenModel/Actions/SNMP/SnmpUpdateAction.cs

+                        privacy_protocol = _snmpConfiguration.PrivacyProtocol,
+                    }
+                };
+            }


There are some redundant codes for these three if statements. Can we combine these three if statements into one?

BengangY · 2023-11-13T08:28:49Z

XenModel/Actions/SNMP/SnmpUpdateAction.cs

+            {
+                {"config", new JavaScriptSerializer().Serialize(configArgObj)}
+            };
+            var resultJson = Host.call_plugin(o.Connection.Session, o.opaque_ref, SnmpXapiConfig.XAPI_SNMP_PLUGIN_NAME,


resultJson is not used. Can remove it, or record some logs.

BengangY · 2023-11-13T08:30:09Z

XenModel/Actions/SNMP/SnmpUpdateAction.cs

+                        authentication_password = _snmpConfiguration.AuthPass,
+                        authentication_protocol = _snmpConfiguration.AuthProtocol,
+                        privacy_password = _snmpConfiguration.PrivacyPass,
+                        privacy_protocol = _snmpConfiguration.PrivacyProtocol,


Remove trailing comma.

BengangY · 2023-11-13T08:35:03Z

XenModel/Actions/SNMP/SnmpUpdateAction.cs

+using System.Web.Script.Serialization;
+using System.Collections.Generic;
+using System.Linq;
+using System.Dynamic;


Remove unnecessary packages.

BengangY · 2023-11-15T01:17:59Z

XenModel/Messages.resx

+    <value>Privacy password is invalid</value>
+  </data>
+  <data name="SNMP_ALLOW_SECURITY_TITLE" xml:space="preserve">
+    <value>Security name is invalid</value>


Security name -> User Name

BengangY · 2023-11-15T01:40:07Z

XenModel/Actions/SNMP/SnmpConfiguration.cs

+
+        public bool Equals(SnmpConfiguration other)
+        {
+            return !(IsSnmpEnabled != other.IsSnmpEnabled ||


It's more clear to use equal than to use not equal. Can be replaced like this:

return (IsSnmpEnabled == other.IsSnmpEnabled && IsLogEnabled == other.IsLogEnabled && ...

BengangY · 2023-11-15T01:41:28Z

XenModel/Actions/SNMP/SnmpConfiguration.cs

+        public SnmpConfiguration() { }
+
+
+        public object Clone()


Not used, can be removed. ICloneable needs not to be implemented.

XenModel/Actions/SNMP/SnmpUpdateAction.cs

BengangY · 2023-11-15T02:07:16Z

XenModel/Actions/SNMP/SnmpUpdateAction.cs

+            var configArgObj = GetConfigArgObj();
+            Dictionary<string, string> configArgDict = new Dictionary<string, string>
+            {
+                {"config", new JavaScriptSerializer().Serialize(configArgObj)}


The value of config is a JSON string. Can we use JsonSerializer to serialize the JSON object?

BengangY · 2023-11-15T02:12:58Z

XenAdmin/SettingsPanels/SnmpEditPage.Designer.cs

+        private GroupBox GeneralConfigureGroupBox;
+        private TableLayoutPanel GeneralConfigTableLayoutPanel;
+
+        private CheckBox EnableSnmpCheckBox;


The height of EnableSnmpCheckBox, DebugLogCheckBox, and SupportV3CheckBox is different. Please adjust them.

YizhouChen1998 · 2023-11-15T09:21:23Z

I have resolved and recommitted the issue you mentioned above. @BengangY

kc284

These are the first comments I have from reading the code (plus the layout tweaks in that patch file I sent you, you can also see it here kc284@4f4a7ff).
I may have some more comments later after I set up a machine and try out the code, but in the mean time you can work on the current stuff.

kc284 · 2023-11-15T14:53:31Z

XenAdmin/Dialogs/PropertiesDialog.cs

@@ -317,6 +318,11 @@ private void Build()
                        dialog.ShowDialog(Program.MainWindow);
                    }
                }
+                if (isPoolOrStandalone)


The page should not be visible at all for servers that don't have the plugin installed. Showing the error "retrieving the SNMP configuration failed" is misleading, especially for LCM servers where SNMP is not supported. We need a version check. Helpers.NileOrGreater, for example, excludes LCM servers. However, it is not enough because it does not exclude stream servers where updates have not been installed yet. We need a plugin version check (or a xapi version check as a compromise; we did this for NRPE).

Also, to keep a uniform behaviour with NRPE, the page should not be visible for users that are not pool admins.

kc284 · 2023-11-15T15:52:31Z

XenModel/Messages.resx

+    <value>SNMP configuration</value>
+  </data>
+  <data name="SNMP_RETRIEVE_FAILED" xml:space="preserve">
+    <value>Failed to retrieve SNMP configuration, please check XenCenter logs.</value>


This contains the product brand, which causes problems for forks that do rebranding. We need to use Failed to retrieve SNMP configuration, please check {0} logs. and then call it in the code as string.Format(Messages.SNMP_RETRIEVE_FAILED, BrandManager.BrandConsole) - or a simpler solution is to say Failed to retrieve SNMP configuration, please check the application logs.
The same applies to the other messages below.

kc284 · 2023-11-15T16:04:44Z

XenModel/Messages.resx

+    <value>Failed to retrieve SNMP configuration, please check XenCenter logs.</value>
+  </data>
+  <data name="SNMP_UPDATE_ERROR1" xml:space="preserve">
+    <value>Common error, please check XenCenter logs for details.</value>


What kind of error is the "common error"? It sounds a bit vague.

kc284 · 2023-11-15T16:10:12Z

XenModel/Actions/SNMP/SnmpConfiguration.cs

+
+        public bool Equals(SnmpConfiguration other)
+        {
+            return (IsSnmpEnabled == other.IsSnmpEnabled &&


The parentheses are not needed. Missing null check: return other != null && ...

Also, very trivial issue, but could you please leave an empty line between the various class declarations so the code is easier to read (just use VS to auto-format the file).

kc284 · 2023-11-15T16:18:48Z

XenModel/Actions/SNMP/SnmpRetrieveAction.cs

+        private readonly IXenObject _clone;
+
+        public SnmpRetrieveAction(IXenObject host, SnmpConfiguration snmpConfiguration, bool suppressHistory)
+            : base(host.Connection, "Retrieving SNMP configuration", "Retrieving SNMP configuration", suppressHistory)


These strings here are shown on the UI, therefore we need to internationalize them, i.e. move them to Messages.resx.

kc284 · 2023-11-15T22:17:05Z

XenAdmin/SettingsPanels/SnmpEditPage.cs

+                if (SupportV2cCheckBox.Checked)
+                {
+                    var communityStr = CommunityTextBox.Text;
+                    if (!RegexCommon.Match(communityStr.Trim()).Success)


Shouldn't we be checking for null first? I.e. if (string.IsNullOrEmpty(communityStr) || !Regex...). Same in the other checks below.

kc284 · 2023-11-15T22:22:03Z

XenAdmin/SettingsPanels/SnmpEditPage.cs

+        }
+        private void EnableSNMPCheckBox_CheckedChanged(object sender, EventArgs e)
+        {
+            if (EnableSnmpCheckBox.Checked)


This if-else block can be replaced with:

DebugLogCheckBox.Enabled = EnableSnmpCheckBox.Checked; SnmpV2cGroupBox.Enabled = EnableSnmpCheckBox.Checked && SupportV2cCheckBox.Checked; etc.

kc284 · 2023-11-15T22:30:34Z

XenAdmin/SettingsPanels/SnmpEditPage.cs

+        {
+            get
+            {
+                _invalidParamToolTip.Dispose();


We don't need to dispose and reconstruct the tooltip every time, we can make it readonly and just hide/show it and change the text. Also, to decide on which control to show it you can use the Tag, but an alternative way would be to use HelpersGUI.ShowBalloonMessage(). Have a look in file GeneralEditPage.cs to see how that tooltip is used.

kc284 · 2023-11-15T22:39:29Z

XenAdmin/SettingsPanels/SnmpEditPage.cs

+        private string _invalidParamToolTipText = "";
+        private static readonly Regex RegexCommon = new Regex(@"^[a-zA-Z0-9-.\#@=:_]{6,32}$");
+        private static readonly Regex RegexEncryptTextBox = new Regex(@"^([a-zA-Z0-9-.\#@=:_]{8,32}|[*]{8})$");
+        private static bool _encryptTextBoxFlag = false;


Setting to false when declaring the field is not needed because it is the default value anyway. Also, why is it static?

kc284 · 2023-11-15T22:49:29Z

XenAdmin/SettingsPanels/SnmpEditPage.cs

+{
+    public partial class SnmpEditPage : UserControl, IEditPage
+    {
+        public string SubText => Messages.SNMP_EDIT_PAGE_TEXT;


Very trivial thing, but since you'll be making changes in this file anyway let's tidy up a bit:

Sort usings alphabetically.

Move the constructor after the private fields, and SubText and Image properties after the constructor.

Move SaveSettings closer to the other IEditPage implementation methods.

Leave a line between all methods so the code can breath a bit (lack of lines is normally a C++ style).

Signed-off-by: Konstantina Chremmou <[email protected]>

kc284

Apart from the above comment I have made some more changes here kc284@68a9b41 (I also sent you this patch privately), please try it out and let me know what you think.

kc284 · 2023-11-28T19:25:27Z

XenAdmin/Dialogs/PropertiesDialog.cs

@@ -317,6 +318,12 @@ private void Build()
                        dialog.ShowDialog(Program.MainWindow);
                    }
                }
+                if (isPoolOrStandalone && Helpers.XapiEqualOrGreater_23_18_0(connection)


23.18.0 is a very early version. In any case, @DeliZhangX mentioned they will be adding a feature flag, so we can now check that one instead of the xapi version (I would also add the Helpers.NileOrGreater check as an extra safety valve for older servers). See XenModel/XenAPI-Extensions/Host.cs for methods checking the feature flags.

YizhouChen1998 requested review from CitrixChris, danilo-delbusso and kc284 as code owners November 8, 2023 02:42

BengangY self-requested a review November 8, 2023 02:49

YizhouChen1998 force-pushed the private/yizhouc/CP-44171 branch from 3534862 to 6bcc4b4 Compare November 13, 2023 07:41

danilo-delbusso assigned YizhouChen1998 Nov 13, 2023

YizhouChen1998 force-pushed the private/yizhouc/CP-44171 branch from 6bcc4b4 to a96c493 Compare November 13, 2023 10:28

kc284 added ITE PR should be reviewed within this iteration and removed ITE PR should be reviewed within this iteration labels Nov 13, 2023

BengangY requested changes Nov 15, 2023

View reviewed changes

YizhouChen1998 force-pushed the private/yizhouc/CP-44171 branch from a96c493 to dd2bcc3 Compare November 15, 2023 09:17

kc284 requested changes Nov 15, 2023

View reviewed changes

kc284 added the needs updating A reviewer has requested changes label Nov 15, 2023

CP-44171: XenCenter: Add SNMP page

ee033f4

YizhouChen1998 force-pushed the private/yizhouc/CP-44171 branch from dd2bcc3 to ee033f4 Compare November 16, 2023 10:00

kc284 and others added 2 commits November 17, 2023 10:12

Layout tweaks.

c94dfcc

Signed-off-by: Konstantina Chremmou <[email protected]>

CP-44171: Resolve PR review comments from Tina

effc7fe

kc284 added updated Changes completed, please review and removed needs updating A reviewer has requested changes labels Nov 22, 2023

kc284 reviewed Nov 29, 2023

View reviewed changes

YizhouChen1998 changed the title ~~CP-44171: XenCenter: Add SNMP page~~ CP-44171: XenCenter: Add SNMP page(Not ready for review) Nov 30, 2023

danilo-delbusso added the do not review yet PR is still being drafted, do not review. label Nov 30, 2023

danilo-delbusso changed the title ~~CP-44171: XenCenter: Add SNMP page(Not ready for review)~~ CP-44171: XenCenter: Add SNMP page Nov 30, 2023

danilo-delbusso marked this pull request as draft November 30, 2023 10:33

YizhouChen1998 closed this Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-44171: XenCenter: Add SNMP page #3254

CP-44171: XenCenter: Add SNMP page #3254

YizhouChen1998 commented Nov 8, 2023

DeliZhangX commented Nov 8, 2023

YizhouChen1998 commented Nov 13, 2023

BengangY Nov 13, 2023

BengangY Nov 13, 2023

BengangY Nov 13, 2023

BengangY Nov 13, 2023

BengangY Nov 15, 2023

BengangY Nov 15, 2023

BengangY Nov 15, 2023

BengangY Nov 15, 2023

BengangY Nov 15, 2023

YizhouChen1998 commented Nov 15, 2023

kc284 left a comment

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 Nov 15, 2023

kc284 left a comment

kc284 Nov 28, 2023

CP-44171: XenCenter: Add SNMP page #3254

CP-44171: XenCenter: Add SNMP page #3254

Conversation

YizhouChen1998 commented Nov 8, 2023

DeliZhangX commented Nov 8, 2023

YizhouChen1998 commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YizhouChen1998 commented Nov 15, 2023

kc284 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kc284 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment