Skip to content

Migrate to public ECR#1256

Merged
google-oss-robot merged 12 commits intokubeflow:masterfrom
PatrickXYS:migrate
Mar 19, 2021
Merged

Migrate to public ECR#1256
google-oss-robot merged 12 commits intokubeflow:masterfrom
PatrickXYS:migrate

Conversation

@PatrickXYS
Copy link
Member

@PatrickXYS PatrickXYS commented Mar 17, 2021

Resolves #1205

It's still WIP, not familiar with jsonnet syntax, and it's has been deprecated years...

There's requirement to migrate from jsonnet-generated workflow to python-generated workflow, otherwise, it's difficult to maintain.

/hold
Hold until test succeed

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Not familiar with jsonnet either so I'll defer this to others to review.

@coveralls
Copy link

coveralls commented Mar 17, 2021

Coverage Status

Coverage remained the same at 71.479% when pulling d08b364 on PatrickXYS:migrate into 2ba261f on kubeflow:master.

@PatrickXYS
Copy link
Member Author

Looks like it failed for https://github.com/kubeflow/tf-operator/blob/047d6af30e5a6a26c5a5a51500a1cdface418ac3/sdk/python/test/test_e2e.py

Client get timeout for requests, don't know why it happen. Retest to see if it's reproducible

/test kubeflow-tf-operator-presubmit

@PatrickXYS
Copy link
Member Author

I'll create another PR to test if original test pass or not

Yao Xiao added 2 commits March 18, 2021 11:26
@PatrickXYS
Copy link
Member Author

Failed for wrong tf-operator tag, fixed now, let's see if test passes

@PatrickXYS
Copy link
Member Author

Failed for

$ kc describe pods -n kubeflow tf-job-operator-5fd4f497fd-rc4sz 

flag provided but not defined: -alsologtostderr

@PatrickXYS
Copy link
Member Author

PatrickXYS commented Mar 18, 2021

@PatrickXYS
Copy link
Member Author

@PatrickXYS
Copy link
Member Author

/unhold

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for your contribution! 🎉 👍

@ChanYiLin
Copy link
Member

/lgtm
Thanks 👍

@PatrickXYS
Copy link
Member Author

Can any of you help approve?

I'll hold here first, since we're kind of urgent in release

/hold

@Jeffwan
Copy link
Member

Jeffwan commented Mar 19, 2021

@PatrickXYS Can you document debug information somewhere?

fieldRef:
fieldPath: metadata.name
image: gcr.io/kubeflow-images-public/tf-operator:v0.6.0
image: public.ecr.aws/j1r0q0g6/training/tf-operator
Copy link
Member

Choose a reason for hiding this comment

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

CI already cover image build to public ECR?

Copy link
Member Author

Choose a reason for hiding this comment

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

CI push to private ECR
CD push to public ECR

The registry is defined differently in the prow_config.yaml file

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

- examples/*
params:
registry: "gcr.io/kubeflow-ci"
registry: "809251082950.dkr.ecr.us-west-2.amazonaws.com/tf-operator"
Copy link
Member Author

Choose a reason for hiding this comment

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

@Jeffwan CI push to private ECR

- postsubmit
params:
registry: "gcr.io/kubeflow-images-public"
registry: "public.ecr.aws/j1r0q0g6/training/tf-operator"
Copy link
Member Author

Choose a reason for hiding this comment

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

@Jeffwan CD push to public ECR

@PatrickXYS
Copy link
Member Author

@Jeffwan I'll issue a follow up PR to document debug information in test folder

@PatrickXYS PatrickXYS requested a review from Jeffwan March 19, 2021 16:47
@PatrickXYS
Copy link
Member Author

I don't intend to push, but let's get it fixed by this week, since 03/23 will be the target date to cut 1.3RC.

/cc @gaocegege @ChanYiLin @johnugeorge

@ChanYiLin
Copy link
Member

/approve
Since this PR has already passed the the test and we will have another follow up PR
Then that's merge it first

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChanYiLin, PatrickXYS

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@ChanYiLin
Copy link
Member

ChanYiLin commented Mar 19, 2021

Looks like we don't define the flag -alsologtostderr, I'm confused why it's using in the deployment

This is because we originally have another deployment file for testing not the one under manifest
The file under manifest is copy from kubeflow/manifest
Sorry for not replying this message asap

@PatrickXYS
Copy link
Member Author

/unhold

Unhold to merge

@google-oss-robot google-oss-robot merged commit cd2fc1f into kubeflow:master Mar 19, 2021
@PatrickXYS PatrickXYS mentioned this pull request Mar 19, 2021
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.

[Test] Add support for public ECR image

7 participants