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.

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

docker image allows --chain param + shell #1079

Conversation

asiniscalchi
Copy link

@asiniscalchi asiniscalchi commented Nov 6, 2023

The execution of the Astar collator image via Docker is encountering an error:

➜  ~ docker run staketechnologies/astar-collator:v5.23.0 --chain astar
docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "--chain": executable file not found in $PATH: unknown.
ERRO[0000] error waiting for container:            

This patch corrects the issue, making the Docker image compatible with the ParityTech Helm charts, as detailed here:
https://github.com/paritytech/helm-charts/tree/main/charts/node

@asiniscalchi asiniscalchi changed the title CMD -> ENTRYPOINT docker image allow --chain param Nov 6, 2023
@asiniscalchi asiniscalchi changed the title docker image allow --chain param docker image allows --chain param Nov 6, 2023
@Dinonard
Copy link
Member

Dinonard commented Nov 6, 2023

@mc2eqe can you please take a look?

@mc2eqe
Copy link
Contributor

mc2eqe commented Nov 6, 2023

Hi @asiniscalchi. Thank you for submitting your proposal and helping out. Please, can you explain in more detail why do you need this image to align with Helm charts? Do you plan to run it on Kubernetes?
Our Docker image is built to be run on Docker Linux machines, like described in this doc: https://docs.astar.network/docs/build/nodes/archive-node/docker

@asiniscalchi
Copy link
Author

asiniscalchi commented Nov 7, 2023

The PR introduces two distinct improvements to the Docker image:

  1. Change from CMD to ENTRYPOINT:
    This modification shifts the container's main executable specification from the CMD directive to ENTRYPOINT. The primary advantage of this is the encapsulation of the executable within the image, which simplifies the command needed to run the container. Instead of having to specify the executable each time, like so:
    docker run staketechnologies/astar-collator:latest astar-collator --chain astar
    Users can now simply pass the necessary arguments directly, as the ENTRYPOINT specifies the executable:
    docker run staketechnologies/astar-collator:latest --chain astar

This change is particularly beneficial for Kubernetes deployments, where the ENTRYPOINT of a Docker image typically specifies the starting command.

  1. Retention of the shell (/bin/sh):
    The Docker image previously had steps to remove /usr/bin and /usr/sbin, which included the shell. The Helm chart by Parity Tech relies on the shell within the container to execute a sequence of operations. For instance, the persist-generated-node-key init container script uses /bin/sh to perform actions such as generating a node key if it doesn't already exist:
- name: persist-generated-node-key
  image: {{ .Values.image.repository }}:{{ .Values.image.tag }}
  command: [ "/bin/sh" ]
  args:
    - -c
    - |
      set -eu {{ if .Values.initContainers.persistGeneratedNodeKey.debug }}-x{{ end }}
      ...
      # Script continues to check and generate the node key

Maintaining the shell in the image ensures compatibility with these scripts and prevents the failure of Helm chart operations that depend on shell access within the container.

The PR's intent is to provide a Docker image that not only maintains backward compatibility but also enhances the image's usability within Kubernetes environments, especially when managed via Helm charts.

An example of our current usage in k8s:

repositories:
  - name: parity
    url: https://paritytech.github.io/helm-charts/

helmDefaults:
  createNamespace: false
  waitForJobs: true

namespace: astar

releases:
  - name: collator
    chart: parity/node
    version: 5.1.0
    values:
      - image:
          repository: freeverseio/astar
          tag: cd9c353d5a23c4545a16e4d02f78ff3a36ad3d53
      - node:
          chain: astar
          command: "astar-collator"
          replicas: 1
          role: collator
          perNodeServices:
            paraP2pService:
              enabled: true
              type: LoadBalancer
          isParachain: true
          allowUnsafeRpcMethods: true
          customChainspecUrl: https://raw.githubusercontent.com/freeverseio/laos/main/ownership-chain/specs/astar-local-raw.json
          collatorRelayChain:
            customChainspecUrl: https://raw.githubusercontent.com/freeverseio/laos-ownership-node/dev/specs/rococo-freeverse-chainspec.json

@asiniscalchi asiniscalchi changed the title docker image allows --chain param docker image allows --chain param + shell Nov 7, 2023
@mc2eqe
Copy link
Contributor

mc2eqe commented Nov 7, 2023

@asiniscalchi Thank you for clarifying and explaining your proposal. Yes I agree, we should address this to be inlined with Kubernetes and Parity Helm charts.
If we make a change to an ENTRYPOINT, the change influence other Astar Docker users who run collator with CMD arguments so we are not yet eager to make the change with this PR.
I propose that we open new Issue on this repo, if you agree, with your comment in this PR as a request for an enhancement.
For now, if you need a solution you can always modify our code and build new image with your own image repository destination.
Dockerfile https://github.com/AstarNetwork/Astar/blob/master/third-party/docker/Dockerfile needs just astar collator binary to be uploaded in an image.
Once again, thank you for your engagement and effort.

@asiniscalchi
Copy link
Author

seems good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants