ksonnet package for WeaveFlux #1232

jasonsmithio · 2018-07-18T16:39:23Z

This is v0.1 of the KubeFlow ksonnet package, enabling Weaveworks Flux on Kubeflow clusters.

This change is

jlewi · 2018-07-18T19:48:15Z

/unassign @jimexist
/unassign @wbuchwalter
/assign jlewi

jlewi · 2018-07-18T19:48:23Z

/ok-to-test

jlewi · 2018-07-18T19:49:34Z

kubeflow/kubeflux/prototypes/kubeflux.jsonnet

+
+local giturl = updatedParams.giturl;
+local namespace = updatedParams.namespace;
+//local imageTag = import "param://imageTag";


Please delete commented out code.

jlewi · 2018-07-18T19:50:00Z

kubeflow/kubeflux/prototypes/kubeflux.jsonnet

+// updatedParams uses the environment namespace if
+// the namespace parameter is not explicitly set
+local updatedParams = params {
+  namespace: if params.namespace == "null" then env.namespace else params.namespace,


You can use the env namespace always you should not take namespace as a parameter.

Think I got it

jlewi · 2018-07-18T19:51:05Z

kubeflow/kubeflux/prototypes/kubeflux.jsonnet

+// the namespace parameter is not explicitly set
+local updatedParams = params {
+  namespace: if params.namespace == "null" then env.namespace else params.namespace,
+  giturl: if params.giturl == "null" then env.giturl else params.giturl,


You can't get giturl from env. It will always come from params.
env refers to values set as part of your ksonnet enviornment like namespace.

This SHOULD be fixed

jlewi · 2018-07-18T19:52:21Z

kubeflow/kubeflux/all.libsonnet

+        "selector": {
+          "name": "flux"
+        },
+        "type": "LoadBalancer"


This would expose a port by default that's open to the world. We don't want to do this. This should be a parameter and users should have to explicitly choose to set the type to LoadBalancer in order to open up a port.

Suggestions on how to fix this? I am thinking of a

ks param set LoadBalancer Deploy

I am just not sure how the code would look in the parameters and all.libsonnet

Also, should we give them more than one option for say a proxy?

ks param set port (Proxy/LoadBalancer)?

I would probably just add a parameter for the ServiceType that defaults to ClusterIP
See here
https://github.com/kubeflow/kubeflow/blob/master/kubeflow/core/prototypes/jupyterhub.jsonnet#L8

We could probably get away with out adding port for now.

Looks like this still needs to be fixed.

We need to expose the flux service to the outside world so that the "fluxctl" CLI tool can access the tool. The LoadBalancer was to prevent a user from having to do a portforward. I think a better option may be a proxy option as the service does need to be exposed

jlewi · 2018-07-18T19:52:52Z

kubeflow/kubeflux/all.libsonnet

+      }
+    },
+
+    nodeport:: {


Why do we have two different services?

It may just be a remnant from the conversion of Flux from yaml to ksonnet. Removed

jlewi · 2018-07-18T19:53:48Z

kubeflow/kubeflux/all.libsonnet

+            "containers": [
+              {
+                "args": [
+                  "--ssh-keygen-dir=/var/fluxd/keygen",


How are keys handled? Can the user provide a deploy key via a K8s secret?

I don't see why they couldn't create an additional secret for the key.
I am looking at the documentation here: https://github.com/weaveworks/flux/blob/master/site/standalone/setup.md

There are two options
"fluxctl identity" can generate one after deployment

kubectl create secret generic flux-git-deploy --from-file=identity=/path/to/private_key

Maybe create a parameter for option two?

Right; They can create a key but they need some to specify that the provided secret should be added to the pod. So you probably need a parameter (secret name) and if that is set mount that into the pod.

There is an existing "flux-git-deploy" secret created in this process. It looks like it mounts to a volume. Should we add a secret in addition to that or is there a reason we should create a new one.

So the user has to download the key and then add it to their GITHUB repository as a deploy key? Is that documented somewhere.

So I am looking here:
https://github.com/weaveworks/flux/blob/master/site/daemon.md

and "flux-git-deploy" seems to be used to store the private SSH key as a secret for GitHub.

This documentation (https://www.weave.works/docs/cloud/latest/tasks/deploy/manual-configuration/) suggests that one use "fluxctl identity" to generate a public key while the private key is stored in "flux-git-deploy". You then go to Github and add the Key in the settings.

jlewi · 2018-07-19T00:52:42Z

Can you update the PR title please.

jlewi · 2018-07-20T14:42:52Z

Took a quick look; seems like some comments still need to be addressed. Let me know when this is ready for another look.

jasonsmithio · 2018-07-22T20:15:53Z

The comments should be removed.

jlewi

Reviewed 1 of 1 files at r5.
Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @jlewi, @thejaysmith, @wbuchwalter, and @jimexist)

kubeflow/kubeflux/all.libsonnet, line 216 at r1 (raw file):

Previously, TheJaySmith (Jay Smith) wrote…

We need to expose the flux service to the outside world so that the "fluxctl" CLI tool can access the tool. The LoadBalancer was to prevent a user from having to do a portforward. I think a better option may be a proxy option as the service does need to be exposed

Can you open an issue for this? How WeaveFlux recommend securing the exposed service when exposing it outside the cluster.

kubeflow/kubeflux/README.md, line 3 at r5 (raw file):

KubeFlux

Is there really such a thing as Kubeflux? This is just WeaveFlux deployed via ksonnet. Why introduce the term KubeFlux; Why not just call it WeaveFlux?

kubeflow/kubeflux/README.md, line 30 at r5 (raw file):

We will then setup our FLUX_URL and test fluxctl
=======
KubeFlux is an integration allowing Kubeflow users to leverage [WeaveWorks &#174; Flux]("https://www.weave.works/oss/flux/") for GitOps. KubeFlux utilizes the [stand-alone]("https://github.com/weaveworks/flux/tree/master/site/standalone") implementation. Being stand-alone, most of the maintenance is manual. If you are looking for a more managed solution, we recommend you look at [Weave Cloud &#174;]("https://www.weave.works/product/cloud/").

It looks like you are duplicating the same text as above. e.g. the description of KubeFlux and the installation guide appear to be repeated.

kubeflow/kubeflux/prototypes/kubeflux.jsonnet, line 15 at r1 (raw file):

Previously, TheJaySmith (Jay Smith) wrote…

Think I got it

Delete updatedParams you aren't using it anymore.

kubeflow/kubeflux/prototypes/kubeflux.jsonnet, line 6 at r5 (raw file):

// @shortDescription A Flux meets Kubeflow
// @param name string Name to give to each of the components
// @optionalParam namespace string null Namespace to use for the components. It is automatically inherited from the environment if not set.

Remove namespace as a parameter; namespace will be set by env.

jasonsmithio

Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @jlewi, @thejaysmith, @wbuchwalter, and @jimexist)

kubeflow/kubeflux/all.libsonnet, line 216 at r1 (raw file):

Opened this issue fluxcd/flux#1244
Let's see what results we get

jasonsmithio

Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @jlewi, @thejaysmith, @wbuchwalter, and @jimexist)

kubeflow/kubeflux/README.md, line 3 at r5 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

KubeFlux
Is there really such a thing as Kubeflux? This is just WeaveFlux deployed via ksonnet. Why introduce the term KubeFlux; Why not just call it WeaveFlux?

Was going for a fun name. I have fixed it. I am still referring to the package as kubeflux though.

kubeflow/kubeflux/README.md, line 30 at r5 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

It looks like you are duplicating the same text as above. e.g. the description of KubeFlux and the installation guide appear to be repeated.

Duplicates fixed

jasonsmithio

Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @jlewi, @wbuchwalter, and @jimexist)

kubeflow/kubeflux/README.md, line 3 at r5 (raw file):

Previously, TheJaySmith (Jay Smith) wrote…

Was going for a fun name. I have fixed it. I am still referring to the package as kubeflux though.

Done.

kubeflow/kubeflux/README.md, line 30 at r5 (raw file):

Previously, TheJaySmith (Jay Smith) wrote…

Duplicates fixed

Done.

kubeflow/kubeflux/prototypes/kubeflux.jsonnet, line 15 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Delete updatedParams you aren't using it anymore.

Done.

kubeflow/kubeflux/prototypes/kubeflux.jsonnet, line 6 at r5 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Remove namespace as a parameter; namespace will be set by env.

Done.

jlewi · 2018-07-24T04:33:36Z

Jay it looks like the package is still named Kubeflux and not WeaveFlux. While I do like the name Kubeflux I think it will lead to confusion and we should just call it WeaveFlux because that's what it is.

Also Can rebase off of master and squash your changes. This will pull in the latest changes on master which should pull in some fixes to the E2E tests.

You will also need to run scripts/autoformat_jsonnet.sh to autoformat the jsonnet.

jasonsmithio

autoformat_jsonnet
I went ahead and changed "KubeFlux" to "WeaveFlux" and the package is now "weaveflux". I also squashed the commits.

I ran scripts/autoformat_jsonnet.sh so it should have worked but we will see. If it doesn't work, best practices for using it?

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @jlewi, @wbuchwalter, and @jimexist)

jlewi · 2018-07-25T04:51:58Z

@thejaysmith It looks like the autoformat worked. But it doesn't look like you rebased off the latest code of master because the minikube and tf serving tests are running. If you do that it will probably fix the tests.

jlewi · 2018-07-25T04:53:14Z

kubeflow/weaveflux/README.md

@@ -0,0 +1,38 @@
+# KubeFlux


you missed this "Kubeflox"

jasonsmithio · 2018-07-25T12:30:41Z

Hello,

I rebased and removed all KubeFlux!

googlebot · 2018-07-31T17:37:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-31T17:42:04Z

CLAs look good, thanks!

jlewi · 2018-08-01T12:59:13Z

/retest

jlewi

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @jlewi, @wbuchwalter, and @jimexist)

