Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions cmd/gpu-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,12 @@ func main() {
WithRestartOnlyPredicate(predicates.DriverPodRestartOnly(upgradeLogger))

if err = (&controllers.UpgradeReconciler{
Client: mgr.GetClient(),
Log: upgradeLogger,
Scheme: mgr.GetScheme(),
StateManager: clusterUpgradeStateManager,
OperatorMetrics: operatorMetrics,
Client: mgr.GetClient(),
Log: upgradeLogger,
Scheme: mgr.GetScheme(),
StateManager: clusterUpgradeStateManager,
OperatorMetrics: operatorMetrics,
OperatorNamespace: operatorNamespace,
}).SetupWithManager(ctx, mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "Upgrade")
os.Exit(1)
Expand Down
32 changes: 32 additions & 0 deletions controllers/gpucluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1"
nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1"
"github.com/NVIDIA/gpu-operator/controllers/clusterinfo"
"github.com/NVIDIA/gpu-operator/internal/conditions"
Expand Down Expand Up @@ -120,6 +121,21 @@ func (r *GPUClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
}
r.singleton = instance

// DRA requires all driver management through NVIDIADriver CRs: surface an unmet
// prerequisite on this CR's status and hold off deploying operands until it is met.
if msg, err := r.validatePrerequisites(ctx); err != nil {
return ctrl.Result{}, err
} else if msg != "" {
logger.V(consts.LogLevelWarning).Info("GPUCluster prerequisite not met", "reason", msg)
if err := r.updateCRStatus(ctx, instance, nvidiav1alpha1.NotReady); err != nil {
return ctrl.Result{}, err
}
if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.PrerequisiteNotMet, msg); condErr != nil {
logger.Error(condErr, "failed to set condition")
}
return ctrl.Result{RequeueAfter: time.Minute}, nil
}

// The operand states render ResourceClaimTemplates with adminAccess: true, which the
// kube-scheduler only admits from a labeled namespace; label it before syncing states.
if err := r.ensureAdminAccessLabel(ctx); err != nil {
Expand Down Expand Up @@ -162,6 +178,22 @@ func (r *GPUClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
return ctrl.Result{RequeueAfter: time.Minute}, nil
}

// validatePrerequisites checks the cross-CR rules that gate DRA enablement, returning
// a message describing the first unmet prerequisite or an empty string when all are met.
func (r *GPUClusterReconciler) validatePrerequisites(ctx context.Context) (string, error) {
clusterPolicies := &gpuv1.ClusterPolicyList{}
if err := r.List(ctx, clusterPolicies); err != nil {
return "", fmt.Errorf("error listing ClusterPolicy objects: %w", err)
}
for i := range clusterPolicies.Items {
cp := &clusterPolicies.Items[i]
if !cp.Spec.Driver.UseNvidiaDriverCRDType() {
return fmt.Sprintf("ClusterPolicy %s does not have driver.useNvidiaDriverCRD enabled; migrate driver management to NVIDIADriver CRs before enabling DRA", cp.Name), nil
}
}
return "", nil
}

// ensureAdminAccessLabel patches the operator namespace with the label required by the
// kube-scheduler to allow adminAccess: true in ResourceClaim/ResourceClaimTemplate
// objects. The label is deliberately never removed: it is namespace-level configuration
Expand Down
56 changes: 49 additions & 7 deletions controllers/gpucluster_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"sort"
"testing"
"time"

"github.com/stretchr/testify/require"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -71,14 +72,17 @@ func newGPUClusterReconciler(t *testing.T, objs ...client.Object) (*GPUClusterRe
}

// fakeStateManager returns canned SyncState results so the controller tests don't load
// real manifests. GetWatchSources is promoted from the embedded (nil) interface and is
// never called here — only SetupWithManager calls it, which these tests skip.
// real manifests. It records the last info catalog passed to SyncState so tests can
// assert on its entries. GetWatchSources is promoted from the embedded (nil) interface
// and is never called here — only SetupWithManager calls it, which these tests skip.
type fakeStateManager struct {
state.Manager
results state.Results
results state.Results
lastCatalog state.InfoCatalog
}

func (f *fakeStateManager) SyncState(_ context.Context, _ interface{}, _ state.InfoCatalog) state.Results {
func (f *fakeStateManager) SyncState(_ context.Context, _ interface{}, catalog state.InfoCatalog) state.Results {
f.lastCatalog = catalog
return f.results
}

Expand Down Expand Up @@ -213,18 +217,56 @@ func TestGPUClusterTeardownDrainsClaimConsumersFirst(t *testing.T) {
require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: plugin.Name, Namespace: "test-namespace"}, ds))
}

