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.

Sign up for GitHub

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

Jump to bottom

chore: update rockcraft project, add sanity test, and tox.ini file #85

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

chore: update rockcraft project, add sanity test, and tox.ini file #85

wants to merge 4 commits into from

Conversation

NohaIhab
Copy link
Contributor

@NohaIhab NohaIhab commented May 20, 2024 •
edited
Loading

closes #84

Summary

  • Updates the rockcraft.yaml to 1.8 version for CKF 1.8
  • Refactors the rockcraft.yaml to be consistent with the Dockerfile's copying files and building
  • Modifies the pebble service name to match the 1.8 charm
  • Adds tox file to run the workflows on the volumes rockcraft project
  • Adds sanity tests to check the expected files exist in the rock
  • Adds .gitignore to the repo to avoid committing unwanted files

Testing

  1. Build the rock
tox -e pack
  1. Export the rock to Docker
tox -e export-to-docker
  1. Run a container with the image and replace the entrypoint with a shell
docker run --rm -ti volumes-web-app:1.8.0 exec /bin/bash
  1. Go to the working-dir /src
cd /src
  1. Run the entrypoint command
_daemon_@f2a80c2d3f9f:/src$ gunicorn -w 3 --bind 0.0.0.0:5000 --access-logfile - entrypoint:app

Expected output:

[2024-05-20 12:20:09 +0000] [22] [INFO] Starting gunicorn 22.0.0
[2024-05-20 12:20:09 +0000] [22] [INFO] Listening at: http://0.0.0.0:5000 (22)
[2024-05-20 12:20:09 +0000] [22] [INFO] Using worker: sync
[2024-05-20 12:20:09 +0000] [23] [INFO] Booting worker with pid: 23
[2024-05-20 12:20:09 +0000] [24] [INFO] Booting worker with pid: 24
[2024-05-20 12:20:09 +0000] [25] [INFO] Booting worker with pid: 25
[2024-05-20 12:20:12 +0000] [23] [ERROR] Exception in worker process
...

Because the container image needs kubeconfig and proper expose of a the serving port, the expected output will be followed by the following errors:

kubernetes.config.config_exception.ConfigException: Service host/port is not set.
kubernetes.config.config_exception.ConfigException: Invalid kube-config file. No configuration found.

Another test you can do is run the upstream image and compare the output, it should be the same:

docker run docker.io/kubeflownotebookswg/volumes-web-app:v1.8.0

@NohaIhab NohaIhab requested a review from a team as a code owner May 20, 2024 12:10
NohaIhab
NohaIhab commented May 20, 2024
View reviewed changes
volumes-web-app/rockcraft.yaml
serve:
override: replace
kubeflow-volumes:
override: merge
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing override to merge because there are envs set in the charm's pebble layer that are needed.
Although I'm not sure about the behavior in case of replace in the rock if the envs will still be set, but having it as merge will make sure they are set.

misohu
misohu reviewed May 21, 2024
View reviewed changes
Copy link
Member

@misohu misohu left a comment

Choose a reason for hiding this comment

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

Small comments. Can we also add link to dockerfile to the top ?

volumes-web-app/rockcraft.yaml
python3 setup.py bdist_wheel
cp dist/kubeflow-1.1-py3-none-any.whl $CRAFT_STAGE
Copy link
Member

Choose a reason for hiding this comment

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

So we decided to remove this but this means the built wheel stays in build phase for ROCK and wont be available in the final image. I think we need to keep this.

volumes-web-app/rockcraft.yaml

mkdir -p $CRAFT_STAGE/frontend-src/node_modules/kubeflow
cp -r $CRAFT_STAGE/frontend-lib-src/dist/kubeflow/* $CRAFT_STAGE/frontend-src/node_modules/kubeflow

npm run build -- --output-path=./dist/default --configuration=production
Copy link
Member

Choose a reason for hiding this comment

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

Same problem as above we are actually never moving the built components to staging phase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you sure? but we are doing cd $CRAFT_STAGE/frontend-src/ on L88 before running the build so this means the built components are in the staging directory

rgildein
rgildein reviewed May 21, 2024
View reviewed changes
Copy link

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

Left some comments here, but I'm completely new here (no experience with rocks or team process).

volumes-web-app/rockcraft.yaml
@@ -4,117 +4,133 @@ description: |
This web app is responsible for allowing the user to manipulate PVCs in their Kubeflow
cluster. To achieve this it provides a user friendly way to handle the lifecycle of PVC
objects.
version: v1.7.0_1
version: "1.8.0"
Copy link

Choose a reason for hiding this comment

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

Is there reason to removing v? In the description, we are still mention volumes-web-app:v1.8.0, is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the new convention for all ROCKs in the team to not have the v
The description was a typo, good observation

volumes-web-app/tests/test_rock.py
rock_version = check_rock.get_version()
LOCAL_ROCK_IMAGE = f"{rock_image}:{rock_version}"

# assert the rock contains the expected files
Copy link

Choose a reason for hiding this comment

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

Should we also check the stdout of those runs below? Since like this we only check that src directory exists and /src/entrypoint.py exists.

volumes-web-app/tests/test_rock.py
LOCAL_ROCK_IMAGE = f"{rock_image}:{rock_version}"

# assert the rock contains the expected files
subprocess.run(
Copy link

Choose a reason for hiding this comment

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

idea: Maybe using docker-py will be easier to use.

volumes-web-app/tox.ini
LOCAL_CHARM_DIR=charm_repo

[testenv:pack]
passenv = *
Copy link

Choose a reason for hiding this comment

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

not-blocking: I think that passenv = * can be defined as "global" in [testenv].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgildein rgildein rgildein left review comments

@misohu misohu misohu left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update rockcraft project of volumes-web-app for CKF 1.8
3 participants
@NohaIhab @misohu @rgildein