sealer icon indicating copy to clipboard operation
sealer copied to clipboard

merge config did not take effect

Open sfeng1996 opened this issue 2 years ago • 7 comments

What happened:

i want to use sealer config to update calico network interface IP discovery rule,but not effective this is my config clusterfile

---
apiVersion: sealer.aliyun.com/v1alpha1
kind: Config
metadata:
  creationTimestamp: null
  name: nodeAddr
spec:
  data: |
    spec:
      calicoNetwork:
        nodeAddressAutodetectionV4:
          interface: "eth.*|en.*|wlp.*|bond.*"
  path: etc/custom-resources.yaml
  strategy: merge
status: {}

this is "etc/custom-resources.yaml" after sealer apply

apiVersion: operator.tigera.io/v1
kind: Installation
metadata:
  name: default
spec:
  calicoNetwork:
    ipPools:
    - blockSize: 26
      cidr: 100.64.0.0/10
      encapsulation: VXLANCrossSubnet
      natOutgoing: Enabled
      nodeSelector: all()
  registry: sea.hub:5000

---
apiVersion: operator.tigera.io/v1
kind: APIServer
metadata:
  name: default
spec: {}

i guess 'etc/custom-resource.yaml' have not key of 'nodeAddressAutodetectionV4:\n interface:' , so sealer deepMerge config did not take effect

Environment:

  • sealer version (use sealer version): v0.8.5
  • kubernetes image: v1.21.12

sfeng1996 avatar Jun 08 '22 06:06 sfeng1996

Merge is only suitable for merging existing structures, and there are too many uncertainties for attributes that do not exist in this way, such as:

---
apiVersion: sealer.aliyun.com/v1alpha1
kind: Config
metadata:
  creationTimestamp: null
  name: nodeAddr
spec:
  data: |
    spec:
      calicoNetwork:
        nodeAddressAutodetectionV4:
          interface: "eth.*|en.*|wlp.*|bond.*"
  path: etc/custom-resources.yaml
  strategy: merge
status: {}

result:

apiVersion: operator.tigera.io/v1
kind: Installation
metadata:
  name: default
spec:
  calicoNetwork:
    nodeAddressAutodetectionV4:
      interface: "eth.*|en.*|wlp.*|bond.*"
    ipPools:
    - blockSize: 26
      cidr: 100.64.0.0/10
      encapsulation: VXLANCrossSubnet
      natOutgoing: Enabled
      nodeSelector: all()
  registry: sea.hub:5000

---
apiVersion: operator.tigera.io/v1
kind: APIServer
metadata:
  name: default
spec: 
  calicoNetwork:
  nodeAddressAutodetectionV4:
    interface: "eth.*|en.*|wlp.*|bond.*"

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

bxy4543 avatar Jun 08 '22 07:06 bxy4543

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

@bxy4543 It is the limitation of current functionality. While we should clarify that as clearly as possible. Otherwise, user tries, then user abandons. We need quite a long time to improve the service quality. 😢

allencloud avatar Jun 08 '22 07:06 allencloud

Merge is only suitable for merging existing structures, and there are too many uncertainties for attributes that do not exist in this way, such as:

---
apiVersion: sealer.aliyun.com/v1alpha1
kind: Config
metadata:
  creationTimestamp: null
  name: nodeAddr
spec:
  data: |
    spec:
      calicoNetwork:
        nodeAddressAutodetectionV4:
          interface: "eth.*|en.*|wlp.*|bond.*"
  path: etc/custom-resources.yaml
  strategy: merge
status: {}

result:

apiVersion: operator.tigera.io/v1
kind: Installation
metadata:
  name: default
spec:
  calicoNetwork:
    nodeAddressAutodetectionV4:
      interface: "eth.*|en.*|wlp.*|bond.*"
    ipPools:
    - blockSize: 26
      cidr: 100.64.0.0/10
      encapsulation: VXLANCrossSubnet
      natOutgoing: Enabled
      nodeSelector: all()
  registry: sea.hub:5000

---
apiVersion: operator.tigera.io/v1
kind: APIServer
metadata:
  name: default
spec: 
  calicoNetwork:
  nodeAddressAutodetectionV4:
    interface: "eth.*|en.*|wlp.*|bond.*"

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

i know what you mean,can you develop a new strategy for similar situation,the overwrite strategy is not convenient for long text files

sfeng1996 avatar Jun 08 '22 07:06 sfeng1996

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

@bxy4543 It is the limitation of current functionality. While we should clarify that as clearly as possible. Otherwise, user tries, then user abandons. We need quite a long time to improve the service quality. 😢

yes, I will explain it in all the docs.

bxy4543 avatar Jun 08 '22 08:06 bxy4543

@sfeng1996 I think we can provide a shallowMerge strategy to achieve this feature.

bxy4543 avatar Jun 08 '22 08:06 bxy4543

@sfeng1996 I think we can provide a shallowMerge strategy to achieve this feature.

tks,look forward to it

sfeng1996 avatar Jun 08 '22 09:06 sfeng1996

@sfeng1996 I think we can provide a shallowMerge strategy to achieve this feature.

@bxy4543 , my opinion about this is could we find another way to do merge rather then provide shallowMerge, thats because it make our config function more complexed. on the other hand, merge strategy native means add a new key which is not exist on the dest.

kakaZhou719 avatar Jul 08 '22 07:07 kakaZhou719