Add proxy agent runner for kubeflow pipeline #988

IronPan · 2019-03-19T01:18:46Z

This is the initial change to add a proxy agent for a kubeflow pipeline cluster.

The container should be launched as a K8s Deployment in the same namespace that kubeflow is deployed.

The agent runner will register to the proxy server and store the metadata in a ConfigMap. In a failover case that the agent die, the new agent should pick up the metadata from the ConfigMap.

This change is

IronPan · 2019-03-19T01:18:55Z

/assign @ojarjur

IronPan · 2019-03-19T15:30:15Z

/test kubeflow-pipeline-e2e-test

IronPan · 2019-03-19T17:39:37Z

/assign @vicaire @hongye-sun @paveldournov

IronPan · 2019-03-19T18:36:28Z

/test kubeflow-pipeline-e2e-test

IronPan · 2019-03-19T18:42:42Z

/assign @abdallag

k8s-ci-robot · 2019-03-19T18:42:43Z

@IronPan: GitHub didn't allow me to assign the following users: abdallag.

Note that only kubeflow members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @abdallag

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.

k8s-ci-robot · 2019-03-19T18:42:43Z

@IronPan: GitHub didn't allow me to assign the following users: abdallag.

Note that only kubeflow members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @abdallag

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.

hongye-sun

The PR looks good to me. Just want to clarify several questions for the context of the PR.

proxy/Dockerfile

hongye-sun · 2019-03-19T19:39:40Z

proxy/attempt-register-vm-on-proxy.sh

@@ -0,0 +1,80 @@
+#!/bin/bash
+#
+# Copyright 2017 Google Inc.


hongye-sun · 2019-03-19T19:40:01Z

proxy/Dockerfile

@@ -0,0 +1,21 @@
+# Ping to a specific version of invert proxy agent


nit: add license header.

proxy/Dockerfile

proxy/attempt-register-vm-on-proxy.sh

hongye-sun · 2019-03-19T19:56:06Z

proxy/get_proxy_url.py

+      status_code = requests.head(url).status_code
+    except requests.ConnectionError:
+      pass
+    expected_codes = frozenset([307, 401])


Not quite sure why those 2 codes are expected, especially for unauthorized.

thanks! 307 is legit. 401 is not good. I'll remove it.

proxy/get_proxy_url.py

hongye-sun · 2019-03-20T01:08:06Z

proxy/get_proxy_url.py

+    except requests.ConnectionError:
+      pass
+    expected_codes = frozenset([307])
+    # 307 - Temporary Redirect, Proxy server sends this if VM has access rights.


nit: it's still weird that the endpoint only returns 307 as normal response status and not 200. Could you confirm if it's the case with the Jaas team?

yeah i double checked with slava earlier and this is intended behavior.

3xx codes are pretty normal.
Codes < 400 describe success.

hongye-sun

/lgtm

IronPan · 2019-03-20T06:48:35Z

/test kubeflow-pipeline-e2e-test

IronPan · 2019-03-20T15:55:46Z

/approve

k8s-ci-robot · 2019-03-20T15:55:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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~~ [IronPan]

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 · 2019-03-20T15:56:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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~~ [IronPan]

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

IronPan · 2019-03-20T15:56:54Z

/hold

IronPan · 2019-03-20T15:57:15Z

wait for @ojarjur review

Ark-kun · 2019-03-20T20:43:30Z

proxy/get_proxy_url.py

+  raise ValueError("No working URL found")
+
+
+main()


Usually the main() call is put inside

if __name__ == '__main__':

This way the module can be loaded and used like a library.

Ark-kun · 2019-03-20T20:44:22Z

proxy/get_proxy_url.py

+      status_code = requests.head(url).status_code
+    except requests.ConnectionError:
+      pass
+    expected_codes = frozenset([307])


JFYI, the plain set also exists.
Nothing wrong with frozenset (immutable) though.

ack. thanks

Ark-kun · 2019-03-20T20:45:05Z

proxy/get_proxy_url.py

+"""CLI tool that returns URL of the proxy for particular zone and version."""
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function


JFYI, These imports are probably only needed for python 2.

indeed. removed

Ark-kun · 2019-03-20T20:51:11Z

Some general notes:

