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

Show dependency graph also for cloned/restarted jobs #4507

Merged
merged 7 commits into from
Feb 21, 2022

Conversation

Martchus
Copy link
Contributor

  • Do not blindly follow the clone chain to the latest job anymore
    • Display always the current job, even if it is not the latest clone
    • Avoid following the clone chain to the latest job for children if
      the child's clone is not a dependency of the parent job anymore
    • Avoid following the clone chain to the latest job for parents
  • Do not display (directly) chained children of jobs when the clone depth
    does not match the one from the current job to avoid dragging too many
    jobs into the tree
  • See https://progress.opensuse.org/issues/69976

assets/javascripts/test_result.js Show resolved Hide resolved
lib/OpenQA/WebAPI/Controller/Test.pm Outdated Show resolved Hide resolved
lib/OpenQA/WebAPI/Controller/Test.pm Outdated Show resolved Hide resolved
lib/OpenQA/WebAPI/Controller/Test.pm Outdated Show resolved Hide resolved
@Martchus
Copy link
Contributor Author

Martchus commented Feb 16, 2022

Just a few examples how it looks like. All graphs (except the last example) are real examples from our production instance.

A fully restarted dependency tree looks definitively as it should.
Original tree: grafik
Restarted tree: grafik


Partially restarted trees also look good most of the time.
Original tree: grafik
Where only the following jobs have been restarted: screenshot_20220216_141734

Btw, these are directly chained dependencies (and therefore the "parent chain" has been restarted and not only the child).


However, partially restarted trees can also look like this: screenshot_20220216_142013

In this case restarted children of another "root" job are dragged into the graph. So far I haven't found a good solution to avoid this (without impairing other cases). However, I don't consider it that bad. None of the parallel clusters is only shown half and it wouldn't drag any further chained children of the wrong "restart depth" into the graph.

The graph of the latest jobs is also not affected at all (so there's no regression): screenshot_20220216_142652


By the way, a partial restart plus another partial restart as described in #4498 (comment) looks like this:

The initial cluster is rendered exactly like one would expect it: screenshot_20220216_143228

This is the graph for the first clone of root1 (after root1 has been restarted without #4498 and root2 has been restarted separately in addition and then root1 has been restarted again with #4498): screenshot_20220216_143415

It looks a bit confusing but also shows the problem I was getting at in the other PR quite nicely (that child1 has been restarted twice). So I guess having it rendered this way isn't the worst.

The graph for the latest jobs looks still simple (so again, no regression): screenshot_20220216_144127


Maybe I can still tweak it further to simplify graphs of restarted jobs. However, considering those graphs weren't displayed before at all and graphs for the latest jobs are still as before I already consider this an improvement.

Of course I still need to adapt tests but I wanted to wait for feedback first.

mergify bot added a commit that referenced this pull request Feb 16, 2022
@okurz
Copy link
Member

okurz commented Feb 16, 2022

The example about "A fully restarted dependency" shows that in one case the "ppc64le-hmc-4disk" is coming first, and the other time at end. Can this be influenced by ordering before the data is rendered or is it really something non-predictable by the graphics rendering?

The part about "In this case restarted children of another "root" job are dragged into the graph" is a bit worrying and I don't really understand it but I would accept it for now. Rest looks fine.

@Martchus
Copy link
Contributor Author

About the first question: This is indeed inconsistent but unrelated to this change. The order in which nodes are added to the tree depends on the current job and I selected a different job in these screenshots. I suppose we could simply sort the nodes in the end, possibly even in JavaScript.

One thing I've forgotten to mention: The code should ensure that no nodes are added to the tree which are not somehow connected to other nodes. So all jobs being dragged into the graph should at least indirectly depend on the current job. (That's what the $clone->is_child_of($as_child_of) check is supposed to ensure.)

@Martchus Martchus force-pushed the dependency-graph branch 2 times, most recently from f1bf0d3 to 04630f5 Compare February 17, 2022 16:49
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #4507 (5c496bc) into master (767e5a3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4507   +/-   ##
=======================================
  Coverage   97.96%   97.97%           
=======================================
  Files         374      374           
  Lines       34063    34108   +45     
=======================================
+ Hits        33371    33416   +45     
  Misses        692      692           
Impacted Files Coverage Δ
lib/OpenQA/Schema/Result/Jobs.pm 98.40% <100.00%> (+0.02%) ⬆️
lib/OpenQA/WebAPI/Controller/Test.pm 94.54% <100.00%> (+0.16%) ⬆️
t/ui/16-tests_dependencies.t 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 767e5a3...5c496bc. Read the comment docs.

@Martchus Martchus marked this pull request as ready for review February 18, 2022 10:27
t/ui/16-tests_dependencies.t Outdated Show resolved Hide resolved
Martchus added a commit to Martchus/openQA that referenced this pull request Feb 18, 2022
So far the order of elements within the dependency tree could be different
depending on the current job. This change makes it consistent by sorting
the edges before passing them to the rendering library. (Sorting nodes does
not help and also does not seem to be required.)

See os-autoinst#4507 (comment)
Martchus added a commit to Martchus/openQA that referenced this pull request Feb 18, 2022
So far the order of elements within the dependency tree could be different
depending on the current job. This change makes it consistent by sorting
the edges before passing them to the rendering library. (Sorting nodes does
not help and also does not seem to be required.)

See os-autoinst#4507 (comment)
I suppose it makes sense to *not* switch tabs when clicking
on a node within the dependencies tab.
* Do not blindly follow the clone chain to the latest job anymore
    * Display always the current job, even if it is not the latest clone
    * Avoid following the clone chain to the latest job for children if
      the child's clone is not a dependency of the parent job anymore
    * Avoid following the clone chain to the latest job for parents
* Do not display (directly) chained children of jobs when the clone depth
  does not match the one from the current job to avoid dragging too many
  jobs into the tree
* See https://progress.opensuse.org/issues/69976
If a child hasn't been restarted as often as its parent it still makes
sense to show that child in the restarted parent's dependency tree like it
was the case before 87f9c47 (as tested by
`t/ui/16-tests_dependencies.t`).
Martchus added a commit to Martchus/openQA that referenced this pull request Feb 18, 2022
So far the order of elements within the dependency tree could be different
depending on the current job. This change makes it consistent by sorting
the edges before passing them to the rendering library. (Sorting nodes does
not help and also does not seem to be required.)

See os-autoinst#4507 (comment)
@mergify mergify bot merged commit ea8a408 into os-autoinst:master Feb 21, 2022
@Martchus Martchus deleted the dependency-graph branch February 21, 2022 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants