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

src(buildpack): add python #60

Merged
merged 5 commits into from
Mar 30, 2021
Merged

src(buildpack): add python #60

merged 5 commits into from
Mar 30, 2021

Conversation

lance
Copy link
Member

@lance lance commented Feb 2, 2021

This commit adds a python buildpack and simple invoker. This is awork in progress. Existing issues:

  • Extremely slow on first build (>10 minutes) subsequent builds are really quick
  • Attempt to install python 3.8 on UBI images results in 3.6?
  • The installed pip is very old
  • No user dependencies capability yet
  • Only CloudEvents functions are supported at the moment
  • Still needs a production runtime - at the moment, it's development mode only

This commit adds a python buildpack and simple invoker. This is a
work in progress. Existing issues:

- Extremely slow on first build (>10 minutes)
- Attempt to install python 3.8 on UBI images results in 3.6?
- The installed pip is very old
- No user dependencies capability yet
@lance lance added the artifacts/buildpacks Issues related to the runtime buildpacks label Feb 2, 2021
@lance lance requested a review from a team February 2, 2021 23:38
@lance lance self-assigned this Feb 2, 2021
@lance lance marked this pull request as draft February 2, 2021 23:38
@zroubalik
Copy link
Contributor

* Extremely slow on first build (>10 minutes)

@lance any idea why?

@lance
Copy link
Member Author

lance commented Feb 3, 2021

@zroubalik really no idea why it's so slow. It happens when pulling in dependencies for flask, etc. If you can try a build locally and let me know if you see the same that would be great.

@lance
Copy link
Member Author

lance commented Feb 4, 2021

@zroubalik it should be running for CE now

Base automatically changed from master to main March 9, 2021 15:17
lance added 2 commits March 24, 2021 18:33
This causes the flask app to run in production mode.

Signed-off-by: Lance Ball <[email protected]>
@lance
Copy link
Member Author

lance commented Mar 25, 2021

@matejvasek @zroubalik ptal - I think this might be ready to land. The first three issues noted in the PR description are... 🤷

@lance
Copy link
Member Author

lance commented Mar 25, 2021

Considering creating and publishing a python package for the server so that we can support local testing and running on localhost similar to the Node.js buildpack.

@lance lance marked this pull request as ready for review March 26, 2021 15:13
@lance
Copy link
Member Author

lance commented Mar 26, 2021

Considering creating and publishing a python package for the server so that we can support local testing and running on localhost similar to the Node.js buildpack.

https://github.com/boson-project/parliament

Sample usage:

$ cat func.py
from cloudevents.http import CloudEvent, to_binary

# ---------------------------------------
# Function template
# context is a dictionary with the Flask request object and 
# and a cloud_event key for incoming events
# ---------------------------------------
def main(context):
    print(f"Method: {context['request'].method}")
    attributes = {
      "type": "com.example.fn",
      "source": "https://example.com/fn"
    }
    data = { "message": "Howdy!" }
    event = CloudEvent(attributes, data)
    headers, body = to_binary(event)
    return body, 200, headers
$ pip install parliament-functions
$ parliament .

@matejvasek
Copy link
Contributor

@lance
I got:

2021/03/29 17:04:36.506665 INFO:   ===> BUILDING
2021/03/29 17:04:36.506696 DEBUG:  Running the builder on OS linux with:
2021/03/29 17:04:36.506701 DEBUG:  Container Settings:
2021/03/29 17:04:36.506705 DEBUG:    Args: /cnb/lifecycle/builder
2021/03/29 17:04:36.506708 DEBUG:    System Envs: CNB_PLATFORM_API=0.4
2021/03/29 17:04:36.506712 DEBUG:    Image: pack.local/builder/696e6c706d6c71716b6e:latest
2021/03/29 17:04:36.506715 DEBUG:    User: 
2021/03/29 17:04:36.506725 DEBUG:    Labels: map[author:pack]
2021/03/29 17:04:36.506727 DEBUG:  Host Settings:
2021/03/29 17:04:36.506731 DEBUG:    Binds: pack-layers-qxdrlskzuf:/layers pack-app-nvzharrrsw:/workspace
2021/03/29 17:04:36.506735 DEBUG:    Network Mode: host
[builder] ---> Python Function Buildpack
[builder] /cnb/buildpacks/dev.boson.python/0.0.1/lib/build.sh: line 13: syntax error in conditional expression: unexpected token `;'
[builder] ERROR: failed to build: exit status 2
Error: executing lifecycle. This may be the result of using an untrusted builder: failed with status code: 145

I don't know why, lib/build.sh looks good to me.

@matejvasek
Copy link
Contributor

shellcheck buildpacks/python/lib/build.sh                                                                                                                      ST 4   lance/add-python 

In buildpacks/python/lib/build.sh line 13:
  if [[ ! -f "${layer_dir}/environments/functions/bin/flask"]]; then
  ^-- SC1009: The mentioned syntax error was in this if expression.
     ^-- SC1073: Couldn't parse this test expression. Fix to allow more checks.
             ^-- SC1019: Expected this to be an argument to the unary condition.
                                                              ^-- SC1020: You need a space before the ]].
                                                              ^-- SC1072: Missing space before ]. Fix any mentioned problems and try again.

For more information:
  https://www.shellcheck.net/wiki/SC1020 -- You need a space before the ]].
  https://www.shellcheck.net/wiki/SC1019 -- Expected this to be an argument t...
  https://www.shellcheck.net/wiki/SC1072 -- Missing space before ]. Fix any m...

@matejvasek
Copy link
Contributor

matejvasek commented Mar 29, 2021

Hmm maybe I checked out wrong remote for this.
I fetched from your fork.

@lance
Copy link
Member Author

lance commented Mar 29, 2021

Strange... I definitely ran this BP over and over again last week.

@lance
Copy link
Member Author

lance commented Mar 29, 2021

Is there an easy way to throttle the number of revisions created?

Oh, sorry. I have not updated that. You should pull from the lance/add-python branch on this repo.

@matejvasek
Copy link
Contributor

@lance It's my fault I wrongfully assumed it's from your repo, the PR is OK, I think.

@matejvasek
Copy link
Contributor

matejvasek commented Mar 29, 2021

Yup it works from [email protected]:boson-project/buildpacks.git.

@matejvasek
Copy link
Contributor

@lance the python Function seems to react to SIGINT but it doesn't react to SIGTERM I think it should.

@matejvasek
Copy link
Contributor

Also I noticed that the python Function runs under user id 1000, whereas other functions (e.g. golang) run under uid of local user. I don't know if its good or bad or neither; or if it's deliberate.

@zroubalik
Copy link
Contributor

zroubalik commented Mar 29, 2021

Also I noticed that the python Function runs under user id 1000, whereas other functions (e.g. golang) run under uid of local user. I don't know if its good or bad or neither; or if it's deliberate.

Good catch, we should probably unify this, at least for the sake of consistency

@lance
Copy link
Member Author

lance commented Mar 29, 2021

Also I noticed that the python Function runs under user id 1000, whereas other functions (e.g. golang) run under uid of local user. I don't know if its good or bad or neither; or if it's deliberate.

Hmm this is confusing. Shouldn't they all run as user 1000?

ARG cnb_uid=1000
ARG cnb_gid=1001
# Create user and group
RUN groupadd -g ${cnb_gid} cnb && \
useradd -u ${cnb_uid} -g cnb -s /bin/bash cnb
# Set required CNB information
ENV CNB_USER_ID=${cnb_uid}
ENV CNB_GROUP_ID=${cnb_gid}

@matejvasek
Copy link
Contributor

Also I noticed that the python Function runs under user id 1000, whereas other functions (e.g. golang) run under uid of local user. I don't know if its good or bad or neither; or if it's deliberate.

Hmm this is confusing. Shouldn't they all run as user 1000?

ARG cnb_uid=1000
ARG cnb_gid=1001
# Create user and group
RUN groupadd -g ${cnb_gid} cnb && \
useradd -u ${cnb_uid} -g cnb -s /bin/bash cnb
# Set required CNB information
ENV CNB_USER_ID=${cnb_uid}
ENV CNB_GROUP_ID=${cnb_gid}

Yes it should. Let me double check that. Maybe I used different docker daemons in different console windows. I sometimes use podman and sometimes dockerd might be caused by that.

@lance
Copy link
Member Author

lance commented Mar 29, 2021

the python Function seems to react to SIGINT but it doesn't react to SIGTERM I think it should.

I am a little stumped by this. The ENTRYPOINT command is

source $environments_layer/functions/bin/activate && cd $app_layer && parliament .

If I run the equivalent commands locally and send SIGTERM to the process, it shuts down gracefully.

❯ parliament .
fish: “parliament .” terminated by signal SIGTERM (Polite quit request)

Do you think it would be OK if we don't let this block landing the PR, and instead open an issue for it? I did confirm that, for whatever reason, SIGTERM does not terminate the process when running the container.

@matejvasek
Copy link
Contributor

@lance With regard to UID -- it was indeed caused by using podman vs. docker.

@matejvasek
Copy link
Contributor

Do you think it would be OK if we don't let this block landing the PR, and instead open an issue for it? I did confirm that, for whatever reason, SIGTERM does not terminate the process when running the container.

👍 I think that's OK. It's not any critical it will only prolong ksvc shutdown I think.

I can confirm, local run with sigterm work, but it doesn't work in container.

@lance
Copy link
Member Author

lance commented Mar 29, 2021

Added an issue for SIGTERM here #66

@matejvasek
Copy link
Contributor

LGTM

@matejvasek
Copy link
Contributor

Just one thing. I tried local development using venv. I created env in the project directory.
Then I got:

$f run  
ERROR: failed to launch: modify env: add layer env: apply env files from dir '/layers/dev.boson.python/app/env': read /layers/dev.boson.python/app/env/lib64: is a directory

I think that we somehow should let user know he is not supposed create env in the project directory.

@lance
Copy link
Member Author

lance commented Mar 29, 2021

I think that we somehow should let user know he is not supposed create env in the project directory.

@matejvasek do you know if that is standard practice for Python devs? When I was making all of this, I set up an environment in ~/src/environments and of course did not see this issue.

@matejvasek
Copy link
Contributor

I think that we somehow should let user know he is not supposed create env in the project directory.

@matejvasek do you know if that is standard practice for Python devs? When I was making all of this, I set up an environment in ~/src/environments and of course did not see this issue.

Don't know, jetbrains IDE did that for me, but now I see it's creating venv (not env) by default. So I guess it's OK. Would be good to have seasoned python programmer to review this.

@lance
Copy link
Member Author

lance commented Mar 29, 2021

venv is the standard naming that I have seen used as I worked on this.

@matejvasek
Copy link
Contributor

matejvasek commented Mar 29, 2021

venv is the standard naming that I have seen used as I worked on this.

Then it's all right I think.

@lance lance merged commit 1fa12bc into main Mar 30, 2021
@lance lance deleted the lance/add-python branch March 30, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
artifacts/buildpacks Issues related to the runtime buildpacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants