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

Abrt support #6189

Closed
wants to merge 8 commits into from

Conversation

@marusak
Copy link
Member

commented Mar 23, 2017

This PR implements design
https://github.com/cockpit-project/cockpit/wiki/Feature:-ABRT

This PR needs upstream Libreport (https://github.com/abrt/libreport), and newest ABRT. All of this will be in Fedora 26. However, if older ABRT or Libreport are installed or are not available at all no changes are visible for users.
If you want to test this PR now, you will need a few thinks:
abrt >= 2.8.0
libreport - must be build from upstream
satyr >= 0.22

It is also possible to use repository that I created in copr.
https://copr.fedorainfracloud.org/coprs/mmarusak/ABRT-for-cockpit-PR/

When everything installed you need to run:
service abrtd start
service abrt-ccpp stop
service abrt-journal-core start
ulimit -c unlimited
setenforce 0

All of these steps are only necessary because behaviour of Libreport is changing quite a bit from Fedora 26 onwards.

Then log in as root. (You need to be a sudo user to read problems. Otherwise none problems will be displayed.)

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2017

I was able to run this, but I couldn't get anything to show up under Problems.
Is there an easy way to generate a problem that will show up there?

@marusak

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2017

Yes, there are two easy ways how to do it.
The easiest is to install will-crash. Then you can run for example will-abort or will-segfault.
Or if you don't want to install anything, you can kill sleep for example.
sleep 10000m &
kill -SIGSEGV %%

@marusak

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2017

I see that 3 tests failed.
selenium/chrome and selenium/firefox failed due to test50ChangeTabLogs in test/avocado/selenium-base.py. This is my fault that I forgot to update test when I changed buttons in b1bc82b I will fix this test.
However test verify/fedora-25 timed out and I am not sure why.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2017

Unfortunately I am still unable to run this on Fedora 25. I tried all the steps above, but no success.
I'll try to ping you on irc when you're around again.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2017

Works great in general! Thank you so much for getting this together!
Mostly small styling nits from my side:

The header looks a bit too tall, and the text is a bit too big, compared to other places we use this pattern:
screenshot from 2017-03-29 15-11-15

Compare it to how it looks on the Storage page and the react examples page

The column that starts with "reason" starts a lot further down than the column that starts with "Directory". The left column needs to line up with the right one at the top:
screenshot from 2017-03-29 15-12-17

I get an errors along the lines of "Reporting was unsucessful. Try running reporter-ureport -d /var/spool/abrt/ccpp-2017-03-29-10:55:36-24549" when I try to Report an issue.
The error bar is probably better placed below Logs >> Entry and above the header box, so it doesn't break the layout.
screenshot from 2017-03-29 15-15-20

Even though I got an error that the reporting failed, the spinner still spins like it's trying to report.

Clicking another tab while the spinner is still going starts displaying the contents of that above the tab area for some reason.
screenshot from 2017-03-29 15-22-12

The expander and text is closer to the top than the bottom, and the rows are only expandable if clicking right on the text. I think it would be better if it was possible to expand and collapse by clicking anywhere in the row.
screenshot from 2017-03-29 15-26-30

Again, really nice work!
Certainly mergable from a UI standpoint once the above items are addressed.
Looking forward to see this in!

@andreasn andreasn added the needswork label Mar 29, 2017
@marusak marusak force-pushed the marusak:abrt_support branch 2 times, most recently from f1b9873 to 69d40bc Mar 30, 2017
@marusak

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

Thanks for the review. I addressed all of yours points and they are all fixed now.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2017

Thank you for the quick fixes!

I went over it again, and found a few, tiny issues more.

  • The delete icon is different from what we use in the rest of Cockpit, and what's recommended by Patternfly (see http://www.patternfly.org/styles/icons/). I would suggest to use pficon-remove (we use the name pficon-delete elsewhere in Cockpit it seems, but the style is the same...will fix that in the other places in Cockpit)

  • I get this dropdown with only one thing in it when I've successfully reported a problem.
    screenshot from 2017-03-31 20-06-16
    I would suggest to make this a link instead, as that communicates the outcome of the action a bit clearer (ie. that it will open a URL in the browser).

  • There is a bit of space between the tabs. Tried to figure out how it's different from what we use in other places in Cockpit, but haven't been able to nail it down quite.
    screenshot from 2017-03-31 20-10-29

  • The entire rows are clickable now. Awesome! The pointer is different depending on where you hover in the row still though. Needs a cursor: pointer; on :hover

Apart from those, everything looks and behaves as expected!

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2017

The entire rows are clickable now. Awesome! The pointer is different depending on where you hover in the row still though. Needs a cursor: pointer; on :hover

Realized a better solution is might be to give width: 100%; and display: block; to the a-element.
http://stackoverflow.com/questions/4861230/make-a-link-have-100-width

@garrett is better at this kind of stuff though. What do you think, Garrett?

@marusak marusak force-pushed the marusak:abrt_support branch 2 times, most recently from 17381ee to 645f147 Apr 1, 2017
@marusak

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2017

Great points.

The delete icon is different from what we use in the rest of Cockpit...

Fixed.

I get this dropdown with only one thing in it when I've successfully reported a problem.

That is on purpose. You can get more than one URL. There can be BZs for example.

There is a bit of space between the tabs.

https://www.patternfly.org/pattern-library/widgets/#tabs it seems you are non-standard from patternfly. I was not able to find out what you have different from patternfly. I will try more, but if somebody knows what is different I would be happy to learn that.

Needs a cursor: pointer; on :hover

Done. Only cursor: pointer on panel-heading was necessary. I also added grey colour of panel on hover. Reason why I have not used width: 100%; and display: block; to the a-element is that it did not cover the whole panel only the middle part (in vertical way).

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2017

it seems you are non-standard from patternfly

Indeed!
@garrett said he'll look into this. Can be fixed as a followup (either that this get fixed to be more like the rest of cockpit, or that the rest of cockpit gets fixed to be more like patternfly). So let's leave it like this for now.

That is on purpose. You can get more than one URL. There can be BZs for example.

Is there any support for that in this PR, or is it for future-proofing?
What other entrys can it have in there?
Will it usually be one, two or more entrys?

The dropdown seems to cause page-resize here.
screenshot from 2017-04-03 18-07-03
One workaround might be to override the dropdown-menu class sets. With something like 0 or auto might need an important flag too.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

Looking good. I noticed you recorded this video: https://www.youtube.com/watch?v=jYm7jt27X04

@marusak marusak force-pushed the marusak:abrt_support branch from 645f147 to 8331de0 Apr 4, 2017
@marusak

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2017

@andreasn

Is there any support for that in this PR, or is it for future-proofing?
What other entrys can it have in there?
Will it usually be one, two or more entrys?

As I was trying to answer your questions I realised that my approach was not ideal. There can be more than one entry. But the entry would be only available if you used other ABRT tools such as gnome-abrt or abrt-cli. When I was doing that, my main idea was that in future when reporting to BZ will be added, that the BZ link could show under that dropdown (Because ABRT does provide all URLs in one array). However I realised that more than 2 links will not be often if at all. And also it would be confusing for users to have two reporting button but results would show under one dropdown.
There are still some questions how the reporting in future should look like. When we decide to implement that, I will write all usecases and options and will ask you for you option. But that is not in this PR.
In short I dropped the dropdown menu and only 'Reported' clickable string is displayed as in https://raw.githubusercontent.com/cockpit-project/cockpit-design/master/crashes/crashes-journal.png . That also solved the problem with page-resize.

@stefwalter I have. It was mainly for my manager since he wanted to see as this integration is going. Any comments, ideas?

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

@marusak I'll let @andreasn make the judgement calls on the functionality and design.

But as far as the video. It's great that you recorded a video. We usually require a video or screenshot recorded for new user visible features before they get merged. That way we can include them in the (weekly) release notes.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

However I realised that more than 2 links will not be often if at all. And also it would be confusing for users to have two reporting button but results would show under one dropdown.
There are still some questions how the reporting in future should look like. When we decide to implement that, I will write all usecases and options and will ask you for you option. But that is not in this PR.

Makes sense. Thanks!

@garrett

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2017

I saw the video and heard the distracting background noise. (Was this recorded on an internal microphone on a laptop with high load and fans?) Since I taught myself how to use Kdenlive and Audacity recently, I figured I'd try to put them to use and see if I could help clean up the audio.

After downloading the video from YouTube, I opened it in Kdenlive, split out the audio track, processed the audio to remove the background noise and adjust the levels (somewhat), then merged it all back together in Kdenlive, and exported it. Then, of course, I uploaded it to YouTube.

New video, with cleaned-up audio @ https://www.youtube.com/watch?v=Uw7LIJpfLI0

This gave me some real-world usage of Kdenlive & Audacity before I get the videos from Andreas. It also tested my laptop to make sure it is set up properly to perform all these tasks.

Also, @marusak, I would be happy to give you the modified video file to replace yours, if you wish. 😄

@marusak

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2017

@garrett Yes, the noise was due to my laptop overheating and there was nothing I could do in the time of recording the video. I was not happy with the sound quality at all.
I am genuinely surprised how you could edit the noise out. Great work!
It would be very kind from you if you could send me the edited sound on my email mmarusak@redhat.com.
Thanks:)

