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

Proposal: Try using Earthly instead of Make for one release #5711

Merged
merged 11 commits into from
May 28, 2024

Conversation

negz
Copy link
Member

@negz negz commented May 17, 2024

Description of your changes

This PR adds a one-pager proposing we adopt https://earthly.dev. Earthly would replace the build submodule.

I propose we try Earthly in crossplane/crossplane and crossplane/crossplane-runtime, for one release. After releasing Crossplane v1.17 we'd assess whether we wanted to commit to Earthly, revert to the build submodule, or something else. I don't think we should attempt to migrate providers until we've lived with Earthly for a while here.

The PR also contains a proof-of-concept port of most of this repository's Makefile to an Earthly Earthfile. I believe the only thing missing is the rules that publish binaries and Helm charts to S3. Take a look at the Earthfile and the GitHub Actions output to get a sense of what working with Earthly is like.

I have:

  • Read and followed Crossplane's contribution process.
  • Run earthly +reviewable to ensure this PR is ready for review.
  • Added or updated unit tests.
  • Added or updated e2e tests.
  • Linked a PR or a docs tracking issue to document this change.
  • Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

@negz negz force-pushed the third-rock-from-the-sun branch 6 times, most recently from 50360d8 to 8640ba7 Compare May 18, 2024 02:47
@negz

This comment was marked as outdated.

@negz negz force-pushed the third-rock-from-the-sun branch from 8640ba7 to 15a6d57 Compare May 18, 2024 03:08
@negz

This comment was marked as outdated.

@negz negz force-pushed the third-rock-from-the-sun branch 4 times, most recently from 5048c46 to 469458d Compare May 22, 2024 00:42
@negz negz changed the title Experiment with Earthly Proposal: Try using Earthly instead of Make for one release May 22, 2024
@negz negz marked this pull request as ready for review May 22, 2024 00:58
@negz negz requested a review from a team as a code owner May 22, 2024 00:58
@negz negz requested review from turkenh, bobh66 and phisco May 22, 2024 00:58

detect-noop:
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intentionally removing the no-op detection or is that also to be reimplemented?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did remove it intentionally. My thinking was that when Earthly has a hot cache it'll handle this automatically. It only rebuilds if the code changes, so changing a Markdown file or similar will be a no-op at the Earthly level.

TBD as to whether that will work though. We'd need to setup remote caching for Earthly to be able to handle it automatically.

There's also a handful of relatively long running CI jobs that don't run in Earthly (yet?) - e.g. fuzz tests and CodeQL. We'd need to either get those running in Earthly, or reintroduce detect-noop. I'd personally prefer to get everything running in Earthly if only to keep things simple and consistent.

Copy link
Member

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Thanks @negz for exploring the options here 🙌

I might be one of those who might have invested most to the build submodule, and after some pain and tears, I am feeling relatively comfortable with working with it. However, I can totally see how frustrating it could be for a newcomer. I like the syntax of the Earthfile, especially the fact that it looks like a multi-stage Dockerfile. It look much simpler at the cost of introducing a dependency to an additional tool (different than Make which is available on any system).

I am not sure how it would work when we need to run some docker commands or kind which creates k8s cluster a container since it is already running in a container, but given that you added e2e, I assume it works fine 🤔

Overall, I am fine with experimenting as long as we could have feature parity with the build submodule especially with versioning and artifact publishing / promotion.

design/one-pager-build-with-earthly.md Show resolved Hide resolved
design/one-pager-build-with-earthly.md Show resolved Hide resolved
@negz negz force-pushed the third-rock-from-the-sun branch 3 times, most recently from 42e7be3 to 13603e5 Compare May 23, 2024 01:37
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@negz negz force-pushed the third-rock-from-the-sun branch 5 times, most recently from 8b47f2d to 0895b79 Compare May 23, 2024 19:25
@negz
Copy link
Member Author

negz commented May 28, 2024

Without yet reading and understanding this (so sorry obviously 😂), my general concern would be that this port misses some functionality from the build submodule that we either didn't really know about or that won't get exercised until it's time for the next real release.

