Suspend standalone Job by default#141
Merged
Merged
Conversation
Implements a solution for issue #482 by automatically suspending standalone Kubernetes Jobs
(those without ownerReferences) during migration to prevent them from re-executing on the target cluster.
Changes:
- Added suspendStandaloneJob() function that sets spec.suspend: true on standalone Jobs
- Introduced opt-out mechanism via crane.konveyor.io/job-idempotent: "true" annotation for Jobs that are safe to re-run
- Jobs with ownerReferences (e.g., created by CronJobs) remain whited out as before
- Updated existing Job tests to expect suspend patches
Behavior:
- By default: standalone Jobs are suspended → safe migration, prevents accidental re-runs
- With annotation: Jobs marked as idempotent run automatically → supports init Jobs, DB migrations
- User can manually unsuspend Jobs post-migration: kubectl patch job <name> -p '{"spec":{"suspend":false}}'
While Kubernetes expects Jobs to be idempotent, many real-world Jobs are not (e.g., one-shot data imports, billing operations). This conservative approach prevents data corruption or duplicate charges while allowing explicit opt-in for idempotent workloads.
Fixes: migtools/crane#482
Signed-off-by: Marek Aufart <maufart@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds automatic suspension of standalone Kubernetes Jobs during transformation by introducing an exported annotation constant ChangesStandalone Job suspension during transformation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transform/kubernetes/kubernetes_test.go (1)
1048-1062:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNo-patch scenarios are currently unverified.
Line 1048 skips patch assertions when
PatchResponseJsonis empty, so cases like “DoNotSuspendStandaloneJobWithIdempotentAnnotation” and “SuspendStandaloneJobAlreadySuspended” do not actually verify that no suspend patch is emitted.Proposed test harness fix
- if len(c.PatchResponseJson) != 0 { + if len(c.PatchResponseJson) != 0 { expectPatch, err := jsonpatch.DecodePatch([]byte(c.PatchResponseJson)) if err != nil { t.Error(err) } if len(resp.Patches) != 0 { ok, err := internaljsonpatch.Equal(resp.Patches, expectPatch) if !ok || err != nil { actual, _ := json.Marshal(resp.Patches) t.Error(fmt.Sprintf("Invalid patches. Actual: %s, Expected: %v", actual, c.PatchResponseJson)) } } else { t.Error(fmt.Sprintf("Patches Expected: %#v, none found", expectPatch)) } + } else if len(resp.Patches) != 0 { + actual, _ := json.Marshal(resp.Patches) + t.Error(fmt.Sprintf("Unexpected patches. Actual: %s, Expected: none", actual)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transform/kubernetes/kubernetes_test.go` around lines 1048 - 1062, The test currently skips patch verification when PatchResponseJson is empty, leaving no-patch scenarios unverified. Add an else clause after the existing if len(c.PatchResponseJson) != 0 block to verify that when no patches are expected, resp.Patches is also empty. This ensures test cases like "DoNotSuspendStandaloneJobWithIdempotentAnnotation" and "SuspendStandaloneJobAlreadySuspended" actually verify that no suspend patch is emitted.
🧹 Nitpick comments (1)
transform/kubernetes/kubernetes_test.go (1)
941-941: ⚡ Quick winUse the exported annotation constant in test data.
Line 941 hardcodes
crane.konveyor.io/job-idempotent; usingkubernetes.CraneJobIdempotentAnnotationavoids drift if the key changes.Proposed cleanup
- "annotations": map[string]interface{}{ - "crane.konveyor.io/job-idempotent": "true", - }, + "annotations": map[string]interface{}{ + kubernetes.CraneJobIdempotentAnnotation: "true", + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transform/kubernetes/kubernetes_test.go` at line 941, The test data at the hardcoded annotation key "crane.konveyor.io/job-idempotent" should use the exported constant kubernetes.CraneJobIdempotentAnnotation instead. Replace the hardcoded string literal with a reference to the constant to ensure the test stays synchronized if the annotation key value is ever updated in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transform/kubernetes/kubernetes.go`:
- Around line 1038-1039: The comment above the condition `if !found ||
!suspended` on line 1038 is inaccurate and misleading. The condition actually
patches when the suspend field is missing (not found) OR when suspend is false
(not suspended), but the comment incorrectly states it patches when suspend is
already true. Update the comment to accurately describe the actual logic: that
the patch is applied when the suspend field doesn't exist or when it's currently
set to false, ensuring the Job gets suspended by setting it to true.
---
Outside diff comments:
In `@transform/kubernetes/kubernetes_test.go`:
- Around line 1048-1062: The test currently skips patch verification when
PatchResponseJson is empty, leaving no-patch scenarios unverified. Add an else
clause after the existing if len(c.PatchResponseJson) != 0 block to verify that
when no patches are expected, resp.Patches is also empty. This ensures test
cases like "DoNotSuspendStandaloneJobWithIdempotentAnnotation" and
"SuspendStandaloneJobAlreadySuspended" actually verify that no suspend patch is
emitted.
---
Nitpick comments:
In `@transform/kubernetes/kubernetes_test.go`:
- Line 941: The test data at the hardcoded annotation key
"crane.konveyor.io/job-idempotent" should use the exported constant
kubernetes.CraneJobIdempotentAnnotation instead. Replace the hardcoded string
literal with a reference to the constant to ensure the test stays synchronized
if the annotation key value is ever updated in the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 871d59d8-3fb0-410e-af5c-a65336060585
📒 Files selected for processing (2)
transform/kubernetes/kubernetes.gotransform/kubernetes/kubernetes_test.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements a solution for issue #482 by automatically suspending standalone Kubernetes Jobs (those without ownerReferences) during migration to prevent them from re-executing on the target cluster.
Changes:
Behavior:
While Kubernetes expects Jobs to be idempotent, many real-world Jobs are not (e.g., one-shot data imports, billing operations). This conservative approach prevents data corruption or duplicate charges while allowing explicit opt-in for idempotent workloads.
Fixes: migtools/crane#482
Summary by CodeRabbit
crane.konveyor.io/job-idempotentannotation to prevent suspension for specific jobs.