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.

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

Remove deployment updates from autodetect loop #869

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

rubenvp8510
Copy link
Collaborator

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

In #454 we introduced the detection/update of deployments to address #442, But after seen this closely, I think this is not the right approach. We should run the scan only at start.

More than that, In my latest tests debugging for fix #855. I see that we don't need to run the scan any more (operator-sdk 0.12.0) , because for some reason (I'm still not sure why) the reconciliation run on each deployment at bootstrap time (see logs). Not sure when this behaviour changed.

bootstrap-operator.txt

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. Just need one clarification before we merge it:

because for some reason (I'm still not sure why) the reconciliation run on each deployment at bootstrap time (see logs). Not sure when this behaviour changed.

It would be good to check the changelogs and/or ask the Operator SDK team (Slack or mailing list) about this. Otherwise, we might end up depending on a behavior that might be a bug, to be fixed in a future version.

@kevinearls
Copy link
Contributor

@objectiser @jpkrohling @rubenvp8510 What is the status of this? E2E tests are broken on OpenShift without this.

@jpkrohling
Copy link
Contributor

@rubenvp8510 please, find evidence that this new behavior is the intended one, rather than a bug that has been introduced in the latest Operator SDK versions.

@rubenvp8510
Copy link
Collaborator Author

rubenvp8510 commented Jan 28, 2020

I asked on kubernetes-operator slack channel.

Ruben Vargas Today at 7:38 AM
Hi, question, I'm developing an operator that watches deployments and inject sidecars on deployments containing certain annotations. If the operator is not running and a new deployment is created, is an expected behavior when the operator runs the first time it scan all deployments when it start watching for it?

joelanford 3 hours ago
Yep, that’s correct.Also btw, this sounds like a use case for a mutating admission webhook. If you haven’t already, you may want to consider that as an option as well.

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

@jpkrohling As the question seems to have been answered, and you previously indicated LGTM (subject to that answer) I will merge.

@objectiser objectiser merged commit d169d79 into jaegertracing:master Jan 31, 2020
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.

Duplicated "Injecting sidecar" message
4 participants