[thc1006]

Closes #1174. Closes #1177. Closes #1179.

A provisioning request that could not be created was reported as progressing, one that did not exist was reported as existing, and the northbound API was never listening. They are the same path, so they are here together.

What was happening

get_package_variant and the three other API calls flattened every failure into one shape:

{"status": False, "reason": "..."}

create_package_variant passed that up, and cluster_creation_request then ran:

while status["status"]:

which does not execute, so the function returned the progressing it had initialised. A missing token, a 403 and a 500 all arrived that way. The measured path is a token file that is not there:

create() with TOKEN naming a file that does not exist  ->  progressing

check_o2ims_provisioning_request asked for the collection:

f"{KUBERNETES_BASE_URL}/apis/o2ims.provisioning.oran.org/v1alpha1/provisioningrequests"

so a legitimately empty list came back with no status key, took the "no status yet" branch, and was reported as this request progressing, whatever name was asked for. The requested name was never sent. It also carried an implicit PackageVariant lookup that returned a bool where manager.py:90 indexed a mapping.

threading.Thread(target=run_flask, daemon=True).start was evaluated at import and never called, so nothing listened and nothing said so.

What changes

One transport. Every API call carries a freshly read token, the trust settings #1170 resolved, a bounded timeout and no redirect. Failures are classified rather than collapsed: config, auth, not found, conflict, invalid, unavailable, protocol, transport, each carrying whether another attempt can help, and whether a write may have landed. A token that cannot be read is a configuration failure of its own and no request is sent.

The body is decoded once, after the status is known. Logging it first meant a 403 answered in HTML raised JSONDecodeError only when debug logging was on, and a Kubernetes Status error body was read as a Cluster's nested conditions.

Named lookups. ProvisioningRequest is read by name, cluster-scoped so no namespace in the path, and a collection answered to a named request is a protocol error rather than progress.

PackageVariant ownership. The name is chosen by the request, so an existing one of that name proved nothing. It is matched on the request uid and the cluster it targets, carried in annotations. metadata.label was a string where Kubernetes wants a labels map. A create whose answer was lost, or which lost a race, is settled by reading the resource back:

POST answer lost -> read back -> owned by this request  -> that is the resource
POST answer lost -> read back -> not there              -> the create did not land, retry

One observation per pass. The handlers were async def running synchronous HTTP and time.sleep, which blocks the event loop Kopf watches every resource with. Each pass now makes one observation and hands the retry back to Kopf. The budget counted sleeps, so the time the requests themselves took did not count and a restart was granted a fresh one; it is measured from the request's own creationTimestamp, which needs no schema change and survives a restart. The Ready condition is found by type: conditions[-1] meant the same conditions in a different order decided differently. The budget covers rendering too. It used to be checked only once rendering had finished, so a package that never rendered kept the request progressing for good.

Resource identifiers come from the objects they stand for. A fresh uuid4() on every observation traced to nothing and changed each time it was read; nothing traceable is an empty set rather than a placeholder.

Business status before the handler stops. A Kopf failure is recorded in an annotation, not in the resource, so a creation that failed permanently left the request's own status saying nothing. The cluster subhandler also raised on the previous step's result, so a cluster that failed was recorded as failed and then not raised for. kopf.PermanentError was constructed with event kwargs and never raised.

The northbound API is served. Bound in a startup handler, so a port already in use stops the operator instead of leaving it running with no API, and closed in a cleanup handler. Flask's development server is not meant to serve this, so waitress does. It listens on 127.0.0.1 unless NBI_HOST says otherwise: it has no authentication and acts with the operator's service account, and leaving the port undeclared does not keep it off the pod network. kubectl port-forward reaches it under the caller's own RBAC. If the serving thread ends by itself, the operator exits, so the pod is restarted even where no probe is set.

Both route paths ended in a space. A collection GET returned from inside its loop, so only the first request was ever answered, with a jsonify Response nested inside another dict that cannot be serialised. Provisioning state was read a level above where it is recorded, so every request read as PROGRESSING. Invalid input was answered 500. The resource set was the string test/sample. Requests are created with the SMO's request id as their identity rather than their display name, which is not unique, and an id that cannot be a Kubernetes object name is refused rather than quietly rewritten into one the caller cannot ask with again.

