[kfctl web app] frontend & backend update to agree with api changes #1372

kunmingg · 2018-08-15T23:13:35Z

Update API calls to backend service.
Now all manual steps are covered by backend:

Update:

one-click-to-deploy works now on any projects.
use DM config based on the one used by kfctl.
backend security enhancement: make sure auth token only shorted lived in memory.
ingress compatibility: simplify ingress rule & handle health check.

Follow up:

(recommended) auto redirect when detect that IAP is up.
(optional) polish UI elements

This change is

yebrahim · 2018-08-16T05:13:53Z

components/gcp-click-to-deploy/src/DeployForm.tsx

+      {
+        body: JSON.stringify(
+          {
+            Msg: 'Echo from web app?',


I would prefer the communication with the Go backend is transparent to the user. Echoes would seem strange here, we can just display a message saying "Could not reach server" or something.

yebrahim · 2018-08-16T05:15:45Z

components/gcp-click-to-deploy/src/DeployForm.tsx

+          const msg = JSON.parse(response.body).Reply;
+          this._appendLine('From Backend' + msg);
+        } else {
+          this._appendLine('Backend error: ' + response.statusCode);


We should probably fail the deployment at this point, no? What's the point of continuing if we won't be able to pass on the token and create the service account key, etc?

yebrahim · 2018-08-16T05:22:38Z

components/gcp-click-to-deploy/src/DeployForm.tsx

+      },
+      (error, response, body) => {
+        if (!error) {
+          const msg = JSON.parse(response.body).Reply;


Let's always wrap JSON.parse calls in a try/catch, especially since we don't have string API typings (protos or such). If the server decides to return something else here, the client should handle this gracefully by showing an error dialog, instead of things just stopping to work.

yebrahim · 2018-08-16T05:23:28Z

components/gcp-click-to-deploy/src/DeployForm.tsx

    do {
      getAttempts++;
+      await wait(getTimeout);


Why move the wait up here? We should only wait after each call fails right? I think that's what the old code does.

We want to always wait:
even call succeeded, if the returned status is "RUNNING", we will still wait for 20s.
So in total we wait up to 20*30 (10 min), till deployment manager successfully set up every thing (usually finish within 5 min).

yebrahim · 2018-08-16T05:25:40Z

components/gcp-click-to-deploy/src/DeployForm.tsx

        continue;
      }
      status = curStatus.operation!.status!;
-    } while (status !== 'DONE' && getAttempts < 20);
+      this._appendLine(`Status of ${deploymentName}: ` + status);
+    } while (status !== 'DONE' && getAttempts < 30);



We should add one more check on getAttempts after the while loop is done. If it's equal to 30, that means we've waited too long, and we should show a suitable timeout error message.

yebrahim · 2018-08-16T05:26:07Z

components/gcp-click-to-deploy/src/DeployForm.tsx

        continue;
      }
      status = curStatus.operation!.status!;
-    } while (status !== 'DONE' && getAttempts < 20);
+      this._appendLine(`Status of ${deploymentName}: ` + status);
+    } while (status !== 'DONE' && getAttempts < 30);


We need to check other values of the deployment status, and exit the loop with an error if the deployment failed for example.

kunmingg · 2018-08-17T00:18:44Z

/hold
Will update DM configs and add apply call.

jlewi · 2018-08-17T04:50:26Z

Thanks.

jlewi

Reviewable status: 0 of 10 files reviewed, 8 unresolved discussions (waiting on @yebrahim, @kunmingg, @pdmack, @lluunn, and @jlewi)

bootstrap/cmd/bootstrap/app/ksServer.go, line 569 at r2 (raw file):

		defer a.mux.Unlock()

		roleBinding := v1.ClusterRoleBinding{

Can you add a TODO to create a K8s yaml file in the app directory that defines the cluster role binding?

components/gcp-click-to-deploy/src/configs/cluster.jinja, line 25 at r2 (raw file):

#}
{% set TYPE_NAME = NAME_PREFIX + '-type' %}
{% set NAMESPACE_COLLECTION = '/api/v1/namespaces' %}

Do we still need a copy of the DM configs in cluster gcp-click-to-deploy-app?

I think we use to be loading them into the frontend and doing the DM substitution in the JS frontend; but I thought that's no longer the case.

Isn't the go backend doing the substitution now?

Can the go backend download the DM configs from the GitHub URL?

If we need a copy of the DM configs, can we just have the build process copy the existing configs into the container so we don't have have to worry about them getting out of sync?

kunmingg · 2018-08-22T07:28:13Z

@jlewi
About DM configs:
Because our priority is making deploy app works properly, this PR focus on making everything works.
(Tested and now the app works properly).
So take a shortcut here to unblock @lluunn 's web app update.

DM config update is the only piece left on frontend side, will migrate to backend, and merge with script's DM config in following PRs of this sprint.

jlewi · 2018-08-22T12:32:02Z

Makes sense thanks.

/lgtm
/approve

This looks good on my end.

@yebrahim Any additional comments?

/hold.

jlewi · 2018-08-22T23:34:17Z

Looks like an Argo test failure
/test all

yebrahim · 2018-08-23T04:06:14Z

components/gcp-click-to-deploy/src/DeployForm.tsx

+    do {
+      initAttempts++;
+      await wait(initTimeout);
+    } while (!readiness && initAttempts < 10);


How does this work? readiness isn't being changed in the loop body. This will always either wait for 10*2000 ms or not wait at all.

readiness is changed in previous call back function, here the loop is to make sure call back updated the status.

If you're awaiting the request call (line 426), then the next code block will execute only when that call is finished and the callback is executed. It seems to me what you're trying to do here is run the request in parallel to this code block (starting line 456), which isn't how you should do this.

In fact, I just noticed the request library isn't installed as a dependency, only it's typings. This should not be working.
You can just use the built-in fetch API. It's supported by all major browsers.

Which one is not working? This PR has been deployed to kubeflow.dev

This code that calls request(... has not been deployed to kubeflow.dev. It's been a while since there's been a deployment as far as I'm aware.

The code I meant by not working is the call using request, that library isn't installed as part of the app, and this call will fail if the execution gets to it.

We deployed this PR earlier on a new domain name. request seems works.

Seems request pkg was installed via @kubernetes/client-node as a dependency, but we don't call k8s from js any more. Let me clean up unused dependencies.

I see, that makes sense then, although we should explicitly install request as a dependency rather than rely on its existence by chance.

Can you send me the new domain name?

My original comment is still valid I believe. If you want to wait for the request to come back use a Promise (either using request or using the build-in fetch), rather than manually wait in a loop.

new address is: kfdeploy.kubeflow.dev
merge PR to unblock @lluunn
Will use promise in following PRs.

yebrahim · 2018-08-23T04:08:19Z

Just the one comment. Otherwise
/lgtm

kunmingg · 2018-08-24T20:45:28Z

/retest

…hrough bash

kunmingg · 2018-08-24T22:20:47Z

/retest

jlewi · 2018-08-25T22:38:39Z

@kunmingg Has @yebrahim's comment about the request package been addressed? I looked and couldn't tell if the dependencies had been cleaned up.

I'll lgtm it and if there are no more changes needed you can just cancel the hold

/lgtm
/approve
/hold

k8s-ci-robot · 2018-08-25T22:38:41Z

[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

pdmack · 2018-08-26T15:32:37Z

/unassign @pdmack

lluunn · 2018-08-29T22:23:41Z

/lgtm

kunmingg · 2018-08-29T22:25:56Z

/hold cancel

kunmingg · 2018-08-29T22:51:21Z

/retest

…ubeflow#1372) * frontend update to integrate with backend api change * handle review feedbacks * more frontend & backend api update; deployment manager config update * update image name; update UI default value * secuirty update; use ksonnet api to apply components instead of run through bash * edit path for ingress * update for ingress rules * clean up js dependencies; update GKE version

* Change kubeflow-images-public to kubeflowkatib * Remove kubeflow-images-public from setup-katib script * Add test for early stopping * Fix early stopping settings * Add tags to all examples * Fix Katib config * Fix empty var in UI * Fix comment * Trigger Travis

…rom the secret (kubeflow#1372) * refactor: pipelines profile controller should get minio access keys from the secret * do not print secrets in log

kunmingg requested review from jlewi and yebrahim August 15, 2018 23:13

k8s-ci-robot requested review from lluunn and pdmack August 15, 2018 23:13

googlebot added the cla: yes label Aug 15, 2018

k8s-ci-robot added the size/M label Aug 15, 2018

yebrahim reviewed Aug 16, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold label Aug 17, 2018

kunmingg force-pushed the frontend branch from 24c9a40 to e2d6ea3 Compare August 21, 2018 22:58

k8s-ci-robot added size/XXL and removed size/M labels Aug 21, 2018

jlewi suggested changes Aug 22, 2018

View reviewed changes

k8s-ci-robot assigned jlewi Aug 22, 2018

k8s-ci-robot added lgtm approved and removed lgtm labels Aug 22, 2018

yebrahim reviewed Aug 23, 2018

View reviewed changes

k8s-ci-robot assigned yebrahim Aug 23, 2018

k8s-ci-robot added lgtm and removed lgtm labels Aug 23, 2018

kunmingg changed the title ~~[gcp-deployer] frontend update to integrate with backend api change~~ [kfctl web app] frontend & backend update to agree with api changes Aug 24, 2018

kunmingg added 2 commits August 24, 2018 14:55

frontend update to integrate with backend api change

2cefeae

handle review feedbacks

cb92631

kunmingg added 5 commits August 24, 2018 14:55

more frontend & backend api update; deployment manager config update

683c8e0

update image name; update UI default value

5bc5a2a

secuirty update; use ksonnet api to apply components instead of run t…

2b0835e

…hrough bash

edit path for ingress

b4aff36

update for ingress rules

c1a7f72

kunmingg force-pushed the frontend branch from 7f8237e to c1a7f72 Compare August 24, 2018 21:56

jlewi approved these changes Aug 25, 2018

View reviewed changes

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

k8s-ci-robot removed the request for review from pdmack August 26, 2018 15:32

k8s-ci-robot removed the lgtm label Aug 29, 2018

clean up js dependencies; update GKE version

7ee2c77

kunmingg force-pushed the frontend branch from bc51ff0 to 7ee2c77 Compare August 29, 2018 22:23

k8s-ci-robot assigned lluunn Aug 29, 2018

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

k8s-ci-robot removed the do-not-merge/hold label Aug 29, 2018

k8s-ci-robot merged commit b4c341c into kubeflow:master Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kfctl web app] frontend & backend update to agree with api changes #1372

[kfctl web app] frontend & backend update to agree with api changes #1372

kunmingg commented Aug 15, 2018 •

edited

yebrahim Aug 16, 2018

yebrahim Aug 16, 2018

yebrahim Aug 16, 2018

yebrahim Aug 16, 2018

kunmingg Aug 16, 2018

yebrahim Aug 16, 2018

yebrahim Aug 16, 2018

kunmingg commented Aug 17, 2018

jlewi commented Aug 17, 2018

jlewi left a comment

kunmingg commented Aug 22, 2018

jlewi commented Aug 22, 2018

jlewi commented Aug 22, 2018

yebrahim Aug 23, 2018

kunmingg Aug 24, 2018

yebrahim Aug 24, 2018

yebrahim Aug 24, 2018

kunmingg Aug 24, 2018

yebrahim Aug 24, 2018

kunmingg Aug 25, 2018

kunmingg Aug 25, 2018

yebrahim Aug 27, 2018 •

edited

kunmingg Aug 29, 2018

yebrahim commented Aug 23, 2018

kunmingg commented Aug 24, 2018

kunmingg commented Aug 24, 2018

jlewi commented Aug 25, 2018

k8s-ci-robot commented Aug 25, 2018

pdmack commented Aug 26, 2018 •

edited

lluunn commented Aug 29, 2018

kunmingg commented Aug 29, 2018

kunmingg commented Aug 29, 2018

[kfctl web app] frontend & backend update to agree with api changes #1372

[kfctl web app] frontend & backend update to agree with api changes #1372

Conversation

kunmingg commented Aug 15, 2018 • edited

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

kunmingg commented Aug 17, 2018

jlewi commented Aug 17, 2018

jlewi left a comment

Choose a reason for hiding this comment

kunmingg commented Aug 22, 2018

jlewi commented Aug 22, 2018

jlewi commented Aug 22, 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

yebrahim Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yebrahim commented Aug 23, 2018

kunmingg commented Aug 24, 2018

kunmingg commented Aug 24, 2018

jlewi commented Aug 25, 2018

k8s-ci-robot commented Aug 25, 2018

pdmack commented Aug 26, 2018 • edited

lluunn commented Aug 29, 2018

kunmingg commented Aug 29, 2018

kunmingg commented Aug 29, 2018

kunmingg commented Aug 15, 2018 •

edited

yebrahim Aug 27, 2018 •

edited

pdmack commented Aug 26, 2018 •

edited