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

Pr/v1.13 backport 2023 01 23 2 #23238

Closed
wants to merge 5 commits into from
Closed

Conversation

aanm
Copy link
Member

@aanm aanm commented Jan 23, 2023

Once this PR is merged, you can update the PR labels via:

$ for pr in 23180 21744 22844; do contrib/backporting/set-labels.py $pr done 1.13; done

or with

$ make add-label BRANCH=v1.13 ISSUES=23180,21744,22844

joamaki and others added 5 commits January 23, 2023 11:20
This introduces a set of centrally managed resource.Resource[T] objects. These aim
to replace the current k8s/watchers and k8s/watchers/subscriber, with a single
interface (Resource[T]) that one can be observed and its store queried from any cell.
With this we'll gradually remove K8sWatcher by moving code from k8s/watchers/ into feature
specific packages that subscribe to Resource[T]. See pkg/k8s/resource/example.go for usage
example of Resource[T].

To start things off, in this patch the Resource[*Service] and Resource[*Node] are introduced
into the object graph and the existing service and node watchers are refactored to be
implemented on top of it. This keeps the existing code working, but allows new cells to
use these objects directly without dependency on K8sWatcher.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Add Resource[*slim_corev1.Namespace] to the shared resources and refactor
the namespace watcher to use it.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
In PR #22397 the nodespecer implementation was turned into a cell. That
implementation used the localNodeStore to get access to a generic
node object that would contain annotations, labels and podCIDRs.

However, it turns out that the localNodeStore doesn't function as one
might expect and didn't register updates properly.

This commit changes the implementation to use resources to subscribe
to changes in the node objects of the API server directly. This is
very much like the old pre-modularization implementation, but now using
resources instead of informers.

Fixes: #23155
Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
As Resource[T] was used more it became apparent that making it an
stream.Observable and having Event[T] be an interface makes it inconvenient
to use:

- Almost always we're subscribing to it for the whole duration of the
  application and do not need to handle completion errors. Consuming the events
  via a channel is also often the preferred way as it always "for-select"
  over multiple resources.

- Event[T] contains almost the same payload for all event types, we might
  as well keep it simple and just use a single struct.

This changes Observe() into `Events(ctx, opts) <-chan Event[T]` and moves
rate limiting and error handling to be per-subscriber options. Event[T]
becomes a single struct with a kind tag to specific event type.

In order to handle large datasets at startup the events are now emitted
immediately rather than waiting for informer to synchronize the store
first.

The BenchmarkResource in resource_test.go was also ported to current master
and the difference between the unbuffered channel approach to callback was
about 2x (2600ns/op for channels, 1350ns/op for callbacks), which is negligble
enough. Benchmark was also ran with buffered event channel with no difference
to unbuffered.  Unbuffered is preferable as allows better chances for coalescing
away the intermediate updates for objects with the same key.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
The commit "resource: Refactor the API for simplified usage" was tested
against older master, before the commit "cli: Add configuration flag for service LB"
which used the older API, causing the build to fail.

This updates StartCECController() to use the new Resource.Events method.

Fixes: f3a9b0a ("resource: Refactor the API for simplified usage")
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Jan 23, 2023
@aanm
Copy link
Member Author

aanm commented Jan 24, 2023

replaced by #23276

@aanm aanm closed this Jan 24, 2023
@aanm aanm deleted the pr/v1.13-backport-2023-01-23-2 branch January 24, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants