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

[JENKINS-72371] rework node monitor configuration #8719

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Nov 25, 2023

This PR reworks the way node monitors are configured. It ensures that also monitors which are set to ignored, can be configured.

Previously, there was a checkbox before each monitor, where the behaviour was not totally clear. It meant that the monitor is ignored but still collecting data and not disabled as one might think. But when the monitor was ignored any configuration was lost and default values were used. Due to this behaviour the Versions Node Monitor plugin implemented its own disconnect checkbox.

Before:
image

After:
image

See JENKINS-72371.

Testing done

Before:

  1. Go to computer/configure
  2. Set Value for Free Temp Space to 2 GiB and 3 GiB warning, apply and reload page
  3. Uncheck Free Space Space (the config fields are no longer visible) apply and reload page
  4. Check the file nodeMonitors.xml in JENKINS_HOME, there are the default values of 1GiB and 2 GiB warning.
  5. Check Free Space Space in the UI, the config fields reappear and contain the defaults

After:

  1. Go to computer/configure
  2. Set Value for Free Temp Space to 2 GiB and 3 GiB warning, apply and reload page
  3. Check Don't Mark agents temporarily offline for Free Space Space apply and reload
  4. Check the file nodeMonitors.xml in JENKINS_HOME, there values of 2GiB and 3GiB warning are still there
  5. The UI shows the configured values and not the defaults

Proposed changelog entries

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

This PR reworks the way node monitors are configured. It ensures that
also monitors which are set to ignored, can be configured.
Previously, there was a checkbox before each monitor, where the
behaviour was not totally clear. It meant that the monitor is ignored
but still collecting data and not disabled as one might think. But when
the monitor was disabled any configuration was lost and default values
were used.
@mawinter69 mawinter69 marked this pull request as ready for review November 25, 2023 17:25
@NotMyFault NotMyFault requested a review from a team November 27, 2023 19:36
@NotMyFault NotMyFault added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Nov 27, 2023
@NotMyFault NotMyFault requested review from a team December 1, 2023 21:14
@NotMyFault NotMyFault added needs-security-review Awaiting review by a security team member needs-ath-build Needs to run through the full acceptance-test-harness suite labels Dec 1, 2023
@Kevin-CB Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Dec 13, 2023
Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

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

Looks fine security wise.

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

looks neat

@NotMyFault NotMyFault requested a review from a team December 14, 2023 20:37
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

(Removed ready for merge as I see it was flagged as needing ATH)

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 15, 2023
@timja timja removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 15, 2023
timja added a commit to jenkinsci/acceptance-test-harness that referenced this pull request Dec 16, 2023
@timja timja added ath-successful This PR has successfully passed the full acceptance-test-harness suite and removed needs-ath-build Needs to run through the full acceptance-test-harness suite labels Dec 16, 2023
@timja
Copy link
Member

timja commented Dec 16, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 16, 2023
@NotMyFault NotMyFault merged commit 7258cad into jenkinsci:master Dec 18, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants