Skip to content

fix(makefile): standardized image targets #1015

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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

andyatmiami
Copy link
Contributor

Description

  • deleted logs/ directory and added it to .gitignore

  • removed RELEASE_PYTHON_VERSION and standardized on PYTHON_VERSION makefile variable

  • helper functions to parse makefile target and extract important metadata as makefile variables

  • add retries to podman push in build_image makefile function

  • dynamically build workbench directory / dockerfile filename based on target

  • standardized makefile image targets as ----

  • single deploy-% target for all images

  • single undeploy-% target for all images

  • singe test-% target for all images

  • new e2e-% target that runs $* + deploy-$* + test-$* + undeploy-$*

  • updated/simplified make_test.py in light of Makefile changes

  • pass kustomize output to kubectl via stdin to avoid accidental checkin of personal settings

  • refactored notebooks/ repo file hierarchy to consistently leverage subfolders for accelerator-specific resources

    • renamed runtimes folder to runtime to match target name
    • jupyter/cuda + jupyter/rocm
    • runtime/cuda + runtime/rocm
  • updated kustomize resources for consistency

    • image name used an manifest name prefix
    • -workbench used as manifest name suffix
    • using labels transformer as commonLabels deprecated
    • containerPort named workbench-port
    • removed spec.containers.command from codeserver/rstudio to let server start
    • images.newTag aligned with makefile target
    • added emptyDir volume mount to all workloads
    • added startupProbe to our accelerator images
    • using term "workbench" as opposed to "notebook" consistently throughout manifests
  • updated various Dockerfile to match new folder hierarchy where necessary

  • refactored test_jupyter_with_papermill to support testing needs of all workbenches + runtimes

    • scripts/makefile_utils directory created
    • numerous usability enhancements to the logic
      • reduce hardcoding of "magic" strings by parsing kustomize output to identify workload names and ports
      • scan for open port and use that when verifying container starts via kubectl port-forward
      • confirms container starts for all workbenches (not just jupyter)
      • confirms required libraries installed in container (now applied to jupyter notebooks as well)
      • moved all validate-xxx target logic into script for better consolidated maintenance
      • relies on makefile to pass metadata parsed from target name to avoid duplicating logic
  • TODO:

    • fix any problems in GHA due to above changes
    • add NAMING.md file to explain the "rules" around our makefile target names and all the places in our development flow that is impacted
    • fix openshift/release due to above changes
    • cleanup now-defunct/legacy makefile targets

Related-to: https://issues.redhat.com/browse/RHOAIENG-23291

How Has This Been Tested?

TODO

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Copy link
Contributor

openshift-ci bot commented Apr 11, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Apr 11, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andyatmiami for approval. For more information see the Code Review Process.

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

- deleted logs/ directory and added it to .gitignore

- removed RELEASE_PYTHON_VERSION and standardized on PYTHON_VERSION makefile variable

- helper functions to parse makefile target and extract important metadata as makefile variables

- add retries to podman push in build_image makefile function

- dynamically build workbench directory / dockerfile filename based on target

- standardized makefile image targets as <accelerator>-<feature>-<scope>-<os>-<python version>

- single deploy-% target for all images

- single undeploy-% target for all images

- singe test-% target for all images

- new e2e-% target that runs $* + deploy-$* + test-$* + undeploy-$*

- updated/simplified make_test.py in light of Makefile changes

- pass kustomize output to kubectl via stdin to avoid accidental checkin of personal settings

- refactored notebooks/ repo file hierarchy to consistently leverage subfolders for accelerator-specific resources
	- renamed runtimes folder to runtime to match target name
	- jupyter/cuda + jupyter/rocm
	- runtime/cuda + runtime/rocm

- updated kustomize resources for consistency
	- image name used an manifest name prefix
	- -workbench used as manifest name suffix
	- using labels transformer as commonLabels deprecated
	- containerPort named workbench-port
	- removed spec.containers.command from codeserver/rstudio to let server start
	- images.newTag aligned with makefile target
	- added emptyDir volume mount to all workloads
	- added startupProbe to our accelerator images
	- using term "workbench" as opposed to "notebook" consistently throughout manifests

- updated various Dockerfile to match new folder hierarchy where necessary

- refactored test_jupyter_with_papermill to support testing needs of all workbenches + runtimes
	- scripts/makefile_utils directory created
	- numerous usability enhancements to the logic
		- reduce hardcoding of "magic" strings by parsing kustomize output to identify workload names and ports
		- scan for open port and use that when verifying container starts via kubectl port-forward
		- confirms container starts for all workbenches (not just jupyter)
		- confirms required libraries installed in container (now applied to jupyter notebooks as well)
		- moved all validate-xxx target logic into script for better consolidated maintenance
		- relies on makefile to pass metadata parsed from target name to avoid duplicating logic

