Bring Your Own DRANET Provider (BYODP) - #223
Conversation
✅ Deploy Preview for dranet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
83a4e42 to
0d437fa
Compare
| func (db *DB) GetProfileConfig(deviceName, profile string, claimUID types.UID) (*apis.NetworkConfig, error) { | ||
| p := db.getProfileProvider() | ||
| if p == nil { | ||
| return nil, fmt.Errorf("current cloud provider does not support dynamic profiles") |
There was a problem hiding this comment.
This means that the user hasn't configured a dynamic profile provider (e.g. they didn't start the dranet daemon with --profile-provider=webhook or the clooud provider does not implement it, however, if this function is being called, it strictly means that a Pod is attempting to start with a specific Profile explicitly requested in its DRANET Network Config
|
|
||
| var hint discovery.CloudProviderHint | ||
| // Auto-discover cloud provider if not explicitly set | ||
| if cloudProviderHint == "" { |
There was a problem hiding this comment.
I understand the intent, the concern is that the code currently makes explicit cloud-provider failures indistinguishable from no underlay provider requested
I would suggest sth like this:
explicitCloudProvider := cloudProviderHint != ""
cloudInst, err = discovery.GetInstanceProperties(ctx, hint, webhookURL)
if err != nil {
if explicitCloudProvider {
klog.Fatalf("failed to initialize cloud provider %q: %v", hint, err)
}
klog.Infof("failed to initialize auto-discovered cloud provider %q: %v", hint, err)
cloudInst = nil
}
There was a problem hiding this comment.
This keeps the same behavior as before, we do not want to make a breaking change because this will cause that dranets that worked before to start to fail ... also we explicitily indicated in the flag is just a hint because of this
| if err := json.NewDecoder(resp.Body).Decode(&caps); err == nil { | ||
| p.caps = caps | ||
| } else { | ||
| p.caps = Capabilities{CloudProvider: true, ProfileProvider: true} |
There was a problem hiding this comment.
This returns a provider even when /health failed, because all errors above are swallowed
For explicit webhook use, startup should fail if the capability contract cannot be validated, otherwise the dranet pod can become ready with a misconfigured webhook and fail later during claim preparation
There was a problem hiding this comment.
good catch, fixed, added new commits for simplifying reviews
| if devCfg.Claim.Namespace == claim.Namespace && devCfg.Claim.Name == claim.Name { | ||
| if devCfg.NetworkInterfaceConfigInPod.Profile != "" { | ||
| if err := np.netdb.ReleaseProfileConfig(deviceName, devCfg.NetworkInterfaceConfigInPod.Profile, claim.UID); err != nil { | ||
| klog.Errorf("failed to release profile config for claim %v: %v", claim.NamespacedName, err) |
There was a problem hiding this comment.
it is better to return the ReleaseProfileConfig error instead of only logging it, otherwise unprepare reports success, deletes the claim state, and kubelet will not retry leaked profile/IPAM cleanup
There was a problem hiding this comment.
Everything on pod teardown use to get swallowed because a retry does not imply a retry of the operation and the risk of causing deadlock situations on exit.
This is also how CNI works and is how is also specified
Plugins should generally complete a DEL action without error even if some resources are missing
and how NRI works too, we are trying to document the NRI contract too containerd/nri#286
I understand your concern but is like a defer x.Close() , is on the provider to ensure there is no leaks
This e2e test uses a simple Python webhook to demonstrate the power and flexibility of Dranet's architecture. It shows why we do not need to couple traditional networking constructs (like IPAM) to the core API. At its core, IPAM is just an internal process of assigning an IP to an interface. The interface itself only cares about the addresses, routes, and MTU assigned to it. By abstracting these details behind the Profile and Cloud Provider interfaces, Dranet becomes highly portable. It can seamlessly integrate with any kind of IPAM or environment-specific configuration mechanism out there without having to implement or hardcode them natively. The included webhook script dynamically returns the necessary custom attributes, MTU overrides (via cloud intent), and IP addresses (via user intent profile), which Dranet successfully applies to the interface.
|
/lgtm |
|
/hold latest e2e test is not working correctly, will unhold once I'm understanding better the problem |
|
|
||
| # Wait for the interface to be discovered and validate the custom attribute from webhook | ||
| sleep 5 | ||
| run kubectl --context kind-dranet-test-cluster get resourceslices --field-selector spec.nodeName="$NODE_NAME" -o jsonpath='{.items[0].spec.devices[?(@.name=="dummy1")].attributes.dra\.net\/webhook-attr.string}' |
There was a problem hiding this comment.
the hyphen fails the apiserver validation webhook-attr and the ResourceSlice was never updated, I can see how this can happen in prod
|
these may fix e2e issues:
|
4e98645 to
f38ee84
Compare
|
/hold cancel tests are passing, covering custom webhooks and wherabouts ipam |
This refactor removes the redundant 'profile' string parameter from the GetProfileConfig and ReleaseProfileConfig interface methods across the driver, inventory, and cloudprovider packages, replacing it with the full 'NetworkConfig' struct. Reasoning & Trade-offs: Passing the entire NetworkConfig gives the node-level webhook full context regarding the user and cloud intents. This allows the webhook to behave like a Validating Admission Controller, enabling it to intelligently accept or deny configurations (e.g., rejecting an invalid static IP request) using HTTP status codes. While this approach grants powerful node-level validation, it comes with the trade-off of delayed feedback. Because validation happens asynchronously during the DRA NodePrepareResources phase rather than at the API Server, invalid configurations result in Pods getting stuck in a Pending state and Kubelet repeatedly retrying NodePrepareResources, rather than an immediate API rejection on 'kubectl apply'. Includes updates to test mocks, a new webhook unit test, and expanded documentation explaining these architectural trade-offs.
| * **Kubelet Retry Loops**: Standard Kubernetes behavior is to retry failed resource preparations. A persistent denial (like a 400 Bad Request) will cause the Kubelet to continuously retry `NodePrepareResources`, which can generate unnecessary load on the node and webhook server compared to an upfront API rejection. | ||
|
|
There was a problem hiding this comment.
I would like to add one more sentence here.
* **Idempotency**: The kubelet may retry `NodePrepareResources`, so DRANET can call this more than once for the same `(device, claimUID)`. It must return an equivalent result without allocating additional resources (e.g. key the allocation by `claimUID`, as `whereabouts` does via `CNI_CONTAINERID`).
| * `POST /ReleaseProfileConfig`: Frees stateful resources (e.g., releasing an IP address). Also receives the full `NetworkConfig`. Should return `200 OK` on success or if the resource was already released (idempotency). | ||
|
|
There was a problem hiding this comment.
I would like to add one more sentence here.
* **Best-effort teardown**: A failed `ReleaseProfileConfig` is logged but not retried by DRANET (teardown must not block pod deletion). The provider therefore owns leak reclamation and must be able to garbage-collect orphaned allocations on its own, otherwise resources leak permanently.
This extracts the getDeviceNetworkConfig logic from prepareResourceClaim to allow for exhaustive unit testing of the configuration precedence. A comprehensive table-driven test was added to validate that User configurations properly override Cloud and Profile/Webhook intents, and that Webhook blocking correctly bubbles up errors.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anson627, aojea, kanlkan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
/kind documentation
What this PR does / why we need it:
This PR introduces the Bring Your Own DRANET Provider (BYODP) feature, allowing users to provide custom implementations for both hardware discovery and user intent via external webhooks.
Instead of hardcoding bare-metal or CNI-specific logic directly into DRANET, we can now delegate these responsibilities to an external HTTP REST server or Unix domain socket. This ensures DRANET's core runtime remains solid, statically verifiable, and predictable, while still exposing flexibility for third-party extensions.
This PR specifically introduces:
whereaboutsor from a Cloud Provider)./healthon startup to negotiatecloudProviderandprofileProvidercapability support.cmd/webhook-whereabouts) that wraps the standard CNIwhereaboutsplugin to deliver dynamic IPAM profiles.Which issue(s) this PR is related to:
Fixes: #103
Special notes for your reviewer:
To make the review process this PR has 3 commits:
pkg/inventoryinto a distinctpkg/cloudprovider/discoverypackage.ProfileProviderinterface integration in the inventory DB, and comprehensive architectural documentation (site/content/docs/contributing/webhook-providers.md).webhook-whereaboutsreference module and the associated E2E bats tests verifying the dynamic whereabouts CNI integration.4
Does this PR introduce a user-facing change?