armadaproject / armada-operator

Apache License 2.0
13 stars 10 forks source link

feat: added extraInitCommand to lookout migration jobs. #245

Closed sync-by-unito[bot] closed 1 year ago

sync-by-unito[bot] commented 1 year ago

When deploying on a cluster with istio injection enabled, the istio sidecar does not detect that a job has finished, and keeps the pod alive and unready forever. Running with

extraInitCommand: "curl -fsI -X POST http://localhost:15020/quitquitquit ;"

the job completes cleanly.

┆Issue is synchronized with this Jira Task by Unito

sync-by-unito[bot] commented 1 year ago

➤ Geoffrey Wilson commented:

It might be better to separate this into one or more InitContainers. For instance, for LookoutMigration we have InitContainers config: https://github.com/armadaproject/armada/blob/b7473e0e1443d6bdf759c1d5a9b60c728bf3b206/deployment/lookout-migration/templates/job.yaml

EDIT scratch that, I understand you need to run after the container succeeds, not before

sync-by-unito[bot] commented 1 year ago

➤ Wessel Valkenburg (prevue.ch) commented:

suprjinx Thanks for the suggestion and the edit. Indeed, this runs after the job. Before I saw your edit, I looked up these two examples, which suggest that this method is common practice (if it is not the only solution): https://github.com/minio/minio/blob/master/helm/minio/templates/post-job.yaml#L128 https://github.com/apache/pulsar-helm-chart/blob/master/charts/pulsar/templates/pulsar-cluster-initialize.yaml#L113

Either way, this PR is just a suggestion, I figured it's faster to just submit the PR so the code can be discussed, rather than creating an issue and spending time on explaining in words what is clearer in code.

kannon92 Thanks for asking, on of your colleagues reached out to me on LinkedIn, I'll try to have a call with them to exchange ideas. The brief answer is that I plan to use Armada in the way it is intended to use, for large ML parameter searches.

sync-by-unito[bot] commented 1 year ago

➤ Kevin Hannon commented:

Yep! I’m not as familiar with Istio so I wonder if there are other issues if you try to use it with Armada.

And great! Please don’t be a stranger on your experience using Armada. We are on the Armada channel under CNCF if you have any questions.

sync-by-unito[bot] commented 1 year ago

➤ C~+ commented:

@valkenburg-prevue-ch Here's the link to the CNCF Slack #armada channel - https://cloud-native.slack.com/archives/C03T9CBCEMC

sync-by-unito[bot] commented 1 year ago

➤ codecov[bot] commented:

Codecov ( https://codecov.io/gh/armadaproject/armada/pull/2105?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject ) ReportPatch and project coverage have no change.

Comparison is base (0d0aa09) ( https://codecov.io/gh/armadaproject/armada/commit/0d0aa097f61d9bfedaefdc3847a588225a79783e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject ) 58.55% compared to head (cdd2a20) ( https://codecov.io/gh/armadaproject/armada/pull/2105?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject ) 58.55%.

Additional details and impacted files @@ Coverage Diff @@ 1. master #2105 +/- ## ======================================= Coverage 58.55% 58.55% ======================================= Files 227 227 Lines 28495 28495 ======================================= Hits 16684 16684 Misses 10498 10498 Partials 1313 1313 Flag Coverage Δ armada-server 58.55% <ø> (ø) Flags with carried forward coverage won't be shown. Click here ( https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject#carryforward-flags-in-the-pull-request-comment ) to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us ( https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject ). Have a feature suggestion? Share it here. ( https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject )

:umbrella: View full report in Codecov by Sentry ( https://codecov.io/gh/armadaproject/armada/pull/2105?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject ).

:loudspeaker: Do you have feedback about the report comment? Let us know in this issue ( https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=armadaproject ).

sync-by-unito[bot] commented 1 year ago

➤ Wessel Valkenburg (prevue.ch) commented:

Just to report on a related topic: pulsar can run fine in an istio mesh, and the easiest way is to move to the streamnative pulsar operator for deploying a pulsar cluster. See this discussion ( https://github.com/apache/pulsar/discussions/19703#discussioncomment-5250906 ).

sync-by-unito[bot] commented 1 year ago

➤ Kevin Hannon commented:

Just to report on a related topic: pulsar can run fine in an istio mesh, and the easiest way is to move to the streamnative pulsar operator for deploying a pulsar cluster. See this discussion ( https://github.com/apache/pulsar/discussions/19703#discussioncomment-5250906 ).

Interesting. Thank you for following up with the pulsar community. Would you mind creating an issue on the armada board to suggest a migration to the streamnative pulsar operator?