Skip to content

update kfctl_ibm KfDef to kustomize v3 #1246

Merged
k8s-ci-robot merged 8 commits into
kubeflow:masterfrom
adrian555:issue1096
Jun 22, 2020
Merged

update kfctl_ibm KfDef to kustomize v3 #1246
k8s-ci-robot merged 8 commits into
kubeflow:masterfrom
adrian555:issue1096

Conversation

@adrian555

Copy link
Copy Markdown
Member

Which issue is resolved by this Pull Request:
Resolves #1096

Description of your changes:
Follow the instruction from #1062

  • remove all parameters and overlays from kfctl_ibm.yaml
  • create a kubeflow-apps stack for Kubeflow applications required on IBM cloud
  • create kubeflow-config global configuration for several configurations
  • create specific v3 version of certain applications to run on IBM cloud

Checklist:

  • [ X ] Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@kubeflow-bot

Copy link
Copy Markdown
Contributor

This change is Reviewable

@adrian555

Copy link
Copy Markdown
Member Author

cc @animeshsingh @jlewi

@jlewi

jlewi commented Jun 12, 2020

Copy link
Copy Markdown
Contributor

@adrian555 what are all the subdirectories in stacks/ibm/application for?

/assign @vpavlin

@adrian555

adrian555 commented Jun 12, 2020

Copy link
Copy Markdown
Member Author

what are all the subdirectories in stacks/ibm/application for?

@jlewi there are about three categories

  1. some application, such as istio/add-anonymous-filter, has to install in a different namespace istio-system instead of kubeflow. I have to create a kustomization.yaml to specify the namespace. Similar to what istio/gcp-1-1-6 is doing.

  2. some application, such as katib/katib-controller, does not have a v3 kustomize version. I have to create a v3 kustomization.yaml to specify the resources including the overlays required by IBM cloud deployment.

  3. some application, such as jupyter/jupyter-web-app, has a v3 kustomize version. However it does not work well for the deployment. Particular for the namePrefix use in the jupyter-web-app/base_v3 seems to be an issue appending the prefix for the Application and VirtualService too.

I originally created an ibm sub-folder under each individual application. But I figured they might be better put together under a central place, hence the choice of stacks/ibm.

Please let me know your suggestion otherwise, thanks.

Comment thread stacks/ibm/application/katib-controller/kustomization.yaml Outdated
@jlewi

jlewi commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

Thanks for the explanation.

Particular for the namePrefix use in the jupyter-web-app/base_v3 seems to be an issue appending the prefix for the Application and VirtualService too

Can you elaborate please?

@adrian555

Copy link
Copy Markdown
Member Author

@jlewi Sure, the generated metadata.name of the Application and VirtualService from base_v3 becomes jupyter-web-app-jupyter-web-app instead of jupyter-web-app. If the name has been referenced somewhere currently, it could be problematic.

@jlewi

jlewi commented Jun 16, 2020

Copy link
Copy Markdown
Contributor

Sure, the generated metadata.name of the Application and VirtualService from base_v3 becomes jupyter-web-app-jupyter-web-app instead of jupyter-web-app. If the name has been referenced somewhere currently, it could be problematic.

Should we fix this by updating the istio and application overlays?

It looks like base is using "namePrefix" and the resources are named so that they don't include the prefix
https://github.com/kubeflow/manifests/blob/master/jupyter/jupyter-web-app/base/kustomization.yaml

However in the application and istio overlays kustomization.yaml isn't using namePrefix
https://github.com/kubeflow/manifests/blob/master/jupyter/jupyter-web-app/overlays/application/kustomization.yaml

https://github.com/kubeflow/manifests/blob/master/jupyter/jupyter-web-app/overlays/application/application.yaml#L4

So should we just update those kustomization.yamls to use namePrefix and rename the application and virtual services appropriately?

@adrian555

adrian555 commented Jun 16, 2020

Copy link
Copy Markdown
Member Author

@jlewi for v3 version in base_v3 directory, I think we should. But since I do not know if it has been used by GCP or others, I was concerned my change would impact other stacks. Therefore I created IBM's version.

On the other hand, for the base version in base directory. If we continue to use namePrefix for Application and VirtualService, we will end up new names other than jupyter-web-app. So I am not sure whether we should update there. I am afraid there may be upgrade complication. It seems the old VirtualService will not be removed from the upgrade process then we will end up with two virtual services.

