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

kdump #5587

Merged
merged 8 commits into from Jan 25, 2017

Conversation

Projects
None yet
6 participants
@dperpeet
Copy link
Member

commented Dec 13, 2016

  • local filesystem dump target
  • test settings before writing config
  • get system service status of kdump correctly
  • detect if kdump is enabled at boot time, show hint how to enable if not
  • get reserved memory
  • toggle compression
  • integration tests
  • Video on Youtube

@dperpeet dperpeet added the bot label Dec 13, 2016

@dperpeet dperpeet force-pushed the dperpeet:kdump branch 6 times, most recently from b66a426 to ca31c09 Dec 14, 2016

@stefwalter stefwalter added the needswork label Jan 3, 2017

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from ca31c09 to 8f0dffd Jan 10, 2017

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Looks like a good start. A couple of css issues that I need to take a closer look at.

I tried to change to non-local storage, but got this:
screenshot from 2017-01-10 15-49-52

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

These tooltips looks different from each other.

screenshot from 2017-01-10 15-51-14

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Does this mean I have no reserved memory?

screenshot from 2017-01-10 15-54-15

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from 8f0dffd to d80a8a1 Jan 11, 2017

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2017

Does this mean I have no reserved memory?

Added a hint what to do in this case.

These tooltips looks different from each other.

Yes, there are still issues with the popover. The css needs to be changed.

I tried to change to non-local storage, but got this:

It is currently scoped down to support only local storage. The menu has an "other" entry because we don't want to overwrite those when writing the config.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

it seems the text being off between the two rows is due to label adding an extra 5px as margin-bottom

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

I think we worked around this in the network dialogs and elsewhere.

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from 68c3133 to 32471da Jan 11, 2017

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

.form-table-ct td:first-child label { margin-bottom: 0; }

Will fix the uneven rows

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

The misalignment in the location dialog seems to be due to <label> being around the <input id="kdump-settings-local-directory">

placeholder="/var/crash" then needs to be value="/var/crash" to not look weird

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

So with regards to the Other option in the dropdown. I'm still sceptical about the mere existence of this one, since if you're setting the location in kdump.conf, why not set the compression there as well?
But I'm interested in hearing what others think as well. @stefwalter @garrett

screenshot from 2017-01-12 18-08-24

If we keep it, I think it makes sense to put the text directly in the dropdown, so:
Location [ Use the setting in /etc/kdump.conf. / ]

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

I managed to get the checkbox under control with this html
<div class="checkbox"><label><input type="checkbox">Compress crash dumps to save space</label></div>

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from 32471da to e55dc1d Jan 13, 2017

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

I managed to get the checkbox under control with this html

Compress crash dumps to save space

This works, but the text with the checkbox is lighter now. I also added a margin to align the checkbox itself a bit better vertically.

.form-table-ct td:first-child label { margin-bottom: 0; }

Will fix the uneven rows

thanks, used

The misalignment in the location dialog seems to be due to being around the

placeholder="/var/crash" then needs to be value="/var/crash" to not look weird

changed

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from e55dc1d to ca1a0e0 Jan 13, 2017

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

There is a big white empty space on the right side of the dialog, so I think it would be a good idea to use the class cockpit-modal-md from system.css (is that global btw?) to make the dialog 400px wide.
screenshot from 2017-01-13 14-10-23

@dperpeet dperpeet force-pushed the dperpeet:kdump branch 2 times, most recently from b2a8fae to 158bfca Jan 17, 2017

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

OnOff switch related changes moved to #5752

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

rebased due to new fedora-24 image on master

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

I think this commit needs:

diff --git a/test/verify/naughty-fedora-25/5750-atomic-storage-reset-regressed b/test/verify/naughty-fedora-25/5750-atomic-storage-reset-regressed
new file mode 100644
index 0000000..1e9ead9
--- /dev/null
+++ b/test/verify/naughty-fedora-25/5750-atomic-storage-reset-regressed
@@ -0,0 +1,5 @@
+Traceback (most recent call last):
+  File "./verify/check-docker-storage", line *, in testDevmapperVGroup
+    self.testDevmapper("dockah")
+  File "./verify/check-docker-storage", line *, in testDevmapper
+    b.wait_not_present(".modal-dialog:contains(Add Additional Storage)")
@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Added a commit for the Fedora 25 issue.

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from 9116a74 to c65826a Jan 23, 2017

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

Added a commit for the Fedora 25 issue.

I had to change this a bit

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from c65826a to 6ad4bf3 Jan 23, 2017

@larskarlitski
Copy link
Contributor

left a comment

Nice work!

  1. I noticed that the "Crash dump location" spinner is running forever when kexec-tools is not installed. Clicking the "Loading" link gives an Oops in that case.

  2. Please add kexec-tools to the Vagrantfile as well, on line 45.

  3. I cannot toggle the "Compress crash dumps to save space". How can go about debugging this?