var all_p = [entry['PROBLEM_DIR'], entry['PROBLEM_DUPHASH'], entry['PROBLEM_UUID']];
var p;
var known = false;
for (var i = 0; i < 3; i++) {

This comment has been minimized.

Copy link
@mvollmer

mvollmer Apr 6, 2017

Member

The "3" should be replaced with all_p.length, I'd say.

This comment has been minimized.

Copy link
@marusak

marusak Apr 10, 2017

Author Member

Fixed


for (var i = 0; i < lines.length - 1; i++) {
var line = lines[i].split(delimiter);
result += '<tr> <td style="test-align: right">' + line[0];

This comment has been minimized.

Copy link
@mvollmer

mvollmer Apr 6, 2017

Member

text-align

This comment has been minimized.

Copy link
@mvollmer

mvollmer Apr 6, 2017

Member

We can not use inline styles like this because of the Content-Security-Policy. Just use class="text-right".

This comment has been minimized.

Copy link
@marusak

marusak Apr 10, 2017

Author Member

Fixed

for (var i = 0; i < lines.length - 1; i++) {
var line = lines[i].split(delimiter);
result += '<tr> <td style="test-align: right">' + line[0];
result += '<td style="test-align: left">' + line[1];

This comment has been minimized.

Copy link
@mvollmer

mvollmer Apr 6, 2017

Member

class="text-left"

This comment has been minimized.

Copy link
@marusak

marusak Apr 10, 2017

Author Member

Fixed

@mvollmer

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

Random comment: The small journal panels (on the Network and Storage page, for example) wont show the "Problem" entries, I guess. This doesn't matter much right now since one can't do anything from those journal panels. We can better with them in general.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

Random comment: The small journal panels (on the Network and Storage page, for example) wont show the "Problem" entries, I guess. This doesn't matter much right now since one can't do anything from those journal panels. We can better with them in general.

Does that mean that if I get a crash on udisksd or smartd, those won't show up on the logs of the individual pages?

@mvollmer

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

This should have a integration test, but for that we need to test on Fedora 26. I think we can wait for that, since it's unlikely that the journal code will change a lot in the near future.

@mvollmer

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

Does that mean that if I get a crash on udisksd or smartd, those won't show up on the logs of the individual pages?

Correct.

@mvollmer mvollmer added the blocked label Apr 6, 2017
@mvollmer

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

Blocked on #6069

@garrett

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

@marusak: Sent! Check your inbox (@ redhat.com).

tab.append(d_btn);
tab.append(r_btn);

out.html(tab);

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 15, 2017

Member

out is a <table> and you can't put things like a <ul> in there. Maybe put it within a <caption>? It's probably best to put a number of <table>s into the <li>s, though.

if (elem in args){
val = args[elem][2];
c1.append(
$('<tr>').append(

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 15, 2017

Member

This puts a <tr> inside a <div>. I don't think that's allowed.

@marusak

This comment has been minimized.

Copy link
Member Author

commented May 15, 2017

The first table has different column width than the second (the one starting with "Directory"), and the first column in the first table is too narrow.
I think it would be good enough to just have one long table in the "Problem info" tab. Otherwise we should probably use a Patternfly/Bootstrap grid to get proper responsive behavior.

Making it one column is much easier. If you think that is good enough.

@garrett

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

It's also possible to use a bit of CSS to force wrapping when there isn't space for it.

Add word-break: break-all; to the appropriate value area in the CSS... and/or whitespace: nowrap; to the label CSS.

@mvollmer

This comment has been minimized.

Copy link
Member

commented May 16, 2017

I see wrong PIDs in the journal:

screenshot from 2017-05-16 14-29-44

Note how systemd-coredump reports a different PID than abrt. The abrt PID seems to come from the first crash of will_segfault.

This seems to be a bug in abrt itself, since the message in the journal is already wrong, not just how Cockpit renders it.

}
//Do not render unknown (deleted) problems
if (! known)
return "";

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 16, 2017

Member

Returning "" from render_line to omit a line doesn't seem to work. I can see things like two day headers right after each other with nothing in between.

screenshot from 2017-05-16 14-04-25

I think we should never filter out journal lines (other than indicated by the filter widgets). All of them need to be displayed.

Also, I think we should not change the countfor abrt journal lines since there is no hint that we now show a different kind of count. The existing count is only for identical journal lines that sit right next to each other, in order to compress the display. It's not a statistic of how often a certain event has happened.

If we want to change the service, I think it should be done outside of render_line, in format_entry.

This comment has been minimized.

Copy link
@marusak

marusak May 16, 2017

Author Member

So you think that if the same problem happens more than once, we should display first, second and all other occurrences? It may however look a bit weird when you filter out only problems and there will be the same line several times.

I cannot agree with you about the count though. I think it is quite an important information. You will definitely pay more attention if you see that something crashed several times.

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 17, 2017

Member

So you think that if the same problem happens more than once, we should display first, second and all other occurrences?

Yes. The journal is a time-line of events. We shouldn't mess with that by omitting entries. Let's say something bad happened on your server around 14:23 o'clock. You go to the journal to see what has happened around that time. If abrt has written something to the log around that time you want to see it. It should not be omitted because the same thing has also happened later at 16:15.

I cannot agree with you about the count though. I think it is quite an important information. You will definitely pay more attention if you see that something crashed several times.

The list of journal entries is not the place to show this kind of summary, I'd say. We can collect statistics from the journal and show them in a separate "Notifications" UI, in a very prominent place. We some old ideas about this here: https://github.com/cockpit-project/cockpit/wiki/Notifications, maybe abrt is the thing that can drive this.

This comment has been minimized.

Copy link
@marusak

marusak May 17, 2017

Author Member

Yes. The journal is a time-line of events....

Yeah, this makes a better sense.

The list of journal entries is not the place to show this kind of summary...

Ok than. The design specified that this should dispaly the problem count and I liked that idea. However, if you believe it is not a good feature, we can omit it easily.

... "Notifications" UI ...

I like this! When it gets implemented one day, it would be useful to inform about ABRT problems there.

This comment has been minimized.

Copy link
@andreasn

andreasn May 17, 2017

Contributor

In the design, I intended the counter only for things happening right after each other (to reduce noise from services that fails constantly and spits out lines), so it's consistent with how the rest of the logs page work. Sorry if the design was unclear there.

For situations where a crash happens, say, once a day, I feel it's important that you see that it's reoccurring on a predictable time-basis, and that the entry's for different days don't get collapsed down.

I agree with the idea of the notification area. It will be needed also for things like software updates being available and other critical things, so let's solve that as part of that.

@@ -31,6 +31,8 @@

var journal = { };

var displayable_problems = { };

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 16, 2017

Member

This global variable should be avoided. The displayable_problems is a parameter for journal.render so we should pass it directly to that function.

However, displayable_problems is not needed at all when we make the changes suggested below for render_line.

'last_occurrence', 'os_release', 'pkg_fingerprint', 'pkg_vendor',
'runlevel', 'tid', 'time', 'uid', 'uuid'];

var displayable_problems = {};

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 16, 2017

Member

This isn't kept up-to-date when new problems appear while the journal page is open, no?

I think we should not use any data from org.freedesktop.problems for rendering the journal itself (since it is currently only used to suppress certain journal lines, which I think we should never do), but use it only for the entry details page. When rendering that page, we can query explicitly for the problem data we need, without needing to keep a copy of all existing data in the browser. (Does the org.freedesktop.Problems2 API allow querying by ID or UUID?)

This comment has been minimized.

Copy link
@marusak

marusak May 16, 2017

Author Member

It is not. However if you decide that that we should display all logs - first, second, nth occurrence of the same problem, than we can drop this - probably. One of the purposes of this was to know the count of each problem so we could only render the last log.

'container', 'container_uuid', 'container_cmdline',
'container_id', 'container_image' ];

var problem_info_2 = ['Directory', 'username', 'abrt_version', 'architecture', 'global_pid', 'kernel',

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 16, 2017

Member

lastr_occurence and time are rendered as integers. We should render them as date/time strings.

// write into general tab non-ABRT related items
var keys = Object.keys(entry).sort();
$.each(keys, function(i, key) {
if (key !== 'MESSAGE' && !key.startsWith('PROBLEM_')) {

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 16, 2017

Member

startsWith is unfortunately not portable enough. You could use key.indexOf("PROBLEM_") == 0. Why do we filter out the PROBLEM_ fields, actually? I think it would be okay to keep them.

This comment has been minimized.

Copy link
@marusak

marusak May 16, 2017

Author Member

Because it contains some ABRT related information, that are better displayed in Problem info. I found it redundant.

function create_problem(out, entry) {
var problem;
var all_p = [entry['PROBLEM_DIR'], entry['PROBLEM_DUPHASH'], entry['PROBLEM_UUID']];
for (var i = 0; i < 3; i++) {

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 16, 2017

Member

3 -> all_p.length

@mvollmer

This comment has been minimized.

Copy link
Member

commented May 16, 2017

Here is a simple test: mvollmer@72c0fd8

It currently fails because displayable_problems starts out as empty and never gets updated with the new problem, as indicated in my other review comments.

@marusak

This comment has been minimized.

Copy link
Member Author

commented May 16, 2017

Here is a simple test: mvollmer/cockpit@72c0fd8

Thanks!

I am going to fix all of yours comments and then try this test. If it works, than I may try to add another tests.

However it may take a while since my final state examination is getting closer and I need to study for this.

@mvollmer

This comment has been minimized.

Copy link
Member

commented May 17, 2017

I am going to fix all of yours comments and then try this test.

Great! I have a pretty concrete idea of the changes that I would make, so I might work a bit on this while you study. :-)

Thanks a lot for the work so far!

@marusak

This comment has been minimized.

Copy link
Member Author

commented May 17, 2017

Great! I have a pretty concrete idea of the changes that I would make, so I might work a bit on this while you study. :-)

That would be awesome:) If you have anything, feel free to force push anytime.

Thank you for the reviews and help!

@@ -28,6 +28,71 @@ $(function() {
cockpit.translate();
var _ = cockpit.gettext;

var problems_client = cockpit.dbus('org.freedesktop.problems');

This comment has been minimized.

Copy link
@mvollmer

mvollmer May 19, 2017

Member

This should use { "superuser": "try" } to match how journalctl is run. If possible, this will then talk to the daemon as root and be able to see more problems.

marusak added 7 commits Mar 6, 2017
This commit makes it possible to filter only crashes caught by ABRT.

Firstly the button group for selecting priority was replaced by
dropdown list. This also meant rewriting the piece of code that set
'active' class to change text of selected priority level.

Secondly, when option `Only Problems` was selected another condition is
added to match only items that have syslog identifier equal to
`abrt-notification`.

Signed-off-by: Matej Marusak <mmarusak@redhat.com>
In the log overview display different icon for problems (logs with
        priority lower that 4 have a triangle with exclamation mark,
        ABRT's problems after this commit have cross mark).

Signed-off-by: Matej Marusak <mmarusak@redhat.com>
Since log entry from ABRT can contain quite some data and some of them
are rather long strings, three thing were done in this commit:
1. Split data to three categories - General, Problem Info and Problem
Details
2. Remove all "PROBLEM_" prefixes to make it more readable
3. Long items (in Problem details) show as accordion

Signed-off-by: Matej Marusak <mmarusak@redhat.com>
Signed-off-by: Matej Marusak <mmarusak@redhat.com>
Signed-off-by: Matej Marusak <mmarusak@redhat.com>
FAF is platform for collecting and analysis of crashes. This is first
step for reporting problems from Cockpit. Second step will be reporting
to Bugzilla.

When problem is reported, display clickable 'Reported' text which sends you
to FAF.
When problem is reportable, display button for reporting.

Signed-off-by: Matej Marusak <mmarusak@redhat.com>
Signed-off-by: Matej Marusak <mmarusak@redhat.com>
@marusak marusak force-pushed the marusak:abrt_support branch 3 times, most recently from 855ee91 to b0887a9 Jun 20, 2017
@marusak

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2017

I finally got back to this. Fixed all issues that @mvollmer mentioned above.
I also wrote integration tests.
@mvollmer what do you think of this now?

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

I triggered a run of the integration tests.

Signed-off-by: Matej Marusak <mmarusak@redhat.com>
@marusak marusak force-pushed the marusak:abrt_support branch from b0887a9 to 785c4e1 Jun 26, 2017
@martinpitt martinpitt removed the needswork label Jun 26, 2017
@mvollmer

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@mvollmer what do you think of this now?

Very nice!

@mvollmer mvollmer closed this in f0ffc99 Jun 28, 2017
@martinpitt martinpitt referenced this pull request Jul 5, 2017
1 of 1 task complete
@martinpitt

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

Some screenshots for the release notes:

abrt-list

abrt-details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.