@jbw976 I've spent over a week working only on this PR. So I'm fairly confident it's complete, but I can't guarantee I didn't miss anything. There's a lot of history in our Makefiles, and it's pretty tough to grok what things do and why they do it. I don't think we should let perfect be the enemy of good here though - I expect this PR will need followup work to fix up something or other.

Will it be possible to add support for out of cluster crossplane run similar to #5745 ? IIUC currently there is no related Earthly target

@ytsarev There's a hack target that builds Crossplane, spins up kind, and deploys the local Crossplane to it. We could also add a target that runs Crossplane outside of the cluster, but I'm not really sure why it's worth doing that personally. What makes it preferable to run Crossplane out of the cluster?

negz added 5 commits May 28, 2024 10:40
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
So that it stops warning about it.

Signed-off-by: Nic Cope <nicc@rk0n.org>
The current format uses }, { style, which I find makes it much more
laborious to edit, especially when I want to quickly copy and modify a
block.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
@negz negz force-pushed the third-rock-from-the-sun branch from b82aa03 to 3093e1e Compare May 28, 2024 17:41
@jbw976
Copy link
Member

jbw976 commented May 28, 2024

What makes it preferable to run Crossplane out of the cluster?

My instincts are that "out of the cluster" isn't the important part here - the ability to iterate on build/run rapidly is the important part.

That hack target sounds like a good start (sounds similar to cluster/local/kind.sh up --> helm-install actually), then perhaps we may need to follow up to update that target (or create a new one) that lets you update that Crossplane binary with new changes rapidly.

We could reimplement this with Earthly but I suspect it's not used.

Signed-off-by: Nic Cope <nicc@rk0n.org>
contributing/README.md Show resolved Hide resolved
Earthfile Show resolved Hide resolved
I'm not currently reimplementing the crds.clean target that trims the
leading --- from the CRDs because it's unclear whether it's still
needed.

Signed-off-by: Nic Cope <nicc@rk0n.org>
contributing/README.md Outdated Show resolved Hide resolved
# local environment, not a container. The kind cluster will keep running until
# you run the unhack target. Run hack again to rebuild Crossplane and restart
# the kind cluster with the new build.
hack:
Copy link
Member

Choose a reason for hiding this comment

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

the hack command ran into an error for me, it looks like unhack was executed and could not execute the kind binary 🤔

❯ earthly +hack
 Init 🚀
————————————————————————————————————————————————————————————————————————————————

           buildkitd | Found buildkit daemon as docker container (earthly-buildkitd)

 Build 🔧
————————————————————————————————————————————————————————————————————————————————

              logbus | Setting organization "crossplane" and project "crossplane"
               +hack | --> FROM +base
               +hack | darwin/arm64
               +hack | --> BUILD +unhack
             +unhack | --> FROM +base
         +kind-setup | --> FROM +base
        c/curl:8.8.0 | --> Load metadata curlimages/curl:8.8.0 linux/arm64
Warning: you are not logged into registry-1.docker.io, you may experience rate-limitting when pulling images
         +kind-setup | --> FROM curlimages/curl:8.8.0
         +kind-setup | [----------] 100% FROM curlimages/curl:8.8.0
         +kind-setup | --> RUN curl -fsSLo kind https://github.com/kubernetes-sigs/kind/releases/download/${KIND_VERSION}/kind-${NATIVEOS}-${NATIVEARCH}&&chmod +x kind
             +unhack | darwin/arm64
             +unhack | --> COPY +kind-setup/kind .hack/kind
             +unhack | darwin/arm64
             +unhack | --> RUN .hack/kind delete cluster --name crossplane-hack
             +unhack | /bin/sh: .hack/kind: cannot execute binary file
             +unhack | ERROR Earthfile:95:2
             +unhack |       The command
             +unhack |           RUN .hack/kind delete cluster --name crossplane-hack
             +unhack |       did not complete successfully. Exit code 126

================================== ❌ FAILURE ===================================

             +unhack *failed* | Repeating the failure error...
             +unhack *failed* | darwin/arm64
             +unhack *failed* | --> RUN .hack/kind delete cluster --name crossplane-hack
             +unhack *failed* | /bin/sh: .hack/kind: cannot execute binary file
             +unhack *failed* | ERROR Earthfile:95:2
             +unhack *failed* |       The command
             +unhack *failed* |           RUN .hack/kind delete cluster --name crossplane-hack
             +unhack *failed* |       did not complete successfully. Exit code 126
               +hack | earthfile2llb for +unhack: Earthfile:95:2 apply RUN: unlazy force execution: error calling LocalhostExec: exit code: 126
               +hack | in		+unhack
🛰️ Reuse cache between CI runs with Earthly Satellites! 2-20X faster than without cache. Generous free tier https://cloud.earthly.dev

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the problem was that TARGETOS binaries won't work for LOCALLY targets, since TARGETOS will always be Linux. I've pushed a commit that I hope will fix this. Verified it still works when run from Linux.

.github/renovate.json5 Show resolved Hide resolved
.github/workflows/promote.yml Show resolved Hide resolved
This is hacky and I hate it, but I can't think of a better approach.

Signed-off-by: Nic Cope <nicc@rk0n.org>

generate.done: crds.clean crds.patch

gen-kustomize-crds:
Copy link
Member Author

Choose a reason for hiding this comment

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

I've intentionally skipped reimplementing this target - my hunch is it's not used. We can add it if folks really need it, but I'm not sure how valuable it is.

Earthfile Show resolved Hide resolved
negz added 2 commits May 28, 2024 11:43
Its version isn't specified there anymore.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
@ytsarev
Copy link
Member

ytsarev commented May 28, 2024

What makes it preferable to run Crossplane out of the cluster?

For me, the iteration process is incredibly quick. I can run the crossplane core process locally in a terminal tab, receive immediate feedback, make changes, restart the process, and repeat. I can also use advanced local debugging tools like Delve if needed.

This method is significantly faster than the hack target: building the project, deploying it locally with Helm, getting the pod logs, updating the code, and then redeploying. Maybe modern tools like tilt.dev might help streamline this, but they introduce additional complexity compared to a simple make/earth target.

| Run linters | ~3 minutes | ~4 minutes |
| Run unit tests | ~3 minutes | ~2.5 minutes |
| Publish artifacts | ~12 minutes | ~14 minutes |
| Run E2E tests | ~12 minutes | ~14 minutes |
Copy link
Member

Choose a reason for hiding this comment

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

Curious @negz if you got a sense for any impact on reliability and/or performance of running e2e tests locally. I just thought of this now while running earthly -P +e2e (which I believe runs -test-suite=base). It took about 20 mins to run that suite and it had 10 failures:

 DONE 114 tests, 13 skipped, 10 failures in 1196.054s

I'm not sure if that's slower or less reliable - have you developed a sense for that while testing yourself? 🤔

Copy link
Member Author

@negz negz May 28, 2024

Choose a reason for hiding this comment

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

No idea.

That said, I can't see why it would be slower or less reliable. At the end of the day it's the same code running in a Docker container spun up using kind whether that was done by Make or Earthly.

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW I just ran earthly -P +e2e locally and everything passed. I'm not sure how long it took though - I don't get the DONE summary for whatever reason.

TARGETPLATFORM is incorrect on MacOS, where TARGETOS will be Linux.
There we want USERPLATFORM, i.e. Darwin.

Signed-off-by: Nic Cope <nicc@rk0n.org>
@negz negz force-pushed the third-rock-from-the-sun branch from 4eb5645 to ba9e43f Compare May 28, 2024 21:51
@negz negz merged commit 2d526c7 into crossplane:master May 28, 2024
17 of 18 checks passed
@negz negz deleted the third-rock-from-the-sun branch May 28, 2024 23:27
@jbw976 jbw976 added this to the v1.17 milestone May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants