Skip to content

Update kubernetes plugin for Job selectors#138

Merged
aufi merged 2 commits into
migtools:mainfrom
aufi:job-resource-cleanup
Jun 17, 2026
Merged

Update kubernetes plugin for Job selectors#138
aufi merged 2 commits into
migtools:mainfrom
aufi:job-resource-cleanup

Conversation

@aufi

@aufi aufi commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Addressing The output.yaml Job spec contains manualSelector: false with an explicit spec.selector bearing source cluster UIDs (batch.kubernetes.io/controller-uid, controller-uid). This configuration fails Kubernetes API validation with: Job.batch "wordpress-install" is invalid: spec.selector: Invalid value: ... : selector not auto-generated. When manualSelector: false, Kubernetes rejects explicit selectors; it only accepts auto-generated ones.

Related to migtools/crane#308

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced Kubernetes Job transformation to remove both current and legacy controller-UID identifiers from job metadata, pod template labels, and selector settings, with selector cleanup behavior respecting manual selector configuration and applied after PVC renaming.
  • Tests

    • Added comprehensive test scenarios covering removal of current and legacy controller-UID keys across metadata, selector, and template permutations.

Review Change Stack

Addressing The output.yaml Job spec contains manualSelector: false with an explicit spec.selector bearing source cluster UIDs (batch.kubernetes.io/controller-uid, controller-uid). This configuration fails Kubernetes API validation with: Job.batch "wordpress-install" is invalid: spec.selector: Invalid value: ... : selector not auto-generated. When manualSelector: false, Kubernetes rejects explicit selectors; it only accepts auto-generated ones.

Related to migtools/crane#308

Signed-off-by: Marek Aufart <maufart@redhat.com>
@coderabbitai

coderabbitai Bot commented Apr 28, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3a6796bd-da6c-40f5-b824-ae8e290ce21b

📥 Commits

Reviewing files that changed from the base of the PR and between b809359 and 3059734.

📒 Files selected for processing (2)
  • transform/kubernetes/kubernetes.go
  • transform/kubernetes/kubernetes_test.go

📝 Walkthrough

Walkthrough

The transform now strips both batch.kubernetes.io/controller-uid and legacy controller-uid from resource metadata annotations and labels. For batch/v1 Jobs it removes controller-uid keys from pod template labels and conditionally removes or prunes /spec/selector depending on spec.manualSelector. Tests were added for both keys and selector behaviors.

Changes

Job Controller-UID Removal

Layer / File(s) Summary
Strip fields list update
transform/kubernetes/kubernetes.go
Adds batch.kubernetes.io/controller-uid and controller-uid to the global fieldsToStrip for metadata.annotations and metadata.labels.
Job UID removal implementation
transform/kubernetes/kubernetes.go
Adds removeJobControllerUID to remove controller-uid keys from /spec/template/metadata/labels and either delete /spec/selector (when spec.manualSelector is unset/false) or remove only the matchLabels entries for controller-uid when spec.manualSelector is true.
Job transform wiring and tests
transform/kubernetes/kubernetes.go, transform/kubernetes/kubernetes_test.go
Calls removeJobControllerUID from the Job transform after PVC renaming and adds test cases verifying removal of both namespaced and legacy controller-uid keys from metadata, selector matchLabels, and pod template labels across manualSelector variants.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Poem

🐰 I hopped through Jobs at break of day,
Snipping stray uids along the way,
Old and new keys both made to flee,
Now selectors and labels run tidy and free.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: updating the kubernetes plugin to handle Job selectors, particularly removing controller-uid from Job selector settings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
transform/kubernetes/kubernetes.go (1)

925-979: The PR description lists both label formats; consider whether legacy controller-uid (without batch.kubernetes.io/ prefix) needs handling.

The PR description mentions both batch.kubernetes.io/controller-uid and controller-uid, but the implementation only removes the prefixed version. If the tool must process Job specs exported from pre-1.12 Kubernetes clusters during migration, the legacy controller-uid label should also be removed from matchLabels and template labels for consistency.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@transform/kubernetes/kubernetes.go` around lines 925 - 979,
removeJobControllerUID currently only removes the prefixed label
"batch.kubernetes.io/controller-uid"; update it to also detect and remove the
legacy "controller-uid" key in both selector.matchLabels and
spec.template.metadata.labels. In the manualSelector true branch (function
removeJobControllerUID) and in the template-labels removal block, check for both
"batch.kubernetes.io/controller-uid" and "controller-uid" (use
unstructured.NestedString or NestedMap presence checks), build the JSON pointer
paths via escapeJSONPointer for each key, decode the remove patch and append to
patches for each found key, and preserve existing error handling and return
behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@transform/kubernetes/kubernetes.go`:
- Around line 925-979: removeJobControllerUID currently only removes the
prefixed label "batch.kubernetes.io/controller-uid"; update it to also detect
and remove the legacy "controller-uid" key in both selector.matchLabels and
spec.template.metadata.labels. In the manualSelector true branch (function
removeJobControllerUID) and in the template-labels removal block, check for both
"batch.kubernetes.io/controller-uid" and "controller-uid" (use
unstructured.NestedString or NestedMap presence checks), build the JSON pointer
paths via escapeJSONPointer for each key, decode the remove patch and append to
patches for each found key, and preserve existing error handling and return
behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c4d021cc-578c-421b-b3fb-dbe9d188675d

📥 Commits

Reviewing files that changed from the base of the PR and between a9f07d5 and b809359.

📒 Files selected for processing (2)
  • transform/kubernetes/kubernetes.go
  • transform/kubernetes/kubernetes_test.go

Comment thread transform/kubernetes/kubernetes.go
Comment thread transform/kubernetes/kubernetes_test.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
@aufi aufi requested a review from msajidmansoori12 May 29, 2026 11:49

@stillalearner stillalearner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@midays midays left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@aufi aufi merged commit 71b323a into migtools:main Jun 17, 2026
2 checks passed
@aufi aufi self-assigned this Jun 17, 2026
@aufi aufi moved this to In progress in Crane Development Jun 17, 2026
@github-project-automation github-project-automation Bot moved this from In progress to Done in Crane Development Jun 17, 2026
aufi added a commit to migtools/crane that referenced this pull request Jun 19, 2026
There was a recent change in KubernetesPlugin regarding to Job resource
cleanup in migtools/crane-lib#138

Updating crane-lib dependency to get this update into crane.

Related to migtools/crane-lib#138

Signed-off-by: Marek Aufart <maufart@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants