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

Add support for sidecars #42

Merged
merged 2 commits into from
Mar 21, 2020
Merged

Add support for sidecars #42

merged 2 commits into from
Mar 21, 2020

Conversation

fenglixa
Copy link
Member

@fenglixa fenglixa commented Mar 20, 2020

Fixes #37
After the fixes, compiled sidecars tekton pipeline could be ran successful:

Name:           sidecar-run-lkg6w
Namespace:      default
Pipeline Ref:   sidecar

🌡️  Status

STARTED          DURATION   STATUS
14 minutes ago   1 minute   Succeeded

📦 Resources

 No resources

⚓ Params

 No params

🗂  Taskruns

 NAME                                 TASK NAME   STARTED          DURATION     STATUS
 ∙ sidecar-run-lkg6w-echo-7qcqc       echo        14 minutes ago   20 seconds   Succeeded
 ∙ sidecar-run-lkg6w-download-tf88f   download    14 minutes ago   36 seconds   Succeeded

This change is Reviewable

@fenglixa
Copy link
Member Author

/retest

@ckadner
Copy link
Member

ckadner commented Mar 20, 2020

Thanks for your PR @fenglixa.

The sidecar use case looks good, but could please separate out the second with_param use case into a separate PR? LoopArguments and ParallelFor should be tackled independently.

@ckadner ckadner mentioned this pull request Mar 20, 2020
27 tasks
@fenglixa
Copy link
Member Author

@ckadner, Great thanks for your reivew comments. I addressed them, please help check whether it's OK for your now?

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

/lgtm

@ckadner
Copy link
Member

ckadner commented Mar 20, 2020

Thank you @fenglixa this is great!

@ckadner
Copy link
Member

ckadner commented Mar 20, 2020

just for reference, this is the output from my pipeline run:

tkn pipeline start sidecar --showlog:

Pipelinerun started: sidecar-run-v8jt7
Waiting for logs to be available...
[download : download] Connecting to localhost:5678 (127.0.0.1:5678)
[download : download] saving to '/tekton/results/downloaded'
[download : download] downloaded           100% |********************************|    14  0:00:00 ETA
[download : download] '/tekton/results/downloaded' saved

[download : sidecar-echo] 2020/03/20 08:20:46 Server is listening on :5678
[download : sidecar-echo] 2020/03/20 08:20:58 localhost:5678 127.0.0.1:47932 "GET / HTTP/1.1" 200 14 "Wget" 13.619µs
[download : sidecar-echo] 2020/03/20 08:20:59 [ERR] Unknown signal terminated
[echo : echo] hello world

@ckadner
Copy link
Member

ckadner commented Mar 20, 2020

/assign @animeshsingh

@fenglixa
Copy link
Member Author

@ckadner, good catch, acturally, I also noticed this output, but after investigated, it is an known issue of current implementation of tekton pipeline for sidecars, which has no any function impacts.
It was mentioned in the readme of tekton pipeline:
https://github.com/tektoncd/pipeline/blob/master/docs/developers/README.md
With the content of:

There are known issues with the existing implementation of sidecars:

  • When the nop image does provide the sidecar's command, the sidecar will continue to run even after nop has been swapped into the sidecar container's image field. See the issue tracking this bug for the issue tracking this bug. Until this issue is resolved the best way to avoid it is to avoid overriding the nop image when deploying the tekton controller, or ensuring that the overridden nop image contains as few commands as possible.
  • kubectl get pods will show a Completed pod when a sidecar exits successfully but an Error when the sidecar exits with an error. This is only apparent when using kubectl to get the pods of a TaskRun, not when describing the Pod using kubectl describe pod ... nor when looking at the TaskRun, but can be quite confusing.

@fenglixa
Copy link
Member Author

@animeshsingh, could you help review? I may have another PR which is based on this fixes. Thanks very much!

@ckadner ckadner removed their assignment Mar 20, 2020
@animeshsingh
Copy link
Collaborator

captured the output comments in an issue to come back to
#44

@animeshsingh
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot
Copy link

[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

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 removed the lgtm label Mar 21, 2020
@k8s-ci-robot
Copy link

@fenglixa: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@fenglixa
Copy link
Member Author

fenglixa commented Mar 21, 2020

Resolved the conflicts for compiler_tests.py
@animeshsingh Could you provide /lgtm lable again for merging it? Thanks

@fenglixa fenglixa requested a review from ckadner March 21, 2020 01:01
@animeshsingh
Copy link
Collaborator

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 9be5a87 into kubeflow:master Mar 21, 2020
@fenglixa
Copy link
Member Author

Thanks @animeshsingh

@fenglixa fenglixa deleted the support_sidecar branch March 21, 2020 01:17
ckadner pushed a commit to ckadner/kfp-tekton that referenced this pull request Mar 25, 2020
gmfrasca pushed a commit to gmfrasca/data-science-pipelines-tekton that referenced this pull request Oct 3, 2022
feat(frontend|backend): Assign reqeuest/limit hardware specs on ML Pipelines deployments
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.

Add support for sidecars
4 participants