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

Showing output automatically upon completion and animated icon #578

Merged
merged 4 commits into from
Jun 15, 2024

Conversation

erwindon
Copy link
Owner

@erwindon erwindon commented May 9, 2024

Is your feature request related to a problem? Please describe.
I find it annoying when I'm waiting for a state/highstate to complete but all I get is a little reload icon color to see if it's done.
image

Describe the solution you'd like
I think the arrow icon should rotate or be animated when running and then, instead of changing to green or red based on the result, it should simply display the output automatically without having to click on it...

Describe alternatives you've considered
Any kind of animated icon that shows it is still in progress, but for showing the output, I don't see why it wouldn't be that way. There is a callback making the icon change color so it should probably be simple enough to make the call to show the output. Something that is always wanted.

It could be an option in the advanced section as well to please everyone.

Additional context
I'm using SaltGUI on a lot of minions and the less click I do, the happier I am! This is clearly a polishing feature, but those features could make this tool production ready.

Thanks a lot!

@erwindon
Copy link
Owner

@gseguinbourgeois
The salt system has a few limitations in this area.

  1. it is far too costly to update the job-display on a "ret" event from every minion that was involved in the operation; and
  2. by the time you switched to this view, already a few events may have been missed; and
  3. there is no "job-finished" event. So we must "count" the events to get an indication on when the job is finished. which is hard due to the previous argument.

but...
did you start the state/highstate command from SaltGUI or from the commandline?
when you start it from SaltGUI, please try to run the command with "async" by using the small button right of "Run command".
this will start the job in the background (just like salt --async) and immediately start monitoring it.
this will have a more useful view of the progress of a highstate job.
even more when you enable setting state_events: True in the main configuration file.
please let me know whether this is already sufficient...

@erwindon
Copy link
Owner

erwindon commented May 8, 2024

@gseguinbourgeois ping

@gseguinbourgeois
Copy link
Author

I tried looking at the code to understand why and when the little loading icon changed color. Not sure it has to do with salt events.
Now that the Ctrl+Click opening new tabs is there, I am used to open a couple of tabs at the same time and wait for them to be ready to show something. Usually, it's either the highstate or a really long state that I run on only one minion at a time.

I am always running commands from the commandline and I tried using --async. I am not sure what good it brings.
This is obviously only a nice to have feature and I though it would be follow the same code path as the color changing icon...
If it's too much trouble, I can live with that.

Thanks for your replies.

@erwindon
Copy link
Owner

erwindon commented May 9, 2024

Not sure it has to do with salt events.

correct, it does!

SaltGUI uses events for the following parts:

  • monitor updates for jobs in the jobs-side-panel
  • monitor updates for jobs in the jobs-main-panel/page
  • monitor updates for a job in the job-panel/page
  • monitor updates for jobs that were started asynchronously in the command-popup-panel
  • monitor updates for beacons on the beacons-per-minion-panel/page
  • monitor events on the events-panel/page
  • monitor key-updates on the keys-panel/page
  • monitor multi-master/syndic info on the keys-panel/page

for job monitoring, there are a few types of events available:

  • for the start of a job (1 event for all minions together)
  • for the finish of a job (1 event per minion)
  • for the finish of a highstate task (1 event per task, only when enabled in the 'master' salt settings)

unfortunately, there is no "end-of-job-for-all-minions" that we can use to refresh a whole page just once.
the best we can do with these events is to refresh the page for each finished minion.
but that will put a strain on the number of api-calls and on the user's eyes.

when you start a salt-command using SaltGUI you can select to run it asynchronously too.
in that case SaltGUI starts monitoring the events for that job that it just started.
the screenshot below is of such a job,

afbeelding
the hourglass indicates that a job for that minion has not finished yet

afbeelding
same job, but a few seconds later when all minions have finished.

@gseguinbourgeois
Copy link
Author

I think what I'm talking about refers to this:

// update all finished jobs (page)

Instead of showing a tooltip saying "click to refresh", why not refresh directly?
All of this code could be more proactive IMHO:
image

I hope this will help...

@erwindon erwindon assigned erwindon and unassigned gseguinbourgeois May 9, 2024
@erwindon
Copy link
Owner

erwindon commented May 9, 2024

@gseguinbourgeois
my explanation above had 2 purposes:

  1. to show an alternative (but only for jobs started within SaltGUI); and
  2. explain that processing of events is not useful for this use-case.

That piece of code is part of function _handleRunnerJobsActive, which processes the response from salt function (runner) jobs.active. That function would have to be repeated until the job/jobs are indeed finished.

With all that information combined there are now 2 solutions available:
A) use a plain timer to update any job-info for jobs that were previously running. It may take a few seconds beyond the end of the job because the timer has no knowledge about the actual (per-minion) job-completion moment.
B) use event-processing to update the job info when a minion has completed the job. This must use a throttling-mechanism because there may be a lot of minions involved, and we do not want an API-storm.

Currently, I'm thinking of using (A) with a fixed interval of 5 seconds when used. A play/stop button will be added to the relevant screens. The timer will be cancelled when the job (or all jobs) are finished. The state of the play/stop button will be remembered within the session. So only the first time you need to activate it.

I've converted this issue into a (still empty) PR. You'll see some code appear there later. I may ask you a few times to try it.

@erwindon erwindon marked this pull request as draft May 9, 2024 20:33
@gseguinbourgeois
Copy link
Author

Thank you for taking the time to give all this information.
It is really appreciated.

