profile-controller: Extend tests for using images of each PR #6820

apo-ger · 2022-12-07T12:30:10Z

This PR resolves part of: #6766

Changes:

Publish the docker image, only when a PR is merged.
Remove kind and manifests GH action tests
Introduce a GH action intergration test to ensure that on each PR commit:
- The docker image can be built
- The manifests can be built
- The manifests with the updated image can be applied in a KinD cluster
  - Spin up a KinD cluster
  - Load the image in KinD cluster https://kind.sigs.k8s.io/docs/user/quick-start/#loading-an-image-into-your-cluster
  - Use sed to update the image before applying manifests
- The Pods become Ready

kimwnasptd · 2022-12-07T17:28:38Z

Thanks for the PR @apo-ger! I realized that the master branch needs the following two PRs from the release branch that we never cherry-picked

To ensure that we are using the DockerHub registry and that we when the releasing/version/VERSION file changes then each public-workflow will also push an image with the tag defined.

We'll need this because I'd like us to use the latest tag in the images of master. But we need to import this work first

kimwnasptd

Left some minor comments. Looks good in general!

.github/workflows/prof_controller_intergration_test.yaml

.github/workflows/prof_controller_docker_publish.yaml

Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

apo-ger · 2022-12-09T11:08:40Z

@kimwnasptd The tests were failing after #6825 was merged as the applied manifests were looking for the docker.io/kubeflownotebookswg/profile-controller:v1.5.0 and docker.io/kubeflownotebookswg/kfam:v1.5.0 images which don't exist. This is expected since these changes were initially introduced in release-v1.6 branch.

I've updated the PR to use that tag v1.6.0 to unblock for now. But this will be resolved once and for all when we update the tag to latest in manifests/workflows as mentioned in #6766 (comment)

I'd argue that we should also trigger the profile-controller intergration-test workflow when kfam code changes. I've refactored the Intergration test for this as well.

kimwnasptd

Sounds good @apo-ger! Left some more comments towards that direction

kimwnasptd · 2022-12-09T11:21:09Z

.github/workflows/prof_controller_intergration_test.yaml

+    - name: Build Kfam Image
+      run: |
+        cd components/access-management
+        make docker-build IMG=${{env.KFAM_IMG}}


Let's avoid building the kfam image here. Since in the next iterations #6766 (comment) we'll be using the latest tag we should be able to use that.

Also currently we use the v1.6.0 tag for kfam, so we don't need to build the image

kimwnasptd · 2022-12-09T11:21:25Z

.github/workflows/prof_controller_intergration_test.yaml

+    - name: Load Images into KinD Cluster 
+      run: |
+        kind load docker-image ${{env.PROFILE_IMG}}:${{env.TAG}}
+        kind load docker-image ${{env.KFAM_IMG}}:${{env.TAG}}


we can remove this one as well

kimwnasptd · 2022-12-09T11:25:42Z

.github/workflows/prof_controller_intergration_test.yaml

+        export CURRENT_PROFILE_IMG=docker.io/kubeflownotebookswg/profile-controller:v1.6.0
+        export CURRENT_KFAM_IMG=docker.io/kubeflownotebookswg/kfam:v1.6.0
+        export PR_PROFILE_IMG=${{env.PROFILE_IMG}}:${{env.TAG}}
+        export PR_KFAM_IMG=${{env.KFAM_IMG}}:${{env.TAG}}


Let's only update the profile image. We'll just use the KFAM image currently in the manifests, which will become latest in the future

kimwnasptd · 2022-12-09T11:26:04Z

.github/workflows/prof_controller_intergration_test.yaml

+
+env:
+  PROFILE_IMG: profile-controller
+  KFAM_IMG: kfam


I think we can remove this one as well

kimwnasptd · 2022-12-09T11:31:48Z

.github/workflows/prof_controller_intergration_test.yaml

+        export PR_PROFILE_IMG=${{env.PROFILE_IMG}}:${{env.TAG}}
+        export PR_KFAM_IMG=${{env.KFAM_IMG}}:${{env.TAG}}
+
+        kustomize build overlays/kubeflow | sed "s#$CURRENT_PROFILE_IMG#$PR_PROFILE_IMG#g" | sed "s#$CURRENT_KFAM_IMG#$PR_KFAM_IMG#g" | kubectl apply -f -


And lastly, same here. Let's just update the profile controller image for now

.github/workflows/prof_controller_docker_publish.yaml

kimwnasptd · 2022-12-09T15:32:58Z

And update on my last round of comments, after chatting a little bit with @apo-ger as well. We decided to keep one workflow file for building/testing both KFAM and Profiles. But this is not ideal and we need to create some issues for next steps and expose some context:

KFAM is using a stable version of Profiles

Right now KFAM is using the Profiles code from a specific (and very old) commit https://github.com/kubeflow/kubeflow/blob/master/components/access-management/go.mod#L38

This means that there's no way to test now if a change in the Profiles will break KFAM. We need to re-evaluate how to have KFAM uses the code from the Profiles Controller. We need KFAM to always use the latest code of Profiles and not bind it to a specific commit.

We had done this for the Notebook Controller https://github.com/kubeflow/kubeflow/blob/v1.5-branch/components/notebook-controller/go.mod#L18-L21

But looking at Notebooks I realized we had a regression on this in v1.6 :-)

kimwnasptd · 2022-12-09T15:38:15Z

So, the ideal state I have in mind is:

KFAM code should use the local profiles code
Have one workflow that builds/tests both Profiles and KFAM images when the code of either one changes
- We want to ensure changes in the Profiles API won't break KFAM

