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

Fix: Enable portable flag for Docker container builds #1602

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

ovaistariq
Copy link
Contributor

Enable portable flag for Docker container builds

Fixes issue #1146

x.py Show resolved Hide resolved
@aleksraiden
Copy link
Contributor

Thanks for investigation, @ovaistariq! In my mind, options 1 and 0 are similar (with some case) - the compiler do as best as it can. But we need to can manual add this variable to compiler process. So, in case of #1146 a best way is change PORTABLE=znver1 (if CPU are a first family of EPYC arch). Change a default PORTABLE to 1 maybe a wrong case - as describe in rocksdb source - usually nothing to do; compiler default is typically the most general - but for hi-load system it's a wrong way, because we cant use an advanced CPU features and acceleration, 1 is a simple baseline CPU.

Maybe we can detect a right CPU platform directly in x.py Python script and use it?

@ovaistariq
Copy link
Contributor Author

Thanks for investigation, @ovaistariq! In my mind, options 1 and 0 are similar (with some case) - the compiler do as best as it can. But we need to can manual add this variable to compiler process. So, in case of #1146 a best way is change PORTABLE=znver1 (if CPU are a first family of EPYC arch). Change a default PORTABLE to 1 maybe a wrong case - as describe in rocksdb source - usually nothing to do; compiler default is typically the most general - but for hi-load system it's a wrong way, because we cant use an advanced CPU features and acceleration, 1 is a simple baseline CPU.

Maybe we can detect a right CPU platform directly in x.py Python script and use it?

In my opinion the Docker images are supposed to be portable for the platform they are built on. So an image built for the amd64 platform should be able to run on all amd64 machines.

Secondly, the Docker image may be built on a machine with a different hardware spec then the machine the container is going to run on. For example in this case, the image is built on GitHub runners which are running Intel CPUs while the container is running on a machine with AMD CPUs.

@git-hulk
Copy link
Member

git-hulk commented Jul 21, 2023

@aleksraiden This PR only changes the docker image PORTABLE value to 1. I think this makes a lot of sense since we cannot promise users are always running on the intel CPU like @ovaistariq mentioned.

@git-hulk git-hulk merged commit 5de3acc into apache:unstable Jul 21, 2023
git-hulk pushed a commit to git-hulk/kvrocks that referenced this pull request Jul 30, 2023
RocksDB PORTABLE was set to 0 after apache#1516 and it may return an illegal instruction error
when running on the AMD platform. This PR fixes this issue by changing the default value
of PORTABLE to 1 so that it can compile the rocksdb without platform special instructions.
@git-hulk git-hulk mentioned this pull request Jul 31, 2023
git-hulk pushed a commit that referenced this pull request Aug 1, 2023
RocksDB PORTABLE was set to 0 after #1516 and it may return an illegal instruction error
when running on the AMD platform. This PR fixes this issue by changing the default value
of PORTABLE to 1 so that it can compile the rocksdb without platform special instructions.
p1u3o pushed a commit to p1u3o/incubator-kvrocks that referenced this pull request Aug 1, 2023
RocksDB PORTABLE was set to 0 after apache#1516 and it may return an illegal instruction error
when running on the AMD platform. This PR fixes this issue by changing the default value
of PORTABLE to 1 so that it can compile the rocksdb without platform special instructions.
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.

4 participants