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

Multiplatform build #17

Merged
merged 8 commits into from
Nov 7, 2023
Merged

Multiplatform build #17

merged 8 commits into from
Nov 7, 2023

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Nov 6, 2023

This PR fixes multi-platform build for specific arm architectures. Before it got stuck on npm ci... command (e.g. https://github.com/kubeshop/monokle-admission-controller/actions/runs/6745376525/job/18337069349#step:8:399).

There is an issues with Node v19+ and arm based images. It is described in nodejs/docker-node#1794 and nodejs/docker-node#1335. At this moment it works with v18 based images and so I downgraded to it.

It builds fine with such changes (see this run https://github.com/kubeshop/monokle-admission-controller/actions/runs/6768309227/job/18392485679).

Changes

  • As described above.

Fixes

  • None.

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@f1ames f1ames force-pushed the f1ames/feat/multiplatform-build branch 2 times, most recently from 02ef5c2 to c15e379 Compare November 6, 2023 09:07
@f1ames f1ames marked this pull request as ready for review November 6, 2023 09:14
Copy link
Contributor

@murarustefaan murarustefaan left a comment

Choose a reason for hiding this comment

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

First of all, why are we building for these many ARM versions?

MacOS, which was the source of this issue, is using arm v8 on 64-bit platforms. Therefore, the other build architectures are really useful for ppl running on Raspberry PI Model 3B and below. Again, While the good practice would be supporting that, I'm not sure it's worth pinning to a lower-than LTS image version of the node interpreter just for that sole purpose.

Secondly, Node LTS is 20, not 18, yet the image you are pinning to resolves to both Node18 as well as an older version of alpine.

Therefore, what I suggest first removing all unused architectures (anything but arm v8 and amd64), using Node20 with a pinned version instead of using lts tags (because as you saw, some of them are not updated and you might get older image versions or you will not know which one you are on) and then either trying the Google Distroless images (as that is a best practice for these kinds of containers) or go node-slim if alpine is still not working.

What do you think @f1ames?

@f1ames
Copy link
Contributor Author

f1ames commented Nov 6, 2023

First of all, why are we building for these many ARM versions?

MacOS, which was the source of this issue, is using arm v8 on 64-bit platforms. Therefore, the other build architectures are really useful for ppl running on Raspberry PI Model 3B and below. Again, While the good practice would be supporting that, I'm not sure it's worth pinning to a lower-than LTS image version of the node interpreter just for that sole purpose.

Secondly, Node LTS is 20, not 18, yet the image you are pinning to resolves to both Node18 as well as an older version of alpine.

Good point. Yes, I just thought of it as a good practice to provide support for multiple architectures. But as you say, it might be not needed, it's also not our target audience at this point.
Now, as for pinning down to v18, it also seems to be an issue for arm v8 so dropping others will not help here (I can recheck to be 100% sure).

And the entire reason for downgrading was that for arm architecture, there is an issue with npm i starting from node v19, so that's why I downgraded to v18.

Therefore, what I suggest first removing all unused architectures (anything but arm v8 and amd64), using Node20 with a pinned version instead of using lts tags (because as you saw, some of them are not updated and you might get older image versions or you will not know which one you are on) and then either trying the Google Distroless images (as that is a best practice for these kinds of containers) or go node-slim if alpine is still not working.

Sure, it make sense. Just as I mentioned above it seems that even for linux/arm64/v8 only, it will not build with node v19+. In that case, would it make sense to build with node v18 and have production image using v20 (based on alpine or distroless you mentioned)?

There is an issues with Node v19+ and arm based images. It is described
in nodejs/docker-node#1794 and
nodejs/docker-node#1335. At this moment it works
with v18 based images and so I downgraded to it.
@f1ames f1ames force-pushed the f1ames/feat/multiplatform-build branch from c15e379 to 7e659e9 Compare November 6, 2023 14:33
Copy link
Contributor Author

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

@murarustefaan let me know if you plan to do more tweaking here. I tested built image and it runs fine. From my perspective it's ready to be merged (apart from reverting commented stuff).

Copy link
Contributor

@murarustefaan murarustefaan left a comment

Choose a reason for hiding this comment

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

lgtm!
please do not forget to revert all commented changes before merging :D

@f1ames f1ames merged commit 51efa93 into main Nov 7, 2023
17 checks passed
@f1ames f1ames deleted the f1ames/feat/multiplatform-build branch November 7, 2023 08:24
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.

2 participants