Skip to content

Commit

Permalink
chore: ntsc config not supported (#10056)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kkunapuli
kkunapuli authored Oct 16, 2024
1 parent 2e8de9b commit 315f65d
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 31 deletions.
24 changes: 12 additions & 12 deletions e2e_tests/tests/cluster/test_config_policies.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@ def test_set_config_policies() -> None:
print(data)
assert data.rstrip() == stdout.rstrip()

# valid path with ntsc type
# valid path with experiment type
stdout = detproc.check_output(
sess,
[
"det",
"config-policies",
"set",
"ntsc",
"experiment",
"--workspace",
workspace_name,
"--config-file",
Expand Down Expand Up @@ -79,7 +79,7 @@ def test_set_config_policies() -> None:
"det",
"config-policies",
"set",
"ntsc",
"experiment",
"--config-file",
conf.fixtures_path("config_policies/valid.yaml"),
],
Expand All @@ -93,7 +93,7 @@ def test_set_config_policies() -> None:
"det",
"config-policies",
"set",
"ntsc",
"experiment",
"--workspace",
workspace_name,
],
Expand All @@ -116,7 +116,7 @@ def test_describe_config_policies() -> None:
"det",
"config-policies",
"set",
"ntsc",
"experiment",
"--workspace",
workspace_name,
"--config-file",
Expand All @@ -131,7 +131,7 @@ def test_describe_config_policies() -> None:
"det",
"config-policies",
"describe",
"ntsc",
"experiment",
"--workspace",
workspace_name,
],
Expand All @@ -147,7 +147,7 @@ def test_describe_config_policies() -> None:
"det",
"config-policies",
"describe",
"ntsc",
"experiment",
"--workspace",
workspace_name,
"--yaml",
Expand All @@ -164,7 +164,7 @@ def test_describe_config_policies() -> None:
"det",
"config-policies",
"describe",
"ntsc",
"experiment",
"--workspace",
workspace_name,
"--json",
Expand All @@ -183,7 +183,7 @@ def test_describe_config_policies() -> None:
"det",
"config-policies",
"describe",
"ntsc",
"experiment",
],
"the following arguments are required: --workspace",
)
Expand All @@ -204,7 +204,7 @@ def test_delete_config_policies() -> None:
"det",
"config-policies",
"set",
"ntsc",
"experiment",
"--workspace",
workspace_name,
"--config-file",
Expand All @@ -218,7 +218,7 @@ def test_delete_config_policies() -> None:
"det",
"config-policies",
"delete",
"ntsc",
"experiment",
"--workspace",
workspace_name,
],
Expand All @@ -232,7 +232,7 @@ def test_delete_config_policies() -> None:
"det",
"config-policies",
"delete",
"ntsc",
"experiment",
],
"the following arguments are required: --workspace",
)
24 changes: 14 additions & 10 deletions master/internal/api_config_policies_intg_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ func TestAuthZCanModifyConfigPolicies(t *testing.T) {
WorkloadType: model.NTSCType,
ConfigPolicies: validNTSCConfigPolicyYAML,
})
require.NoError(t, err)
require.Error(t, err)
}

var cpAuthZ *mocks.ConfigPolicyAuthZ
Expand Down Expand Up @@ -623,18 +623,19 @@ invariant_config:
`
invariant_config:
description: "test\nspecial\tchar"
`, nil,
`, fmt.Errorf(configpolicy.NotSupportedConfigPolicyErr),
},
{
"YAML simple NTSC config with resources", model.NTSCType,
`
invariant_config:
resources:
slots: 1
`, nil,
`, fmt.Errorf(configpolicy.NotSupportedConfigPolicyErr),
},
{
"YAML partial NTSC config", model.NTSCType, validNTSCConfigPolicyYAML, nil,
"YAML partial NTSC config", model.NTSCType, validNTSCConfigPolicyYAML,
fmt.Errorf(configpolicy.NotSupportedConfigPolicyErr),
},

