Operator api server modularization #24228

pippolo84 · 2023-03-07T20:34:53Z

Modularize the operator api server into its own independent cell. The refactoring aims to keep the same behavior of the previous implementation. The refactored server:

adds a custom handler for /healthz as an alias to /v1/healthz
in case of an empty server address option, it starts two HTTP servers, one listening on "127.0.0.1:0" and the other on "[::1]:0", in case the user is running ipv4 or ipv6 only, respectively

To answer the health checks, the operator api server should always be up and running, whether the operator has been elected leader or not.
Therefore, the health handler behavior should change based on the operator instance being elected leader or not, so it is refactored to accept a function that returns the current leading status of the operator instance.

Integration style unit tests have been added for both the handlers and the server.

Fixes: #24064

operator/api/cell.go

operator/api/server.go

operator/cmd/root.go

joamaki · 2023-03-20T09:43:15Z

operator/cmd/root.go

+			operatorCfg *operatorOption.OperatorConfig,
+		) api.SharedConfig {
+			return api.SharedConfig{
+				EnableK8s: operatorCfg.EnableK8s,


I think it'd be preferable to this SharedConfig to instead check for k8s enabled status via the k8s clientset: clientset.IsEnabled(), rational being that it's better to have only a single source of truth.

Hmm, or is the problem that the k8s client gets configured and we want to use EnableK8s to overriding? Alternative I suppose is to explicitly Disable() the client based on EnableK8s in an Invoke. That would also make sure that the heartbeats aren't running. Though need to be careful that the Invoke runs early enough before any calls to clientset.IsEnabled().

... almost feels like the cleanest solution would be to make enable-k8s part of client.Config, but that would also then introduce it to cilium-agent and others. Still maybe not a bad idea?

I agree that the best solution is to move enable-k8s as part of client.Config.
I don't think it will be a problem to introduce it to other binaries. The original PR for this was mentioning that the other binaries were already working correctly. Specifically, I think the agent was behaving properly because of the clientset.IsEnabled() checks.
Instead the operator had some code paths that where not guarded by that, so an additional flag was added (adding a new one was easier, I suppose).

operator/api/health.go

nathanjsweet

LGTM for cli, sig-k8s, api

christarazi

LGTM mostly. My comments are around commit squashing, especially to squash test commits with the implementation commits and any cmdref changes squashed into the commits that introduced them.

operator/api/metrics_test.go

operator/api/health_test.go

Documentation/cmdref/cilium-operator-alibabacloud_hive.md

pippolo84 · 2023-04-04T09:02:17Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Cilium operator was not able to get into ready state

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

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Cilium operator was not able to get into ready state

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

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

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Cilium operator was not able to get into ready state

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.

christarazi

LGTM, just one non-blocking nit: I think we meant to squash "operator/api: Add api server tests" with the previous commit.

pippolo84 · 2023-04-07T20:45:32Z

LGTM, just one non-blocking nit: I think we meant to squash "operator/api: Add api server tests" with the previous commit.

You're right, I was missing that. Squashed 👍

pippolo84 · 2023-04-08T09:23:38Z

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Cilium operator was not able to get into ready state

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.24-kernel-5.4/1668/

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

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Cilium operator was not able to get into ready state

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/1684/

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

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Cilium operator was not able to get into ready state

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/845/

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

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

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Cilium operator was not able to get into ready state

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

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.

christarazi · 2023-04-09T19:16:14Z

Seems like a legit failure with the Operator

Modularize operator api server metrics handler into its own independent cell. This will improve the testability of the handler. The old code is left untouched to complete the server refactoring in another commit. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Add HTTP response "Not Implemented" for the oeprator api server health handler. The code is generated with go-swagger running `make generate-operator-api`. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Modularize operator api server health handler into its own independent cell. This will improve the testability of the handler. The old code is left untouched to complete the server refactoring in another commit. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Avoid initialization of leader election context and cancel function at package import time to allow restarting the operator multiple times in controlplane testing. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Modularize the operator api server into its own independent cell. The refactoring aims to keep the same behavior of the previous implementation. Specifically, the refactored server: - adds a custom handler for /healthz as an alias to /v1/healthz - starts two HTTP servers, one listening on "127.0.0.1:0" and the other on "[::1]:0", in case the user is running ipv4 or ipv6 only Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Avoid specifying a port for the operator API server in controlplane testing. This can cause port clashing in the test environment, leading to test flakyness. Instead, let the kernel pick a random available port using ":0". Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Even if the operator is not leading, it should answer to the health check requests. In other words, the operator api server should always be up and running, whether the operator has been elected leader or not. In case the operator is elected leader, the health handler should actually check the server version and, if configured, the connection to the kvstore, while it should simply return a `200 OK` response code if the operator is not leading. To configure the health handler behavior after the leader election takes place, the health handler accepts a function to get the current leading status of the operator instance. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

The CNPStatusNodesGC test verifies the garbage collection of the stale status nodes updates at the operator startup, when the related config option is set. The test is divided in two parts: the first one verifies that the stale updates are not cleaned up when the GC is disabled, while the second one verifies the opposite. The first issue with the current code is the incorrect call to DeleteObjects, that is lacking the references to the objects to delete. IOW, the DeleteObjects calls is a no-op, and the second part of the test depends on what the first part leaves in the object trackers. Fixing this issue revealed another one. The modularization of the operator api server impacted the timing of the operator startup sequence. This sequence includes the legacyCell, that contains the code responsible for the startup of all the operator subsystems not yet modularized, including the nodes status updates GC. This resulted in the one-shot GC running before the object trackers held the CNPs needed for the test, ultimately leading to a test failure. To fix this timing issue, we insert all the k8s objects needed for the test before restarting the agent and the operator. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

The enable-k8s option has been introduced to avoid some operator code paths that assumed to run in a k8s environment. Instead of passing around the flag, we use it to configure the clientset, so that the check `clientset.IsEnabled()` will keep that too into account and avoid the execution of kubernetes-specific code. The operator api server cell and tests have been updated accordingly. Related: cilium#21344 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 · 2023-04-12T09:59:58Z

/test

pippolo84 · 2023-04-12T13:50:56Z

Seems like a legit failure with the Operator

Previous revision was incorrectly trying to check for external kvstore status from the operator replica not elected leader. Fixed it with this patch:

--- a/operator/api/health.go
+++ b/operator/api/health.go
@@ -61,11 +61,9 @@ func (h *healthHandler) Handle(params operator.GetHealthzParams) middleware.Resp
                return operator.NewGetHealthzNotImplemented()
        }
 
-       if !h.isOperatorLeading() {
-               if err := h.checkStatus(); err != nil {
-                       h.log.WithError(err).Warn("Health check status")
-                       return operator.NewGetHealthzInternalServerError().WithPayload(err.Error())
-               }
+       if err := h.checkStatus(); err != nil {
+               h.log.WithError(err).Warn("Health check status")
+               return operator.NewGetHealthzInternalServerError().WithPayload(err.Error())
        }
 
        return operator.NewGetHealthzOK().WithPayload("ok")
@@ -74,7 +72,12 @@ func (h *healthHandler) Handle(params operator.GetHealthzParams) middleware.Resp
 // checkStatus verifies the connection status to the kvstore and the
 // k8s apiserver and returns an error if any of them is unhealthy
 func (h *healthHandler) checkStatus() error {
-       if h.kvstoreEnabled() {
+       // We check if we are the leader here because only the leader has
+       // access to the kvstore client. Otherwise, the kvstore client check
+       // will block. It is safe for a non-leader to skip this check, as the
+       // it is the leader's responsibility to report the status of the
+       // kvstore client.
+       if h.isOperatorLeading() && h.kvstoreEnabled() {
                client := kvstore.Client()
                if client == nil {
                        return errors.New("kvstore client not configured")

pippolo84 · 2023-04-12T13:54:45Z

test-runtime is hitting #24342
Tracked the flake in the related issue, rerunning.

pippolo84 · 2023-04-12T13:55:01Z

/test-runtime

pippolo84 · 2023-04-12T14:24:18Z

/test-1.26-net-next

pippolo84 added area/operator Impacts the cilium-operator component release-note/misc This PR makes changes that have no direct user impact. area/modularization labels Mar 7, 2023

pippolo84 force-pushed the pr/pippolo84/operator-api-server-modularization branch 5 times, most recently from 187b6e7 to cb1206f Compare March 10, 2023 16:48

pippolo84 force-pushed the pr/pippolo84/operator-api-server-modularization branch 4 times, most recently from 4128fa6 to 2c4547d Compare March 19, 2023 23:08

pippolo84 marked this pull request as ready for review March 19, 2023 23:48

pippolo84 requested review from a team as code owners March 19, 2023 23:48

pippolo84 requested review from christarazi, nathanjsweet and joamaki March 19, 2023 23:48

joamaki requested changes Mar 20, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/operator-api-server-modularization branch from 2c4547d to c3a0f21 Compare March 21, 2023 11:42

pippolo84 requested review from a team as code owners March 21, 2023 11:42

pippolo84 requested a review from joamaki March 21, 2023 14:09

nathanjsweet approved these changes Mar 24, 2023

View reviewed changes

nathanjsweet requested a review from a team March 24, 2023 17:25

joamaki approved these changes Mar 28, 2023

View reviewed changes

christarazi reviewed Apr 4, 2023

View reviewed changes

operator/api/metrics_test.go Outdated Show resolved Hide resolved

operator/api/health_test.go Outdated Show resolved Hide resolved

Documentation/cmdref/cilium-operator-alibabacloud_hive.md Outdated Show resolved Hide resolved

pippolo84 force-pushed the pr/pippolo84/operator-api-server-modularization branch from 3a14638 to 96b531b Compare April 4, 2023 07:59

pippolo84 requested a review from christarazi April 4, 2023 08:00

christarazi approved these changes Apr 7, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/operator-api-server-modularization branch from 96b531b to 920d9aa Compare April 7, 2023 20:44

pippolo84 force-pushed the pr/pippolo84/operator-api-server-modularization branch from 920d9aa to 53f14f0 Compare April 12, 2023 09:34

pippolo84 added 9 commits April 12, 2023 11:45

operator/api: Add 501 response to /healthz endpoint

0f0e491

Add HTTP response "Not Implemented" for the oeprator api server health handler. The code is generated with go-swagger running `make generate-operator-api`. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

operator: Create leader election context in runOperator

81f9c0e

Avoid initialization of leader election context and cancel function at package import time to allow restarting the operator multiple times in controlplane testing. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 force-pushed the pr/pippolo84/operator-api-server-modularization branch from 53f14f0 to 6930f26 Compare April 12, 2023 09:46

pippolo84 mentioned this pull request Apr 12, 2023

CI: RuntimeDatapathPrivilegedUnitTests_Run_Tests: TestControlPlane/CNPStatusNodesGC #24734

Closed

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 12, 2023

dylandreimerink merged commit d5005bf into cilium:master Apr 13, 2023
57 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator api server modularization #24228

Operator api server modularization #24228

pippolo84 commented Mar 7, 2023 •

edited

joamaki Mar 20, 2023

pippolo84 Mar 21, 2023

nathanjsweet left a comment

christarazi left a comment

pippolo84 commented Apr 4, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

christarazi left a comment

pippolo84 commented Apr 7, 2023

pippolo84 commented Apr 8, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

christarazi commented Apr 9, 2023

pippolo84 commented Apr 12, 2023

pippolo84 commented Apr 12, 2023 •

edited

pippolo84 commented Apr 12, 2023

pippolo84 commented Apr 12, 2023

pippolo84 commented Apr 12, 2023

Operator api server modularization #24228

Operator api server modularization #24228

Conversation

pippolo84 commented Mar 7, 2023 • edited

joamaki Mar 20, 2023

Choose a reason for hiding this comment

pippolo84 Mar 21, 2023

Choose a reason for hiding this comment

nathanjsweet left a comment

Choose a reason for hiding this comment

christarazi left a comment

Choose a reason for hiding this comment

pippolo84 commented Apr 4, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

christarazi left a comment

Choose a reason for hiding this comment

pippolo84 commented Apr 7, 2023

pippolo84 commented Apr 8, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

christarazi commented Apr 9, 2023

pippolo84 commented Apr 12, 2023

pippolo84 commented Apr 12, 2023 • edited

pippolo84 commented Apr 12, 2023

pippolo84 commented Apr 12, 2023

pippolo84 commented Apr 12, 2023

pippolo84 commented Mar 7, 2023 •

edited

pippolo84 commented Apr 4, 2023 •

edited by maintainer-s-little-helper bot

pippolo84 commented Apr 8, 2023 •

edited by maintainer-s-little-helper bot

pippolo84 commented Apr 12, 2023 •

edited