Is the root directory the best place for this program? Maybe we can have something like deployment directory and put all deployment-related files there.
From user's perspective, how are they supposed to use/launch this?

ojarjur

I feel like there is enough logic in the get_proxy_url.py module to justify adding unit tests for it; at least something that exercises the url_for_zone method.

ojarjur · 2019-03-19T19:40:42Z

proxy/Dockerfile

@@ -0,0 +1,21 @@
+# Ping to a specific version of invert proxy agent


s/Ping/Pin/

IronPan · 2019-03-21T23:59:44Z

added the tests as @ojarjur suggested.

Is the root directory the best place for this program? Maybe we can have something like deployment directory and put all deployment-related files there.

I think adding to root is fine. Pipeline won't have its own deployment, and i'm considering moving this code to kubeflow repo once stable. Hosting here just for faster release.

From user's perspective, how are they supposed to use/launch this?

It's launched automatically as part of deployment. see kubeflow/kubeflow#2746

Ark-kun · 2019-03-22T19:17:50Z

/lgtm

IronPan · 2019-03-22T19:32:29Z

/hold cancel

IronPan added 2 commits March 18, 2019 18:14

add proxy runner

60c5621

Merge remote-tracking branch 'upstream/master' into proxy

24503a4

k8s-ci-robot requested review from jlewi and vicaire March 19, 2019 01:18

k8s-ci-robot added size/L labels Mar 19, 2019

k8s-ci-robot assigned ojarjur Mar 19, 2019

update comments

ffc9e63

k8s-ci-robot assigned hongye-sun, paveldournov and vicaire Mar 19, 2019

IronPan mentioned this pull request Mar 19, 2019

add proxy agent image to cloud builder #996

Merged

hongye-sun reviewed Mar 19, 2019

View reviewed changes

address comments

949db59

hongye-sun reviewed Mar 20, 2019

View reviewed changes

k8s-ci-robot added the lgtm label Mar 20, 2019

hongye-sun reviewed Mar 20, 2019

View reviewed changes

k8s-ci-robot added the approved label Mar 20, 2019

k8s-ci-robot added the do-not-merge/hold label Mar 20, 2019

Ark-kun reviewed Mar 20, 2019

View reviewed changes

ojarjur reviewed Mar 20, 2019

View reviewed changes

add tests

a2e935c

k8s-ci-robot removed lgtm labels Mar 21, 2019

k8s-ci-robot assigned Ark-kun Mar 22, 2019

k8s-ci-robot added the lgtm label Mar 22, 2019

k8s-ci-robot removed do-not-merge/hold labels Mar 22, 2019

k8s-ci-robot merged commit 44f6f93 into master Mar 22, 2019

IronPan deleted the proxy branch March 27, 2019 17:54

IronPan restored the proxy branch April 5, 2019 01:23

IronPan deleted the proxy branch May 30, 2019 07:16

Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023

test(kfp): Upgrade to 1.8.0-rc.3 (kubeflow#988)

8fae9ff

		@@ -0,0 +1,21 @@
		# Ping to a specific version of invert proxy agent

Add proxy agent runner for kubeflow pipeline #988

Add proxy agent runner for kubeflow pipeline #988

Conversation

IronPan commented Mar 19, 2019 • edited by jlewi Loading

IronPan commented Mar 19, 2019

IronPan commented Mar 19, 2019

IronPan commented Mar 19, 2019

IronPan commented Mar 19, 2019

IronPan commented Mar 19, 2019

k8s-ci-robot commented Mar 19, 2019

k8s-ci-robot commented Mar 19, 2019

hongye-sun left a 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

hongye-sun left a comment

Choose a reason for hiding this comment

IronPan commented Mar 20, 2019

IronPan commented Mar 20, 2019

k8s-ci-robot commented Mar 20, 2019

k8s-ci-robot commented Mar 20, 2019

IronPan commented Mar 20, 2019

IronPan commented Mar 20, 2019

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

Ark-kun commented Mar 20, 2019

ojarjur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IronPan commented Mar 21, 2019 • edited Loading

Ark-kun commented Mar 22, 2019

IronPan commented Mar 22, 2019

IronPan commented Mar 19, 2019 •

edited by jlewi

Loading

IronPan commented Mar 21, 2019 •

edited

Loading