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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Sidecar Scopes #41647

Closed
wants to merge 16 commits into from
Closed

Update Sidecar Scopes #41647

wants to merge 16 commits into from

Conversation

sschepens
Copy link
Contributor

@sschepens sschepens commented Oct 26, 2022

Please provide a description of this PR:
With Issue #41453 it was raised that currently Istio rebuilds it's internal indexes whenever any configuration changes.

This is a starting PR to start processing Sidecar updates in a more efficient manner.
Whenever only sidecar resources are updated, we can safely only process changed sidecars, using the old index but:

  • deleting sidecar scopes which were deleted
  • regenerating sidecar scopes which were updated
  • creating sidecar scopes which were created

breaking changes:

  • sidecar ordering: currently sidecars were ordered by creation date, this allowed that if multiple sidecars matched a workload, the sidecar that was actually selected was predictable (oldest sidecar). I'm guessing this shouldn't actually be a problem because it's not something anyone should be doing and the behavior is actually contrary to what documentation specifies. The behavior of the system is undefined if more than one selector-less Sidecar configurations exist in a given namespace. The behavior of the system is undefined if two or more Sidecar configurations with a workloadSelector select the same workload instance.

questions:

  • I'm actually reusing and modifying the previous sidecarIndex, could this actually be a problem? 馃 Copying the maps would probably be too costly in the long run, but maybe we need to use sync.Map.
  • I'm detecting resource deletions whenever we get a nil response from store.Get, I really don't know if this is correct or if there would be a better way of knowing this.

@howardjohn @hzxuzhonghu @ramaraochavali since you were involved in the issue.

I'm sending this as a draft to see if the approach is correct and have some feedback.

My intentions would be to continue expanding this way of efficient processing of updates into all resources eventually, one at a time.

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 26, 2022
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Oct 26, 2022
@istio-testing
Copy link
Collaborator

Hi @sschepens. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@sschepens sschepens marked this pull request as ready for review October 27, 2022 15:22
@sschepens sschepens requested a review from a team as a code owner October 27, 2022 15:22
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 27, 2022
@hzxuzhonghu
Copy link
Member

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Oct 28, 2022
@hzxuzhonghu
Copy link
Member

@sschepens I think you can apply to be istio member, the it will no need manually label ok-to-test

@hzxuzhonghu
Copy link
Member

I think the most common scenario is service/vs, etc updates, not sidecar itself. So I wonder can we only do delta update sidecarScope with the granularity of resource type.

@sschepens
Copy link
Contributor Author

@sschepens I think you can apply to be istio member, the it will no need manually label ok-to-test

Sure, would love, is there any documentation on the process?

I think the most common scenario is service/vs, etc updates, not sidecar itself. So I wonder can we only do delta update sidecarScope with the granularity of resource type.

Yes I know this is the case, I just want to start making incremental changes, after this PR, I will add only to update Sidecars which had services/vs/drs changed, and then some other more changes. Breaking this up makes it easier to review.

@sschepens sschepens requested review from a team as code owners October 28, 2022 14:00
@hzxuzhonghu
Copy link
Member

@sschepens sschepens changed the title [Draft] Update Sidecar Scopes Update Sidecar Scopes Oct 31, 2022
@sschepens
Copy link
Contributor Author

@sschepens FYI https://github.com/istio/community/blob/master/ROLES.md#member

thanks @hzxuzhonghu created a PR for adding myself as member istio/community#856

@sschepens
Copy link
Contributor Author

@howardjohn would really like your feedback here

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Nov 28, 2022
@istio-policy-bot
Copy link

馃毀 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-10-29. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/perf and scalability lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants