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

resource: Add Resource[Endpoints] and adapt existing watchers #23977

Merged
merged 5 commits into from Jun 5, 2023

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Feb 23, 2023

Adds Resource[*Endpoints] which moves the resolving of the Endpoint or EndpointSlice into the constructor for the resource. K8sWatcher refactored to use the Resource[*Endpoints] and code cleaned up to work with just *Endpoints rather than the 3 different kinds. Some messiness around collapsing them into one type and hiding the underlying resource kind. Please review those changes carefully (CachesSynced, metrics in endpoints watcher, ipcache.ResourceKindEndpoint). I've tried to keep existing behavior as much as I could, but could've potentially missed something.

See individual commits and #23734 for motivation.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 23, 2023
@joamaki joamaki added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 23, 2023
@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch 2 times, most recently from 326c10a to 01d77ee Compare March 1, 2023 16:30
@github-actions
Copy link

github-actions bot commented Apr 1, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 1, 2023
@github-actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Apr 16, 2023
@joamaki joamaki reopened this May 2, 2023
@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from 01d77ee to 076a3d8 Compare May 2, 2023 13:49
@joamaki joamaki changed the base branch from master to main May 2, 2023 14:00
@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from 076a3d8 to 6a4a97d Compare May 2, 2023 14:07
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label May 3, 2023
@joamaki joamaki self-assigned this May 5, 2023
@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from 6a4a97d to 39cabe5 Compare May 15, 2023 12:33
@joamaki
Copy link
Contributor Author

joamaki commented May 22, 2023

/test

@joamaki joamaki marked this pull request as ready for review May 22, 2023 12:40
@joamaki joamaki requested review from a team as code owners May 22, 2023 12:40
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

Now looks good to me 👍

Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

LGTM for me

@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from c71a567 to 2fbc948 Compare May 29, 2023 13:06
@joamaki
Copy link
Contributor Author

joamaki commented May 29, 2023

/test

@christarazi christarazi added area/daemon Impacts operation of the Cilium daemon. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/modularization labels May 29, 2023
@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from 2fbc948 to bfbf7fc Compare May 30, 2023 08:24
@joamaki
Copy link
Contributor Author

joamaki commented May 31, 2023

/test

@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from bfbf7fc to 33984b4 Compare May 31, 2023 10:46
@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Jun 1, 2023

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing Tests with XDP, direct routing, DSR with Geneve and Maglev

Failure Output

FAIL: Can not connect to service "http://[fd04::11]:31599" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/351/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@joamaki
Copy link
Contributor Author

joamaki commented Jun 1, 2023

/test-1.26-net-next

@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from 33984b4 to e7257e8 Compare June 1, 2023 14:01
@YutaroHayakawa
Copy link
Member

/test

@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Jun 2, 2023

Oh, it conflicts again. @joamaki can't I get the write access to your fork (maybe it's hard)? So that I can take care of resolve conflict and trigger the CI while you are sleeping.

Add the option to transform the object before it's added to the store.
Useful in situations where we want to store a subset of the data or
a derivate object vs the upstream object.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
In preparation of making endpoints a Resource, allow transforming K8s
Endpoints/EndpointSlices into our internal representation (k8s.Endpoints)
and to store it in cache.Store, this adds the object metadata to it.

The "EndpointSliceID" is included in k8s.Endpoints so it doesn't
need to be passed separately.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Add Resource[*Endpoints] which probes the api-server capabilities
to pick the right upstream resource kind and expose endpoints with
our internal type rather than requiring each use-site to figure
this out on their own.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
- Remove endpoint and endpoint_slice watchers
- Add "endpoints watcher" adapted from endpoint_slice watcher that uses
  Resource[*Endpoints] instead of informer. This keeps things the same
  downstream.
- Adapts BGP speaker to only handle *k8s.Endpoints
- Removes endpoint/endpointslice informers from operator and use resource instead

Closes: cilium#23734
Signed-off-by: Jussi Maki <jussi@isovalent.com>
This was used by the EndpointSlice watcher to check whether there
really existed an EndpointSlice in addition to the API server
supporting them. Since EndpointSlices have been the default for
many versions now, we don't need this level of paranoia.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/joamaki/endpoints-resource branch from e7257e8 to ed3573b Compare June 2, 2023 07:16
@joamaki
Copy link
Contributor Author

joamaki commented Jun 2, 2023

/test

@joamaki
Copy link
Contributor Author

joamaki commented Jun 2, 2023

test-1.16-4.19 likely hitting #25524? Since test on 1.16 is important (to check that Endpoint object handling works) I'll rerun the test.

error: unable to upgrade connection: Authorization error (user=kube-apiserver-kubelet-client, verb=create, resource=nodes, subresource=proxy)

@joamaki
Copy link
Contributor Author

joamaki commented Jun 2, 2023

/test-1.16-4.19

@joamaki joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 2, 2023
@borkmann borkmann merged commit 12a3adf into cilium:main Jun 5, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. area/modularization ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants