chore: return error if workspace invariant config violates global co…

…nstraint CM-575: #in-review
determined-ai · Oct 17, 2024 · 58048fb · 58048fb
1 parent 3ca3418
commit 58048fb
Show file tree

Hide file tree

Showing 3 changed files with 313 additions and 31 deletions.
diff --git a/master/internal/api_config_policies.go b/master/internal/api_config_policies.go
@@ -50,9 +50,11 @@ func (a *apiServer) validatePoliciesAndWorkloadType(
 	_, priorityEnabledErr := a.m.rm.SmallerValueIsHigherPriority()
 	switch workloadType {
 	case model.ExperimentType:
-		return configpolicy.ValidateExperimentConfig(globalConfigPolicies, configPolicies, priorityEnabledErr)
+		return configpolicy.ValidateExperimentConfig(globalConfigPolicies, configPolicies,
+			priorityEnabledErr)
 	case model.NTSCType:
-		return configpolicy.ValidateNTSCConfig(globalConfigPolicies, configPolicies, priorityEnabledErr)
+		return configpolicy.ValidateNTSCConfig(globalConfigPolicies, configPolicies,
+			priorityEnabledErr)
 	default:
 		return status.Errorf(codes.InvalidArgument, fmt.Sprintf(invalidWorkloadTypeErr+": %s.", workloadType))
 	}
@@ -127,7 +129,8 @@ func (a *apiServer) PutWorkspaceConfigPolicies(
 		return nil, err
 	}
 
-	err = a.validatePoliciesAndWorkloadType(globalConfigPolicies, req.WorkloadType, req.ConfigPolicies)
+	err = a.validatePoliciesAndWorkloadType(globalConfigPolicies, req.WorkloadType,
+		req.ConfigPolicies)
 	if err != nil {
 		return nil, err
 	}

diff --git a/master/internal/configpolicy/utils.go b/master/internal/configpolicy/utils.go
@@ -14,7 +14,6 @@ import (
 
 	"github.com/determined-ai/determined/master/pkg/model"
 	"github.com/determined-ai/determined/master/pkg/protoutils"
-	"github.com/determined-ai/determined/master/pkg/schemas/expconf"
 )
 
 const (
@@ -81,11 +80,19 @@ func ValidateExperimentConfig(
 		return err
 	}
 
+	// Warn the user when fields specified in workspace config policies overlap with global config
+	// policies (since these fields will be overridden by the respective fields in the global
+	// policies).
+	var globalConstraints *model.Constraints
 	if globalConfigPolicies != nil {
-		checkAgainstGlobalConfig[model.Constraints](globalConfigPolicies.Constraints, cp.Constraints, "invalid constraints")
-		checkAgainstGlobalConfig[expconf.ExperimentConfig](
-			globalConfigPolicies.InvariantConfig, cp.InvariantConfig, InvalidExperimentConfigPolicyErr,
-		)
+		globalConstraints, err = UnmarshalConfigPolicy[model.Constraints](*globalConfigPolicies.Constraints,
+			InvalidExperimentConfigPolicyErr)
+		if err != nil {
+			ConfigPolicyWarning(err.Error())
+			return nil
+		}
+		configPolicyOverlap(globalConfigPolicies.Constraints, cp.Constraints)
+		configPolicyOverlap(globalConfigPolicies.InvariantConfig, cp.InvariantConfig)
 	}
 
 	if cp.Constraints != nil {
@@ -95,10 +102,20 @@ func ValidateExperimentConfig(
 	if cp.InvariantConfig != nil {
 		if cp.InvariantConfig.RawResources != nil {
 			checkAgainstGlobalPriority(priorityEnabledErr, cp.InvariantConfig.RawResources.RawPriority)
+
+			// Verify the workspace invariant config doesn't conflict with workspace constraints.
 			if err := checkConstraintConflicts(cp.Constraints, cp.InvariantConfig.RawResources.RawMaxSlots,
 				cp.InvariantConfig.RawResources.RawSlotsPerTrial, cp.InvariantConfig.RawResources.RawPriority); err != nil {
 				return status.Errorf(codes.InvalidArgument, fmt.Sprintf(InvalidExperimentConfigPolicyErr+": %s.", err))
 			}
+
+			// Verify the workspace invariant config doesn't conflict with global constraints.
+			if globalConstraints != nil {
+				if err := checkConstraintConflicts(globalConstraints, cp.InvariantConfig.RawResources.RawMaxSlots,
+					cp.InvariantConfig.RawResources.RawSlotsPerTrial, cp.InvariantConfig.RawResources.RawPriority); err != nil {
+					return status.Errorf(codes.InvalidArgument, fmt.Sprintf(InvalidExperimentConfigPolicyErr+": %s.", err))
+				}
+			}
 		}
 	}
 
@@ -120,11 +137,21 @@ func ValidateNTSCConfig(
 		  please remove "invariant_config" section and try again`
 		return status.Errorf(codes.InvalidArgument, fmt.Sprintf(NotSupportedConfigPolicyErr+": %s.", msg))
 	}
+
+	// Warn the user when fields specified in workspace config policies overlap with global config
+	// policies (since these fields will be overridden by the respective fields in the global
+	// policies).
+	var globalConstraints *model.Constraints
 	if globalConfigPolicies != nil {
-		checkAgainstGlobalConfig[model.Constraints](globalConfigPolicies.Constraints, cp.Constraints, "invalid constraints")
-		checkAgainstGlobalConfig[model.CommandConfig](
-			globalConfigPolicies.InvariantConfig, cp.InvariantConfig, InvalidNTSCConfigPolicyErr,
-		)
+		globalConstraints, err = UnmarshalConfigPolicy[model.Constraints](*globalConfigPolicies.Constraints,
+			InvalidNTSCConfigPolicyErr)
+		if err != nil {
+			ConfigPolicyWarning(err.Error())
+			return nil
+		}
+		configPolicyOverlap(globalConfigPolicies.Constraints, cp.Constraints)
+		configPolicyOverlap(
+			globalConfigPolicies.InvariantConfig, cp.InvariantConfig)
 	}
 
 	if cp.Constraints != nil {
@@ -141,10 +168,20 @@ func ValidateNTSCConfig(
 			slots = &cp.InvariantConfig.Resources.Slots
 		}
 
+		// Verify the workspace invariant config doesn't conflict with workspace constraints.
 		if err := checkConstraintConflicts(cp.Constraints, cp.InvariantConfig.Resources.MaxSlots,
 			slots, cp.InvariantConfig.Resources.Priority); err != nil {
 			return status.Errorf(codes.InvalidArgument, fmt.Sprintf(InvalidNTSCConfigPolicyErr+": %s.", err))
 		}
+
+		// Verify the workspace invariant config conflict with global constraints.
+		if globalConstraints != nil {
+			if err := checkConstraintConflicts(globalConstraints,
+				cp.InvariantConfig.Resources.MaxSlots, slots,
+				cp.InvariantConfig.Resources.Priority); err != nil {
+				return status.Errorf(codes.InvalidArgument, fmt.Sprintf(InvalidNTSCConfigPolicyErr+": %s.", err))
+			}
+		}
 	}
 
 	return err
@@ -180,25 +217,9 @@ func checkConstraintConflicts(constraints *model.Constraints, maxSlots, slots, p
 	return nil
 }
 
-// checkAgainstGlobalConfig is a generic to check constraints & invariant configs against the global config.
-func checkAgainstGlobalConfig[T any](
-	globalConfigPolicies *string,
-	config *T,
-	errorMsg string,
-) {
-	if globalConfigPolicies != nil && config != nil {
-		global, err := UnmarshalConfigPolicy[T](*globalConfigPolicies, errorMsg)
-		if err != nil {
-			ConfigPolicyWarning(err.Error())
-			return
-		}
-		configPolicyConflict(global, config)
-	}
-}
-
-// configPolicyConflict compares two different configurations and
-// returns an error if both try to define the same field.
-func configPolicyConflict(config1, config2 interface{}) {
+// configPolicyOverlap compares two different configurations and
+// warns the user when both configurations define the same field.
+func configPolicyOverlap(config1, config2 interface{}) {
 	v1 := reflect.ValueOf(config1)
 	v2 := reflect.ValueOf(config2)