engine: remove ftp_proxy hack #7228
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
jedevc
reviewed
May 1, 2024
jedevc
reviewed
May 1, 2024
jedevc
approved these changes
May 1, 2024
This removes the ftp_proxy hack we've had for a while for passing uncached metadata to containers (previously a lot, but recently trimmed down to just ServerID). Instead, we take advantage of the new custom executor added recently and use that to set the _DAGGER_SERVER_ID env var in containers. The plumbing passes the ServerID via job values, which get read by the solver's ResolveOp callback and used to tell the Executor for ExecOps which ServerID to set in the container. There's quite a bit more cleanup coming to everything involved here, but wanted to get this first step out since that hack, besides being annoying, was kind of dubious in nature and was a suspect in some strange behavior reported sometimes. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
I don't see that this is actually used or ends up modifying anything. Suspect it's vestigial from pre-OTEL days so removing it. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
This fixes the problem in the previous commit where Dockerfiles with a syntax pragma hit the executor directly before a ServerID could be set. This wasn't the shortest fix, but it seems to be the best long term one. The end result is that the Worker provided to LLBSolvers is always scoped to the server, so we don't even need to be careful about ensuring the right executor is being used everywhere. It just will be now because it's the only executor available to buildkit internals we use. This required a few adjustments from the previous code: 1. The custom executor has been expanded to also be a custom worker. Fortunately, it almost entirely reuses functionality from upstream's base.Worker, so this doesn't another big chunk of code to maintain. 2. llbSolver is now created in each buildkit.Client and setup with a worker.Controller with the Worker scoped to the current ServerID. 3. Rather than passing the ServerID through job keys, we just pass the whole Worker and use that to resolve ops. 4. Some more general cleanup of worker initialization made possible by these changes. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
|
Okay fixed that problem with Dockerfiles that use syntax pragmas in the not-fastest-but-best-long-term way. The high-level idea is the same as before but with some adjustments described in commit message here: 7baea5b
Side note: I noticed during those changes that
|
|
Also worth noting: the tests have been way less flaky so far (knock on wood, etc.). My educated guess is that ftp_proxy settings were resulting in different vtx digests that ended up edge merged, but now vtx digests end up the same more often and don't need to go through the (currently buggy, pending upstream fixes) edge merge logic? I did still see the |
|
I just pivoted to start on proxy support and immediately realized that I very much want the changes here because otherwise I'll just be recreating the same problems we had with |
kpenfound
pushed a commit
to kpenfound/dagger
that referenced
this pull request
May 2, 2024
* engine: remove ftp_proxy hack This removes the ftp_proxy hack we've had for a while for passing uncached metadata to containers (previously a lot, but recently trimmed down to just ServerID). Instead, we take advantage of the new custom executor added recently and use that to set the _DAGGER_SERVER_ID env var in containers. The plumbing passes the ServerID via job values, which get read by the solver's ResolveOp callback and used to tell the Executor for ExecOps which ServerID to set in the container. There's quite a bit more cleanup coming to everything involved here, but wanted to get this first step out since that hack, besides being annoying, was kind of dubious in nature and was a suspect in some strange behavior reported sometimes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: remove apparently unused opTrackingGateway I don't see that this is actually used or ends up modifying anything. Suspect it's vestigial from pre-OTEL days so removing it. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: pass worker around instead of custom executor This fixes the problem in the previous commit where Dockerfiles with a syntax pragma hit the executor directly before a ServerID could be set. This wasn't the shortest fix, but it seems to be the best long term one. The end result is that the Worker provided to LLBSolvers is always scoped to the server, so we don't even need to be careful about ensuring the right executor is being used everywhere. It just will be now because it's the only executor available to buildkit internals we use. This required a few adjustments from the previous code: 1. The custom executor has been expanded to also be a custom worker. Fortunately, it almost entirely reuses functionality from upstream's base.Worker, so this doesn't another big chunk of code to maintain. 2. llbSolver is now created in each buildkit.Client and setup with a worker.Controller with the Worker scoped to the current ServerID. 3. Rather than passing the ServerID through job keys, we just pass the whole Worker and use that to resolve ops. 4. Some more general cleanup of worker initialization made possible by these changes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * backfill integ test coverage for dockerfile w/ syntax pragma Signed-off-by: Erik Sipsma <erik@sipsma.dev> * add comment pointing to original runc executor implementation Signed-off-by: Erik Sipsma <erik@sipsma.dev> --------- Signed-off-by: Erik Sipsma <erik@sipsma.dev> Signed-off-by: kpenfound <kyle@dagger.io>
vikram-dagger
pushed a commit
to vikram-dagger/dagger
that referenced
this pull request
May 3, 2024
* engine: remove ftp_proxy hack This removes the ftp_proxy hack we've had for a while for passing uncached metadata to containers (previously a lot, but recently trimmed down to just ServerID). Instead, we take advantage of the new custom executor added recently and use that to set the _DAGGER_SERVER_ID env var in containers. The plumbing passes the ServerID via job values, which get read by the solver's ResolveOp callback and used to tell the Executor for ExecOps which ServerID to set in the container. There's quite a bit more cleanup coming to everything involved here, but wanted to get this first step out since that hack, besides being annoying, was kind of dubious in nature and was a suspect in some strange behavior reported sometimes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: remove apparently unused opTrackingGateway I don't see that this is actually used or ends up modifying anything. Suspect it's vestigial from pre-OTEL days so removing it. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: pass worker around instead of custom executor This fixes the problem in the previous commit where Dockerfiles with a syntax pragma hit the executor directly before a ServerID could be set. This wasn't the shortest fix, but it seems to be the best long term one. The end result is that the Worker provided to LLBSolvers is always scoped to the server, so we don't even need to be careful about ensuring the right executor is being used everywhere. It just will be now because it's the only executor available to buildkit internals we use. This required a few adjustments from the previous code: 1. The custom executor has been expanded to also be a custom worker. Fortunately, it almost entirely reuses functionality from upstream's base.Worker, so this doesn't another big chunk of code to maintain. 2. llbSolver is now created in each buildkit.Client and setup with a worker.Controller with the Worker scoped to the current ServerID. 3. Rather than passing the ServerID through job keys, we just pass the whole Worker and use that to resolve ops. 4. Some more general cleanup of worker initialization made possible by these changes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * backfill integ test coverage for dockerfile w/ syntax pragma Signed-off-by: Erik Sipsma <erik@sipsma.dev> * add comment pointing to original runc executor implementation Signed-off-by: Erik Sipsma <erik@sipsma.dev> --------- Signed-off-by: Erik Sipsma <erik@sipsma.dev>
vikram-dagger
pushed a commit
to vikram-dagger/dagger
that referenced
this pull request
May 3, 2024
* engine: remove ftp_proxy hack This removes the ftp_proxy hack we've had for a while for passing uncached metadata to containers (previously a lot, but recently trimmed down to just ServerID). Instead, we take advantage of the new custom executor added recently and use that to set the _DAGGER_SERVER_ID env var in containers. The plumbing passes the ServerID via job values, which get read by the solver's ResolveOp callback and used to tell the Executor for ExecOps which ServerID to set in the container. There's quite a bit more cleanup coming to everything involved here, but wanted to get this first step out since that hack, besides being annoying, was kind of dubious in nature and was a suspect in some strange behavior reported sometimes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: remove apparently unused opTrackingGateway I don't see that this is actually used or ends up modifying anything. Suspect it's vestigial from pre-OTEL days so removing it. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: pass worker around instead of custom executor This fixes the problem in the previous commit where Dockerfiles with a syntax pragma hit the executor directly before a ServerID could be set. This wasn't the shortest fix, but it seems to be the best long term one. The end result is that the Worker provided to LLBSolvers is always scoped to the server, so we don't even need to be careful about ensuring the right executor is being used everywhere. It just will be now because it's the only executor available to buildkit internals we use. This required a few adjustments from the previous code: 1. The custom executor has been expanded to also be a custom worker. Fortunately, it almost entirely reuses functionality from upstream's base.Worker, so this doesn't another big chunk of code to maintain. 2. llbSolver is now created in each buildkit.Client and setup with a worker.Controller with the Worker scoped to the current ServerID. 3. Rather than passing the ServerID through job keys, we just pass the whole Worker and use that to resolve ops. 4. Some more general cleanup of worker initialization made possible by these changes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * backfill integ test coverage for dockerfile w/ syntax pragma Signed-off-by: Erik Sipsma <erik@sipsma.dev> * add comment pointing to original runc executor implementation Signed-off-by: Erik Sipsma <erik@sipsma.dev> --------- Signed-off-by: Erik Sipsma <erik@sipsma.dev>
vikram-dagger
pushed a commit
to vikram-dagger/dagger
that referenced
this pull request
May 3, 2024
* engine: remove ftp_proxy hack This removes the ftp_proxy hack we've had for a while for passing uncached metadata to containers (previously a lot, but recently trimmed down to just ServerID). Instead, we take advantage of the new custom executor added recently and use that to set the _DAGGER_SERVER_ID env var in containers. The plumbing passes the ServerID via job values, which get read by the solver's ResolveOp callback and used to tell the Executor for ExecOps which ServerID to set in the container. There's quite a bit more cleanup coming to everything involved here, but wanted to get this first step out since that hack, besides being annoying, was kind of dubious in nature and was a suspect in some strange behavior reported sometimes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: remove apparently unused opTrackingGateway I don't see that this is actually used or ends up modifying anything. Suspect it's vestigial from pre-OTEL days so removing it. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * engine: pass worker around instead of custom executor This fixes the problem in the previous commit where Dockerfiles with a syntax pragma hit the executor directly before a ServerID could be set. This wasn't the shortest fix, but it seems to be the best long term one. The end result is that the Worker provided to LLBSolvers is always scoped to the server, so we don't even need to be careful about ensuring the right executor is being used everywhere. It just will be now because it's the only executor available to buildkit internals we use. This required a few adjustments from the previous code: 1. The custom executor has been expanded to also be a custom worker. Fortunately, it almost entirely reuses functionality from upstream's base.Worker, so this doesn't another big chunk of code to maintain. 2. llbSolver is now created in each buildkit.Client and setup with a worker.Controller with the Worker scoped to the current ServerID. 3. Rather than passing the ServerID through job keys, we just pass the whole Worker and use that to resolve ops. 4. Some more general cleanup of worker initialization made possible by these changes. Signed-off-by: Erik Sipsma <erik@sipsma.dev> * backfill integ test coverage for dockerfile w/ syntax pragma Signed-off-by: Erik Sipsma <erik@sipsma.dev> * add comment pointing to original runc executor implementation Signed-off-by: Erik Sipsma <erik@sipsma.dev> --------- Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the ftp_proxy hack we've had for a while for passing uncached metadata to containers (previously a lot, but recently trimmed down to just ServerID).
Instead, we take advantage of the new custom executor added recently and use that to set the _DAGGER_SERVER_ID env var in containers. The plumbing passes a custom worker around via job values, which get read by the solver's ResolveOp callback and used to tell the Executor for ExecOps which ServerID to set in the container.
There's quite a bit more cleanup coming to everything involved here, but wanted to get this first step out since that hack, besides being annoying, was kind of dubious in nature and was a suspect in some strange behavior reported sometimes.
FTR, this is another spin-off of work on #6916. It was self-contained and beneficial enough that it seemed worth getting out for the next release.