@adrian555

Copy link
Copy Markdown
Member Author

@jlewi BTW, what is your opinion on stack/ibm/application directory to keep all kustomization for IBM specific? If you prefer the other way, I can also change to tensorboard/installs/ibm, istio/istio/installs/ibm, and so on.

@jlewi

jlewi commented Jun 17, 2020

Copy link
Copy Markdown
Contributor

@jlewi BTW, what is your opinion on stack/ibm/application
LGTM

I was concerned my change would impact other stacks. Therefore I created IBM's version
Sounds good. Might be worth filing a bug to fix this and maybe add some unittests to validate.

I am afraid there may be upgrade complication.

My assumption is that people should be using a tool that handles pruning. In which case the old object would no longer exist and get deleted and a new one would be created.

Don't think that's necessarily relevant to this PR though.

@adrian555

Copy link
Copy Markdown
Member Author

@jlewi I opened #1285 to track the base_v3 version of jupyter-web-app application. Any more comments on the PR? If not, can we move forward with this PR? Thanks.

@adrian555

Copy link
Copy Markdown
Member Author

@jlewi @animeshsingh I think we would like to make this into release 1.1? Thanks.

@shawnzhu

shawnzhu commented Jun 20, 2020

Copy link
Copy Markdown
Member

@adrian555 I've tested this PR (with tweaked repo URL to point the very branch) on k8s v1.18.3 on IBM cloud (VPC gen 2, block storage class works out of box) and the following components work very well when testing some examples:

  • notebook servers
  • pipelines
  • profiles
  • artifacts

The only problem is about seldon which is about SeldonIO/seldon-core#1675 that has nothing to do with this PR.

I would love to get dex (w/o anonymous user) and istio 1.3.1 included in future kfctl_ibm.yaml but I will follow your suggestion on whether using this PR or future PRs.

@krishnadurai

Copy link
Copy Markdown
Contributor

@adrian555 is this ready for review? Given @shawnzhu's comment.

I would love to get dex (w/o anonymous user) and istio 1.3.1 included in future kfctl_ibm.yaml but I will follow your suggestion on whether using this PR or future PRs.

@jlewi

jlewi commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

@adrian555 Since this is largely modifying IBM specific files; I consider myself a participant and not an approver (https://stumblingabout.com/tag/oarp/)

Someone from IBM should LGTM/approve this.

If this PR is only touching IBM specific files and you still need me to approve because of OWNERs files just let me know. In follow on PRs you should look to create appropriate OWNERs files in IBM scoped directories to allow IBMer's to approve IBM PRs.

@animeshsingh

Copy link
Copy Markdown
Contributor

thanks - given that @shawnzhu is testing it at this end, and also owning a production deployment blueprint on ibm cloud, would be best one from IBM to lgtm and approve once he is satisfied.

@adrian555 @jlewi

@animeshsingh

Copy link
Copy Markdown
Contributor

/assign @shawnzhu

@adrian555

Copy link
Copy Markdown
Member Author

@adrian555 is this ready for review? Given @shawnzhu's comment.

I would love to get dex (w/o anonymous user) and istio 1.3.1 included in future kfctl_ibm.yaml but I will follow your suggestion on whether using this PR or future PRs.

@krishnadurai this PR is ready for review. As for dex that @shawnzhu was suggesting, it will be another PR to add a new IBM version of Kubeflow with dex.

@adrian555

Copy link
Copy Markdown
Member Author

@shawnzhu since the test went well, could you please LGTM?

For IBM specific stack, I do have the OWNERS files added. @animeshsingh could you please approve then?

Thanks.

@adrian555

Copy link
Copy Markdown
Member Author

@shawnzhu thanks for the testing. As we discussed last week, we would like to push in a dex version, ie. kfctl_ibm_dex.yaml. That will be built on top of this PR and added the dex application.

@shawnzhu

Copy link
Copy Markdown
Member

/lgtm

@animeshsingh

Copy link
Copy Markdown
Contributor

/approve

@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh

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

The pull request process is described here

Details 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

@k8s-ci-robot k8s-ci-robot merged commit bc5c7a7 into kubeflow:master Jun 22, 2020
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.

v3 version of IBM KFdefs

10 participants