// A ClusterPolicy in the cluster does not disable the GPUCluster: the two stacks
// coexist, with per-node ownership decided by the nvidia.com/gpu-operator.resource-allocation.mode label.
// A ClusterPolicy in the cluster does not disable the GPUCluster, provided it
// delegates driver management to NVIDIADriver CRs: the two stacks coexist, with
// per-node ownership decided by the nvidia.com/gpu-operator.resource-allocation.mode label.
func TestGPUClusterCoexistsWithClusterPolicy(t *testing.T) {
cfg := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}}
cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}}
cp := &gpuv1.ClusterPolicy{
ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"},
Spec: gpuv1.ClusterPolicySpec{
Driver: gpuv1.DriverSpec{UseNvidiaDriverCRD: ptr.To(true)},
},
}
r, c := newGPUClusterReconciler(t, cfg, cp)

gccReconcile(t, r, cfg.Name)

require.Equal(t, nvidiav1alpha1.Ready, gccState(t, c, cfg.Name))
}

// A ClusterPolicy that manages its own driver (useNvidiaDriverCRD=false) is an invalid
// companion for DRA: the GPUCluster reports the unmet prerequisite and deploys nothing.
func TestGPUClusterClusterPolicyDriverPrerequisite(t *testing.T) {
cfg := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}}
cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}}
r, c := newGPUClusterReconciler(t, cfg, cp)
r.conditionUpdater = conditions.NewGPUClusterUpdater(c)

res, err := r.Reconcile(t.Context(), gccRequest(cfg.Name))
require.NoError(t, err)
require.Equal(t, time.Minute, res.RequeueAfter)

require.Equal(t, nvidiav1alpha1.NotReady, gccState(t, c, cfg.Name))
require.Nil(t, r.stateManager.(*fakeStateManager).lastCatalog, "operands must not be synced")

instance := &nvidiav1alpha1.GPUCluster{}
require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: cfg.Name}, instance))
cond := meta.FindStatusCondition(instance.Status.Conditions, conditions.Error)
require.NotNil(t, cond)
require.Equal(t, conditions.PrerequisiteNotMet, cond.Reason)
require.Contains(t, cond.Message, "useNvidiaDriverCRD")

// Toggling the flag to true clears the prerequisite on the next reconcile.
updated := &gpuv1.ClusterPolicy{}
require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: cp.Name}, updated))
updated.Spec.Driver.UseNvidiaDriverCRD = ptr.To(true)
require.NoError(t, c.Update(t.Context(), updated))

gccReconcile(t, r, cfg.Name)
require.Equal(t, nvidiav1alpha1.Ready, gccState(t, c, cfg.Name))
}