Option A seems to answer the need even though we know an event based solution would be cleaner.
The play/pause button already exists in other panels so I guess it fits the current interface.

I will be following this with much excitement!
Cheers!

@erwindon
Copy link
Owner

There are a few places where jobs are handled:

  • side-panel: no timer logic will be added there (neither new-jobs nor updated-jobs);
  • highstate-page: no timer logic will be added there (neither new-jobs nor updated-jobs);
  • issues-page: this page detects long-running jobs, not useful to add timer logic here;
  • jobs-page: no timer logic will be added there for new-jobs; the existing timer-logic may be expanded to include updated-jobs;
  • job-page: timer-logic will be added here.

@erwindon
Copy link
Owner

@gseguinbourgeois
I've added a first implementation of the auto-refresh for a JOB page.
At this moment, the refresh function is always active. I will add the start/stop button to control that later.
Can you please try the code from this branch?
I sometimes use force-push in git, so please use git pull -r to fetch any updates.
The browser-console shows how the update process is progressing. I'll remove that later.
I tested this with 6 minions and using the salt-command test.rand_sleep 60 on all minions (i.e. target=*).

please let me know whether this is the intended behavior and whatever suggestions you still have for this.

@erwindon
Copy link
Owner

@gseguinbourgeois
See also the previous comment...
I've now added a play/pause-button to the Job-page. It is visible only while the job is still running.
The last value is remembered across login-session.

While working on this, I found a general problem in SaltGUI for the use of timers.
Some timers are not de-activated when leaving a page, but there are again(!) started when arriving on that same page.
That may result in 2 (or more) timers running on the same page, depending on how quickly that page is revisited.
Since it is an existing problem, I've created a separate GitHub issue for it: #583

(again:) please let me know whether this is the intended behavior and whatever suggestions you still have for this.

@erwindon
Copy link
Owner

@gseguinbourgeois
Do you have the opportunity to test the software from this branch?
You may spot issues that I missed, and I want to make sure that I did not deviate too much from the original intention...

@erwindon erwindon force-pushed the auto-update-jobinfo branch 2 times, most recently from 3178f4e to 849d247 Compare May 25, 2024 13:11
@erwindon
Copy link
Owner

(note for historic purposes)

I think the arrow icon should rotate or be animated when running

In SaltGUI we hardly use any images and characters do not rotate.
The only images that we use are:

  • the OS images in the column "OS version";
  • the github logo on the login panel;
  • the indicator for links that are external;
  • the favicon (typically in the address bar of the browser).

@erwindon erwindon marked this pull request as ready for review May 25, 2024 13:29
@gseguinbourgeois
Copy link
Author

Animation is just a nice to have and I understand that the arrow could be embedded data or a unicode character.

I tried the branch and wasn't able to see a change in this view:
https://salt-dev/?id=20240528153624658669#job

First I see "(loading)", then I see:
326988488-a432b824-2b48-43d7-ab99-939823d4a169
then the arrow becomes green after a while.

When I click the green arrow, I have the whole result.

I am using command lines not from the Salt GUI. I use this tool only to monitor what is going on so "Manual Run" popup is not really an option for me. Not really using the https://salt-dev/#highstate view either.

Copy link

sonarcloud bot commented May 30, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@erwindon
Copy link
Owner

I tried the branch and wasn't able to see a change in this view: [...]
For a finished job, there is indeed no difference visible.

But for a job that is still running, a play/pause button will be added to the panel.
Initially, the status is "paused" (like the previous behavior), but it can be clicked to change to "play".
In that case, the job view is automatically updated when there was a change. it checks every 5s for that.
This "pause/play" status will be remembered, so that it acts automatically a next time.

You may need to refresh the browser cache. typically CTRL-F5 does that (but simple F5 does not)

@erwindon
Copy link
Owner

erwindon commented Jun 6, 2024

ping?

@erwindon erwindon assigned gseguinbourgeois and unassigned erwindon Jun 7, 2024
@rcmoutinho
Copy link

I remember this working as expected. Are we missing any tests here?

@erwindon
Copy link
Owner

@rcmoutinho

I remember this working as expected

that is my impression too

"I tried the branch and wasn't able to see a change in this view:[...]"

this is what @gseguinbourgeois last wrote. I provided a little bit of extra information and I was awaiting retest-outcome. hence my "ping".

I always try to have all PRs that I create (and that are based on a request done here) to be verified by the original requestor. This verifies again that there are no technical problems and also confirms that the solution matches the original request.
for this change I'm now at stage that I appreciate confirmations from others too...

@rcmoutinho
Copy link

It took me a while to see the Play button on the top of the job item page (not used to it). I wasn't able to confirm it's working as expected. A long highstate process that I tested shows as done although it's still running (so the play button is gone). Not sure what could be happening in this case.

@erwindon
Copy link
Owner

@rcmoutinho

A long highstate process that I tested shows as done although it's still running (so the play button is gone). Not sure what could be happening in this case.

Highstate jobs are expected to behave the same as any other type of job.
Since I tested only with utils.rand_sleep, I will test that scenario now.

@erwindon
Copy link
Owner

@rcmoutinho
I tested SaltGUI with highstate command and found no special behavior.
SaltStack (and thus SaltGUI) does not return a "done" status before a job is really done.

@erwindon erwindon merged commit 2cfc7a8 into master Jun 15, 2024
4 checks passed
@erwindon erwindon deleted the auto-update-jobinfo branch June 15, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants