Add patchMergeKey to VMRule CRDs

[vaskozl]

The CRDs do not define patchMergeKey and patchStrategy for both group names and rule alert fields.

https://github.com/VictoriaMetrics/operator/blob/master/api/v1beta1/vmrule_types.go

The lack of merge keys makes it hard to adjust the rules and alerts with both kubectl patch and kustomize. This is especially prudent when you deploy the default rules with the stack chart, which can be quite noisy.

For example if one uses the stack chart deployed with flux, but wants to adjust some rules after the fact, they may used a kustomize postrenderer (or otherwise directly with kubectl patch) to try and and change a given rule (e.g. increasing the required alert time to 15m -> 60m):

   kind: VMRule    
   apiVersion: operator.victoriametrics.com/v1beta1    
   metadata:    
     name: stack-kubernetes-apps    
     namespace: victoria-metrics    
   spec:    
     groups:    
       - name: kubernetes-apps    
         rules:    
           - alert: KubeDeploymentGenerationMismatch                                              
             for: 60m 

Without the merge key the entire list of groups is overwritten, replacing everything with the list in the patch. The only current solution is to redefine the entirety of the Rules in your patch or to otherwise use a JSON patch with the numeric index of the group/rule you want to patch (which may change as new version of the charts are pushed). While in theory the chart could allow for rule customization, I would expect the CRDs to specify merging on group name and alert rule field.

This currently affects v0.34.0 of the operator and apiVersion operator.victoriametrics.com/v1beta1.

[Haleygo]

/assign

[Haleygo]

@f41gh7 Adding mergeKey for groups is pretty easy. But for rules, alerting and recording rule use different field for their name in the current schema, so mergeKey is not feasible now. Should we consider adding a mandatory name field for both rules in next version?

Looks like prometheus operator has the same problem.

[f41gh7]

@f41gh7 Adding mergeKey for groups is pretty easy. But for rules, alerting and recording rule use different field for their name in the current schema, so mergeKey is not feasible now. Should we consider adding a mandatory name field for both rules in next version?

Looks like prometheus operator has the same problem.

I think only expr is possible candidate for mergeKey. But it maybe non unique, should it be user responsibility for keeping it unique for this case?

For me it makes no sense to add mandatory name field only as mergeKey, it's rare case and it breaks backward-compatibility.

[Haleygo]

I think only expr is possible candidate for mergeKey. But it maybe non unique, should it be user responsibility for keeping it unique for this case?

I think it's weird to make expr unique in that way. We didn't have this requirement when creating vmrule, and if we add it there, what's the purpose, to avoid redundant data for storage? Maybe users want to have the same expr for different metric name for some reason, and can they bypass this limitation by reordering the labels or adding meaningless label?

For me it makes no sense to add mandatory name field only as mergeKey, it's rare case

In @vaskozl 's case, add name as merge key might be helpful.

I saw the operator has no limitations now, only deduplicate rules according to both it's name and all the other properties when operator.victoriametrics.com/vmalert-deduplicate-rules is enable. So maybe we should keep it that way?

[f41gh7]

In @vaskozl 's case, add name as merge key might be helpful.

I saw the operator has no limitations now, only deduplicate rules according to both it's name and all the other properties when operator.victoriametrics.com/vmalert-deduplicate-rules is enable. So maybe we should keep it that way?

It could be useful, but it brings negative user experience. With mandatory name field, you cannot copy-paste rules from non-operator installations or examples from an internet. You have to edit it and add a new field.

For now, I don't have any good solution for it.

[sjmalnati]

Here's a new link to the spec: https://github.com/VictoriaMetrics/operator/blob/master/api/operator/v1beta1/vmrule_types.go

I would also like this feature, but don't have a non-breaking change solution. I wish we could specify the key as alert+record but I don't think that's supported, especially since one of those is always missing.

[AndrewChubatiuk]

listMapKey is now a replacement for patchMergeKey and it supports multiple attributes. it means no name property is needed and rule key can consist of record and alert combination, but neither vmalert nor prometheus requires uniqueness of record or alert properties.

[mvanhorn]

Implemented in #2258.