Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: ntsc config not supported #10056

Merged
merged 13 commits into from
Oct 16, 2024
Merged

chore: ntsc config not supported #10056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
88b52d9
chore: ntsc config not supported
kkunapuli Oct 15, 2024
8981001
fix formatting
kkunapuli Oct 15, 2024
14c5f4b
fix tests
kkunapuli Oct 15, 2024
b92da9e
describe error
kkunapuli Oct 15, 2024
e2cae25
fix tests, again
kkunapuli Oct 15, 2024
9f7caa9
lint
kkunapuli Oct 15, 2024
a7e6cb4
fmt
kkunapuli Oct 15, 2024
16a88d2
e2e test fix
kkunapuli Oct 15, 2024
4224379
use ntsc specific output, too
kkunapuli Oct 15, 2024
a923906
test with experiment instead of ntsc
kkunapuli Oct 15, 2024
f58c851
unneeded file
kkunapuli Oct 15, 2024
7d4aaa1
Merge branch 'main' into kunapuli/no-ntsc-config
kkunapuli Oct 15, 2024
ed1d237
lint
kkunapuli Oct 15, 2024
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
24 changes: 12 additions & 12 deletions e2e_tests/tests/cluster/test_config_policies.py
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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))
Copy link
Contributor Author

@kkunapuli kkunapuli Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was confusing: it just printed out the memory address of both fields. We could determine if the field is a pointer or not and print each case differently ... but it seemed simpler and more clear to simply tell the admin which field conflicts.

return
}
}
Expand Down
Loading