Skip to content
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

CP-44372: Integrate NRPE UI with backend interface #3226

Merged
merged 10 commits into from
Oct 24, 2023

Conversation

BengangY
Copy link
Contributor

Added below use case on XenCenter

Enable/Disable NRPE for host
Configure allow_hosts /Enable Debug/Enable SSL_logging
Set threshold for each check
View existing settings for each host
Validation for the input
Same function for the pool

@danilo-delbusso danilo-delbusso added the ITE PR should be reviewed within this iteration label Sep 15, 2023
@CitrixChris CitrixChris removed the ITE PR should be reviewed within this iteration label Sep 20, 2023
@BengangY BengangY force-pushed the private/bengangy/CP-44371 branch 2 times, most recently from 6ac41a2 to 53fdfa0 Compare September 21, 2023 08:36
@kc284 kc284 added the ITE PR should be reviewed within this iteration label Sep 22, 2023
@CitrixChris CitrixChris removed the ITE PR should be reviewed within this iteration label Sep 25, 2023
@kc284
Copy link
Contributor

kc284 commented Sep 28, 2023

@BengangY I have made some corrections on a branch in my fork, could you please enable PRs on your fork so I can raise a PR to it. Alternatively you can just pull the top 3 commits from here https://github.com/kc284/xenadmin/commits/private/bengangy/CP-44371, but it is better if I could send a PR with comments to explain the changes.

This is one part of corrections, there are some more changes needed, but I didn't make them as they require greater code shifting so I'll leave it to you. Please see my other comments in this PR.

Copy link
Contributor

@kc284 kc284 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@kc284 kc284 added the needs updating A reviewer has requested changes label Sep 28, 2023
@kc284
Copy link
Contributor

kc284 commented Oct 4, 2023

A general remark: normally for new controls we would add localized resx placeholders, but at the moment we have stopped shipping localized resources on master, so the palceholders are not needed. This is the first PR with missing placeholders, so please ensure it is merged after #3238 which disables the unit tests.

@kc284 kc284 added the ITE PR should be reviewed within this iteration label Oct 5, 2023
@kc284 kc284 changed the base branch from master to feature/nrpe October 6, 2023 09:23
@danilo-delbusso danilo-delbusso added updated Changes completed, please review and removed needs updating A reviewer has requested changes labels Oct 12, 2023
Copy link
Member

@danilo-delbusso danilo-delbusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't done a complete review but noticed a couple of things while skim-reading