What the caller is told. Every failure the API server answered a create, list or read with became 502, which says the server broke even when the request was the thing at fault. An object it rejected carries through as 400, a conflict as 409, a missing object as 404 and rate limiting as 503; only the rest are reported as an upstream fault. The identity check accepts what the API server accepts: the real RFC 1123 subdomain, label by label, bounded at 253, so an id like a.-b is refused here rather than sent on and rejected there.

The shape the reference describes. ProvisioningRequestInfo requires provisioningRequestData, provisioningRequestReference, status and provisionedResourceSet. The reference was not answered at all; provisioningRequestId was answered a level above where it is declared; ProvisionedResourceSet asks for nodeClusterId and infrastructureResourceIds while the CR records the same two as oCloudNodeClusterId and oCloudInfrastructureResourceIds, and those were passed straight through, so neither required field was present under the name the API asks for; and a recorded state with no mapping was answered as UNKNOWN, which is not one of the five phases the enum allows.

Failures answer the shape too. ProblemDetails types status as a string and every failure answered an object there, so none of them was what this API documents; the message is the status now, and the time it happened travels alongside, which ProblemDetails allows. load_incluster_config raises ConfigException, which is not an ApiException, so no route caught it and all three answered Flask's HTML error page wherever the client cannot be configured — a northbound caller parsing JSON and getting a web page. That is a 503 naming the API server now, with an errorhandler behind it that answers the documented shape for anything unforeseen and tells an unauthenticated caller only that the request could not be served. It re-raises HTTPException, so werkzeug's 404 for an unknown route stays a 404.

The two specifications also disagree about name and description: the CRD types them as strings and rejects a null, the reference requires them of every answer. An empty string is what both take. Every route's answer is validated against the reference's own schemas, for a fulfilled request, one with no status yet, a collection with items and empty, a 201, and a request written straight to the API server rather than through this API.

Permissions. The ClusterRole had no create verb for provisioningrequests, so the northbound API could not create anything anyway. status and finalizers are separate rules now. The admission rule named apiGroups as admissionregistration.k8s.io/v1, which is not a group, so it granted nothing; no handler registers a webhook, so it is removed rather than corrected into a permission that would then be real and unused.

The probe. Kopf only serves it when --liveness is passed, and get_current_timestamp called datetime.datetime.now on a module imported as from datetime import datetime, so it raised AttributeError. The northbound probe connects to the API and raises PermanentError, which kopf answers with 500. Answering stopped inside a 200 told a kubelet nothing, and the serving thread can stay alive after its listening socket is gone.

Proof manifests

tests/conftest.py puts controllers/ on the path the way the image does, so the modules are imported the way they import each other and that layout is exercised. #1034 and #1047 were both about it.

$ python -m pytest --disable-warnings -q
273 passed

tests/test_utils.py                            154 passed
tests/test_northbound_restapi.py                66 passed
tests/test_provisioning_request_controller.py   29 passed
tests/test_manager.py                           24 passed

$ python -m flake8 controllers
$ echo $?
0

163 of those passed before this change; flake8 controllers reported 142 violations before it and none now.

tox.ini lists py311 as well as py312, and the presubmit image carries only 3.12, so the suite was run on 3.11 too: 273 passed there as well.

The northbound tests go through Flask's own routing and serialization, the operator tests through real kopf.Body, kopf.Spec, kopf.Patch, kopf.Memo and real kopf.PermanentError/TemporaryError, and the listener is bound on a real socket. A route string, a status code, a nested Response object and a thread that was constructed but not started are none of them visible to a direct call of a view function. Passing dictionaries where kopf passes a Spec is how a validation check that rejected every real request went unnoticed.

The read timeout is tested against a socket that accepts and then writes nothing, not against the keyword:

timeout=API_TIMEOUT   raises after the read bound, 1.19s
timeout=None          never returns; killed at 25s

The elapsed time is bounded below as well, because a connection refused outright would satisfy "did not hang" while exercising no timeout at all.

