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

Support postgres as a katib db #1921

Merged

Conversation

anencore94
Copy link
Member

@anencore94 anencore94 commented Jul 30, 2022

What this PR does / why we need it:

  • Implement postgres for katib backend and support it as a built-in feature of katib.
  • Add kustomization configurations for using postgres.
  • Support e2e test on both mysql and postgres.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #915

Checklist:

  • Docs included if any changes are user facing

@coveralls
Copy link

coveralls commented Jul 30, 2022

Coverage Status

Coverage increased (+0.6%) to 73.433% when pulling 13cf73b on anencore94:feature/postgres-for-katib-backend-db into 42bc6a9 on kubeflow:master.

@gaocegege
Copy link
Member

/assign @johnugeorge @andreyvelich

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this great feature! @anencore94
It looks good to me about core logic!

pkg/db/v1beta1/postgres/postgres.go Outdated Show resolved Hide resolved
anencore94 and others added 3 commits August 2, 2022 17:15
Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
- split openconnection to common packages
- add unit test for postgres db
@google-oss-prow google-oss-prow bot added size/XL and removed size/L labels Aug 2, 2022
@johnugeorge
Copy link
Member

Should we keep add postgres support as a separate Kustomization file similar to katib-standalone?

https://github.com/kubeflow/katib/tree/master/manifests/v1beta1/installs

It is good to add this in GitHub actions. In that case, We have to add an extra option in the setup script to take db manager backend(mysql, postgres)
https://github.com/kubeflow/katib/blob/master/.github/workflows/template-e2e-test/action.yaml#L27

What do you think?

@anencore94
Copy link
Member Author

Should we keep add postgres support as a separate Kustomization file similar to katib-standalone?
It is good to add this in GitHub actions. In that case, We have to add an extra option in the setup script to take db manager backend(mysql, postgres)

Yeap, I also think that looks more safe to use both Database :)
So you mean that

  1. add another folder with kustomization configurations with postgres.
  2. add option to setup katib with postgres so that we could run all e2e test for both database.
    right ? @johnugeorge

@johnugeorge
Copy link
Member

Should we keep add postgres support as a separate Kustomization file similar to katib-standalone?
It is good to add this in GitHub actions. In that case, We have to add an extra option in the setup script to take db manager backend(mysql, postgres)

Yeap, I also think that looks more safe to use both Database :) So you mean that

  1. add another folder with kustomization configurations with postgres.
  2. add option to setup katib with postgres so that we could run all e2e test for both database.
    right ? @johnugeorge

Yes. Correct.

@anencore94
Copy link
Member Author

anencore94 commented Aug 6, 2022

@johnugeorge Ok, I'll add that feature with this PR. Thanks for your comments :)

@tenzen-y
Copy link
Member

Did you face the same error on your local machine?

No, In my local minikube environments, validatingwebhook works well : When I create invalid-expereiment.yaml, both enviroments(with mysql or with postgresql) reports expected error message as follows:

Error from server: error when creating "invalid-experiment.yaml": admission webhook "validator.experiment.katib.kubeflow.org" denied the request: unable to get Suggestion config data for algorithm invalid-algorithm: failed to find suggestion config for algorithm: invalid-algorithm in ConfigMap: katib-config

@tenzen-y

@anencore94 I see.
Can you insert kubectl logs -l "katib.kubeflow.org/component in (controller,db-manager,cert-generator)" -n kubeflow --all-containers=true to this section?

@anencore94 anencore94 changed the title WIP:implement postgres for katib db Support postgres for katib db Aug 13, 2022
@anencore94 anencore94 changed the title Support postgres for katib db Support postgres as a katib db Aug 13, 2022
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! @anencore94
I'm looking forward to releasing this feature!

I left a few comments.

manifests/v1beta1/components/postgres/postgres.yaml Outdated Show resolved Hide resolved
pkg/db/v1beta1/common/connection.go Outdated Show resolved Hide resolved
pkg/db/v1beta1/postgres/postgres.go Outdated Show resolved Hide resolved
pkg/db/v1beta1/postgres/postgres.go Outdated Show resolved Hide resolved
pkg/db/v1beta1/postgres/postgres_test.go Outdated Show resolved Hide resolved
test/e2e/v1beta1/scripts/gh-actions/setup-katib.sh Outdated Show resolved Hide resolved
test/e2e/v1beta1/scripts/gh-actions/setup-katib.sh Outdated Show resolved Hide resolved
@tenzen-y
Copy link
Member

tenzen-y commented Aug 15, 2022

@kubeflow/wg-automl-leads Can you re-kick fail GH-Action Jobs?

anencore94 and others added 3 commits August 16, 2022 20:31
Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@anencore94
Copy link
Member Author

@tenzen-y Thanks for your detailed reviews :) I applied your comments.

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thanks for updating PR! @anencore94
/lgtm

@johnugeorge
Copy link
Member

Great work.
One question: Is it better to add readiness probe than static 30 sec sleep?

@anencore94
Copy link
Member Author

anencore94 commented Aug 17, 2022

Great work. One question: Is it better to add readiness probe than static 30 sec sleep?

Absolutely agree. I'll add such feature to katib-controller as a follow-up PR. @johnugeorge

@johnugeorge
Copy link
Member

Thanks
/lgtm

@johnugeorge
Copy link
Member

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anencore94, johnugeorge

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

@google-oss-prow google-oss-prow bot merged commit cc888af into kubeflow:master Aug 19, 2022
@anencore94 anencore94 deleted the feature/postgres-for-katib-backend-db branch August 19, 2022 16:06
@tenzen-y
Copy link
Member

@anencore94
It might be better to add documentation about env vars for PostgreSQL to this section.

@anencore94
Copy link
Member Author

@tenzen-y Sure, Thanks for checking :)

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.

Support PostgreSQL
6 participants