else
dataStore.kdumpMemory = cockpit.format_bytes(value*1024*1024);
} else {
dataStore.kdumpMemory = content.trim();

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jan 24, 2017

Contributor

Which values can be kexec_crash_size? I thought that there's always a value in bytes? Do we need the guessing the size then?

Note that parseInt("256M", 10) would also return 256.

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jan 24, 2017

Author Member

See my comment above, the size can be either in bytes or megabytes, but useful values are <1G generally. The output is a raw number without units, but from the size of the number we can guess the unit.

/*
* This file is part of Cockpit.
*
* Copyright (C) 2016 Red Hat, Inc.

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jan 24, 2017

Contributor

Happy new year!

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jan 24, 2017

Author Member

these files were created last year, so technically they're still accurate...

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

I noticed that the "Crash dump location" spinner is running forever when kexec-tools is not installed. Clicking the "Loading" link gives an Oops in that case.

That shouldn't happen, really, but I can look into a timeout. The cockpit-package depends on kexec-tools.

Please add kexec-tools to the Vagrantfile as well, on line 45.

ok

I cannot toggle the "Compress crash dumps to save space". How can go about debugging this?

Good catch, regression.

@larskarlitski

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

That shouldn't happen, really, but I can look into a timeout. The cockpit-package depends on kexec-tools.

Ah, in that case there's no need to add it to the Vagrantfile.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

I noticed that the "Crash dump location" spinner is running forever when kexec-tools is not installed. Clicking the "Loading" link gives an Oops in that case.

I cleaned up some related use cases and there was an actual bug in the way the config was being parsed. New behavior when kexec-tools isn't installed:

screenshot from 2017-01-24 17-59-17

And when the service is disabled, crashing the kernel for testing is now also disabled:
screenshot from 2017-01-24 17-59-22

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from 6ad4bf3 to e8d64e1 Jan 24, 2017

changes implemented

@larskarlitski

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

Thanks for the quick fixes @dperpeet!

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

The fedora-25 failure looks relevant?

# testBasic (check_kdump.TestKdump)
#
Created symlink /etc/systemd/system/multi-user.target.wants/kdump.service  /usr/lib/systemd/system/kdump.service.
Job for kdump.service failed because the control process exited with error code.
See "systemctl status kdump.service" and "journalctl -xe" for details.
Generating grub configuration file ...
Found linux image: /boot/vmlinuz-4.9.3-200.fc25.x86_64
Found initrd image: /boot/initramfs-4.9.3-200.fc25.x86_64.img
Found linux image: /boot/vmlinuz-4.8.6-300.fc25.x86_64
Found initrd image: /boot/initramfs-4.8.6-300.fc25.x86_64.img
Found linux image: /boot/vmlinuz-0-rescue-1a39617d347a459a988c278bfc248005
Found initrd image: /boot/initramfs-0-rescue-1a39617d347a459a988c278bfc248005.img
done
> Unable to apply settings: Directory /var/crash2 isn't writable or doesn't exist.
Page error: button.btn-default is disabled or somehow not clickable
phantomjs://code/phantom-lib.js 127 ph_click
 1 
 1 
Wrote TestKdump-testBasic-FAIL.png
Journal extracted to TestKdump-testBasic-10.111.122.112-FAIL.log
not ok 123 testBasic (check_kdump.TestKdump) duration: 112s
Traceback (most recent call last):
  File "./verify/check-kdump", line 110, in testBasic
    self.browser.click("button.btn-default")
  File "/build/cockpit/test/common/testlib.py", line 154, in click
    self.call_js_func('ph_click', selector, force)
  File "/build/cockpit/test/common/testlib.py", line 139, in call_js_func
    return self.phantom.eval("%s(%s)" % (func, ','.join(map(jsquote, args))))
  File "/build/cockpit/test/common/testlib.py", line 719, in <lambda>
    return lambda *args: self._invoke(name, *args)
  File "/build/cockpit/test/common/testlib.py", line 742, in _invoke
    raise Error(res['error'])
Error: button.btn-default is disabled or somehow not clickable
@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

The fedora-25 failure looks relevant?

yes, it looks like a test race because of the newly disabled button I put in...

dperpeet and others added some commits Dec 12, 2016

test: Add kdump test dependencies
This adds kexec-tools (kdump test depency) to the test images:
fedora-24
fedora-25
rhel-7
lib: Add id to React dialog pattern
Sometimes it is helpful to assign a specific id to a dialog,
for instance when applying custom CSS or when testing.
test: Change fedora-25 base url
The directory structure on the server seems to have changed.
lib: Add TryRestart to service
Sometimes we need to try to restart a service if it's running.
Since systemd supports that directly, our proxy just needs to expose it.
test: Add known issue to more distros
Issue #5754 also affects rhel-7 and fedora-25

@dperpeet dperpeet force-pushed the dperpeet:kdump branch from e8d64e1 to 6f3c225 Jan 25, 2017

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

Added a wait in the test to make sure the service is running properly before hitting the test button that crashes the system.

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

The two rhel-7 test failures are unrelated, everything else is green now (nice!), and the new waiting looks good. Thanks!

@martinpitt martinpitt merged commit 0dd4ee7 into cockpit-project:master Jan 25, 2017

12 of 13 checks passed

verify/rhel-7 1 tests failed
Details
avocado/fedora-24 Tests passed
Details
container/kubernetes Tests passed
Details
selenium/chrome Tests passed
Details
selenium/firefox Tests passed
Details
semaphoreci The build passed on Semaphore.
Details
verify/centos-7 Tests passed
Details
verify/debian-8 Tests passed
Details
verify/debian-testing Tests passed
Details
verify/fedora-25 Tests passed
Details
verify/fedora-atomic Tests passed
Details
verify/rhel-atomic Tests passed
Details
verify/ubuntu-1604 Tests passed
Details

@dperpeet dperpeet deleted the dperpeet:kdump branch Jan 25, 2017

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