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

optimize: require ARM64 binary #4963

Merged
merged 2 commits into from
Nov 7, 2022
Merged

optimize: require ARM64 binary #4963

merged 2 commits into from
Nov 7, 2022

Conversation

odidev
Copy link
Contributor

@odidev odidev commented Sep 27, 2022

Modified file  .github/workflows/test.yml, dependencies/pom.xml, discovery/seata-discovery-etcd3/pom.xml, discovery/seata-discovery-etcd3/src/test/java/io/seata/discovery/registry/etcd/EtcdRegistryServiceImplTest.java, integration/grpc/pom.xml and serializer/seata-serializer-protobuf/pom.xml for arm64 binary

Signed-off-by: odidev odidev@puresoftware.com

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Merging #4963 (236cdde) into develop (06c2a50) will decrease coverage by 0.50%.
The diff coverage is 46.34%.

❗ Current head 236cdde differs from pull request most recent head 3be80ae. Consider uploading reports for the commit 3be80ae to get more accurate results

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #4963      +/-   ##
=============================================
- Coverage      49.23%   48.72%   -0.51%     
+ Complexity      4117     4079      -38     
=============================================
  Files            737      736       -1     
  Lines          25827    25818       -9     
  Branches        3193     3184       -9     
=============================================
- Hits           12715    12579     -136     
- Misses         11764    11905     +141     
+ Partials        1348     1334      -14     
Impacted Files Coverage Δ
...c/main/java/io/seata/common/ConfigurationKeys.java 0.00% <ø> (ø)
...n/src/main/java/io/seata/common/DefaultValues.java 0.00% <ø> (ø)
...eata/common/exception/EurekaRegistryException.java 100.00% <ø> (ø)
...ava/io/seata/common/util/LowerCaseLinkHashMap.java 0.00% <0.00%> (ø)
...n/src/main/java/io/seata/common/util/PageUtil.java 50.00% <ø> (ø)
...ta/core/rpc/netty/AbstractNettyRemotingClient.java 18.53% <ø> (ø)
...n/java/io/seata/core/rpc/netty/ChannelManager.java 0.00% <0.00%> (ø)
.../io/seata/core/rpc/netty/NettyServerBootstrap.java 0.00% <ø> (ø)
...seata/core/serializer/SerializerClassRegistry.java 0.00% <ø> (ø)
.../io/seata/core/store/DefaultDistributedLocker.java 0.00% <ø> (ø)
... and 81 more


<protobuf.version>3.7.1</protobuf.version>
<grpc.version>1.17.1</grpc.version>
<protobuf-maven-plugin.version>0.6.1</protobuf-maven-plugin.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependencies module should not contains plugin version, I think you should probably define it in the build module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated it in the build module as per your suggestion.

@slievrly
Copy link
Member

@odidev
I think that your PR solves:

  1. Upgrade protobuf to solve the ARM64 compatibility problem.
  2. Check whether there is any problem on the ARM64 platform CI through Github Action Workflow.

@wangliang181230
Copy link
Contributor

https://docs.github.com/en/actions/hosting-your-own-runners/using-self-hosted-runners-in-a-workflow
runs-on support ARM64, is it unnecessary to use scripts to build the ARM64?

@slievrly
Copy link
Member

workflow output

WARNING: The requested image's platform (linux/arm64/v8) does not match the detected host platform (linux/amd64) and no specific platform was requested

https://github.com/seata/seata/actions/runs/3135923871/jobs/5092189894

@slievrly
Copy link
Member

@odidev Any responses to the questions above?

@odidev
Copy link
Contributor Author

odidev commented Sep 29, 2022

Any responses to the questions above?

@slievrly almost done, few things are left, it will take little more time. And sorry for the delay.

@odidev
Copy link
Contributor Author

odidev commented Sep 29, 2022

WARNING: The requested image's platform (linux/arm64/v8) does not match the detected host platform (linux/amd64) and no specific platform was requested

It's just a warning because, we are running an arm64 script over the amd machine using qemu.. I verified that hosted platform is also compatible for arm64 by using uname-m

bbbbbbbbbb

@odidev
Copy link
Contributor Author

odidev commented Oct 6, 2022

@odidev Any responses to the questions above?

@slievrly I have updated all the changes as per review comments. Please have a look.

@slievrly
Copy link
Member

slievrly commented Oct 9, 2022

https://docs.github.com/en/actions/hosting-your-own-runners/using-self-hosted-runners-in-a-workflow
runs-on support ARM64, is it unnecessary to use scripts to build the ARM64?

@odidev what do you think about of running on ARM64 ?

@odidev
Copy link
Contributor Author

odidev commented Oct 11, 2022

https://docs.github.com/en/actions/hosting-your-own-runners/using-self-hosted-runners-in-a-workflow
runs-on support ARM64, is it unnecessary to use scripts to build the ARM64?

@odidev what do you think about of running on ARM64 ?

@slievrly