XenAdmin/Properties/AssemblyInfo.cs Outdated Show resolved Hide resolved
XenAdmin/SettingsPanels/NRPEEditPage.cs Outdated Show resolved Hide resolved
@kc284 kc284 added needs updating A reviewer has requested changes and removed updated Changes completed, please review labels Oct 17, 2023
XenAdmin/XenAdmin.csproj Outdated Show resolved Hide resolved
XenModel/Messages.resx Show resolved Hide resolved
XenModel/Messages.resx Outdated Show resolved Hide resolved
XenModel/Actions/NRPE/NRPEUpdateAction.cs Outdated Show resolved Hide resolved
XenModel/Actions/NRPE/NRPEUpdateAction.cs Outdated Show resolved Hide resolved
XenModel/Actions/NRPE/NRPERetrieveAction.cs Show resolved Hide resolved
XenModel/XenModel.csproj Outdated Show resolved Hide resolved
XenModel/Actions/NRPE/CheckGroup.cs Outdated Show resolved Hide resolved
XenModel/Actions/NRPE/CheckGroup.cs Outdated Show resolved Hide resolved
XenModel/Actions/NRPE/CheckGroup.cs Outdated Show resolved Hide resolved
@@ -317,6 +318,12 @@ private void Build()
dialog.ShowDialog(Program.MainWindow);
}
}
if (isPoolOrStandalone && Helpers.XapiEqualOrGreater_23_27_0(connection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is isPoolOrStandalone correct here? This means that if someone wants to set different settings for only one host in a multi-host pool, they cannot do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's correct. Have confirmed with Enzo that NRPE page will be shown for a pool and standalone host, not shown for a host in a multi-host pool.

@kc284
Copy link
Contributor

kc284 commented Oct 23, 2023

  • If I enter an invalid value in one of the threshold cells (e.g. 120), and click the OK button, I get an error (little red icon in the cell). If I then change the value to something valid (e.g. 70) and click on a different cell, the little red icon should disappear, but it's still there.
  • If I enter an invalid value in a cell in the last row, click to a different tab, then click the OK button, it takes me back to the NRPE page, which is correct, however, the row with the invalid value is out of view, one has to scroll down to see what's wrong. Ideally, the row containing the invalid cell should be selected or at least we should scroll into view. If there are more than one row with invalid cells, it should be at least the first invalid row in view.

@BengangY
Copy link
Contributor Author

BengangY commented Oct 23, 2023

  • If I enter an invalid value in one of the threshold cells (e.g. 120), and click the OK button, I get an error (little red icon in the cell). If I then change the value to something valid (e.g. 70) and click on a different cell, the little red icon should disappear, but it's still there.

Updated. Now it will check when the datagridview lost focus each time.

  • If I enter an invalid value in a cell in the last row, click to a different tab, then click the OK button, it takes me back to the NRPE page, which is correct, however, the row with the invalid value is out of view, one has to scroll down to see what's wrong. Ideally, the row containing the invalid cell should be selected or at least we should scroll into view. If there are more than one row with invalid cells, it should be at least the first invalid row in view.

Updated. Added a reselection process when error occurs. Code likes:

CheckDataGridView.CurrentCell = CheckDataGridView.Rows[0].Cells[0];
CheckDataGridView.CurrentCell = focusCellWhenErrorOccurs;

It will select the first row and then select the error row. The reason is that if it only selects the error row, if the user already selected the error row and scrolled down the datagrid and click to a different tab, then click the OK button, it will not focus on the error row. So it needs a switch process.

@kc284 kc284 added updated Changes completed, please review and removed needs updating A reviewer has requested changes labels Oct 23, 2023

public AsyncAction SaveSettings()
{
return new NRPEUpdateAction(_clone, _nrpeCurrentConfig, _nrpeOriginalConfig, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't spot this earlier: why are we suppressing the history? This means that there is not record of the action on the events TabPage. Same for line 195 and NRPERetrieveAction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually we do suppress this for more properties that we save. It probably needs revisiting at a higher level, so it is probably a no-action for this PR.

@kc284
Copy link
Contributor

kc284 commented Oct 24, 2023

One more question: I noticed that when we disable NRPE and then we renable it, the previous settings are reset and the user has to set up thresholds again. Is this by design? (Asking because sometimes features do remember the settings when disabled).

@kc284
Copy link
Contributor

kc284 commented Oct 24, 2023

I added a Thread.Sleep() for a few seconds within the Host.call_plugin() method to simulate what happens if, for example, we have a slow network and server requests take while to return. In this case, if I launch the dialog and click the NRPE page, I see an error pop-up:

Which means something is not working correctly if we click on the page while the NRPERetrieveAction is still in progress.

@BengangY
Copy link
Contributor Author

BengangY commented Oct 24, 2023

One more question: I noticed that when we disable NRPE and then we renable it, the previous settings are reset and the user has to set up thresholds again. Is this by design? (Asking because sometimes features do remember the settings when disabled).

By design, all the settings displayed in NRPE UI are retrieved from the host's configuration file when opening the dialog. If customers check 'enabled NRPE' and customize the setting, clicking 'OK' will rewrite the configuration file for each host, which means the previous settings won't be reset by following single-op like disable and re-enable. But there's actually one special case we can discuss, if customers modify thresholds together with checking 'disable NRPE', and then click the 'OK' button, the host will only disable the NRPE without rewriting the configuration file, so next time when enabling NRPE again, those changes won't be seen from UI.

@BengangY
Copy link
Contributor Author

I added a Thread.Sleep() for a few seconds within the Host.call_plugin() method to simulate what happens if, for example, we have a slow network and server requests take while to return. In this case, if I launch the dialog and click the NRPE page, I see an error pop-up:

Which means something is not working correctly if we click on the page while the NRPERetrieveAction is still in progress.

This is an issue caused by my removing the default values of the threshold in datagridview. I will fix it today.

@danilo-delbusso danilo-delbusso added 2 approvals PR has been approved by two reviewers and removed updated Changes completed, please review labels Oct 24, 2023
@kc284 kc284 merged commit 28cdead into xenserver:feature/nrpe Oct 24, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 approvals PR has been approved by two reviewers ITE PR should be reviewed within this iteration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants