Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@dobsonj: This pull request references STOR-2859 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @dobsonj! Some important instructions when contributing to openshift/api: |
|
@dobsonj: GitHub didn't allow me to request PR reviews from the following users: openshift/storage. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThis change adds the cluster-scoped Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This adds the ControllerManager API and feature-gated configuration without any remaining concrete merge-blocking risk identified in the supplied evidence. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 5 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
Bot
added
the
size/L
|
@dobsonj: This pull request references STOR-2859 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@dobsonj: This pull request references STOR-2859 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@config/v1/types_controllermanager.go`:
- Around line 12-15: The +openshift:api-approved annotation in the header
comment of types_controllermanager.go still uses the placeholder URL
"https://github.com/openshift/api/pull/XYZ"; update that annotation to the
actual API approval PR URL (the real pull request number) so it passes API
approval checks—locate the comment block containing
"+openshift:api-approved.openshift.io" and replace the placeholder with the real
PR link.
In `@config/v1/zz_generated.featuregated-crd-manifests.yaml`:
- Around line 199-203: Update the placeholder ApprovedPRNumber value for the CRD
entry with CRDName controllermanagers.config.openshift.io: replace
"https://github.com/openshift/api/pull/XYZ" with the real approval PR URL (or
the correct PR number path) so the ApprovedPRNumber field references the actual
merged/approved PR; ensure the URL is valid and points to the final PR in the
openshift/api repo.
🧹 Nitpick comments (1)
config/v1/types_controllermanager.go (1)
34-42: Addomitemptyto the optional enum field for consistency.
ForceDetachOnTimeoutis marked optional and should useomitemptyin its JSON tag, consistent with other optional enum fields throughout the config/v1 API group (e.g.,Platform,CgroupMode,WorkerLatencyProfile). This keeps manifests clean by omitting the field when unset.♻️ Proposed change
- ForceDetachOnTimeout ForceDetachOnTimeoutPolicy `json:"forceDetachOnTimeout"` + ForceDetachOnTimeout ForceDetachOnTimeoutPolicy `json:"forceDetachOnTimeout,omitempty"`
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_controllermanagers.crd.yaml`:
- Line 5: The api approval annotation api-approved.openshift.io currently
contains a placeholder URL ending with "XYZ"; replace that placeholder with the
actual GitHub PR URL for the OpenShift API approval (the final merged/approved
PR number), e.g. https://github.com/openshift/api/pull/<PR_NUMBER>, ensuring the
annotation value is a valid URL and kept as a single string on the
api-approved.openshift.io line in the CRD manifest so gating will recognize it.
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/v1/types_controllermanager.go`:
- Around line 58-61: The struct ControllerManagerStatus is empty but annotated
with +kubebuilder:validation:MinProperties=1 which makes an empty status
invalid; remove the +kubebuilder:validation:MinProperties=1 annotation from the
ControllerManagerStatus definition in types_controllermanager.go (or
alternatively add actual status fields to ControllerManagerStatus if you intend
to require properties) so that writing an empty {} status is valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/v1/types_controllermanager.go`:
- Around line 35-44: The kubebuilder markers on ControllerManagerSpec are not
being picked up by controller-gen: move/ensure the
`+kubebuilder:validation:MinProperties=1` marker is placed immediately above the
`type ControllerManagerSpec struct {` line (no blank comment gap) and replace
the incorrect `+default="Enabled"` with the kubebuilder form
`+kubebuilder:default=Enabled` (attach it immediately above the
`ForceDetachOnTimeout ForceDetachOnTimeoutPolicy` field or the field definition)
so controller-gen will emit the spec.minProperties and the default for
`ForceDetachOnTimeout`; then re-run the CRD generation to verify the generated
CRD includes `minProperties` and the `default` value.
🧹 Nitpick comments (2)
config/v1/types_controllermanager.go (2)
29-32: Minor: JSON tag inconsistencies.
- Line 29:
Specis marked+requiredbut usesomitzero, which could omit the spec from serialized output if it's a zero value. Consider usingomitemptyfor consistency with typical Kubernetes API patterns, or verify this is the intended behavior.- Line 32:
omitempty,omitzerois redundant—omitzero(Go 1.24+) subsumesomitemptyfor struct types.
49-56: Useconstinstead ofvarfor policy constants.These are immutable enum-like values that should not be modifiable at runtime. Declaring them as
constis idiomatic Go and provides compile-time immutability guarantees.♻️ Proposed fix
-var ( +const ( // ForceDetachOnTimeoutEnabled will allow kube-controller-manager to // force detach volumes based on maximum unmount time and node status. - ForceDetachOnTimeoutEnabled ForceDetachOnTimeoutPolicy = "Enabled" + ForceDetachOnTimeoutEnabled ForceDetachOnTimeoutPolicy = "Enabled" // ForceDetachOnTimeoutDisabled will prevent kube-controller-manager // from force detaching volumes. ForceDetachOnTimeoutDisabled ForceDetachOnTimeoutPolicy = "Disabled" )
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 12445-12452: The generated OpenAPI schema for the field
forceDetachOnTimeout is missing the Enum constraint; regenerate the OpenAPI
output so the SchemaProps for forceDetachOnTimeout includes the Enum values
["Enabled","Disabled"] (the source type has
+kubebuilder:validation:Enum=Enabled;Disabled in types_controllermanager.go).
Re-run the project's OpenAPI generation task (the generator that produces
zz_generated.openapi.go), verify the SchemaProps.Enum for forceDetachOnTimeout
is populated with the two enum strings, and commit the updated
zz_generated.openapi.go so the generated schema matches the source annotation.
In
`@payload-manifests/crds/0000_10_config-operator_01_controllermanagers.crd.yaml`:
- Around line 61-65: The CRD's schema for the status subresource currently sets
"minProperties: 1" on the "status" object but no status properties are declared,
which will prune and reject updates; remove the "minProperties: 1" constraint
from the "status" schema (or alternatively declare the intended status fields
under "status" if you want to enforce at least one field) so status updates are
not rejected; locate the "status:" object in the controllermanagers CRD
definition and delete the "minProperties: 1" line (or add explicit status
property definitions) to fix the issue.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_controllermanagers.crd.yaml`:
- Around line 46-60: Remove the minProperties: 1 constraint from the spec object
so the CRD no longer requires at least one property; specifically edit the spec
block that contains forceDetachOnTimeout (remove the minProperties: 1 line) to
make spec optional and consistent with other config-operator CRDs while leaving
the forceDetachOnTimeout property, its default, enum and description intact.
|
@dobsonj: This pull request references STOR-2859 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
dobsonj
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/cc @gnufied @ingvagabund |
openshift-ci
Bot
added
the
needs-rebase
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
tide/merge-method-squash
There was a problem hiding this comment.
Another pass over the current state. The earlier round looks addressed: scheme registration, payload CRD glob, +enum, singleton rule and the status contract are all right now.
Verified locally on 9547e69: make lint clean, ControllerManager integration tests pass, payload manifest byte-identical to the generated one with bootstrap-required gone. I also ran throwaway suites against envtest to confirm what the MinProperties / MinItems markers actually reject; details inline.
Two things not inline:
- @lance5890's question about the type name is still open.
config.openshift.io/APIServerdeliberately covers both apiservers, so aControllerManagerthat only ever configures KCM is a name that will read wrong if openshift-controller-manager config ever needs a home. v1alpha1 is the point to settle it, so the thread deserves an explicit answer either way. - openshift/enhancements#2095 has no acks yet. The API reviewers guide expects an initial ack before a gate merges into DevPreviewNoUpgrade, so that is the gating item here. The PR description is also stale (still
config/v1, status without conditions) and feeds the squashed commit message.
openshift-ci
Bot
added
size/XXL
openshift-ci
Bot
added
size/XL
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Another pass over 4edbf3b1. The status removal, the gate split, scheme registration, the payload glob and the singleton rule all look right now. Verified locally: make lint clean, make verify-codegen-crds clean, payload manifest byte-identical to the generated one, ControllerManager integration tests green.
Inline comments cover what is left in the code. Three things that do not anchor to a line:
-
openshift/enhancements#2095 has no ack. Every review on it is
COMMENTED, there is no lgtm, and the bot still reportsNOT APPROVED. The API reviewers guide expects an initial ack before a gate merges intoDevPreviewNoUpgrade, so this is the gating item here, not anything in the API itself. -
The EP no longer matches the API. #2095 still documents
.status.conditionswith aProgressingtype (lines 63 and 82-83 of its diff), which4edbf3b1removed. I agree with your conclusion that rollout progress belongs onkubecontrollermanagers.operator.openshift.iostatus, so the EP wants the same edit. It also only namesDisableForceDetachOnTimeoutwhile both gates point readers at it, soControllerManagerConfigshould be described there too. -
The PR description is stale. It still refers to
config/v1,config/v1/types_controllermanager.goandControllerManagerStatus. Withtide/merge-method-squashthat text becomes the commit message.
@JoelSpeed for what it is worth, the three things you asked for in January (an EP, a feature gate, and starting at v1alpha1) are all in place now.
|
@dobsonj: This pull request references STOR-2859 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test verify-client-go |
|
Thanks @saschagrunert for all your help with this.
Understood, I'll chase approvals on the enhancement and then report back once it has merged.
I pushed an update to the EP to drop the condition, and to mention both feature gates in the API section.
I updated the PR description. Everything from "PR Type" down was generated by one of the bots, but I made manual edits. Maybe there's a way to get it regenerated based on current state. Note though that I just came across Joel's comment though that we shouldn't use tide squash on this repo, so I'll plan on doing a final squash of all these commits before merge. |
|
@coderabbitai resume |
✅ Action performedReviews resumed and review finished. |
|
/test minor-images |
1 similar comment
|
/test minor-images |
|
/retest |
Introduce ControllerManager config v1alpha1 API with a forceDetachOnTimeout option to control whether kube-controller-manager force detaches volumes from a node that is not healthy once the volumes have not been unmounted within the maximum unmount time (6 minutes). Co-authored-by: Sascha Grunert <sgrunert@redhat.com>
|
/remove-label tide/merge-method-squash |
openshift-ci
Bot
removed
the
tide/merge-method-squash
|
/retest |
|
The enhancement PR openshift/enhancements#2095 has merged, I've addressed the remaining feedback on this PR, and squashed the commits. So I think this is ready for approvals unless anyone has new feedback to discuss. |
|
@dobsonj: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
8 participants
User description
https://issues.redhat.com/browse/STOR-2859
This PR introduces an API to disable force detach in kube-controller-manager. The design is based on this slack discussion (internal only). There is already a KubeControllerManager operator type to configure cluster-kube-controller-manager-operator, and this PR introduces a ControllerManager config type to configure kube-controller-manager. This is approach is consistent with other workload components like KubeAPIServer / APIServer and KubeScheduler / Scheduler.
controllerManager.spec.forceDetachOnTimeoutwill be used to enable or disable force detach in KCM./cc @openshift/storage
PR Type
Enhancement
Description
Add ControllerManager API for cluster-wide Kubernetes controller manager configuration
Introduce ForceDetachOnTimeoutPolicy to control volume force detach behavior
Generate deepcopy, swagger documentation, and OpenAPI schema definitions
Create CustomResourceDefinition manifest for controllermanagers resource
Diagram Walkthrough
File Walkthrough
types_controllermanager.go
Define ControllerManager API types and policyconfig/v1alpha1/types_controllermanager.go
ControllerManagerstruct with metadata and specfields
ControllerManagerSpecwithForceDetachOnTimeoutPolicyfieldForceDetachOnTimeoutPolicytype with Enabled and Disabledconstants
ControllerManagerListstruct for list operationszz_generated.deepcopy.go
Generate deepcopy functions for ControllerManager typesconfig/v1alpha1/zz_generated.deepcopy.go
ControllerManager
zz_generated.swagger_doc_generated.go
Generate swagger documentation for ControllerManagerconfig/v1alpha1/zz_generated.swagger_doc_generated.go
zz_generated.openapi.go
Generate OpenAPI schema definitions for ControllerManageropenapi/generated_openapi/zz_generated.openapi.go
ControllerManagerList, and ControllerManagerSpec
with properties and dependencies
with items array
0000_10_config-operator_01_controllermanagers.crd.yaml
Create ControllerManager CustomResourceDefinition manifestconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_controllermanagers.crd.yaml
controllermanagers.config.openshift.io
Disabled, and empty string values
zz_generated.featuregated-crd-manifests.yaml
Register ControllerManager in featuregated manifestsconfig/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
manifests
information
AAA_ungated.yaml
Create ungated ControllerManager CRD manifestconfig/v1alpha1/zz_generated.featuregated-crd-manifests/controllermanagers.config.openshift.io/AAA_ungated.yaml
controllermanagers resource
annotations