I have built arm64-binary by using self-hosted arm64 runners which has to be hosted on arm64 machine. If it is fine, then we can get access to ARM H/W through WorksOnArm

Self-Hosted arm64 Runner in GitHub-actions:
Commit Link: odidev@85746ff

GHA Logs Link: https://github.com/odidev/seata/actions/runs/3226451554/jobs/5279982536

If you agree, I will amend the PR.

@slievrly
Copy link
Member

slievrly commented Oct 12, 2022

https://docs.github.com/en/actions/hosting-your-own-runners/using-self-hosted-runners-in-a-workflow
runs-on support ARM64, is it unnecessary to use scripts to build the ARM64?

@odidev what do you think about of running on ARM64 ?

@slievrly

I have built arm64-binary by using self-hosted arm64 runners which has to be hosted on arm64 machine. If it is fine, then we can get access to ARM H/W through WorksOnArm

Self-Hosted arm64 Runner in GitHub-actions: Commit Link: odidev@85746ff

GHA Logs Link: https://github.com/odidev/seata/actions/runs/3226451554/jobs/5279982536

If you agree, I will amend the PR.

Why not just use runs-on: ARM64 ?
https://docs.github.com/en/actions/hosting-your-own-runners/using-self-hosted-runners-in-a-workflow

@odidev
Copy link
Contributor Author

odidev commented Oct 13, 2022

Why not just use runs-on: ARM64 ?

@slievrly

We can't add arm64 runner directly as arm64 runners are not supported directly for arm64.

We can use two methods for running arm64 script over an x64 machine first one is using qemu and second is self-hosted runner for arm64.

for self-hosted-runner, following are the steps.

Adding a self-hosted runner to the repository:

  1. On GitHub.com, navigate to the main page of the repository.

  2. Under odidev/seata, goto the Settings
    87

  3. In the left sidebar, click Actions, then click Runners
    88

  4. After that select the operating system as Linux and architecture as ARM64 for self-hosted runner machine.
    89

  5. Then run these commands on your local machine:
    For download

$ mkdir actions-runner && cd actions-runner
$ curl -o actions-runner-linux-arm64-2.298.2.tar.gz -L https://github.com/actions/runner/releases/download/v2.298.2/actions-runner-linux-arm64-2.298.2.tar.gz
$ echo "803e4aba36484ef4f126df184220946bc151ae1bbaf2b606b6e2f2280f5042e8  actions-runner-linux-arm64-2.298.2.tar.gz" | shasum -a 256 -c
$ tar xzf ./actions-runner-linux-arm64-2.298.2.tar.gz

Configure

$ ./config.sh --url https://github.com/odidev/seata --token AJXNBROLYUMW7SXRHZKTHQDDI62UY
$ ./run.sh

After that use “runs-on: self-hosted” yaml in the workflow file for each job.

@slievrly
Copy link
Member

slievrly commented Oct 18, 2022

@odidev I had some wrong understanding of this solution before, but now I have a clear understanding through github documents. I suggest using the QEMU method, but this workflow takes a long time to execute. I don't think it is suitable to put it on pull_ request. I suggest putting it into the on push branch only for execution.

@odidev
Copy link
Contributor Author

odidev commented Oct 20, 2022

@odidev I had some wrong understanding of this solution before, but now I have a clear understanding through github documents. I suggest using the QEMU method, but this workflow takes a long time to execute. I don't think it is suitable to put it on pull_ request. I suggest putting it into the on push branch only for execution.

@slievrly

It's ok to put it on a branch only for execution. Please let me know if I should commit the code to any specific branch or same branch works for you?

@slievrly
Copy link
Member

slievrly commented Oct 24, 2022

@odidev I had some wrong understanding of this solution before, but now I have a clear understanding through github documents. I suggest using the QEMU method, but this workflow takes a long time to execute. I don't think it is suitable to put it on pull_ request. I suggest putting it into the on push branch only for execution.

@slievrly

It's ok to put it on a branch only for execution. Please let me know if I should commit the code to any specific branch or same branch works for you?

@odidev
I think it's enough to trigger the ARM64 compatibility check workflow when push the code into develop branch.

Signed-off-by: odidev <odidev@puresoftware.com>
@odidev
Copy link
Contributor Author

odidev commented Nov 1, 2022

@slievrly

@odidev I think it's enough to trigger the ARM64 compatibility check workflow when push the code into develop branch.

As per your suggestion, I updated the code into the on-push branch and skipped the pull_request.

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@slievrly
Copy link
Member

slievrly commented Nov 7, 2022

@odidev thanks for your contribution.

@slievrly slievrly added this to the 1.6.0 milestone Nov 7, 2022
@slievrly slievrly added the first-time contributor first-time contributor label Nov 7, 2022
@slievrly slievrly changed the title Require ARM64 binary optimize: require ARM64 binary Nov 7, 2022
@slievrly slievrly added the image label Nov 7, 2022
@slievrly slievrly merged commit 5796d64 into apache:develop Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants