Skip to content

Conversation

@eduardodbr
Copy link
Member

@eduardodbr eduardodbr commented Nov 26, 2025

Motivation

We’ve seen several cases of unexpected behavior that appear to be caused by the write-back mechanism, and Kubernetes docs (and maintainers kubernetes-sigs/controller-runtime#1622) recommend avoiding writes to the informer store. Although turning it off may increase the frequency of 409 Conflict errors, it should help reduce unpredictable controller behavior.

Modifications

Verification

Documentation

@eduardodbr
Copy link
Member Author

/retest

@eduardodbr eduardodbr marked this pull request as ready for review November 30, 2025 18:52
@Joibel Joibel requested a review from Copilot December 1, 2025 11:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR changes the default behavior of the INFORMER_WRITE_BACK environment variable from true to false to address unexpected controller behavior. The change aligns with Kubernetes best practices that recommend avoiding writes to the informer store, though it may result in more 409 Conflict errors.

Key Changes:

  • Modified the switch statement logic in operator.go to make the empty string case default to "false" behavior instead of "true"
  • Updated documentation to reflect the new default value of false
  • Added feature documentation explaining the rationale for this change

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
workflow/controller/operator.go Swapped the case statements to make empty string default to the "false" behavior (no write-back to informer cache) instead of "true"
docs/environment-variables.md Updated the default value for INFORMER_WRITE_BACK from true to false in the environment variables table
.features/pending/disable-write-back-informer.md Added feature documentation explaining the motivation for disabling write-back informer by default

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines +801 to 802
// this does not reduce errors, but does reduce
// conflicts and therefore we log fewer warning messages.
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The comment is now grammatically incomplete after the partial removal. It starts with lowercase "this does not reduce errors" which makes it unclear what "this" refers to. Consider revising to: "Writing back to the informer does not reduce errors, but does reduce conflicts and therefore we log fewer warning messages."

Suggested change
// this does not reduce errors, but does reduce
// conflicts and therefore we log fewer warning messages.
// Writing back to the informer does not reduce errors, but does reduce conflicts and therefore we log fewer warning messages.

Copilot uses AI. Check for mistakes.
return
}
case "false":
// no longer write back to informer cache as default (as per v4.0)
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The comment on line 808 starts with lowercase and lacks proper sentence structure. Consider revising to: "No longer write back to informer cache as default (as per v4.0)."

Suggested change
// no longer write back to informer cache as default (as per v4.0)
// No longer write back to informer cache as default (as per v4.0).

Copilot uses AI. Check for mistakes.
| `EVENT_AGGREGATION_WITH_ANNOTATIONS` | `bool` | `false` | Whether event annotations will be used when aggregating events. |
| `GZIP_IMPLEMENTATION` | `string` | `PGZip` | The implementation of compression/decompression. Currently only "`PGZip`" and "`GZip`" are supported. |
| `INFORMER_WRITE_BACK` | `bool` | `true` | Whether to write back to informer instead of catching up. |
| `INFORMER_WRITE_BACK` | `bool` | `false` | Whether to write back to informer instead of catching up. |
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

There's an extra trailing space after false in the default value column. This should be removed for consistency with other entries in the table.

Suggested change
| `INFORMER_WRITE_BACK` | `bool` | `false` | Whether to write back to informer instead of catching up. |
| `INFORMER_WRITE_BACK` | `bool` | `false` | Whether to write back to informer instead of catching up. |

Copilot uses AI. Check for mistakes.
@Joibel Joibel merged commit 092e36b into argoproj:main Dec 4, 2025
72 of 76 checks passed
guanguxiansheng pushed a commit to guanguxiansheng/argo-workflows that referenced this pull request Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants