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

Add Docker Debugging Fixture #21357

Merged
merged 11 commits into from
Jan 20, 2023
Merged

Conversation

jbfbell
Copy link
Contributor

@jbfbell jbfbell commented Jan 12, 2023

  • docker debugging options
  • refactor image shortening method

What

Add options to allow for Environment Variables to set debugging options on docker containers

How

Docker Compose can inherit environment variables from the host as well as compose multiple yaml files together. For multivalue (lists) in Docker compose it will concat the values together. By adding additional environment variables in the docker-compose.debug.yaml we can add additonal options which are not activated with the standard docker-compose.yaml.

This approach also does not require rebuilding the image since the environment variables are passed in at runtime.

@jbfbell jbfbell temporarily deployed to more-secrets January 12, 2023 22:54 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 12, 2023 22:55 — with GitHub Actions Inactive
@jbfbell jbfbell requested review from gosusnp and a team January 12, 2023 23:41
@jbfbell jbfbell temporarily deployed to more-secrets January 12, 2023 23:53 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 12, 2023 23:53 — with GitHub Actions Inactive
- DEBUG_CONTAINER_IMAGE=${DEBUG_CONTAINER_IMAGE}
- DEBUG_CONTAINER_JAVA_OPTS=-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005
server:
# You will need to create a remote JVM debugging Run Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally use the Ubuntu OS and never had issues with remote debugging (just added this var to the required dockerfile), but for MAC users there may be some issues with IPs. I've heard that this tool may help https://github.com/chipmk/docker-mac-net-connect
Maybe it would be worth also this link to notes. @suhomud knows better how it works on MAC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etsybaev good call out, definitely need that tool for the mac setup

The intent of this change is that you would no longer need to make any changes to Dockerfiles since those changes should never be committed.

@jbfbell jbfbell linked an issue Jan 19, 2023 that may be closed by this pull request
@jbfbell jbfbell temporarily deployed to more-secrets January 19, 2023 00:07 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 19, 2023 00:07 — with GitHub Actions Inactive
Copy link
Contributor

@gosusnp gosusnp left a comment

Choose a reason for hiding this comment

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

I do not have a better suggestion at this time so let's start with this approach. I highlighted a few changes so that it fits better within how we are reading configs currently, will be good for me once those are addressed.

*/
static List<String> localDebuggingOptions(final String containerName) {
final boolean shouldAddDebuggerOptions =
Optional.ofNullable(System.getenv("DEBUG_CONTAINER_IMAGE")).filter(StringUtils::isNotEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the environment lookups to

?

For context, all the config goes through EnvConfigs, we're in the process of migrating to micronaut which should also be handling configuration for us: https://github.com/airbytehq/airbyte/blob/master/airbyte-workers/src/main/resources/application.yml. I am thinking airbyte.container.docker.debug-container-java-opts might make sense for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opting to not implement this change - this is for debugging only and probably should be more broadly integrated into the standard execution process. Could add as a follow on

.map(ProcessFactory.extractShortImageName(containerName)::equals).orElse(false)
&& Optional.ofNullable(System.getenv("DEBUG_CONTAINER_JAVA_OPTS")).isPresent();
if (shouldAddDebuggerOptions) {
return List.of("-e", "JAVA_TOOL_OPTIONS=" + System.getenv("DEBUG_CONTAINER_JAVA_OPTS"), "-p5005:5005");
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: We may want to extract the port to a specific config as well.

.map(ProcessFactory.extractShortImageName(containerName)::equals).orElse(false)
&& Optional.ofNullable(System.getenv("DEBUG_CONTAINER_JAVA_OPTS")).isPresent();
if (shouldAddDebuggerOptions) {
return List.of("-e", "JAVA_TOOL_OPTIONS=" + System.getenv("DEBUG_CONTAINER_JAVA_OPTS"), "-p5005:5005");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely sure how this JAVA_TOOL_OPTIONS=X is getting rebuilt, do we need some extra quotes here?

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 think its ok as is, this is all getting passed into a command List/Array to be sent to the docker run command. Specifically not using JAVA_TOOL_OPTIONS as the env variable so that the container which is running this doesn't try and process it, but the result of this command should effectively be a List with 3 strings

/**
* !! ONLY FOR DEBUGGING, SHOULD NOT BE USED IN PRODUCTION !! If you set the DEBUG_CONTAINER_IMAGE
* environment variable, and it matches the image name of a spawned container, this method will add
* the necessary params to connect a debugger. For example, to enable this for
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be worth adding this information to some dev docs if you know where they are, quick search sent me through all the specific connector's readme.

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'll poke around and see if I can find a good docs home

@jbfbell jbfbell temporarily deployed to more-secrets January 19, 2023 00:39 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 19, 2023 00:39 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 00:46 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 00:46 — with GitHub Actions Inactive
@octavia-squidington-iv octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Jan 20, 2023
@jbfbell jbfbell enabled auto-merge (squash) January 20, 2023 21:27
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 21:28 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 21:28 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 21:57 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 21:57 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 22:05 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 22:05 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2023

Airbyte Code Coverage

File Coverage [75.46%] 🍏
ProcessFactory.java 100% 🍏
DockerProcessFactory.java 72% 🍏
Total Project Coverage 26.76% 🍏

@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 22:32 — with GitHub Actions Inactive
@jbfbell jbfbell temporarily deployed to more-secrets January 20, 2023 22:32 — with GitHub Actions Inactive
@jbfbell jbfbell merged commit ff3726e into master Jan 20, 2023
@jbfbell jbfbell deleted the joseph.bell/21350/docker-debugging branch January 20, 2023 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Fixture for Local Docker Debugging using Environment Variables
5 participants