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
endpoint: move patching update functionality to pkg/endpoint #8968
Conversation
c562ab2
to
0997b90
Compare
test-me-please |
Build failure: https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/14511/ Node lost? |
test-me-please |
test-me-please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally +1. I'm not very keen on ' ValidPatchTransitionState` but I'm not sure if you wanted to keep this a copy-paste refactor or were willing to change the order of operations.
pkg/endpoint/api.go
Outdated
// purposes should a caller choose to try to regenerate this endpoint, as well | ||
// as an error if the Endpoint is being deleted, since there is no point in | ||
// changing an Endpoint if it is going to be deleted. | ||
func (e *Endpoint) ProcessChangeRequest(epTemplate *models.EndpointChangeRequest, newEp *Endpoint) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should endpoint.NewEndpointFromChangeModel
also move here since it's strictly an API related thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually, I'd like to do that. However, I'd have to then perform an endpointmanager lookup in the endpoint to see if the endpoint exists, which I can't do right now because the endpointmanager cannot be provided as a parameter anywhere.
0997b90
to
e3ff094
Compare
test-me-please |
test-me-please |
…oint package This removes the leakage of a lot of internal endpoint implementations unnecessarily. Hide this in a function which the daemon can call instead. Signed-off by: Ian Vernon <ian@cilium.io>
This function is only used by `pkg/endpoint` now, do not export it. Signed-off by: Ian Vernon <ian@cilium.io>
The `newEp` parameter has all the information we need; we don't need to pass in the API model as well. As such, move the `ValidPatchTransitionState` function out of `pkg/endpoint` back to `daemon` since the Endpoint package should not be aware of what a `PATCH` operation is against it. Signed-off by: Ian Vernon <ian@cilium.io>
Signed-off by: Ian Vernon <ian@cilium.io>
cab55b8
to
0f5491b
Compare
Had to resolve conflicts, testing again |
test-me-please |
Running tests again, Cilium operator is taking forever to get ready:
|
test-me-please |
Failure is BPF NodePort with VXLAN: https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/14596/testReport/junit/Suite-k8s-1/10/K8sServicesTest_Checks_service_across_nodes_Tests_NodePort_BPF_Tests_with_vxlan/ Unrelated to these changes - this has been flaky in master. Merging. |
A lot of Endpoint implementation information, including locking, internal fields, etc. was being leaked outside of the Endpoint package in the handling of
PATCH /endpoint
. Factor this into a function within the Endpoint package instead. As a result of this, the functionForcePolicyCompute
is no longer used outside ofpkg/endpoint
, so do not export it.Signed-off by: Ian Vernon ian@cilium.io
This change is