Skip to content
This repository has been archived by the owner on Oct 17, 2018. It is now read-only.

Commit

Permalink
Address feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@xichen2020
xichen2020 committed Oct 11, 2017
1 parent bfb59e5 commit 2e5cbb6
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 119 deletions.
81 changes: 5 additions & 76 deletions rules/ruleset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ func (rs *ruleSet) Clone() MutableRuleSet {
rollupRules[i] = &c
}

// this clone deliberately ignores tagFliterOpts and rollupIDFn
// This clone deliberately ignores tagFliterOpts and rollupIDFn
// as they are not useful for the MutableRuleSet.
return MutableRuleSet(&ruleSet{
uuid: rs.uuid,
Expand All @@ -745,11 +745,6 @@ func (rs *ruleSet) Clone() MutableRuleSet {
}

func (rs *ruleSet) AddMappingRule(mrv MappingRuleView, meta UpdateMetadata) (string, error) {
err := rs.validateMappingRuleUpdate(mrv)
if err != nil {
return "", err
}

m, err := rs.getMappingRuleByName(mrv.Name)
if err != nil && err != errNoSuchRule {
return "", fmt.Errorf(ruleActionErrorFmt, "add", mrv.Name, err)
Expand Down Expand Up @@ -779,10 +774,6 @@ func (rs *ruleSet) AddMappingRule(mrv MappingRuleView, meta UpdateMetadata) (str
}

func (rs *ruleSet) UpdateMappingRule(mrv MappingRuleView, meta UpdateMetadata) error {
err := rs.validateMappingRuleUpdate(mrv)
if err != nil {
return err
}
m, err := rs.getMappingRuleByID(mrv.ID)
if err != nil {
return fmt.Errorf(ruleActionErrorFmt, "update", mrv.ID, err)
Expand Down Expand Up @@ -812,11 +803,6 @@ func (rs *ruleSet) DeleteMappingRule(id string, meta UpdateMetadata) error {
}

func (rs *ruleSet) AddRollupRule(rrv RollupRuleView, meta UpdateMetadata) (string, error) {
err := rs.validateRollupRuleUpdate(rrv)
if err != nil {
return "", err
}

r, err := rs.getRollupRuleByName(rrv.Name)
if err != nil && err != errNoSuchRule {
return "", fmt.Errorf(ruleActionErrorFmt, "add", rrv.Name, err)
Expand Down Expand Up @@ -847,11 +833,6 @@ func (rs *ruleSet) AddRollupRule(rrv RollupRuleView, meta UpdateMetadata) (strin
}

func (rs *ruleSet) UpdateRollupRule(rrv RollupRuleView, meta UpdateMetadata) error {
err := rs.validateRollupRuleUpdate(rrv)
if err != nil {
return err
}

r, err := rs.getRollupRuleByID(rrv.ID)
if err != nil {
return fmt.Errorf(ruleActionErrorFmt, "update", rrv.ID, err)
Expand Down Expand Up @@ -973,10 +954,10 @@ func (rs *ruleSet) latestMappingRules() (map[string]*MappingRuleView, error) {
return nil, err
}
result := make(map[string]*MappingRuleView, len(mrs))
for _, m := range mrs {
for id, m := range mrs {
if len(m) > 0 && !m[0].Tombstoned {
// views included in m are sorted latest first.
result[m[0].ID] = m[0]
result[id] = m[0]
}
}
return result, nil
Expand All @@ -988,10 +969,10 @@ func (rs *ruleSet) latestRollupRules() (map[string]*RollupRuleView, error) {
return nil, err
}
result := make(map[string]*RollupRuleView, len(rrs))
for _, r := range rrs {
for id, r := range rrs {
if len(r) > 0 && !r[0].Tombstoned {
// views included in m are sorted latest first.
result[r[0].ID] = r[0]
result[id] = r[0]
}
}
return result, nil
Expand Down Expand Up @@ -1154,55 +1135,3 @@ type RuleSetSnapshot struct {
MappingRules map[string]*MappingRuleView
RollupRules map[string]*RollupRuleView
}

func (rs ruleSet) validateMappingRuleUpdate(mrv MappingRuleView) error {
for _, m := range rs.mappingRules {
// Ignore tombstoned.
if m.Tombstoned() {
continue
}

n, err := m.Name()
if err != nil {
return err
}

// If the rule getting updated keeps its name, that is fine.
if n == mrv.Name && m.uuid != mrv.ID {
return NewRuleConflictError(fmt.Sprintf("Rule with name: %s already exists", n))
}
}

return nil
}

func (rs ruleSet) validateRollupRuleUpdate(rrv RollupRuleView) error {
for _, r := range rs.rollupRules {
// Ignore tombstoned.
if r.Tombstoned() {
continue
}

n, err := r.Name()
if err != nil {
return err
}

// If the rule getting updated keeps its name, that is fine.
if n == rrv.Name && r.uuid != rrv.ID {
return NewRuleConflictError(fmt.Sprintf("Rule with name: %s already exists", n))
}

// We've already checked that some snapshots exist by checking the name.
latestSnapshot := r.snapshots[len(r.snapshots)-1]
for _, t1 := range latestSnapshot.targets {
for _, t2 := range rrv.Targets {
if t1.sameTransform(t2.rollupTarget()) {
return NewRuleConflictError(fmt.Sprintf("Same rollup transformation: %s: %v already exists", t1.Name, t1.Tags))
}
}
}
}

return nil
}
29 changes: 0 additions & 29 deletions rules/ruleset_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2419,35 +2419,6 @@ func TestUpdateRollupRule(t *testing.T) {
require.NoError(t, err)
}

func TestUpdateRollupRuleDupTarget(t *testing.T) {
mutable, rs, helper, err := initMutableTest()
require.NoError(t, err)

rr, err := rs.getRollupRuleByID("rollupRule5")
require.NoError(t, err)

newFilters := map[string]string{"tag1": "value", "tag2": "value"}
p := []policy.Policy{policy.NewPolicy(policy.NewStoragePolicy(time.Minute, xtime.Minute, time.Hour), policy.DefaultAggregationID)}
// Duplicate target from rollupRule4
newTargets := []RollupTargetView{
RollupTargetView{
Name: "rName3",
Tags: []string{"rtagName1", "rtagName2"},
Policies: p,
},
}

view := RollupRuleView{
ID: rr.uuid,
Name: "foo",
Filters: newFilters,
Targets: newTargets,
}

err = mutable.UpdateRollupRule(view, helper.NewUpdateMetadata(now, testUser))
require.Error(t, err)
}

func TestDeleteRollupRule(t *testing.T) {
mutable, rs, helper, err := initMutableTest()
require.NoError(t, err)
Expand Down
60 changes: 48 additions & 12 deletions rules/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,24 +44,28 @@ func NewValidator(opts ValidatorOptions) Validator {
}

func (v *validator) Validate(rs RuleSet) error {
if err := v.validate(rs); err != nil {
return v.wrapError(err)
}
return nil
}

func (v *validator) validate(rs RuleSet) error {
// Only the latest (a.k.a. the first) view needs to be validated
// because that is the view that may be invalid due to latest update.
latest, err := rs.Latest()
if err != nil {
return NewValidationError(fmt.Sprintf("could not get the latest ruleset snapshot: %v", err))
return fmt.Errorf("could not get the latest ruleset snapshot: %v", err)
}
if err := v.validateMappingRules(latest.MappingRules); err != nil {
return NewValidationError(fmt.Sprintf("could not validate mapping rules: %v", err))
}
if err := v.validateRollupRules(latest.RollupRules); err != nil {
return NewValidationError(fmt.Sprintf("could not validate rollup rules: %v", err))
return err
}
return nil
return v.validateRollupRules(latest.RollupRules)
}

func (v *validator) validateMappingRules(rules map[string]*MappingRuleView) error {
namesSeen := make(map[string]struct{}, len(rules))
for _, view := range rules {
func (v *validator) validateMappingRules(mrv map[string]*MappingRuleView) error {
namesSeen := make(map[string]struct{}, len(mrv))
for _, view := range mrv {
// Validate that no rules with the same name exist.
if _, exists := namesSeen[view.Name]; exists {
return NewRuleConflictError(fmt.Sprintf("mapping rule %s already exists", view.Name))
Expand All @@ -84,9 +88,9 @@ func (v *validator) validateMappingRules(rules map[string]*MappingRuleView) erro
return nil
}

func (v *validator) validateRollupRules(rules map[string]*RollupRuleView) error {
namesSeen := make(map[string]struct{}, len(rules))
for _, view := range rules {
func (v *validator) validateRollupRules(rrv map[string]*RollupRuleView) error {
namesSeen := make(map[string]struct{}, len(rrv))
for _, view := range rrv {
// Validate that no rules with the same name exist.
if _, exists := namesSeen[view.Name]; exists {
return NewRuleConflictError(fmt.Sprintf("rollup rule %s already exists", view.Name))
Expand All @@ -108,6 +112,24 @@ func (v *validator) validateRollupRules(rules map[string]*RollupRuleView) error
}
}
}

// Validate that there are no conflictiing rollup targets.
return v.validateRollupTargets(rrv)
}

func (v *validator) validateRollupTargets(rrv map[string]*RollupRuleView) error {
seen := make([]RollupTarget, 0, len(rrv))
for _, view := range rrv {
for _, target := range view.Targets {
current := target.rollupTarget()
for _, seenTarget := range seen {
if current.sameTransform(seenTarget) {
return NewRuleConflictError(fmt.Sprintf("rollup target with name %s and tags %s already exists", current.Name, current.Tags))
}
}
seen = append(seen, current)
}
}
return nil
}

Expand Down Expand Up @@ -143,3 +165,17 @@ func (v *validator) validatePolicy(t metric.Type, p policy.Policy) error {

return nil
}

func (v *validator) wrapError(err error) error {
if err == nil {
return nil
}
switch err.(type) {
// Do not wrap error for these error types so caller can take actions based on the correct
// error type.
case RuleConflictError:
return err
default:
return NewValidationError(err.Error())
}
}
87 changes: 85 additions & 2 deletions rules/validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ const (
func TestValidatorValidateDuplicateMappingRules(t *testing.T) {
ruleSet := testRuleSetWithMappingRules(t, testDuplicateMappingRulesConfig())
validator := NewValidator(testValidatorOptions())
require.Error(t, ruleSet.Validate(validator))
err := ruleSet.Validate(validator)
require.Error(t, err)
_, ok := err.(RuleConflictError)
require.True(t, ok)
}

func TestValidatorValidateNoDuplicateMappingRulesWithTombstone(t *testing.T) {
Expand Down Expand Up @@ -140,7 +143,10 @@ func TestValidatorValidateMappingRuleCustomAggregationTypes(t *testing.T) {
func TestValidatorValidateDuplicateRollupRules(t *testing.T) {
ruleSet := testRuleSetWithRollupRules(t, testDuplicateRollupRulesConfig())
validator := NewValidator(testValidatorOptions())
require.Error(t, ruleSet.Validate(validator))
err := ruleSet.Validate(validator)
require.Error(t, err)
_, ok := err.(RuleConflictError)
require.True(t, ok)
}

func TestValidatorValidateNoDuplicateRollupRulesWithTombstone(t *testing.T) {
Expand Down Expand Up @@ -236,6 +242,16 @@ func TestValidatorValidateRollupRuleCustomAggregationTypes(t *testing.T) {
}
}

func TestValidatorValidateRollupRuleConflictingTargets(t *testing.T) {
ruleSet := testRuleSetWithRollupRules(t, testConflictingTargetsRollupRulesConfig())
opts := testValidatorOptions()
validator := NewValidator(opts)
err := ruleSet.Validate(validator)
require.Error(t, err)
_, ok := err.(RuleConflictError)
require.True(t, ok)
}

func testDuplicateMappingRulesConfig() []*schema.MappingRule {
return []*schema.MappingRule{
&schema.MappingRule{
Expand Down Expand Up @@ -468,6 +484,73 @@ func testCustomAggregationTypeRollupRulesConfig() []*schema.RollupRule {
}
}

func testConflictingTargetsRollupRulesConfig() []*schema.RollupRule {
return []*schema.RollupRule{
&schema.RollupRule{
Uuid: "rollupRule1",
Snapshots: []*schema.RollupRuleSnapshot{
&schema.RollupRuleSnapshot{
Name: "snapshot1",
Tombstoned: false,
TagFilters: map[string]string{
testTypeTag: testTimerType,
},
Targets: []*schema.RollupTarget{
&schema.RollupTarget{
Name: "rName1",
Tags: []string{"rtagName1", "rtagName2"},
Policies: []*schema.Policy{
&schema.Policy{
StoragePolicy: &schema.StoragePolicy{
Resolution: &schema.Resolution{
WindowSize: int64(10 * time.Second),
Precision: int64(time.Second),
},
Retention: &schema.Retention{
Period: int64(6 * time.Hour),
},
},
},
},
},
},
},
},
},
&schema.RollupRule{
Uuid: "rollupRule2",
Snapshots: []*schema.RollupRuleSnapshot{
&schema.RollupRuleSnapshot{
Name: "snapshot2",
Tombstoned: false,
TagFilters: map[string]string{
testTypeTag: testTimerType,
},
Targets: []*schema.RollupTarget{
&schema.RollupTarget{
Name: "rName1",
Tags: []string{"rtagName1", "rtagName2"},
Policies: []*schema.Policy{
&schema.Policy{
StoragePolicy: &schema.StoragePolicy{
Resolution: &schema.Resolution{
WindowSize: int64(10 * time.Second),
Precision: int64(time.Second),
},
Retention: &schema.Retention{
Period: int64(6 * time.Hour),
},
},
},
},
},
},
},
},
},
}
}

func testRuleSetWithMappingRules(t *testing.T, mrs []*schema.MappingRule) RuleSet {
rs := &schema.RuleSet{MappingRules: mrs}
newRuleSet, err := NewRuleSetFromSchema(1, rs, testRuleSetOptions())
Expand Down

0 comments on commit 2e5cbb6

Please sign in to comment.