- TODO:
	- fix any problems in GHA due to above changes
	- add NAMING.md file to explain the "rules" around our makefile target names and all the places in our development flow that is impacted
	- fix openshift/release due to above changes
	- cleanup now-defunct/legacy makefile targets

Related-to: https://issues.redhat.com/browse/RHOAIENG-23291
@andyatmiami andyatmiami force-pushed the fix/standardize-makefile-targets branch from e43fcde to 2be2b0f Compare April 13, 2025 14:54
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 13, 2025
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 13, 2025
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 13, 2025
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 13, 2025
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 13, 2025
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 13, 2025
@openshift-ci openshift-ci bot removed the size/xxl label Apr 13, 2025
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 14, 2025
@andyatmiami andyatmiami force-pushed the fix/standardize-makefile-targets branch from 5108b11 to 872be10 Compare April 14, 2025 01:15
@openshift-ci openshift-ci bot added size/xxl and removed size/xxl labels Apr 14, 2025
andyatmiami added a commit to andyatmiami/notebooks that referenced this pull request Apr 14, 2025
This is a "piece" of a more comprehensive/interesting PR:
- opendatahub-io#1015

Unfortunately, that PR has grown wildly unwieldy in its size - and immediate feedback received was to try to break it into smaller pieces - so consider this one piece!

The ulitmate goal here on this targetted PR is two-fold:
- standardization irrespective of image build "flavour" our kustomize labelling
- get rid of following warning:

```
$ kubectl kustomize jupyter/minimal/ubi9-python-3.11/kustomize/base
# Warning: 'commonLabels' is deprecated. Please use 'labels' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
...
```

No actual changes are introduced in this PR - simply leveraging the `LabelTransformer` to accomplish what `commonLabels` was previously doing.

Related-to: https://issues.redhat.com/browse/RHOAIENG-23291
andyatmiami added a commit to andyatmiami/notebooks that referenced this pull request Apr 14, 2025
This is a "piece" of a more comprehensive/interesting PR:
- opendatahub-io#1015

Unfortunately, that PR has grown wildly unwieldy in its size - and immediate feedback received was to try to break it into smaller pieces - so consider this one piece!

The ulitmate goal here on this targetted PR is two-fold:
- standardization irrespective of image build "flavour" our kustomize labelling
- get rid of following warning:

```
$ kubectl kustomize jupyter/minimal/ubi9-python-3.11/kustomize/base
...
```

No actual changes are introduced in this PR - simply leveraging the `LabelTransformer` to accomplish what `commonLabels` was previously doing.

Related-to: https://issues.redhat.com/browse/RHOAIENG-23291
andyatmiami added a commit to andyatmiami/notebooks that referenced this pull request Apr 14, 2025
This is a "piece" of a more comprehensive/interesting PR:
- opendatahub-io#1015

Unfortunately, that PR has grown wildly unwieldy in its size - and immediate feedback received was to try to break it into smaller pieces - so consider this one piece!

The ulitmate goal here on this targetted PR is two-fold:
- standardization irrespective of image build "flavour" our kustomize labelling
- get rid of following warning:

```
$ kubectl kustomize jupyter/minimal/ubi9-python-3.11/kustomize/base
...
```

No actual changes are introduced in this PR - simply leveraging the `LabelTransformer` to accomplish what `commonLabels` was previously doing.

Related-to: https://issues.redhat.com/browse/RHOAIENG-23291
andyatmiami added a commit to andyatmiami/notebooks that referenced this pull request Apr 14, 2025
This is a "piece" of a more comprehensive/interesting PR:
- opendatahub-io#1015

Unfortunately, that PR has grown wildly unwieldy in its size - and immediate feedback received was to try to break it into smaller pieces - so consider this one piece!

The ulitmate goal here on this targetted PR is two-fold:
- standardization irrespective of image build "flavour" our kustomize labelling
- get rid of following warning:

```
$ kubectl kustomize jupyter/minimal/ubi9-python-3.11/kustomize/base
...
```

No actual changes are introduced in this PR - simply leveraging the `LabelTransformer` to accomplish what `commonLabels` was previously doing.

Related-to: https://issues.redhat.com/browse/RHOAIENG-23291
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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.

2 participants