So @apo-ger ignore my above comments for not building KFAM. Let's have one workflow for both and have a follow-up issue on making KFAM to use the local code of Profiles, to ensure we won't end up with broken KFAM from Profiles.

Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

* Trigger workflow when kfam code changes since KFAM manifests are deployed with Profile Controller manifests. * Build KFAM image as well and update manifests with sed before deploying them in the KinD cluster. * Update tag in manifests to v1.6.0 since the images with tag v1.5.0 do not exist in Dockerhub. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

apo-ger · 2022-12-09T15:59:50Z

@kimwnasptd Thanks for all the comments and the context! I've also updated the PR to publish the kfam image only when a PR is merged as mentioned in #6766 (comment).

kimwnasptd · 2022-12-12T12:14:12Z

Thank you for the work @apo-ger!

/lgtm
/approve

google-oss-prow · 2022-12-12T12:14:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~.github/OWNERS~~ [kimwnasptd]
~~components/profile-controller/OWNERS~~ [kimwnasptd]

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

google-oss-prow bot added the do-not-merge/work-in-progress label Dec 7, 2022

google-oss-prow bot requested review from kimwnasptd and StefanoFioravanzo December 7, 2022 12:30

google-oss-prow bot added the size/L label Dec 7, 2022

apo-ger force-pushed the feature-extend-kf-tests branch 6 times, most recently from 3d69652 to cbfcb65 Compare December 7, 2022 15:02

apo-ger marked this pull request as ready for review December 7, 2022 15:03

google-oss-prow bot removed the do-not-merge/work-in-progress label Dec 7, 2022

google-oss-prow bot requested a review from elikatsis December 7, 2022 15:03

apo-ger force-pushed the feature-extend-kf-tests branch 2 times, most recently from 9ed9950 to 1aea808 Compare December 7, 2022 15:47

apo-ger mentioned this pull request Dec 8, 2022

Extend tests for using images of each PR #6766

Open

apo-ger force-pushed the feature-extend-kf-tests branch from 1aea808 to 248176d Compare December 8, 2022 13:16

kimwnasptd reviewed Dec 8, 2022

View reviewed changes

Introduce github action workflow for intergration test

a20fc73

Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

apo-ger force-pushed the feature-extend-kf-tests branch 4 times, most recently from 29f36b7 to 8c60846 Compare December 9, 2022 10:56

kimwnasptd reviewed Dec 9, 2022

View reviewed changes

apo-ger force-pushed the feature-extend-kf-tests branch from 8c60846 to f79791b Compare December 9, 2022 15:31

apo-ger force-pushed the feature-extend-kf-tests branch 2 times, most recently from 99c72a8 to f9b985c Compare December 9, 2022 15:34

apo-ger added 4 commits December 9, 2022 17:49

Publish Docker image only when PR is merged

9c2c58c

Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

Remove kind & manifest gh action tests

4d2d3c9

Review fixes

b534e7a

Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>

apo-ger force-pushed the feature-extend-kf-tests branch from f9b985c to ac921f3 Compare December 9, 2022 15:49

google-oss-prow bot assigned kimwnasptd Dec 12, 2022

google-oss-prow bot added the lgtm label Dec 12, 2022

google-oss-prow bot added the approved label Dec 12, 2022

google-oss-prow bot merged commit 253cfcb into kubeflow:master Dec 12, 2022

kimwnasptd mentioned this pull request Feb 21, 2023

create a list of Notebook features in Kubeflow 1.7 #6932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

profile-controller: Extend tests for using images of each PR #6820

profile-controller: Extend tests for using images of each PR #6820

apo-ger commented Dec 7, 2022 •

edited

kimwnasptd commented Dec 7, 2022

kimwnasptd left a comment

apo-ger commented Dec 9, 2022 •

edited

kimwnasptd left a comment

kimwnasptd Dec 9, 2022

kimwnasptd Dec 9, 2022

kimwnasptd Dec 9, 2022

kimwnasptd Dec 9, 2022

kimwnasptd Dec 9, 2022

kimwnasptd commented Dec 9, 2022 •

edited

kimwnasptd commented Dec 9, 2022

apo-ger commented Dec 9, 2022

kimwnasptd commented Dec 12, 2022

google-oss-prow bot commented Dec 12, 2022

profile-controller: Extend tests for using images of each PR #6820

profile-controller: Extend tests for using images of each PR #6820

Conversation

apo-ger commented Dec 7, 2022 • edited

kimwnasptd commented Dec 7, 2022

kimwnasptd left a comment

Choose a reason for hiding this comment

apo-ger commented Dec 9, 2022 • edited

kimwnasptd left a comment

Choose a reason for hiding this comment

kimwnasptd Dec 9, 2022

Choose a reason for hiding this comment

kimwnasptd Dec 9, 2022

Choose a reason for hiding this comment

kimwnasptd Dec 9, 2022

Choose a reason for hiding this comment

kimwnasptd Dec 9, 2022

Choose a reason for hiding this comment

kimwnasptd Dec 9, 2022

Choose a reason for hiding this comment

kimwnasptd commented Dec 9, 2022 • edited

KFAM is using a stable version of Profiles

kimwnasptd commented Dec 9, 2022

apo-ger commented Dec 9, 2022

kimwnasptd commented Dec 12, 2022

google-oss-prow bot commented Dec 12, 2022

apo-ger commented Dec 7, 2022 •

edited

apo-ger commented Dec 9, 2022 •

edited

kimwnasptd commented Dec 9, 2022 •

edited