The real image, the real CMD, non-root:

$ podman run --rm o2ims-smoke:local id
uid=65535(o2ims) gid=65535(o2ims)

$ kopf run /usr/o2ims/src/manager.py --all-namespaces --liveness=...
Activity 'configure' succeeded.
northbound API listening on 127.0.0.1:15000
Activity 'start_northbound' succeeded.

GET /healthz                          -> 200 {"now": "...", "northbound": "accepting"}
GET the collection on 127.0.0.1       -> 200 {"items":[]}
GET the collection on the host's IP   -> connection refused
listening socket destroyed (ss -K)    -> GET /healthz 500, operator still running

$ # SIGTERM
the northbound API stopped            (exit 0, not taken for a crash)

Those are the two ways serving can fail. The other is the thread ending, and
that one is verified through kopf's own event loop rather than by asserting
that a signal was sent:

northbound API listening on 127.0.0.1:5077
PROBE: closing the server without going through cleanup
ERROR the northbound API stopped by itself; stopping the operator so that it
      is restarted
INFO  the northbound API stopped
INFO  Activity 'stop_northbound' succeeded.
PROBE: kopf.run returned after 6.2s

So the cleanup handlers run and the process returns, rather than being killed
where it stands. The unit test covers this half — the signal is sent — and
that run covers kopf's half, that it is acted on.

That run was against an envtest kube-apiserver, with a service account token mounted where a pod has it, on the host network, so the ports there are 15000 and 18080.

The test deployment runs as the user the image ships with rather than overriding the pod to root, declares the health port, and has probes pointing at that endpoint. Reaching it says kopf started; a 500 says the northbound API does not accept connections.

and the endpoints, answered from inside that image:

POST malformed json                 -> 400   (was 500)
POST no clusterName                 -> 400   (was 500)
POST an id a label may not start with -> 400 (was sent on, rejected there)
POST an id past the length limit    -> 400
GET the old trailing-space route    -> 404   (it is gone)

What this does not do

The companion PR is nephio-project/catalog#149, a draft until this is merged and built. Reading nephio/optional/o2ims/app/ at 28f531c:

• rbac.yaml has the same gap this fixes in the test deployment: provisioningrequests carries [get, list, watch, update, patch] and no create, so the northbound API cannot create anything. The inert admissionregistration.k8s.io/v1 rule is there too.
• there are no probes, so the --liveness endpoint this adds is served and never called. Update WatcherAgent to comply to NF CRDs in API repository #149 adds them.
• terminationGracePeriodSeconds: 5 is what this operator now shuts down inside.
• the image is pinned to 2097228064147640320, which predates all of this and does not pass --liveness. Update WatcherAgent to comply to NF CRDs in API repository #149 has to move it to this PR's build before it can merge.

Merging here does not update a running deployment. The northbound API stays on loopback until someone decides what authenticates the callers in front of it.

The node cluster id is the CAPI Cluster's own uid. The infrastructure resource ids are read off spec.infrastructureRef, which Cluster API does not fill in, so that list is empty in practice until this operator is given a real inventory mapping; empty, rather than a uuid4 that changed on every observation and traced to nothing.

An empty resource set is conformant: ProvisionedResourceSet requires the two fields, not values in them, so "" and [] satisfy it. I had said this needed a correction in the api repository first; validating the answers against the reference's own schemas says otherwise, and that claim is withdrawn.

PUT and DELETE on a named request are in the reference and are not implemented here.

#1192 and #1193 are untouched and still open. createToken still lists with the zero value, and the singleton is still published before Start fills it in.

The second commit is an adversarial pass over the first. It found a retryable failure reported as progressing under a message that said it had failed, a start time parsed with one format string that quietly handed out a fresh budget in any other shape, the status mapping and identity check above, a session closed before its answer was read, and a stop test that asserted only what stop_northbound sets either way rather than that the thread had ended.

Release triage is yours.

[nephio-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johnbelamaric for approval by writing /assign @johnbelamaric in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[thc1006]

/test presubmit-nephio-o2ims-python-test