diff --git a/controllers/nvidiadriver_controller.go b/controllers/nvidiadriver_controller.go index 94dfa6212..14ca61011 100644 --- a/controllers/nvidiadriver_controller.go +++ b/controllers/nvidiadriver_controller.go @@ -162,7 +162,11 @@ func (r *NVIDIADriverReconciler) Reconcile(ctx context.Context, req ctrl.Request if err := r.nodeSelectorValidator.Validate(ctx, instance); err != nil { logger.Error(err, "nodeSelector validation failed") instance.Status.State = nvidiav1alpha1.NotReady - if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ConflictingNodeSelector, err.Error()); condErr != nil { + conditionReason := conditions.ConflictingNodeSelector + if errors.Is(err, validator.ErrMultipleDefaultNVIDIADrivers) { + conditionReason = conditions.ReconcileFailed + } + if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditionReason, err.Error()); condErr != nil { logger.Error(condErr, "failed to set condition") } return reconcile.Result{}, nil diff --git a/controllers/nvidiadriver_controller_test.go b/controllers/nvidiadriver_controller_test.go index dd5d2d00b..3934b376d 100644 --- a/controllers/nvidiadriver_controller_test.go +++ b/controllers/nvidiadriver_controller_test.go @@ -47,8 +47,10 @@ import ( // FakeConditionUpdater implements conditions.Updater // It always returns CustomError if set type FakeConditionUpdater struct { - CustomError error - LastErrorState nvidiav1alpha1.State + CustomError error + LastErrorState nvidiav1alpha1.State + LastErrorReason string + LastErrorMessage string } // SetConditionsError always returns CustomError if set @@ -56,6 +58,8 @@ func (f *FakeConditionUpdater) SetConditionsError(ctx context.Context, obj any, if driver, ok := obj.(*nvidiav1alpha1.NVIDIADriver); ok { f.LastErrorState = driver.Status.State } + f.LastErrorReason = condType + f.LastErrorMessage = msg return f.CustomError } @@ -288,6 +292,46 @@ func TestReconcileConflictSetsNotReadyState(t *testing.T) { require.Equal(t, nvidiav1alpha1.NotReady, updater.LastErrorState) } +func TestReconcileMultipleDefaultDriversSetsNotReadyState(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) + require.NoError(t, gpuv1.AddToScheme(scheme)) + + defaultDriver := &nvidiav1alpha1.NVIDIADriver{ + ObjectMeta: metav1.ObjectMeta{Name: "default-a"}, + Spec: nvidiav1alpha1.NVIDIADriverSpec{Default: true}, + } + secondDefaultDriver := &nvidiav1alpha1.NVIDIADriver{ + ObjectMeta: metav1.ObjectMeta{Name: "default-b"}, + Spec: nvidiav1alpha1.NVIDIADriverSpec{Default: true}, + } + cp := &gpuv1.ClusterPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: gpuv1.ClusterPolicySpec{ + Driver: gpuv1.DriverSpec{UseNvidiaDriverCRD: ptr.To(true)}, + }, + } + + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cp, defaultDriver, secondDefaultDriver).Build() + updater := &FakeConditionUpdater{} + reconciler := &NVIDIADriverReconciler{ + Client: client, + Scheme: scheme, + conditionUpdater: updater, + nodeSelectorValidator: validator.NewNodeSelectorValidator(client), + } + + for _, name := range []string{defaultDriver.Name, secondDefaultDriver.Name} { + _, err := reconciler.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: name}, + }) + require.NoError(t, err) + require.Equal(t, nvidiav1alpha1.NotReady, updater.LastErrorState) + require.Equal(t, conditions.ReconcileFailed, updater.LastErrorReason) + require.Contains(t, updater.LastErrorMessage, "multiple default NVIDIADrivers found") + } +} + func TestUpdateCrStatusPreservesNotReadyStateWhenSettingErrorCondition(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) diff --git a/internal/validator/validator.go b/internal/validator/validator.go index 3627d458c..3b79c3839 100644 --- a/internal/validator/validator.go +++ b/internal/validator/validator.go @@ -18,6 +18,7 @@ package validator import ( "context" + "errors" "fmt" "sort" @@ -33,6 +34,9 @@ type Validator interface { Validate(ctx context.Context, cr *nvidiav1alpha1.NVIDIADriver) error } +// ErrMultipleDefaultNVIDIADrivers is returned when more than one NVIDIADriver is configured as the fallback driver. +var ErrMultipleDefaultNVIDIADrivers = errors.New("multiple default NVIDIADrivers found") + // nodeSelectorValidator validates against the nodeSelector type nodeSelectorValidator struct { client client.Client @@ -56,11 +60,25 @@ func (nsv *nodeSelectorValidator) Validate(ctx context.Context, cr *nvidiav1alph return err } - selectedNodeOwners := map[string][]string{} + defaultDriverNames := []string{} for _, driver := range drivers.Items { if err := driver.ValidateNodeSelector(); err != nil { return err } + if driver.IsDefault() { + if !driver.HasDeletionTimestamp() { + defaultDriverNames = append(defaultDriverNames, driver.Name) + } + } + } + + if len(defaultDriverNames) > 1 { + sort.Strings(defaultDriverNames) + return fmt.Errorf("%w: %v", ErrMultipleDefaultNVIDIADrivers, defaultDriverNames) + } + + selectedNodeOwners := map[string][]string{} + for _, driver := range drivers.Items { if driver.IsDefault() { continue } diff --git a/internal/validator/validator_test.go b/internal/validator/validator_test.go index 75b737e2c..32913f4d2 100644 --- a/internal/validator/validator_test.go +++ b/internal/validator/validator_test.go @@ -18,6 +18,7 @@ package validator import ( "context" + "errors" "testing" "github.com/stretchr/testify/assert" @@ -25,6 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client/fake" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" @@ -128,6 +130,38 @@ func TestCheckNodeSelectorIgnoresDefaultDriver(t *testing.T) { assert.NoError(t, err) } +func TestCheckNodeSelectorRejectsMultipleDefaultDrivers(t *testing.T) { + defaultDriver := makeTestDriver("default-a", nil, true) + secondDefaultDriver := makeTestDriver("default-b", nil, true) + + s := scheme.Scheme + err := nvidiav1alpha1.AddToScheme(s) + require.NoError(t, err) + c := fake.NewClientBuilder().WithScheme(s).WithObjects(defaultDriver, secondDefaultDriver).Build() + nsv := NewNodeSelectorValidator(c) + + err = nsv.Validate(context.Background(), defaultDriver) + require.ErrorIs(t, err, ErrMultipleDefaultNVIDIADrivers) + require.EqualError(t, err, "multiple default NVIDIADrivers found: [default-a default-b]") +} + +func TestCheckNodeSelectorIgnoresDeletingDefaultDriver(t *testing.T) { + defaultDriver := makeTestDriver("default-a", nil, true) + deletingDefaultDriver := makeTestDriver("default-b", nil, true) + deletingDefaultDriver.DeletionTimestamp = ptr.To(metav1.Now()) + deletingDefaultDriver.Finalizers = []string{"test-finalizer"} + + s := scheme.Scheme + err := nvidiav1alpha1.AddToScheme(s) + require.NoError(t, err) + c := fake.NewClientBuilder().WithScheme(s).WithObjects(defaultDriver, deletingDefaultDriver).Build() + nsv := NewNodeSelectorValidator(c) + + err = nsv.Validate(context.Background(), defaultDriver) + require.NoError(t, err) + require.False(t, errors.Is(err, ErrMultipleDefaultNVIDIADrivers)) +} + func TestCheckNodeSelectorRejectsReservedOwnerLabel(t *testing.T) { driver := makeTestDriver("", map[string]string{consts.NVIDIADriverOwnerLabel: "other-driver"}, false) diff --git a/tests/scripts/update-nvidiadriver.sh b/tests/scripts/update-nvidiadriver.sh index 384bb3c99..d10467365 100755 --- a/tests/scripts/update-nvidiadriver.sh +++ b/tests/scripts/update-nvidiadriver.sh @@ -13,6 +13,7 @@ source ${SCRIPT_DIR}/checks.sh NVIDIA_DRIVER_NAME="${NVIDIA_DRIVER_NAME:-e2e-driver}" DEFAULT_NVIDIA_DRIVER_NAME="${DEFAULT_NVIDIA_DRIVER_NAME:-e2e-default-driver}" +DUPLICATE_DEFAULT_NVIDIA_DRIVER_NAME="${DUPLICATE_DEFAULT_NVIDIA_DRIVER_NAME:-e2e-duplicate-default-driver}" get_default_nvidiadriver_name() { kubectl get nvidiadriver -o json | @@ -297,6 +298,31 @@ wait_for_nvidiadriver_condition_message() { done } +wait_for_nvidiadriver_ready() { + local driver_name=$1 + local current_time=0 + + echo "Waiting for NVIDIADriver/${driver_name} to report Ready" + while :; do + if kubectl get nvidiadriver/"${driver_name}" -o json | jq -e ' + (.status.state // "") == "ready" and + ([.status.conditions[]? | select(.type == "Ready" and .status == "True")] | length > 0) and + ([.status.conditions[]? | select(.type == "Error" and .status == "True")] | length == 0) + ' >/dev/null; then + break + fi + + if [[ "${current_time}" -gt 120 ]]; then + echo "timeout reached waiting for NVIDIADriver/${driver_name} to report Ready" + kubectl get nvidiadriver/"${driver_name}" -o yaml + exit 1 + fi + + sleep 5 + current_time=$((${current_time} + 5)) + done +} + test_removed_default_field_conflict_preserves_owners() { echo "Testing that clearing the default field makes the CR a normal conflicting NVIDIADriver" unset_default_driver "${DEFAULT_NVIDIA_DRIVER_NAME}" @@ -308,6 +334,19 @@ test_removed_default_field_conflict_preserves_owners() { wait_for_nvidiadriver_owner "${NVIDIA_DRIVER_NAME}" } +test_multiple_default_drivers_are_not_ready() { + echo "Testing that multiple default NVIDIADrivers report a reconciliation failure" + create_nvidiadriver_from "${DEFAULT_NVIDIA_DRIVER_NAME}" "${DUPLICATE_DEFAULT_NVIDIA_DRIVER_NAME}" true + wait_for_nvidiadriver_condition_message "${DEFAULT_NVIDIA_DRIVER_NAME}" "multiple default NVIDIADrivers found" + wait_for_nvidiadriver_condition_message "${DUPLICATE_DEFAULT_NVIDIA_DRIVER_NAME}" "multiple default NVIDIADrivers found" + assert_nvidiadriver_owner_count "${NVIDIA_DRIVER_NAME}" + + kubectl delete nvidiadriver/"${DUPLICATE_DEFAULT_NVIDIA_DRIVER_NAME}" + wait_for_default_nvidiadriver "${DEFAULT_NVIDIA_DRIVER_NAME}" + wait_for_nvidiadriver_ready "${DEFAULT_NVIDIA_DRIVER_NAME}" + wait_for_nvidiadriver_owner "${NVIDIA_DRIVER_NAME}" +} + test_arbitrary_name_default_nvidiadriver create_nvidiadriver wait_for_nvidiadriver_owner "${NVIDIA_DRIVER_NAME}" @@ -316,3 +355,4 @@ check_nvidia_driver_pods_ready test_driver_image_updates test_custom_labels_override test_removed_default_field_conflict_preserves_owners +test_multiple_default_drivers_are_not_ready