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

Update getting-started-multipass.md #1469

Merged
merged 3 commits into from
Jan 29, 2020
Merged

Conversation

ammarn911
Copy link
Contributor

@ammarn911 ammarn911 commented Dec 17, 2019

Updating instructions for fast Kubeflow setup using MicroK8s


This change is Reviewable

Updating instructions for fast Kubeflow setup using MicroK8s
@k8s-ci-robot
Copy link
Contributor

Hi @ammarn911. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@ammarn911
Copy link
Contributor Author

/assign @aronchick

@sarahmaddox
Copy link
Contributor

/ok-to-test
/assign @carmine

@carmine Can you do a review of this update, since you created the original doc?

@sarahmaddox
Copy link
Contributor

@carmine How's it going with finding a review for this PR?

```
$ git clone https://github.com/canonical-labs/kubernetes-tools
$ sudo kubernetes-tools/setup-microk8s.sh
$ snap install microk8s --classic --channel=1.17/stable
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the $ signs in front of the commands, here and below. (It's the standard in the Kubeflow docs to show the commands without the dollar sign, as the dollar may be confusing.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@sarahmaddox It might best to specify the channel as 1.15 as this is listed on https://www.kubeflow.org/docs/started/k8s/overview/ as being the latest currently supported version. I didn't realise a bigger refresh of the instructions was underway, before opening a PR addressing the same thing here: #1519

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ofaz!

@ammarn911 Please do incorporate this change into your PR as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

The version (channel) should be 1.14 not 1.15 as in the first comment in this thread. I've approved PR #1519 which updates the doc to specify channel 1.14.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me as though this PR will overwrite the previous PR which set the channel to 1.14. Please would you set the channel specifically in this PR too, just to ensure it doesn't overwrite the previous one. Apart from that, this LGTM. Thanks for persevering with this change!

Copy link
Contributor

Choose a reason for hiding this comment

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

@ammarn911 could you explain how you tested MicroK8s with K8s 1.17? Last I checked there were still problems with our kustomize manifests kubeflow/manifests#375

Could you provide some verification that it is working? e.g. can your provide the output of

kubectl -n kubeflow get applications
kubectl -n kubeflow get deploy

I'd like to verify that Kubeflow is correctly deploying on 1.17.

Do we have an issue tracking adding continuous integration for Kubeflow on MicroK8s?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jlewi: MicroK8s is deploying with Juju instead of Kustomize, so not running into the 1.16 issues. The microk8s repo itself has some CI running for microk8s.enable kubeflow, if that's what you mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlewi, @sarahmaddox please let me know if there's anything else

Copy link
Contributor

Choose a reason for hiding this comment

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

@knkski @ammarn911 It might make sense to discuss this at a community meeting. Agenda is at http://bit.ly/kf-meeting-notes you should be able to comment to add it for an upcoming agenda.

I think my question is do we have a process (and or owners) for ensuring that Kubeflow on multi-pass is tested and "conformant" ? If not; how should we convey this in the docs?

If MicroK8s isn't using the application manifests in kubeflow/manifests; then that suggests all of the manifests in https://github.com/kubeflow/manifests for individual applications need to be ported to Juju. Is that really what you meant? That seems like a fairly large ongoing maintenance cost. Is that really what's happening and if so what's the plan for keeping them updated?

Looking at kubeflow/manifests#375 e.g.
kubeflow/manifests#375 (comment)

It looks like many of the issues are that the spec for K8s resources changed in 1.16 and therefore various manifests need to be updated. So kustomize is a big orthogonal. Perhaps you aren't hitting these issues simply because you aren't installing the applications causing problems?

One way to avoid blocking this PR would be to include a banner at the top of the page indicating "Alpha support" like the one we have on upgrading deployments
https://www.kubeflow.org/docs/upgrading/upgrade/

We can then figure out what is needed in order to feel confident that we can remove the banner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @jlewi, definitely makes sense to discuss in the community meeting, we'll take it there.

In the meantime, I added the Alpha Support banner to roll out the PR and update related docs.

@sarahmaddox
Copy link
Contributor

Hallo @ammarn911 Thank you for suggesting this much-needed update! I've tested the instructions on a Debian machine, and have suggested some changes to improve readability. I've also asked a question, as I ran into problems in the "enable Kubeflow" step.

@carmine If you have time to take a look that'd be excellent. If not, I'll approve this PR once I get the workflow working on my machine. That means I won't have tested the Multipass side of things.

@sarahmaddox
Copy link
Contributor

Fixes #1113

@ammarn911
Copy link
Contributor Author

Hey @sarahmaddox , thank you for reviewing! Looking into the issue, will update with a fix and resolve your comments.

Updating getting started with Kubeflow doc to address Sarah's comments.
@sarahmaddox
Copy link
Contributor

Hallo @ammarn911 while you're busy on this page, please would you take a look at the following page which also mentions Microk8s. I think it needs some adjustment too:
https://www.kubeflow.org/docs/started/workstation/getting-started-linux/#microk8s

  • Should we simply point the "Microk8s" section on the above doc to your new instructions?
  • Or is it true, as the above doc currently states, that you can install Microk8s and then "Follow the getting started guide for Kubeflow on an existing Kubernetes cluster"?

@ammarn911
Copy link
Contributor Author

ammarn911 commented Jan 29, 2020

Hallo @ammarn911 while you're busy on this page, please would you take a look at the following page which also mentions Microk8s. I think it needs some adjustment too:
https://www.kubeflow.org/docs/started/workstation/getting-started-linux/#microk8s

  • Should we simply point the "Microk8s" section on the above doc to your new instructions?
  • Or is it true, as the above doc currently states, that you can install Microk8s and then "Follow the getting started guide for Kubeflow on an existing Kubernetes cluster"?

Hey @sarahmaddox , I'll open a PR to point it to this doc once it is live. Thank you for recommending!

Adding an "Alpha Support" banner
@jlewi
Copy link
Contributor

jlewi commented Jan 29, 2020

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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:

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

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.

8 participants