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

ARM Architecture Support #5139

Merged
merged 11 commits into from
Feb 5, 2024
Merged

Conversation

AliaksandrRyzhou
Copy link
Contributor

What change does this PR introduce?

These changes will allow us to build docker images for ARM platforms too.

Why was this change needed?

  • This will also lower our cloud cost as ARM is more efficient and typically less expensive.
  • It will allow to use Novu on ARM Platform so is really useful for many users

Other information (Screenshots)

  • We will have to remove the base image: nikolaik/python-nodejs:python3.10-nodejs16-alpine as this couldn't be compiled for arm. Because of it it's really important to make sure that it works fine after the base image changes.
  • Build times might increase significantly...

Example of what we expect to see:
Screenshot from 2024-01-31 17-19-10

Screenshot from 2024-01-31 17-19-15

…into inf-136-arm-support-for-images

# Conflicts:
#	.github/workflows/dev-deploy-inbound-mail.yml
#	.github/workflows/dev-deploy-ws.yml
#	apps/api/package.json
#	apps/inbound-mail/package.json
#	apps/worker/package.json
#	apps/ws/Dockerfile
#	apps/ws/package.json
Copy link

linear bot commented Jan 31, 2024

Copy link

netlify bot commented Jan 31, 2024

Deploy Preview for dev-web-novu failed.

Name Link
🔨 Latest commit eb5db06
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/65bc9d497be3c90008dc9b0d

@@ -1,5 +1,6 @@
# start build stage
FROM nikolaik/python-nodejs:python3.10-nodejs20-alpine as builder
FROM node:20-alpine3.19 as builder
RUN apk add g++ make py3-pip
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this without python?

I am unsure if we need python in this container

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 assumed that python was needed where basic images include python. Also Pawel said that python is used on the build and in some images the ARM architecture build hung without it. I will recheck and let you know 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

@AliaksandrRyzhou Lets double check this, I know its needed for inbound-mail for sure but I have not seen anything else in the project that needs it.

@LetItRock Are you aware of any build tools that leverage python for our project?

I want to make sure we are only doing what we need to cut build times down as much as possible, but @AliaksandrRyzhou if this takes more then 2 hours to debug lets create a ticket and look at it in the future so we can get this moved through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This I get when I try to build the web service without Python
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I will add this to a ticket

@AliaksandrRyzhou Lets fix the last comment and lets get this merged in

apps/webhook/Dockerfile Show resolved Hide resolved
apps/ws/Dockerfile Outdated Show resolved Hide resolved
@AliaksandrRyzhou
Copy link
Contributor Author

@Cliftonz I've made a quick investigation and found out that we can't build the docker images for the ARM platform without Python. It fails even in the images where Python wasn't installed initially. It looks like some npm packages include native dependencies that need to be compiled during installation. These dependencies might be platform-specific and iinstalling Python might provide a necessary build environment for these native dependencies.

@Cliftonz
Copy link
Contributor

Cliftonz commented Feb 2, 2024

Planning to merge February 5th

@AliaksandrRyzhou AliaksandrRyzhou merged commit 4023b29 into next Feb 5, 2024
30 of 36 checks passed
@AliaksandrRyzhou AliaksandrRyzhou deleted the inf-136-arm-support-for-images branch February 5, 2024 06:09
@estubmo
Copy link

estubmo commented May 23, 2024

I wonder if something has broken this PR, because the OS/Arch tab is no longer present in the built images on ghrc

@AliaksandrRyzhou
Copy link
Contributor Author

I wonder if something has broken this PR, because the OS/Arch tab is no longer present in the built images on ghrc

@estubmo Please accept our apologies. We decided to change an approach of a building process in a bit, you will see multi-architecture images soon.

@estubmo
Copy link

estubmo commented May 23, 2024

@AliaksandrRyzhou No problem, thanks for your reply. Do you have access to some arm64 tags I could use in the meantime, preferably for the prod images?

@udleinati
Copy link

Same issue here. @AliaksandrRyzhou if you have any tag that works on arm, it would be great! thanks

@estubmo
Copy link

estubmo commented Jun 15, 2024

Any update on this @AliaksandrRyzhou?

@paulwer
Copy link
Contributor

paulwer commented Jul 23, 2024

@udleinati @estubmo we have a working production on arm with an old image, i dont know it this helps someone in the meantime:
image

@paulwer
Copy link
Contributor

paulwer commented Aug 2, 2024

@AliaksandrRyzhou still no updates for us?

@Cliftonz
Copy link
Contributor

Cliftonz commented Aug 2, 2024

@paulwer @AliaksandrRyzhou and I are not working on Novu anymore.

This is something that will hopefully be fixed in the next version. @SokratisVidros would you be able to speak to this?

@SokratisVidros
Copy link
Contributor

Currently, we are reshuffling the roadmap and aiming at a stable, multi-architecture, Novu V2 docker release in early September.

@paulwer
Copy link
Contributor

paulwer commented Oct 4, 2024

@SokratisVidros is this considdered in the upcomming v2 release?

@SokratisVidros
Copy link
Contributor

@merrcury ☝️

@paulwer
Copy link
Contributor

paulwer commented Oct 9, 2024

@merrcury @SokratisVidros does it help, when I open an other issue for better clarity on this topic or are there further informations for us now?

@merrcury
Copy link
Member

merrcury commented Oct 9, 2024

Hey, @paulwer We are working on V2 release. We will see if we will be able to solve this with that. For community Announcement, Please check discord. Last Announcement .

Otherwise, I'll separately add this to our roadmap. Stay Tuned

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

Successfully merging this pull request may close these issues.

7 participants