[JENKINS-72978] Remove executors widget from Dashboard and nodes page #9126

timja · 2024-04-04T22:39:42Z

Implements https://matrix.to/#/!HKutvjxPnajVyCNLhF:matrix.org/$kfLAa91tXfp6Gv10PgoY8i7GM3GgqfDG_pz39-nWsDQ?via=gitter.im&via=matrix.org&via=mozilla.org

Adds a replacement page with the same information. I thought about redesigning it but I didn't think it made sense to do in this pull request.

This could fairly easily be detached to a plugin if people really want this still.

Testing done

manually checked the widget doesn't exist anymore except on the computer page

Proposed changelog entries

JENKINS-72978, Removed executor status from dashboard, executor status can now be viewed from Manage Jenkins -> Nodes -> Status

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.

The Jira issue, if it exists, is well-described.
Options
Successfully updated the issue's project

There was an error updating the issue's project
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see [examples](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.

The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
Options
Successfully updated the issue's project

There was an error updating the issue's project
There is automated testing or an explanation as to why this change has no tests.

There is automated testing or an explanation as to why this change has no tests.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New public classes, fields, and methods are annotated with `@Restricted` or have `@since TODO` Javadocs, as appropriate.

New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New deprecations are annotated with `@Deprecated(since = "TODO")` or `@Deprecated(forRemoval = true, since = "TODO")`, if applicable.

New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New or substantially changed JavaScript is not defined inline and does not call `eval` to ease future introduction of Content Security Policy (CSP) directives (see [documentation](https://www.jenkins.io/doc/developer/security/csp/)).

New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
Options
Successfully updated the issue's project

There was an error updating the issue's project
For dependency updates, there are links to external changelogs and, if possible, full differentials.

For dependency updates, there are links to external changelogs and, if possible, full differentials.
Options
Successfully updated the issue's project

There was an error updating the issue's project
For new APIs and extension points, there is a link to at least one consumer.

For new APIs and extension points, there is a link to at least one consumer.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

Desired reviewers

@mention

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

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.

Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Changelog entries in the pull request title and/or **Proposed changelog entries** are accurate, human-readable, and in the imperative mood.

Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Proper changelog labels are set so that the changelog can be generated automatically.

Proper changelog labels are set so that the changelog can be generated automatically.
Options
Successfully updated the issue's project

There was an error updating the issue's project
If the change needs additional upgrade steps from users, the `upgrade-guide-needed` label is set and there is a **Proposed upgrade guidelines** section in the pull request title (see [example](https://github.com/jenkinsci/jenkins/pull/4387)).

If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
Options
Successfully updated the issue's project

There was an error updating the issue's project
If it would make sense to backport the change to LTS, a Jira issue must exist, be a _Bug_ or _Improvement_, and be labeled as `lts-candidate` to be considered (see [query](https://issues.jenkins.io/issues/?filter=12146)).

If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

NotMyFault · 2024-04-06T08:03:25Z

This is indeed pretty neat. Would it be useful, for a limited period of time, to display a link in the position of the current executor widget, that takes you to the new overview?

timja · 2024-04-06T16:17:12Z

This is indeed pretty neat. Would it be useful, for a limited period of time, to display a link in the position of the current executor widget, that takes you to the new overview?

I think changelog is probably ok, I've added a link to the nodes page for low privileged users as there was no way for them to get there now

mawinter69 · 2024-04-07T22:30:47Z

A major drawback is that the executors widget is now also removed from individual agents.

It is essential to know when looking at an agent to know if any executors are busy to decide if I can safely delete/disconnect the agent

mawinter69 · 2024-04-08T08:16:14Z

Maybe we can just remove the Factories for views ViewFactoryImpl and ComputerSet ComputerSetFactoryImpl, Can we then reuse the t:executors on the new status page? The main task of showing the running builds is done by executorCell.jelly files which can be found in core and durable-task-step plugin, so in order to redesign it might require changes in that plugin as well. (See also #8459)

timja · 2024-04-08T08:26:09Z

Maybe we can just remove the Factories for views ViewFactoryImpl and ComputerSet ComputerSetFactoryImpl

Makes sense

Can we then reuse the t:executors on the new status page?

What's the reason to reuse it on the status page? Its current setup assumes its in the sidepanel and likely needs some changes to make it fit in properly, I've done a minimal bit so that it looks okay for now though.

mawinter69 · 2024-04-08T09:12:36Z

If we keep the widget for computers and also have the status page we would duplicate the code. And the differences are minimal.
Using

    <l:main-panel>
      <l:app-bar title="${%Status}"/>
      <div id="side-panel">
      <t:executors/>
      </div>
    </l:main-panel>

also works now, just a bit different styling.

Another aspect is the auto refresh, which is missing (also doesn't work with my approach but only because the url to fetch the data is wrong)

timja · 2024-04-08T09:26:14Z

Another aspect is the auto refresh, which is missing (also doesn't work with my approach but only because the url to fetch the data is wrong)

Yeah I wasn't sure if we wanted that or not, can add it though if worth it. There's a smaller risk of people leaving it open forever and lots of polling updates.

also works now, just a bit different styling.

Yeah styling is a bit messed up though I had to make some minor changes to the markup to make it work like removing float:right and a couple of other things

mawinter69

One comment, not really a blocker but something to think about

core/src/main/resources/hudson/model/View/sidepanel.jelly

timja · 2024-04-15T13:05:49Z

/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!

daniel-beck · 2024-04-16T13:29:14Z

I'd like an opportunity to review :) Thanks!

daniel-beck · 2024-04-17T08:33:31Z

What's the intended level of done-ness of this PR? This looks like a reasonable first step and implements the high-level request, but doesn't go much further than that. For example

jenkins/core/src/main/java/hudson/model/View.java

Lines 158 to 161 in c660d27

 /** 

  * If true, only show relevant executors 

  */ 

 protected boolean filterExecutors;

is now obsolete and probably should be made @Deprecated, transient and Boolean.

Some less clear interaction issues:

It's difficult to get from a blocked build in the queue to the UI that allows fixing the blockage, now that the widgets aren't stacked right on top of each other. The hourglass icon is the only added UI element, and it just links to https://www.jenkins.io/redirect/troubleshooting/executor-starvation
It's unclear what the expected flow is for an admin to (wait to) restart Jenkins once it's safe to do so.

This could fairly easily be detached to a plugin if people really want this still.

IMO most of the information being presented should remain accessible to users who administer Jenkins. I don't think anyone cares about which # executor a build is running on, but the rest seems interesting. The widget on the computer view answers the question, which job is currently using this node; while the new Status page does the same across all nodes (like the widget did before) and e.g. tells admins how far a potential "quiet down" has been successful.

Thoughts about delivering this as a UI experiment that users can opt out of, just in case? (Then of course without removal of filterExecutors for now.)

(FTR I'm trying to find the time to look into moving the widget into a popup, like admin monitors, with some similarity to the redesign prototype.)

mawinter69 · 2024-04-17T15:25:15Z

Independent of this change I think the widget could be improved by not showing idle executors. I have a few agents that perform small jobs that usually take not more than 1 minute but sometimes they can also run much longer. So these agents have 15 executors but most of the time only 1 or 2 are busy if at all. The idle executors don't need to be shown here, instead we could add the # of busy/total executors after the agent name
Something like this:

I've seen Jenkins instances that have 3 or 4 agents with 500 executors. At times most are idle and then you need to scroll down a lot if you like to see some other agents

mawinter69 · 2024-04-17T15:41:34Z

What would be also helpful is if one could collapse the executors of a single agent like you can collapse the complete widget

daniel-beck · 2024-04-17T16:49:38Z

not showing idle executors

Basically what I mentioned as

I don't think anyone cares about which # executor a build is running on

The specific executor assignment wasn't even useful information when it was in the sidepanel, only used/total would be relevant IMO. But that's an easy followup (or even independent parallel) change to make.

mawinter69 · 2024-04-17T19:44:19Z

created #9177 with the idle executors removed

Remove executors widget

ef96587

timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Apr 4, 2024

timja requested review from daniel-beck and a team April 4, 2024 22:40

timja added the web-ui The PR includes WebUI changes which may need special expertise label Apr 4, 2024

timja added 3 commits April 6, 2024 16:20

Improvements

8868d87

Lint

0cc4438

More fixups

8915934

timja changed the title ~~Remove executors widget~~ [JENKINS-72978] Remove executors widget Apr 6, 2024

timja marked this pull request as ready for review April 6, 2024 16:31

timja added 4 commits April 8, 2024 20:44

Restore Computer executor

33d198b

Restore computer test

515c32f

Reduce diff

f80fb24

Fix copyright

9d2f50a

timja changed the title ~~[JENKINS-72978] Remove executors widget~~ [JENKINS-72978] Remove executors widget from Dashboard and nodes page Apr 8, 2024

timja requested a review from mawinter69 April 8, 2024 20:59

mawinter69 reviewed Apr 8, 2024

View reviewed changes

core/src/main/resources/hudson/model/View/sidepanel.jelly Outdated Show resolved Hide resolved

Update core/src/main/resources/hudson/model/View/sidepanel.jelly

4fb6202

mawinter69 approved these changes Apr 9, 2024

View reviewed changes

Merge branch 'master' into remove-executors

73860b6

NotMyFault approved these changes Apr 12, 2024

View reviewed changes

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 Apr 15, 2024

daniel-beck removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-72978] Remove executors widget from Dashboard and nodes page #9126

[JENKINS-72978] Remove executors widget from Dashboard and nodes page #9126

timja commented Apr 4, 2024 •

edited

Submitter checklist

Maintainer checklist

NotMyFault commented Apr 6, 2024

timja commented Apr 6, 2024

mawinter69 commented Apr 7, 2024 •

edited

mawinter69 commented Apr 8, 2024

timja commented Apr 8, 2024

mawinter69 commented Apr 8, 2024

timja commented Apr 8, 2024

mawinter69 left a comment

timja commented Apr 15, 2024

daniel-beck commented Apr 16, 2024

daniel-beck commented Apr 17, 2024 •

edited

mawinter69 commented Apr 17, 2024 •

edited

mawinter69 commented Apr 17, 2024

daniel-beck commented Apr 17, 2024

mawinter69 commented Apr 17, 2024

[JENKINS-72978] Remove executors widget from Dashboard and nodes page #9126

Are you sure you want to change the base?

[JENKINS-72978] Remove executors widget from Dashboard and nodes page #9126

Conversation

timja commented Apr 4, 2024 • edited

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

NotMyFault commented Apr 6, 2024

timja commented Apr 6, 2024

mawinter69 commented Apr 7, 2024 • edited

mawinter69 commented Apr 8, 2024

timja commented Apr 8, 2024

mawinter69 commented Apr 8, 2024

timja commented Apr 8, 2024

mawinter69 left a comment

Choose a reason for hiding this comment

timja commented Apr 15, 2024

daniel-beck commented Apr 16, 2024

daniel-beck commented Apr 17, 2024 • edited

mawinter69 commented Apr 17, 2024 • edited

mawinter69 commented Apr 17, 2024

daniel-beck commented Apr 17, 2024

mawinter69 commented Apr 17, 2024

timja commented Apr 4, 2024 •

edited

mawinter69 commented Apr 7, 2024 •

edited

daniel-beck commented Apr 17, 2024 •

edited

mawinter69 commented Apr 17, 2024 •

edited