Add timeout for runner jobs #194
Conversation
WalkthroughThe recent update introduces a timeout feature for Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Runner
participant ControlPlane
User->>ControlPlane: Update controlplane.yml with runner_job_timeout
ControlPlane-->>Runner: Pass configuration settings
User->>Runner: Execute `cpl run` job
Runner->>Runner: Apply job-specific timeout
Note over Runner: Job runs with configured timeout
Runner->>User: Job completion or timeout notification
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
rafaelgomesxyz
force-pushed
the
run-cpln-changes
branch
from
1996772 to
332dec7
Compare
rafaelgomesxyz
force-pushed
the
runner-job-timeout
branch
from
a57b9da to
fe865c3
Compare
rafaelgomesxyz
force-pushed
the
run-cpln-changes
branch
from
332dec7 to
2133ff4
Compare
rafaelgomesxyz
force-pushed
the
runner-job-timeout
branch
from
fe865c3 to
4cb8d5c
Compare
rafaelgomesxyz
force-pushed
the
runner-job-timeout
branch
from
4cb8d5c to
d1510b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- README.md (1 hunks)
- docs/commands.md (1 hunks)
- examples/controlplane.yml (1 hunks)
- lib/command/run.rb (5 hunks)
- spec/command/run_spec.rb (2 hunks)
- spec/dummy/.controlplane/controlplane.yml (1 hunks)
Additional context used
LanguageTool
docs/commands.md
[uncategorized] ~44-~44: You might be missing the article “the” here.
Context: ...mage` - Builds and pushes the image to Control Plane - Automatically assigns image num...
[uncategorized] ~56-~56: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...uantity or are older than the specified amount of days - Specify the max quantity thro...
[grammar] ~57-~57: Possible verb agreement error. Did you mean “specifies”? (Some collective nouns can be treated as both singular and plural, so ‘Specify’ is not always incorrect.)
Context: ...der than the specified amount of days - Specify the max quantity through `image_retenti...
[uncategorized] ~58-~58: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ne/controlplane.ymlfile - Specify the amount of days throughimage_retention_days` ...
[uncategorized] ~72-~72: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... date of the latest image - Specify the amount of days after an app should be consider...
[uncategorized] ~230-~230: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
[uncategorized] ~241-~241: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
[uncategorized] ~252-~252: This expression is usually spelled with a hyphen.
Context: ... is only supported for domains that use path based routing mode and have a route configure...
[typographical] ~376-~376: Unpaired symbol: ‘"’ seems to be missing
Context: ...t isbash, in which case the args ["-c", cmd_to_run] are passed - Providing `--...
[style] ~433-~433: Consider replacing ‘only’ with a different word to let your writing stand out.
Context: ...ne/controlplane.yml` file - This should only be used for temporary apps like review ...CHANGELOG.md
[uncategorized] ~30-~30: You might be missing the article “the” here.
Context: ...4-05-27 ### Fixed - Fixed issue where release script was not running from the app ima...
[uncategorized] ~35-~35: You might be missing the article “the” here.
Context: ...## Added - Added post-creation hook tosetup-appcommand (configurable through...
[style] ~43-~43: To form a complete sentence, be sure to include a subject.
Context: ... non-zero code if any validation fails. Can be disabled by setting `DISABLE_VALIDAT...
[uncategorized] ~54-~54: You might be missing the article “the” here.
Context: ...4-05-15 ### Fixed - Fixed issue wherecleanup-stale-appscommand fails to del...
[uncategorized] ~67-~67: You might be missing the article “the” here.
Context: ...xed - Fixed race conditions when using latest image inruncommand. [PR 163](https:...
[uncategorized] ~67-~67: You might be missing the article “the” here.
Context: ...e conditions when using latest image inruncommand. [PR 163](https://github.co...
[uncategorized] ~83-~83: You might be missing the article “the” here.
Context: ...y-image` command now raises an error if image does not exist. [PR 153](https://github...
[uncategorized] ~106-~106: You might be missing the article “the” here.
Context: ..._app_name_starts_withset totrueincopy-image-from-upstream` command. [PR 1...
[uncategorized] ~118-~118: You might be missing the article “the” here.
Context: ... over config fromcontrolplane.ymlincopy-image-from-upstreamcommand. [PR 1...
[uncategorized] ~129-~129: You might be missing the article “the” here.
Context: ...hub.com/ahangarha). - Fixed issue wheredeletecommand fails to delete apps wit...
[grammar] ~151-~151: The operating system from Apple is written “macOS”.
Context: ...-17 ### Fixed - Fixed failed build on MacOS by adding platform flag and fixed multi...
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ...3-09-20 ### Fixed - Fixed issue wherecopy-image-from-upstreamcommand does n...
[uncategorized] ~191-~191: You might be missing the article “an” here.
Context: ...runcommands fail when not providing image. [PR 68](https://github.com/shakacode/c...
[uncategorized] ~199-~199: You might be missing the article “the” here.
Context: ...s://github.com/rafaelgomesxyz). - Fixedrun:cleanupcommand for all apps that s...
[uncategorized] ~200-~200: You might be missing the article “the” here.
Context: ...s://github.com/rafaelgomesxyz). - Fixedcleanup-old-imagescommand for all apps...
[uncategorized] ~201-~201: You might be missing the article “the” here.
Context: ...s://github.com/rafaelgomesxyz). - Fixed--helpoption. [PR 66](https://github.c...
[uncategorized] ~205-~205: You might be missing the article “the” here.
Context: ... - Added--use-local-tokenoption torun:detachedcommand. [PR 61](https://g...
[uncategorized] ~218-~218: You might be missing the article “the” here.
Context: ...1.0.1] - 2023-06-28 ### Fixed - Fixedcleanup-stale-appscommand when app doe...
[uncategorized] ~218-~218: You might be missing the article “an” here.
Context: ...le-apps` command when app does not have image. [PR 55](https://github.com/shakacode/c...README.md
[style] ~36-~36: Try using a synonym here to strengthen your writing.
Context: ...ons. Control Plane, on the other hand, gives you access to raw cloud computing power. Ho...
[uncategorized] ~63-~63: Possible missing comma found.
Context: ...iration for your own scripts. - Easy to understand Heroku to Control Plane conventions in ...
[style] ~76-~76: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ding the database and other services. - has common environment variables. On Contr...
[style] ~113-~113: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../en/) (required for these helpers). 4. Install Control Plane CLI, and configure access...
[grammar] ~160-~160: The word “setup” is a noun. The verb is spelled with a space.
Context: ...is.ymlandpostgres.yml`, which could setup Redis and Postgres for a testing applic...
[uncategorized] ~386-~386: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...yYWQ3MzA2NGE1YWJjNmVlMGE) for ShakaCode open source projects. ## Environment There are tw...
[uncategorized] ~390-~390: Possible missing article found.
Context: ... we can set up environment variables in Control Plane: - **Inworkload/container/env...
[uncategorized] ~434-~434: Possible missing article found.
Context: ...several options for a database setup on Control Plane: - Heroku Postgres. It is th...
[uncategorized] ~444-~444: Possible missing comma found.
Context: ...d provider for Postgres, e.g., Amazon's RDS can be a quick go-to. Here are [instr...
Markdownlint
docs/commands.md
5-5: null
Fenced code blocks should have a language specified
23-23: null
Fenced code blocks should have a language specifiedREADME.md
544-544: Expected: dash; Actual: asterisk
Unordered list style
545-545: Expected: dash; Actual: asterisk
Unordered list style
546-546: Expected: dash; Actual: asterisk
Unordered list style
14-14: Expected: 1; Actual: 2
Multiple consecutive blank lines
543-543: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
544-544: null
Lists should be surrounded by blank lines
21-21: Expected: ----; Actual: ---
Horizontal rule style
28-28: Expected: ----; Actual: ---
Horizontal rule style
128-128: null
Spaces inside code span elements
143-143: null
Fenced code blocks should have a language specified
Additional comments not posted (11)
spec/dummy/.controlplane/controlplane.yml (1)
146-151: Configuration block fordummy-test-runner-job-timeout-{GLOBAL_IDENTIFIER}added correctly.Please verify if the timeout value of
10is intended for testing purposes, as it deviates significantly from the default discussed in the PR summary.examples/controlplane.yml (1)
95-98: Addedrunner_job_timeoutparameter with a default value of 1000 seconds.Please clarify the discrepancy between the default timeout value mentioned here (1000 seconds) and the 6 hours mentioned in the AI-generated summary.
spec/command/run_spec.rb (2)
116-116: Single quotes added around command strings in test scenarios.This change ensures that the commands are executed as expected during tests.
208-229: New test context forrunner_job_timeoutadded correctly.This test verifies that the job times out as expected, aligning directly with the PR objectives.
docs/commands.md (1)
383-384: Documentation for default job timeout added correctly.This addition is consistent with the changes made in other files and aligns with the PR objectives.
lib/command/run.rb (5)
48-49: The documentation in the long description is clear and concise about the new timeout feature.
94-95: The default values for job timeout and history limit are well-defined and follow the requirements specified in the PR.
113-114: Properly retrieves and sets the job timeout and history limit from the configuration or uses the default values if not specified.
186-193: The job specification withincreate_runner_workloadmethod correctly sets theactiveDeadlineSecondsandhistoryLimitbased on the job timeout and history limit. This ensures that the job respects the configured or default timeout and maintains a history limit.
222-230: Theupdate_runner_workloadmethod correctly checks and updates theactiveDeadlineSecondsandhistoryLimitif they differ from the current settings. This ensures that any changes to the timeout or history limit are applied to existing workloads.CHANGELOG.md (1)
19-19: The changelog entry clearly documents the addition of the timeout feature forrunjobs, linking to the PR and the contributor. This provides transparency and traceability for the changes made.
Fixes #193
Adds a timeout for runner jobs (6 hours by default, but configurable through
runner_job_timeoutincontrolplane.yml), to prevent issues where an interactive job may not finish properly.Summary by CodeRabbit
New Features
runjobs, with a default value of 1000 seconds, set viarunner_job_timeoutincontrolplane.yml.runcommands.Documentation
runner_job_timeoutparameter and default job timeout settings.Tests