Skip to content

krt: make krt and kube client indexes named#55999

Merged
istio-testing merged 5 commits intoistio:masterfrom
sschepens:krt-index-name
Apr 21, 2025
Merged

krt: make krt and kube client indexes named#55999
istio-testing merged 5 commits intoistio:masterfrom
sschepens:krt-index-name

Conversation

@sschepens
Copy link
Copy Markdown
Contributor

@sschepens sschepens commented Apr 18, 2025

Please provide a description of this PR:
Made both krt and kube client Index receive a required name parameter which is used to check uniqueness and allow reusing existing indexes.
Currently all Informers creates a default namespace index, but this is not being reused by krt in any way, so we're duplicating namespace indexes for all informers that krt indexes by namespace.

@sschepens sschepens requested review from a team as code owners April 18, 2025 11:14
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 18, 2025
@sschepens sschepens force-pushed the krt-index-name branch 3 times, most recently from 502cec4 to 69b8342 Compare April 18, 2025 11:42
@sschepens
Copy link
Copy Markdown
Contributor Author

/retest

// NewIndex creates a simple index, keyed by key K, over an informer for O. This is similar to
// NewIndex creates a simple index, keyed by key K, over a collection for O. This is similar to
// Informer.AddIndex, but is easier to use and can be added after an informer has already started.
// Different collection implementations may reuse existing indexes with the same name.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be worth noting: two different informer collections do share the same index. This differs from other collections

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added clarification for this, let me know if it's enough

pc := clienttest.NewWriter[*corev1.Pod](t, c)
c.RunAndWait(test.NewStop(t))
index := kclient.CreateStringIndex[*corev1.Pod](pods, func(pod *corev1.Pod) []string {
index := kclient.CreateStringIndex[*corev1.Pod](pods, "podIp", func(pod *corev1.Pod) []string {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For these common indexes on informers, we may want to make a common package of 'wellknown' indexes. This way its easy to know when we can share an index and its much harder to accidentally define the same name but different extraction

Copy link
Copy Markdown
Contributor Author

@sschepens sschepens Apr 21, 2025

Choose a reason for hiding this comment

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

It seems like a good idea, but would probably be more useful for krt? There seem to be very few indexes we use on kclient (only 4 indexes outside of tests), this one is actually only used in tests. With extended krt usage we would probably reduce the amount of kclient indexes we create.

@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Apr 21, 2025
@howardjohn
Copy link
Copy Markdown
Member

/retest

@istio-testing
Copy link
Copy Markdown
Collaborator

@sschepens: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
integ-ambient-mc_istio 9125d5b link false /test integ-ambient-mc
Details

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-sigs/prow repository. I understand the commands that are listed here.

@istio-testing istio-testing merged commit e750708 into istio:master Apr 21, 2025
29 of 30 checks passed
@sschepens sschepens deleted the krt-index-name branch April 21, 2025 17:40
yxun pushed a commit to yxun/istio that referenced this pull request Apr 23, 2025
* make krt and kube client indexes named to allow reusing indexes

* comment

* comment
tjons pushed a commit to tjons/istio that referenced this pull request Apr 26, 2025
* make krt and kube client indexes named to allow reusing indexes

* comment

* comment
aslakknutsen pushed a commit to aslakknutsen/istio that referenced this pull request Jul 1, 2025
* make krt and kube client indexes named to allow reusing indexes

* comment

* comment
fjglira pushed a commit to fjglira/istio that referenced this pull request Sep 26, 2025
* upstream/master:
  remove 1.23 compatibility profile (istio#56023)
  Create ambient multinetwork flag (istio#55991)
  krt: make krt and kube client indexes named (istio#55999)
  Automator: update proxy@master in istio/istio@master (istio#56026)
  fix istio-revision-tag-default MutatingWebhookConfigurations is not created during installation (istio#56004)
  doc: fix typo in accesslog test (istio#55117)
  fix(networking): Add absolute FQDNs (trailing dot) to VirtualHost domains (istio#56008)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/perf and scalability release-notes-none Indicates a PR that does not require release notes. 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.

4 participants