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

Add background:execute occ command for running cron jobs manually #31617

Merged
merged 4 commits into from
Apr 9, 2019

Conversation

tomneedham
Copy link
Member

@tomneedham tomneedham commented Jun 1, 2018

Description

Added occ:background:execute {id} -f OCC command.

Motivation and Context

Debugging cron jobs is really tricky, often involes touching oc_jobs and triggering cron.php yourself.

This can be used to run the jobs manually, view more debug output from the command, and force running even when the interval has not passed.

How Has This Been Tested?

Manually, testing out the Activity EmailNotification background job.

Screenshots (if appropriate):

screen shot 2018-06-01 at 15 16 38

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tomneedham tomneedham added this to the development milestone Jun 1, 2018
@tomneedham tomneedham self-assigned this Jun 1, 2018
@tomneedham
Copy link
Member Author

Moved over the ConsoleLogger which is a useful utility to create an ILogger from an OutputInterface.

@tomneedham
Copy link
Member Author

Open questions - should we respect the reversed_at field - and if so how? There is no API to retrieve it in IJobList

@butonic
Copy link
Member

butonic commented Jun 1, 2018

The command is part of core so we could pull reversed_at from the DB directly if we don't want to change the public API. The latter would be the right way, but it requires adding a new IJob2. Maybe that is why abstract classes make more sense for public API ...

  • the log lines are prefixed with the app. maybe start with a timestamp, eg
    2018-06-01T16:52:33.1234.Z core: Running job with id 12 and class OCA\Activity\BackgroundJob\ExpireActivities. Last run 0 and interval 86400

  • separate PR: I wonder how admins get the id of a job. It appears in the log, yes. Maybe a separate background:search command that allows you to search by any of the db columns?

  • separate PR: add --workers n and option and a background.jobs.socket config option. That would allow oc to offload job execution as an event to the worker queue by sending them via socket ... or better jet use redis as a job queue for non timed jobs: https://github.com/php-enqueue/enqueue-dev/blob/master/docs/transport/redis.md

  • separate PR: get rid of the wnd console logger and use this once it is merged

@mmattel
Copy link
Contributor

mmattel commented Jun 1, 2018

This is documentation relevant !

@codecov
Copy link

codecov bot commented Jun 4, 2018

Codecov Report

Merging #31617 into master will decrease coverage by 0.01%.
The diff coverage is 54.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #31617      +/-   ##
============================================
- Coverage     65.37%   65.35%   -0.02%     
- Complexity    18588    18617      +29     
============================================
  Files          1213     1215       +2     
  Lines         70380    70487     +107     
  Branches       1295     1295              
============================================
+ Hits          46009    46065      +56     
- Misses        23997    24048      +51     
  Partials        374      374
Flag Coverage Δ Complexity Δ
#javascript 52.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.79% <54.78%> (-0.03%) 18617 <25> (+29)
Impacted Files Coverage Δ Complexity Δ
lib/private/BackgroundJob/JobList.php 80.26% <ø> (ø) 33 <0> (ø) ⬇️
core/register_command.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/BackgroundJob/Job.php 100% <100%> (ø) 11 <0> (+2) ⬆️
core/Command/Background/Queue/Status.php 100% <100%> (ø) 4 <0> (ø) ⬇️
core/Command/Background/Queue/Execute.php 100% <100%> (ø) 8 <8> (?)
core/Command/Background/Queue/Delete.php 100% <100%> (ø) 4 <0> (ø) ⬇️
lib/private/Log/CommandLogger.php 4.54% <4.54%> (ø) 17 <17> (?)
lib/private/BackgroundJob/TimedJob.php 63.63% <52.94%> (-36.37%) 5 <0> (+2)

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 a6aac17...bcc3b19. Read the comment docs.

@mmattel
Copy link
Contributor

mmattel commented Jun 4, 2018

separate PR: I wonder how admins get the id of a job. It appears in the log, yes. Maybe a separate background:search command that allows you to search by any of the db columns?

Why not using a --list option here as it is execution relevant. List the job ID´s with their name and other possible / useful data. That would make sense to me from an admin pov.

@tomneedham
Copy link
Member Author

Pushed a fix to add the time to the log messages in the console

@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Jun 4, 2018

are you aware of https://github.com/owncloud/core/pull/24551/files - let's sync the command names

@DeepDiver1975
Copy link
Member

Why not using a --list option here as it is execution relevant. List the job ID´s with their name and other possible / useful data. That would make sense to me from an admin pov.

#24551 adds a status command for this

@mmattel
Copy link
Contributor

mmattel commented Jun 4, 2018

@tomneedham pls see my comment in #31630 (comment)

@mmattel
Copy link
Contributor

mmattel commented Jun 13, 2018

Question:

at the code locations for $logger->debug (2x)
the naming is different compared to the terms used above in the code
class --> Job
arguments --> Job Arguments

Is this by intention ?

@tomneedham
Copy link
Member Author

in debug mode I would rather be more explicit, and that variable is the class name

@PVince81
Copy link
Contributor

PVince81 commented Apr 5, 2019

as discussed, @jvillafanez will help out. thx

@jvillafanez
Copy link
Member

Except for #31617 (comment) which can't be solved without heavy changes, the rest of the problems have been addressed.

@jvillafanez jvillafanez force-pushed the occ-bgjob branch 2 times, most recently from d63d0d8 to ca22476 Compare April 8, 2019 11:36
@phil-davis phil-davis dismissed jvillafanez’s stale review April 8, 2019 11:41

The reviewer himself has adjusted the code.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Test cases cover the various combiations of ways to use the execute command.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@phil-davis
Copy link
Contributor

@PVince81 please decide if the codecov here is enough.
Last week I had about "51% of diff hit", now @jvillafanez has "54.78% of diff hit (target 65.37%)" which is a little better :)

@jvillafanez
Copy link
Member

I think we can remove the ConsoleLogger and inject a normal one (the one provided by the server) in the execute command. Unless we need the ConsoleLogger for something in particular, I think this should rise the coverage for the patch (less untested code)

@PVince81
Copy link
Contributor

PVince81 commented Apr 8, 2019

@jvillafanez can you make the proposed change ?

@PVince81
Copy link
Contributor

PVince81 commented Apr 9, 2019

ok let's keep the console logger for now, there must have been a reason for adding it...

@PVince81 PVince81 merged commit d181d91 into master Apr 9, 2019
@delete-merged-branch delete-merged-branch bot deleted the occ-bgjob branch April 9, 2019 10:40
@mmattel
Copy link
Contributor

mmattel commented Apr 9, 2019

Questions:

  • Backporting?
  • Availability for 10.2 only?

Both related to documentaion, see owncloud/docs#290
Please suggest/sample text to be added in docs.

@phil-davis
Copy link
Contributor

I am waiting for #34723 backport to be merged. Then I will make a backport of this. It goes with the other new background:queue:status and background:queue:status that are merged in stable10 for 10.2

@phil-davis
Copy link
Contributor

Backport stable10 #34995

@mmattel
Copy link
Contributor

mmattel commented Apr 10, 2019

The doc PR is made. While writing it, I remembered: #31743 (Adding command background:queue:add)

We can now delete a job (yes we ask if we really want to do so) but nontheless an add command is missing... There are for sure default jobs which are part of the tar shipment. A list could be created and documented in the occ command set to reuse/reference...

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants