Skip to content

Conversation

mmikitka
Copy link
Contributor

See issue #1247

@mmikitka
Copy link
Contributor Author

/assign @smukherj1

nlopezgi
nlopezgi previously approved these changes Oct 25, 2019
@nlopezgi
Copy link
Contributor

/gcbrun

@nlopezgi
Copy link
Contributor

Test py_image_complex_layer is failing, can you double check it please

@mmikitka
Copy link
Contributor Author

I'm seeing the following failure, and the cause is not obvious to me:

FAILURE: Expected 'Calling from main module: through py_image_complex_library: Six version: 1.11.0' not found in 'Loaded image ID: sha256:164cea6a3df2588b416bf8393f9a03897e9446eb49ee0f4e7af7ee104c2eebbe

Copy link
Collaborator

@smukherj1 smukherj1 left a comment

Choose a reason for hiding this comment

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

I don't think this change works as is and the failing e2e py_image tests demonstrate the issue. Basically the problem is, nowhere in the py_image macro, app_layer rule or the container_image rule is the entrypoint extracted from the python toolchain and added to the generated image. With this change, the default entrypoint /usr/bin/python is no longer set for the python images and as a result the built python image can no longer be run with bazel run.

For example, without this change when I do bazel run //testdata:py_image_complex I get the output

2322b07b1fca: Loading layer [==================================================>]  20.48kB/20.48kB
a7cfa517d927: Loading layer [==================================================>]  20.48kB/20.48kB
d020e73f9ddd: Loading layer [==================================================>]   51.2kB/51.2kB
cf24a96e44e7: Loading layer [==================================================>]  10.24kB/10.24kB
d9fbbb6dea0e: Loading layer [==================================================>]  10.24kB/10.24kB
833a80dfc7a5: Loading layer [==================================================>]  10.24kB/10.24kB
6ccee0e47b81: Loading layer [==================================================>]  10.24kB/10.24kB
41126e02259a: Loading layer [==================================================>]  10.24kB/10.24kB
6c808181fa57: Loading layer [==================================================>]  30.72kB/30.72kB
Loaded image ID: sha256:d1e762e7f34bb41281ac035114e9e83f59018198009c5ad40f3c45f614f8be88
Tagging d1e762e7f34bb41281ac035114e9e83f59018198009c5ad40f3c45f614f8be88 as bazel/testdata:py_image_complex
Calling from main module: through py_image_complex_library: Six version: 1.11.0
Calling from main module: through py_image_complex_library: Addict version: 2.1.2

after making this change I get

Loaded image ID: sha256:164cea6a3df2588b416bf8393f9a03897e9446eb49ee0f4e7af7ee104c2eebbe
Tagging 164cea6a3df2588b416bf8393f9a03897e9446eb49ee0f4e7af7ee104c2eebbe as bazel/testdata:py_image_complex
standard_init_linux.go:207: exec user process caused "no such file or directory"

However, fixing this is going to be tricky. py_image and py_image3 are macros and thus can't extract info from the python toolchain. app_layer is a rule and has access to toolchains but putting python specific logic in there might not be a good idea.

@nlopezgi nlopezgi dismissed their stale review October 30, 2019 14:27

as per @smukherj1 comment, this PR might not be working as intended.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mmikitka
To complete the pull request process, please assign nlopezgi
You can assign the PR to them by writing /assign @nlopezgi in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mmikitka
Copy link
Contributor Author

Note that the py_binary is in the image entrypoint, and the error message exec user process caused "no such file or directory" is caused by the absence of /usr/bin/env on the system, as referenced in the shebang.

Relevant output of docker inspect bazel/testdata:py_image_complex:

           "Cmd": null,
            "WorkingDir": "/app/testdata/py_image_complex.binary.runfiles/io_bazel_rules_docker",
            "Entrypoint": [
                "/app/testdata/py_image_complex.binary"
            ],

The top of /app/testdata/py_image_complex.binary contains:

#!/usr/bin/env python

And, /usr/bin/env is not in the py_image_base layer.

Nonetheless, this is somewhat beside the point, as we don't want to assume that the Python interpreter resolved by /usr/bin/env is the same as the interpreter specified by the toolchain.

Essentially, we need the toolchain-specific Python interpreter to be the first command in the entrypoint.

@mmikitka
Copy link
Contributor Author

/gcbrun

@mmikitka
Copy link
Contributor Author

Since we don't want language-specific logic (in this case Python) in the app_layer implementation, one solution is to remove the absolute path to Python, and rely on PATH resolution when calling python in the entrypoint.

@nlopezgi
Copy link
Contributor

@mmikitka I don't think that will work as python resolution relies on using which python which is not available on all containers (particularly distroless based ones)

@mmikitka
Copy link
Contributor Author

/assign @nlopezgi

@mmikitka
Copy link
Contributor Author

@nlopezgi Can we require users to explicitly declare the entrypoint in the event that which is not available (e.g., distroless) or the python path cannot be resolved?

Either way, an assumption must be made of the underlying base image i.e., is which available or is /usr/bin/python available.

However, if you can propose a solution that does not require language-specific code in app_layer, I would gladly implement it.

@nlopezgi
Copy link
Contributor

nlopezgi commented Oct 31, 2019

@nlopezgi Can we require users to explicitly declare the entrypoint in the event that which is not available (e.g., distroless) or the python path cannot be resolved?

no, that would break too many existing use cases

Either way, an assumption must be made of the underlying base image i.e., is which available or is /usr/bin/python available.

For now, we will assume the latter, as that is the way it's currently implemented.

However, if you can propose a solution that does not require language-specific code in app_layer, I would gladly implement it.

I can't think of any. Would adding the symlink to /usr/bin/python in the base container be an acceptable workaround for you?

@mmikitka
Copy link
Contributor Author

@nlopezgi I'll resort to explicitly setting the entrypoint or using a symlink as you suggested. Thanks for the conversation, and I think that we can close this issue

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.

6 participants