jlewi · 2018-08-01T13:00:36Z

/lgtm

googlebot · 2018-08-07T14:31:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

jasonsmithio · 2018-08-07T14:38:14Z

/retest

rebase and fix Updated Commit rebased rebase and fix

googlebot · 2018-08-07T16:49:10Z

CLAs look good, thanks!

jlewi · 2018-08-08T20:57:45Z

/lgtm
/approve

k8s-ci-robot · 2018-08-08T20:57:55Z

[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:

~~OWNERS~~ [jlewi]

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

* rebase and fix rebase and fix Updated Commit rebased rebase and fix * rebase and fix rebase and fix Updated Commit rebased rebase and fix * fixed format * autoformat all.libsonnet

* UI improvements * Add db-manager-addr flag Modify README * Fix graphviz * Modify README * Remove ID * Add zoom to graphviz * Remove unused scripts from index * Fix doc * Change npm install to npm ci Commit package-lock * fix npm version * Increase max_old_space_size * Set react-scripts to 3.2.0 * Modify doc

k8s-ci-robot requested review from jimexist and wbuchwalter July 18, 2018 16:39

k8s-ci-robot added size/L needs-ok-to-test labels Jul 18, 2018

jasonsmithio mentioned this pull request Jul 18, 2018

Enable GitOps: Use (Weave Flux or Argo CD) to manage Kubeflow deployments #971

Closed

k8s-ci-robot assigned jlewi Jul 18, 2018

k8s-ci-robot removed the needs-ok-to-test label Jul 18, 2018

jlewi reviewed Jul 18, 2018

View reviewed changes

jasonsmithio changed the title ~~commit for v0.1~~ commit for KubeFlux Jul 19, 2018

jlewi changed the title ~~commit for KubeFlux~~ ksonnet package for KubeFlux Jul 20, 2018

jlewi suggested changes Jul 23, 2018

View reviewed changes

jasonsmithio commented Jul 23, 2018

View reviewed changes

jasonsmithio commented Jul 24, 2018

View reviewed changes

jlewi reviewed Jul 25, 2018

View reviewed changes

kubeflow/weaveflux/README.md Outdated

@@ -0,0 +1,38 @@

# KubeFlux

Copy link

Contributor

jlewi Jul 25, 2018

Choose a reason for hiding this comment

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

you missed this "Kubeflox"

jasonsmithio changed the title ~~ksonnet package for KubeFlux~~ ksonnet package for WeaveFlux Jul 25, 2018

k8s-ci-robot added approved and removed lgtm labels Jul 27, 2018

jlewi approved these changes Aug 1, 2018

View reviewed changes

k8s-ci-robot added lgtm size/XL and removed lgtm size/L labels Aug 1, 2018

googlebot added the cla: no label Aug 7, 2018

jasonsmithio added 2 commits August 7, 2018 12:28

rebase and fix

0101f42

rebase and fix Updated Commit rebased rebase and fix

rebase and fix

b75b49f

rebase and fix Updated Commit rebased rebase and fix

googlebot added cla: yes and removed cla: no labels Aug 7, 2018

k8s-ci-robot added size/L and removed size/XL labels Aug 7, 2018

jasonsmithio added 3 commits August 7, 2018 13:22

fixed format

6030a29

merge

f6df700

autoformat all.libsonnet

5fe8a07

k8s-ci-robot added the lgtm label Aug 8, 2018

k8s-ci-robot merged commit 58b3b61 into kubeflow:master Aug 8, 2018

ksonnet package for WeaveFlux #1232

ksonnet package for WeaveFlux #1232

Conversation

jasonsmithio commented Jul 18, 2018 • edited by jlewi Loading

jlewi commented Jul 18, 2018

jlewi commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlewi commented Jul 19, 2018

jlewi commented Jul 20, 2018 • edited Loading

jasonsmithio commented Jul 22, 2018

jlewi left a comment

Choose a reason for hiding this comment

jasonsmithio left a comment

Choose a reason for hiding this comment

jasonsmithio left a comment

Choose a reason for hiding this comment

jasonsmithio left a comment

Choose a reason for hiding this comment

jlewi commented Jul 24, 2018

jasonsmithio left a comment

Choose a reason for hiding this comment

jlewi commented Jul 25, 2018

Choose a reason for hiding this comment

jasonsmithio commented Jul 25, 2018

googlebot commented Jul 31, 2018

googlebot commented Jul 31, 2018

jlewi commented Aug 1, 2018

jlewi left a comment

Choose a reason for hiding this comment

jlewi commented Aug 1, 2018

googlebot commented Aug 7, 2018

jasonsmithio commented Aug 7, 2018

googlebot commented Aug 7, 2018

jlewi commented Aug 8, 2018

k8s-ci-robot commented Aug 8, 2018

jasonsmithio commented Jul 18, 2018 •

edited by jlewi

Loading

jlewi commented Jul 20, 2018 •

edited

Loading