Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove updates from pod mutating webhook #2584

Merged
merged 4 commits into from
Mar 4, 2024
Merged

Remove updates from pod mutating webhook #2584

merged 4 commits into from
Mar 4, 2024

Conversation

andrewdinunzio
Copy link
Contributor

@andrewdinunzio andrewdinunzio commented Jan 30, 2024 •
edited by pavolloffay
Loading

Description:

The opentelemetry operator sometimes modifies pods in an invalid way. I've only ever experienced this happening on UPDATE events, and since it's not necessary to update pods, this change removes UPDATE from the webhook configs and leaves only CREATE.

Link to tracking Issue: Resolves: #1514

Testing: No tests added.

Documentation: No documentation added.

@andrewdinunzio andrewdinunzio requested a review from a team January 30, 2024 18:40
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jan 30, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@andrewdinunzio andrewdinunzio mentioned this pull request Jan 30, 2024
Closed
@andrewdinunzio
Copy link
Contributor Author

Nevermind, I just had to run find . -name "*.yaml" | xargs dos2unix and now all tests pass for me locally.

@swiatekm
Copy link
Contributor

@andrewdinunzio could you add a changelog entry? You just need to run make chlog-new and fill out the newly created yaml file in ~/.chloggen.

The change itself looks fine to me, but I'd like @jaronoff97 and @pavolloffay to weigh in as well, as it potentially has significant impact.

@andrewdinunzio
Copy link
Contributor Author

@andrewdinunzio could you add a changelog entry? You just need to run make chlog-new and fill out the newly created yaml file in ~/.chloggen.

Added. I set it as a "bug_fix". I don't think it should be a breaking change, but I don't know this repo very well so need confirmation about that.

@pavolloffay
Copy link
Member

@andrewdinunzio could you please resolve conflicts?

@pavolloffay
Copy link
Member

@andrewdinunzio would it as well resolve issues linked in #1514 (comment) ?

@pavolloffay
Copy link
Member

Ping @andrewdinunzio the conflicts needs to be resolved

@andrewdinunzio
Copy link
Contributor Author

Used the main branch in my fork so syncing it closed the PR, but will reopen once I make the updates. Doing that now.

@andrewdinunzio
Copy link
Contributor Author

Updated!

@andrewdinunzio
Copy link
Contributor Author

@pavolloffay if you could check it over once more I think it should be good to go now

@andrewdinunzio
Copy link
Contributor Author

reopening

@andrewdinunzio
Copy link
Contributor Author

looks like it's good to merge?

iblancasa
iblancasa approved these changes Feb 29, 2024
View reviewed changes
.chloggen/main.yaml Outdated Show resolved Hide resolved
@andrewdinunzio
Copy link
Contributor Author

Thanks! I don't have the power to merge :D is there anything this is waiting on?

@swiatekm
Copy link
Contributor

swiatekm commented Mar 1, 2024

Only waiting for @pavolloffay 's approval at this point.

@andrewdinunzio
Copy link
Contributor Author

Gotcha, thanks. He approved, but after fixing the conflict I re-requested his approval

@andrewdinunzio andrewdinunzio reopened this Mar 4, 2024
@pavolloffay pavolloffay merged commit 7b64ec8 into open-telemetry:main Mar 4, 2024
29 of 36 checks passed
@fujin fujin mentioned this pull request Mar 21, 2024
Merged
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
@andrewdinunzio
Remove updates from pod mutating webhook (open-telemetry#2584)
2e18cac 
* Remove 'update' from pod mutating webhook for autoinstrumentation

* Revert shebang

* Add chlog entry

* Rename chloggen file
@fyuan1316 fyuan1316 mentioned this pull request Jul 2, 2024
Closed
@pavolloffay pavolloffay mentioned this pull request Jul 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm approved these changes

@jaronoff97 jaronoff97 jaronoff97 approved these changes

@pavolloffay pavolloffay pavolloffay approved these changes

@iblancasa iblancasa iblancasa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenTelemetry webhook modifies orphaned pods in an invalid way
5 participants
@andrewdinunzio @swiatekm @pavolloffay @iblancasa @jaronoff97