New charm : crashdump

[Louis Bouchard]

This new subordinate charm does the necessary installation and configuration to setup Kernel Crash Dump collection on the Principal service.

[Jorge Niedbalski (niedbalski)]

Hello Louis,

Thanks for your submission!.

I did a brief review on your charm submission, and I have some suggestions:

1) Check your 'make lint' output:

hooks/crashdump_hooks.py:9:1: F401 'render_template' imported but unused
Makefile:4: recipe for target 'lint' failed
make: *** [lint] Error 1

2) My understanding is that this charm will not work under LXC containers,
please make an explicit statement on the README.md file.

3) Check your 'charm proof' output

I: File config.yaml not found.

4) Please add unit tests for covering your hooks code.

[Louis Bouchard (louis)]

Hello Jorge,

I fixed all of your comments & suggestions :
1) fixed
2) Note included
3) I had to add a bogus config.yaml otherwise I had issues with my unit_test (see LP:#1474824)
3) Fixed
4) Unit tests added (89% coverage)

TIA,

...Louis

[Jorge Niedbalski (niedbalski)]

Caribou,

- I added a method on charmhelpers.core.files called sed, that can be used
instead of the iterative loop.

- Also please notice that hooks are run with uid 0, so is not required to check
for privileges.

- Also i suggest you to create a logic like:
On the install hook

def install():
..
open('/var/run/reboot-required', 'w').close()

And on your config-changed

if os.path.isfile("/var/run/reboot-required"):
        os.remove("/var/run/reboot-required")
       subprocess.call(["juju-reboot", "--now"])

[Louis Bouchard (louis)]

Hello Jorge,

I applied your comments and adapted the unit_tests accordingly which turned out not to be so trivial.

The use of subprocess.call(["juju-reboot", "--now"]) required mocking that interfered with your sed() function in charm helpers.

[Jorge Niedbalski (niedbalski)]

Hello Louis,

I spent a few cycles testing this new submission:

- Make lint passes.
- Make unit test passes
- Functional tests passes

I also deployed this on trusty by running:

$ juju deploy ubuntu --to=1
$ juju deploy local:trusty/crashdump
$ juju add-relation ubuntu crashdump

Then I jumped into machine 1, and validated that this has been correctly
installed and configured.

ubuntu@juju-testing-machine-1:~$ cat /proc/cmdline
::::::::::::::
/proc/cmdline
::::::::::::::
BOOT_IMAGE=/boot/vmlinuz-3.13.0-59-generic root=UUID=dff751dc-9710-42a6-9f6b-25d49c190d10 ro console=tty1 console=ttyS0 crashkernel=384M-:128M

Then I forced a sysrq crash

root@juju-testing-machine-1:/home/ubuntu# echo c > /proc/sysrq-trigger

ubuntu@juju-testing-machine-1:/var/crash$ ls
201507311349 kexec_cmd linux-image-3.13.0-59-generic 3.13.0-59.98-201507311349.crash

services:
  crashdump:
    charm: local:trusty/crashdump-1
    exposed: false
    service-status: {}
    relations:
      juju-info:
      - ubuntu
    subordinate-to:
    - ubuntu
  ubuntu:
    charm: cs:trusty/ubuntu-4
    exposed: false
    service-status:
      current: unknown
      since: 31 Jul 2015 13:44:07Z
    relations:
      juju-info:
      - crashdump
    units:
      ubuntu/0:
        workload-status:
          current: unknown
          since: 31 Jul 2015 13:44:07Z
        agent-status:
          current: idle
          since: 31 Jul 2015 13:53:36Z
          version: 1.24.3
        agent-state: started
        agent-version: 1.24.3
        machine: "1"
        public-address: 10.5.0.126
        subordinates:
          crashdump/0:
            workload-status:
              current: unknown
              since: 31 Jul 2015 13:46:55Z
            agent-status:
              current: idle
              since: 31 Jul 2015 13:49:26Z
              version: 1.24.3
            agent-state: started
            agent-version: 1.24.3
            upgrading-from: local:trusty/crashdump-1
            public-address: 10.5.0.126

The only suggestion I would do is to improve the README file to include more precise instructions
on how to install this charm.

Fixing that, LGTM to be promulgated.

[Louis Bouchard (louis)]

Hi Jorge,

I have just enhanced the README.md and added a few details on testing with local provider.

[José Antonio Rey (jose)]

Hey Louis!

Thanks a lot for your submission for the crashdump charm to the charm store. I'm sorry it took so long, but I'll try to help and expedite the process as possible.

# Overview

I took a first look at the charm and everything seemed alright. I understand the default templates since there's a bug, so we can leave those there. Also, can you please check that the year on the copyright is the correct one? Finally, we're no longer using 'revision' files, so feel free to delete that one from the charm :)

# Deployment (with tests)

Here, it automatically failed because my environment was already bootstrapped. Since it couldn't bootstrap again it threw me an error, returning with non-zero status and failing the test automatically. Maybe you can make an exception here, so manual testing with the automated suite can still be done?

And this has me on a loop. Whenever it initially bootstraps, then it asks for it to bootstrap again, which is impossible due to the env being already bootstrapped by the test, causing it to error. Can you please take a look at this too? I believe solving the error above will solve this one.

# Deployment (manual)

So, here I'm going through the steps on the README.md file to deploy it. I followed all the steps as specified on the README file, and it deployed successfully! I executed the same as the test would but with my own hands, and the result would be positive.

# Others

One quick question. Would it be possible to completely remove the service once the master-relation-departed hook is executed? That way the user can rest assured things are back to normal once the relation has been removed.

# Conclusion

I think that's all I've got to say for now. I'm really happy to see the progress you have over here, and if you could fix those bitesizes I would be happy to promulgate the charm. Thanks again for your contributions to the Charm Store. I really appreciate it, and am excited to see the next iteration of this charm!

[José Antonio Rey (jose)]

Oh, and forgot to mention. Please, move the charm back to either 'New' or 'Fix Committed' once it's ready for re-review! Hope to hear back from you soon!

[Louis Bouchard (louis)]

José,

Thanks for the review, but you may have missed the fact that the charm had already been reviewed and made available in the store :

https://jujucharms.com/crashdump/trusty/

So I'll reset the status to "Fix Released" to indicate that status.

[Marco Ceppi (marcoceppi)]

Thanks for the heads up, this has been resolved.