Clarify cluster operator Progressing condition#2829
Clarify cluster operator Progressing condition#2829hongkailiu wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
Hello @hongkailiu! Some important instructions when contributing to openshift/api: |
|
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 selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Review rate limit: 8/10 reviews remaining, refill in 10 minutes and 48 seconds. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1/types_cluster_operator.go (1)
159-166: ⚡ Quick winClarify “any pods-managing owned resource” to avoid re-narrowing intent.
The updated doc now says operators should not report
Progressing“only because resources owned by them, such as DaemonSets and Deployments” are adjusting due to node scale-up / upgrade reboots. While “such as” signals examples, explicitly naming only those two kinds may still lead some CO owners to incorrectly assume the rule is limited to DaemonSets/Deployments.Consider tweaking the sentence to directly match the broader intent (e.g., “owned Kubernetes resources that manage pods, such as …”), or add “including, but not limited to” to make it unambiguous that the guidance applies to any relevant owned pods-managing controller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_cluster_operator.go` around lines 159 - 166, Update the Progressing status doc string to avoid narrowing scope: replace the phrase "such as DaemonSets and Deployments" with wording that clearly covers all owned pod-managing resources, e.g. "owned Kubernetes resources that manage pods (including, but not limited to, DaemonSets and Deployments)"; edit the comment block that documents Progressing in types_cluster_operator.go so the guidance applies to any relevant owned pods-managing controller rather than implying only DaemonSets and Deployments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/v1/types_cluster_operator.go`:
- Around line 159-166: Update the Progressing status doc string to avoid
narrowing scope: replace the phrase "such as DaemonSets and Deployments" with
wording that clearly covers all owned pod-managing resources, e.g. "owned
Kubernetes resources that manage pods (including, but not limited to, DaemonSets
and Deployments)"; edit the comment block that documents Progressing in
types_cluster_operator.go so the guidance applies to any relevant owned
pods-managing controller rather than implying only DaemonSets and Deployments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 194e02aa-c4b9-4f1e-950d-aca16a7e3360
📒 Files selected for processing (1)
config/v1/types_cluster_operator.go
It clarifies that a new node from cluster scaleup or rebooting during cluster upgrade does not trigger COs to go Progressing. DaemonSet is an example and commonly seen in CI. It could be any Kubernetes resource that manages pods. I hope that this helps reduce the confusion from CO owners [1]. Todo: Sync the change to counterpart in o/enhancement [2] if it is accepted. [1]. https://redhat.atlassian.net/browse/OCPBUGS-62629?focusedCommentId=16850780 [2]. https://github.com/openshift/enhancements/blob/master/dev-guide/cluster-version-operator/dev/clusteroperator.md#conditions
Review Summary by QodoClarify cluster operator Progressing condition documentation
WalkthroughsDescription• Clarifies Progressing condition to include all resource types • Expands example from DaemonSets to include Deployments • Improves documentation clarity for cluster operator owners Diagramflowchart LR
A["Progressing Condition<br/>Documentation"] -->|"Clarifies scope"| B["Resources: DaemonSets<br/>and Deployments"]
B -->|"Adjusting to"| C["New nodes from<br/>scaleup or reboot"]
A -->|"Specifies when NOT<br/>to report Progressing"| D["Reconciling known<br/>state"]
File Changes1. config/v1/types_cluster_operator.go
|
|
@hongkailiu: 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. |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking 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 |
Clarify cluster operator Progressing condition
It clarifies that a new node from cluster scaleup or rebooting during cluster upgrade does not trigger COs to go Progressing. DaemonSet is an example and commonly seen in CI. It could be any Kubernetes resource that manages pods.
I hope that this helps reduce the confusion from CO owners [1].
Todo: Sync the change to counterpart in o/enhancement [2] if it is accepted.
[1]. https://redhat.atlassian.net/browse/OCPBUGS-62629?focusedCommentId=16850780
[2]. https://github.com/openshift/enhancements/blob/master/dev-guide/cluster-version-operator/dev/clusteroperator.md#conditions