// First-reconciled wins (mirroring ClusterPolicy): whichever instance reconciles first
// claims ownership, regardless of name or creationTimestamp.
func TestGPUClusterSingleton(t *testing.T) {
Expand Down
66 changes: 30 additions & 36 deletions controllers/nvidiadriver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type NVIDIADriverReconciler struct {
//+kubebuilder:rbac:groups=nvidia.com,resources=nvidiadrivers,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups=nvidia.com,resources=nvidiadrivers/status,verbs=get;update;patch
//+kubebuilder:rbac:groups=nvidia.com,resources=nvidiadrivers/finalizers,verbs=update
//+kubebuilder:rbac:groups=nvidia.com,resources=gpuclusters,verbs=get;list;watch

// Reconcile is part of the main kubernetes reconciliation loop which aims to
// move the current state of the cluster closer to the desired state.
Expand Down Expand Up @@ -98,53 +99,45 @@ func (r *NVIDIADriverReconciler) Reconcile(ctx context.Context, req ctrl.Request
return reconcile.Result{}, nil
}

// Get the singleton NVIDIA ClusterPolicy object in the cluster.
clusterPolicyList := &gpuv1.ClusterPolicyList{}
if err := r.List(ctx, clusterPolicyList); err != nil {
wrappedErr := fmt.Errorf("error getting ClusterPolicy list: %w", err)
logger.Error(err, "error getting ClusterPolicy list")
instance.Status.State = nvidiav1alpha1.NotReady
if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil {
logger.Error(condErr, "failed to set condition")
}
return reconcile.Result{}, wrappedErr
}

if len(clusterPolicyList.Items) == 0 {
err := fmt.Errorf("no ClusterPolicy object found in the cluster")
logger.Error(err, "failed to get ClusterPolicy object")
// Source the cluster-wide host root from the active configuration: a ClusterPolicy takes
// precedence, otherwise the controller runs standalone against the GPUCluster.
clusterPolicy, gpuCluster, err := resolveActiveConfig(ctx, r.Client)
if err != nil {
logger.Error(err, "error resolving active cluster configuration")
instance.Status.State = nvidiav1alpha1.NotReady
if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil {
logger.Error(condErr, "failed to set condition")
}
return reconcile.Result{}, err
}
clusterPolicyInstance := clusterPolicyList.Items[0]

// Ensure the NVIDIADriver CR has a consumer: either the ClusterPolicy delegates its
// driver to the NVIDIADriver CRD, or a GPUCluster exists. GPUCluster does
// not manage the driver itself — it is either preinstalled on the host (no NVIDIADriver
// CR) or installed via NVIDIADriver CRs, so any CR that exists alongside one is in use.
if !clusterPolicyInstance.Spec.Driver.UseNvidiaDriverCRDType() {
gpuClusters := &nvidiav1alpha1.GPUClusterList{}
if err := r.List(ctx, gpuClusters); err != nil {
wrappedErr := fmt.Errorf("error getting GPUCluster list: %w", err)
logger.Error(err, "error getting GPUCluster list")
instance.Status.State = nvidiav1alpha1.NotReady
if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil {
logger.Error(condErr, "failed to set condition")
}
return reconcile.Result{}, wrappedErr
}
if len(gpuClusters.Items) == 0 {
msg := "useNvidiaDriverCRD is not enabled in ClusterPolicy and no GPUCluster exists"

// Ensure the NVIDIADriver CR has a consumer: a ClusterPolicy that delegates its
// driver via useNvidiaDriverCRD, or, absent a ClusterPolicy, a GPUCluster. A
// non-delegating ClusterPolicy disables reconciliation even when a GPUCluster
// exists, since DRA requires driver management through NVIDIADriver CRs.
var hostRoot string
switch {
case clusterPolicy != nil:
if !clusterPolicy.Spec.Driver.UseNvidiaDriverCRDType() {
msg := "useNvidiaDriverCRD is not enabled in ClusterPolicy"
logger.V(consts.LogLevelWarning).Info("NVIDIADriver reconciliation skipped", "reason", msg)
instance.Status.State = nvidiav1alpha1.Disabled
if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.Reconciled, msg); condErr != nil {
logger.Error(condErr, "failed to set condition")
}
return reconcile.Result{}, nil
}
hostRoot = clusterPolicy.Spec.HostPaths.RootFS
case gpuCluster != nil:
hostRoot = gpuCluster.Spec.HostPaths.RootFS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct me if I am wrong, but in the scenario where both ClusterPolicy and GPUCluster exist, will the .spec.hostPaths.rootFS from GPUCluster have the higher order of precedence?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ClusterPolicy takes precedence today. Now, I believe this combination is only valid when the ClusterPolicy sets useNvidiaDriverCRD=true, and in that case ClusterPolicy's hostPaths.rootFS is used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that this hostRoot value gets used here

- name: host-root
          hostPath:
            path: {{ .HostRoot | default "/" }}

Thinking about this more, it doesn't make much sense to make this global value and we are better off getting this value from the NVIDIADriver CR. For this, we would need to add a new field called hostRoot to the NVIDIADriver CR. This is the behaviour I envision in the following scenarios:

  • If the GPUCluster resource is present (whether by itself or along with ClusterPolicy, then we get the hostRoot from the NVIDIADriver CR, else we fallback to the GPUCluster hostRoot. Here we will never get the hostRoot from ClusterPolicy. This way we ensure zero dependency on ClusterPolicy in a GPUCluster flow
  • If only ClusterPolicy is present, then we either get the hostRoot from the NVIDIADriver CR (the newly added field), else we fallback to the ClusterPolicy.Spec.HostPaths.RootFS

This can be done in a follow-up PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that when we add hostRoot to the NVIDIADriver CR, it'll be an additive change and it will be an optional field. If this is empty, it will be defaulted to "/" as we currently do today

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note, the host root is needed for many of our daemonsets, not just the driver. So we still need this field to be present in the GPUCluster CR. What is the benefit in having users configure this field in multiple places (GPUCluster and NVIDIADriver CRs)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, the rest of the HostPaths fields are used correctly in operands whether they come from GPUCluster or ClusterPolicy. For the driver daemonset however, it's just the .HostPaths.RootFS field that is relevant

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm in favor of moving hostRoot to NVIDIADriver and removing it entirely from GPUCluster.

In the legacy ClusterPolicy path, we can get the hostRoot from either NVIDIADriver CR (if specified) or from ClusterPolicy.Spec.HostPaths.RootFS when useNvidiaDriverCRD=true, and from ClusterPolicy.Spec.HostPaths.RootFS only when useNvidiaDriverCRD=false.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After discussing internally, we have come to a consensus on how to move forward with this. The main issue we that we want to address (in a follow-up PR) is that there should be no cross-CR or circular dependency between the custom resources.

  • The hostRoot will be hard-coded as / for now.
  • We had between two approaches to fix this problem. Either adding a new hostRoot field to the NVIDIADriver CRD or adding a new environment variable to determine the hostRoot to be set in both the driver daemonset and the DRA driver kubelet plugin daemonset. This is still ongoing.

default:
err := fmt.Errorf("no ClusterPolicy or GPUCluster object found in the cluster")
logger.Error(err, "failed to retrieve hostPaths information. No ClusterPolicy or GPUCluster object found in the cluster")
instance.Status.State = nvidiav1alpha1.NotReady
if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil {
logger.Error(condErr, "failed to set condition")
}
return reconcile.Result{}, err
}

// Create a new InfoCatalog which is a generic interface for passing information to state managers
Expand All @@ -153,8 +146,8 @@ func (r *NVIDIADriverReconciler) Reconcile(ctx context.Context, req ctrl.Request
// Add an entry for ClusterInfo, which was collected before the NVIDIADriver controller was started
infoCatalog.Add(state.InfoTypeClusterInfo, r.ClusterInfo)

// Add an entry for Clusterpolicy, which is needed to deploy the driver daemonset
infoCatalog.Add(state.InfoTypeClusterPolicyCR, clusterPolicyInstance)
// Add the host root, which is needed to deploy the driver daemonset
infoCatalog.Add(state.InfoTypeHostRoot, hostRoot)

// Verify the nodeSelector configured for this NVIDIADriver instance does
// not conflict with any other instances. This ensures only one driver
Expand Down Expand Up @@ -405,6 +398,7 @@ func (r *NVIDIADriverReconciler) SetupWithManager(ctx context.Context, mgr ctrl.
gpuClusterMapFn := func(ctx context.Context, _ *nvidiav1alpha1.GPUCluster) []reconcile.Request {
return r.enqueueAllNVIDIADrivers(ctx)
}

err = c.Watch(
source.Kind(
mgr.GetCache(),
Expand Down
94 changes: 92 additions & 2 deletions controllers/nvidiadriver_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,14 @@ func TestReconcile(t *testing.T) {
expectedLog: "useNvidiaDriverCRD is not enabled in ClusterPolicy",
},
{
name: "driver CRD false but GPUCluster exists → reconciliation proceeds",
name: "driver CRD false with GPUCluster → reconciliation still skips driver",
useCRD: ptr.To(false),
gpuClusterExists: true,
validator: &FakeNodeSelectorValidator{
CustomError: errors.New("fake list error"),
},
error: nil,
expectedLog: "nodeSelector validation failed",
expectedLog: "useNvidiaDriverCRD is not enabled in ClusterPolicy",
},
{
name: "ClusterPolicy has driver CRD true but validator errors",
Expand Down Expand Up @@ -240,6 +240,96 @@ func TestReconcile(t *testing.T) {
}
}

// TestReconcileStandalone covers the no-ClusterPolicy path: the controller falls back
// to the GPUCluster for the cluster-wide configuration, and fails early when
// neither object exists.
func TestReconcileStandalone(t *testing.T) {
scheme := runtime.NewScheme()
require.NoError(t, nvidiav1alpha1.AddToScheme(scheme))
require.NoError(t, gpuv1.AddToScheme(scheme))

cp := &gpuv1.ClusterPolicy{
ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"},
Spec: gpuv1.ClusterPolicySpec{
Driver: gpuv1.DriverSpec{
UseNvidiaDriverCRD: ptr.To(true),
},
HostPaths: gpuv1.HostPathsSpec{RootFS: "/cp-root"},
},
}
gpuCluster := &nvidiav1alpha1.GPUCluster{
ObjectMeta: metav1.ObjectMeta{Name: "gpu-cluster-config"},
Spec: nvidiav1alpha1.GPUClusterSpec{
HostPaths: gpuv1.HostPathsSpec{RootFS: "/gpuCluster-root"},
},
}

tests := []struct {
name string
objects []client.Object
expectedErr string
expectedHostRoot string
}{
{
name: "no ClusterPolicy, GPUCluster provides the host root",
objects: []client.Object{gpuCluster},
expectedHostRoot: "/gpuCluster-root",
},
{
name: "ClusterPolicy preferred over GPUCluster",
objects: []client.Object{cp, gpuCluster},
expectedHostRoot: "/cp-root",
},
{
name: "neither ClusterPolicy nor GPUCluster",
expectedErr: "no ClusterPolicy or GPUCluster object found in the cluster",
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
driver := &nvidiav1alpha1.NVIDIADriver{
ObjectMeta: metav1.ObjectMeta{Name: "test-driver"},
}

c := fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(append([]client.Object{driver}, tc.objects...)...).
WithStatusSubresource(&nvidiav1alpha1.NVIDIADriver{}).
Build()

updater := &FakeConditionUpdater{}
stateManager := &fakeStateManager{results: state.Results{Status: state.SyncStateReady}}

reconciler := &NVIDIADriverReconciler{
Client: c,
Scheme: scheme,
conditionUpdater: updater,
nodeSelectorValidator: &FakeNodeSelectorValidator{},
stateManager: stateManager,
}

req := ctrl.Request{NamespacedName: types.NamespacedName{Name: driver.Name}}
_, err := reconciler.Reconcile(context.Background(), req)

if tc.expectedErr != "" {
require.ErrorContains(t, err, tc.expectedErr)
require.Equal(t, nvidiav1alpha1.NotReady, updater.LastErrorState)
return
}
require.NoError(t, err)

hostRoot, ok := stateManager.lastCatalog.Get(state.InfoTypeHostRoot).(string)
require.True(t, ok, "info catalog must hold a host root string")
require.Equal(t, tc.expectedHostRoot, hostRoot)

instance := &nvidiav1alpha1.NVIDIADriver{}
require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: driver.Name}, instance))
require.Equal(t, nvidiav1alpha1.Ready, instance.Status.State)
})
}
}

func TestReconcileConflictSetsNotReadyState(t *testing.T) {
scheme := runtime.NewScheme()
require.NoError(t, nvidiav1alpha1.AddToScheme(scheme))
Expand Down
Loading
Loading