// Invalid experiment invariant config policies (YAML).
Expand Down Expand Up @@ -861,7 +862,7 @@ invariant_config:
// Additional NTSC combinatory tests (YAML).
{
"YAML NTSC valid config valid constraints", model.NTSCType,
validNTSCConfigPolicyYAML + validConstraintsPolicyYAML, nil,
validNTSCConfigPolicyYAML + validConstraintsPolicyYAML, fmt.Errorf(configpolicy.NotSupportedConfigPolicyErr),
},
{
"YAML NTSC valid constraints invalid constraints", model.NTSCType,
Expand Down Expand Up @@ -938,7 +939,7 @@ func TestValidatePoliciesAndWorkloadTypeJSON(t *testing.T) {
`{ "invariant_config": {
"description": "test\nspecial\tchar"
}
}`, nil,
}`, fmt.Errorf(configpolicy.NotSupportedConfigPolicyErr),
},
{
"JSON simple NTSC config with resources", model.NTSCType,
Expand All @@ -947,9 +948,12 @@ func TestValidatePoliciesAndWorkloadTypeJSON(t *testing.T) {
"slots": 1
}
}
}`, nil,
}`, fmt.Errorf(configpolicy.NotSupportedConfigPolicyErr),
},
{
"JSON partial NTSC config", model.NTSCType, "{" + validNTSCConfigPolicyJSON + "}",
fmt.Errorf(configpolicy.NotSupportedConfigPolicyErr),
},
{"JSON partial NTSC config", model.NTSCType, "{" + validNTSCConfigPolicyJSON + "}", nil},

// Invalid experiment invariant config policies (JSON).
{
Expand Down Expand Up @@ -1192,8 +1196,8 @@ func TestValidatePoliciesAndWorkloadTypeJSON(t *testing.T) {

// Additional NTSC combinatory tests (JSON).
{
"JSON NTSC valid config valid constraints", model.NTSCType,
"{" + validNTSCConfigPolicyJSON + "," + validConstraintsPolicyJSON + "}", nil,
"JSON NTSC valid config invalid constraints", model.NTSCType,
"{" + validConstraintsPolicyJSON + "}", nil,
},
{
"JSON NTSC valid constraints invalid constraints", model.NTSCType,
Expand Down
31 changes: 22 additions & 9 deletions master/internal/configpolicy/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ import (
const (
// GlobalConfigConflictErr is the error reported when an invariant config has a conflict
// with a value already set in the global config.
GlobalConfigConflictErr = "conflict between global and task config policy"
GlobalConfigConflictErr = "conflict between global and workspace config policy"
// InvalidExperimentConfigPolicyErr is the error reported by an invalid experiment config policy.
InvalidExperimentConfigPolicyErr = "invalid experiment config policy"
// InvalidNTSCConfigPolicyErr is the error reported by an invalid NTSC config policy.
InvalidNTSCConfigPolicyErr = "invalid ntsc config policy"
// NotSupportedConfigPolicyErr is the error reported when admins attempt to set NTSC invariant config.
NotSupportedConfigPolicyErr = "not supported"
)

// ConfigPolicyWarning logs a warning for the configuration policy component.
Expand Down Expand Up @@ -113,6 +115,11 @@ func ValidateNTSCConfig(
if err != nil || cp == nil {
return err // Handle error for nil cp or unmarshalling error.
}
if cp.InvariantConfig != nil {
msg := `invariant config policies for tasks is not yet supported,
please remove "invariant_config" section and try again`
return status.Errorf(codes.InvalidArgument, fmt.Sprintf(NotSupportedConfigPolicyErr+": %s.", msg))
}
if globalConfigPolicies != nil {
checkAgainstGlobalConfig[model.Constraints](globalConfigPolicies.Constraints, cp.Constraints, "invalid constraints")
checkAgainstGlobalConfig[model.CommandConfig](
Expand Down Expand Up @@ -153,15 +160,21 @@ func checkConstraintConflicts(constraints *model.Constraints, maxSlots, slots, p
if constraints == nil {
return nil
}
if priority != nil && *constraints.PriorityLimit != *priority {
return fmt.Errorf("invariant config & constraints are trying to set the priority limit")
if priority != nil && constraints.PriorityLimit != nil {
if *constraints.PriorityLimit != *priority {
return fmt.Errorf("invariant config & constraints are trying to set the priority limit")
}
}
if maxSlots != nil && *constraints.ResourceConstraints.MaxSlots != *maxSlots {
return fmt.Errorf("invariant config & constraints are trying to set the max slots")
if maxSlots != nil && constraints.ResourceConstraints != nil && constraints.ResourceConstraints.MaxSlots != nil {
if *constraints.ResourceConstraints.MaxSlots != *maxSlots {
return fmt.Errorf("invariant config & constraints are trying to set the max slots")
}
}
if slots != nil && *constraints.ResourceConstraints.MaxSlots < *slots {
return fmt.Errorf("invariant config has %v slots per trial. violates constraints max slots of %v",
*slots, *constraints.ResourceConstraints.MaxSlots)
if slots != nil && constraints.ResourceConstraints != nil && constraints.ResourceConstraints.MaxSlots != nil {
if *constraints.ResourceConstraints.MaxSlots < *slots {
return fmt.Errorf("invariant config has %v slots per trial. violates constraints max slots of %v",
*slots, *constraints.ResourceConstraints.MaxSlots)
}
}

return nil
Expand Down Expand Up @@ -211,7 +224,7 @@ func configPolicyConflict(config1, config2 interface{}) {
if field1.IsValid() && field2.IsValid() && !field1.IsZero() && !field2.IsZero() {
// For non-pointer fields, compare directly if both are non-zero
if !reflect.DeepEqual(field1.Interface(), field2.Interface()) {
ConfigPolicyWarning(fmt.Sprintf("%s: %v, %v", GlobalConfigConflictErr, field1.Interface(), field2.Interface()))
ConfigPolicyWarning(fmt.Sprintf("%s: field=%s", GlobalConfigConflictErr, v1.Type().Field(i).Name))
return
}
}
Expand Down

0 comments on commit 315f65d

Please sign in to comment.