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

feat: expose core db connection settings #1056

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

LiuShuaiyi
Copy link
Contributor

Allow to change core.spec.database.maxIdleConnections and core.spec.database.maxOpenConnections in harborcluster.

@github-actions github-actions bot requested review from chlins and holyhope July 7, 2023 22:55
// +kubebuilder:validation:Optional
// +kubebuilder:validation:Minimum=0
// +kubebuilder:default=50
MaxIdleConnections *int32 `json:"maxIdleConnections,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this set here and not in HarborDatabaseSpec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is passed to Core and eventually Harbor core deployment to configure the max connections between core pods and the db, ultimately goes to https://sourcegraph.com/github.com/goharbor/harbor@cbb211e6702517fddcc8b9a7341e8a3479211ca7/-/blob/src/common/dao/pgsql.go?L99.

This is different from the configuring DB's server-side max connections.

Therefore I think this should be in harbor_types instead of in HarborDatabaseSpec. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood after explanation on Slack as well - thank you!

Copy link
Collaborator

@thcdrt thcdrt left a comment

Choose a reason for hiding this comment

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

LGTM

@LiuShuaiyi
Copy link
Contributor Author

LiuShuaiyi commented Jul 17, 2023

Please don't merge yet (hopefully some of the checks will break). It's missing at least charts/harbor-operator/templates/crds.yaml.


edited: nvm just found that charts/harbor-operator/templates/crds.yaml was removed.

@thcdrt
Copy link
Collaborator

thcdrt commented Jul 18, 2023

@MarcelMue @wy65701436 @chlins should we merge it now ? Or do we wait to recreate 1.4 release branch first ?

@MarcelMue
Copy link
Collaborator

@MarcelMue @wy65701436 @chlins should we merge it now ? Or do we wait to recreate 1.4 release branch first ?

I think it's fine to merge now. Also why is CI stuck so weirdly? I am uncertain why github actions are not running.

@thcdrt
Copy link
Collaborator

thcdrt commented Jul 20, 2023

@MarcelMue @wy65701436 @chlins should we merge it now ? Or do we wait to recreate 1.4 release branch first ?

I think it's fine to merge now. Also why is CI stuck so weirdly? I am uncertain why github actions are not running.

I just asked me the same question in another PR, I don't understand neither... @wy65701436 @chlins do you have an idea ?

@LiuShuaiyi
Copy link
Contributor Author

Gentle ping @chlins @holyhope. Looks like I need an approval from a maintainer to trigger the workflows.

Allow to change core.spec.database.maxIdleConnections and
core.spec.database.maxOpenConnections in harborcluster.

Signed-off-by: Eric Liu <shuaiyi@google.com>
@LiuShuaiyi
Copy link
Contributor Author

The PR failed at test the following test:

Test Case - Tag Immutability                                          Filesystem      Size  Used Avail Use% Mounted on
overlay          84G   76G  7.4G  92% /
tmpfs            64M     0   64M   0% /dev
shm              64M     0   64M   0% /dev/shm
/dev/root        84G   76G  7.4G  92% /drone
none            3.4G     0  3.4G   0% /tmp
| FAIL |
1 != 0

I don't think this failure is related to my change, right? Is it possible to re-run the test or merge it?

@MarcelMue MarcelMue merged commit 4f460cb into goharbor:main Aug 8, 2023
32 of 33 checks passed
@OrlinVasilev
Copy link
Member

@LiuShuaiyi congrats on merging your first PR here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants