Substra / hlf-k8s

Initializes an Hyperledger Fabric network (orchestrator distributed mode)
https://docs.substra.org
Apache License 2.0
31 stars 20 forks source link

Disable hooks and toolbox by default + remove ClusterRole #54

Closed AurelienGasser closed 4 years ago

AurelienGasser commented 4 years ago

A bunch of changes:

AurelienGasser commented 4 years ago

@Kelvin-M I have two questions:

  1. Don't you think it would be better to keep the secret deletion hook enabled by default? I can't imagine a scenario when you want to helm delete the release but keep the secrets. Keeping the secrets is actually dangerous since the next helm install won't work (wrong secrets).

  2. It looks like the chaincode uninstall hook isn't needed:

$ docker ps | grep mycc | wc -l
2
☐ ~/code/hlf-k8s$ helm list
NAME                    REVISION    UPDATED                     STATUS      CHART           APP VERSION NAMESPACE
network-orderer         1           Tue May  5 15:25:41 2020    DEPLOYED    hlf-k8s-1.1.1               orderer  
network-org-1-peer-1    1           Tue May  5 15:25:48 2020    DEPLOYED    hlf-k8s-1.1.1               org-1    
network-org-2-peer-1    1           Tue May  5 15:25:56 2020    DEPLOYED    hlf-k8s-1.1.1               org-2    
☐ ~/code/hlf-k8s$ helm get hooks network-org-1-peer-1
☐ ~/code/hlf-k8s$ skaffold delete
Cleaning up...
release "network-orderer" deleted
release "network-org-1-peer-1" deleted
release "network-org-2-peer-1" deleted
☐ ~/code/hlf-k8s$ docker ps | grep mycc | wc -l
0
  1. delete both peers in skaffold.yaml > deploy
  2. skaffold run
  3. kubectl apply -n org-1 -f ./renderer-peer-1.yaml (renderer-peer-1.yaml)
  4. ./tools/test-dev-network.sh 1 => all good, chaincode running
  5. kubectl delete -n org-1 -f ./renderer-peer-1.yaml
    • Edit: même effet avec kubectl delete pod network-org-1-peer-1-d4c4cd8c-4d82n -n org-1
  6. docker ps => chaincode container has disappeared
Kelvin-M commented 4 years ago

@AurelienGasser

  1. I agree with your first point. But as @AlexandrePicosson said, I you just render the manifest and use it with kubectl apply and not helm, the secret deletion jobs will be useless ( but maybe it's not a issue to keep it enable by default).

  2. Chaincode uninstall does two things :

              docker rm -f nid1-{{ $.Release.Name }}-{{ .name }}-{{ .version }} || true
              docker rmi -f $(docker images -q 'nid1-{{ $.Release.Name }}-{{ .name }}-{{ .version }}*') || true

The main issue does not comes with the fact that the containers is up (normally it doesn't but it's a sanity first step to remove all containers up). The main issue is the presence of the image in the local docker registry (through the docker daemon). In fact, if you change the chaincode code version, peer will not rebuild the image as is it still present in the local registry. So you can run into issue by having a unwanted (old) version of the chaincode because of this remaining image.

Can you check with docker images | grep 'mycc' instead ?

AurelienGasser commented 4 years ago

After conversation with @Kelvin-M we decided to enable both hooks by default. because it corresponds desired behavior in the typical use-case.