Skip to content

Introduce masterServiceAnnotations & replicaServiceAnnotations #2161

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

Merged
merged 11 commits into from
Jan 11, 2023

Conversation

owenthereal
Copy link
Contributor

@owenthereal owenthereal commented Jan 5, 2023

Introduce masterServiceAnnotations & replicaServiceAnnotations to the Postgresql CRD. The following behavior is implemented

  • masterServiceAnnotations takes precedence over serviceAnnotations for master service if not empty.
  • replicaServiceAnnotations takes precedence over serviceAnnotations for replica service if not empty.
  • The existing definition of serviceAnnotations continue to work for backward compatibility when neither masterServiceAnnotations nor replicaServiceAnnotations are defined.

This closes #1927

@FxKu
Copy link
Member

FxKu commented Jan 6, 2023

Could you add the new field also to the CRD schema (please, follow alphabetical sorting)

owenthereal added a commit to owenthereal/postgres-operator that referenced this pull request Jan 6, 2023
First, global config, then ServiceAnnotations overriding, then MasterServiceAnnotations and ReplicaServiceAnnotations.

This addresses
zalando#2161 (comment).
@owenthereal
Copy link
Contributor Author

@FxKu:

Could you add the new field also to the CRD schema (please, follow alphabetical sorting)

example manifest
helm chart
go representation

Addressed in 030c482 & db7befa

Mind taking another look? I have addressed all feedback, and this PR should be ready to merge. Cheers!

@FxKu FxKu added this to the 1.9 milestone Jan 9, 2023
owenthereal added a commit to owenthereal/postgres-operator that referenced this pull request Jan 9, 2023
First, global config, then ServiceAnnotations overriding, then MasterServiceAnnotations and ReplicaServiceAnnotations.

This addresses
zalando#2161 (comment).
@owenthereal owenthereal force-pushed the master branch 2 times, most recently from fc96e71 to 1a5a7a2 Compare January 9, 2023 17:23
owenthereal added a commit to owenthereal/postgres-operator that referenced this pull request Jan 9, 2023
@owenthereal owenthereal requested review from FxKu and removed request for Jan-M, sdudoladov, idanovinda, hughcapet and jopadi January 9, 2023 17:27
Comment on lines 48 to 52
// import (
// "k8s.io/client-go/kubernetes"
// clientsetscheme "k8s.io/client-go/kubernetes/scheme"
// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme"
// )
Copy link
Contributor

Choose a reason for hiding this comment

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

@owenthereal what is your golang version installed. For 1.17 and 1.18 generated files should not be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using 1.19 locally. I regenerated the code with 1.18 in 3fb0b42.

It would be nice to introduce https://asdf-vm.com or the like to this project to auto-switch or prompt to install the right Go version.

Copy link
Contributor

@dmvolod dmvolod Jan 11, 2023

Choose a reason for hiding this comment

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

Not sure we need an additional tools for that, as golang supports multiple versions management by itself
https://go.dev/doc/manage-install

As an option, some validation steps with autogeneration/formatting/linting can be added as part of the #2135

@@ -2380,3 +2384,18 @@ func ensurePath(file string, defaultDir string, defaultFile string) string {
}
return file
}

// mergeAnnotations merged annotations that entries in b override the ones in a.
func mergeAnnotations(a, b map[string]string) map[string]string {
Copy link
Contributor

Choose a reason for hiding this comment

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

When we will be on golang 1.18 with #2164, maps.Copy from the golang.org/x/exp/maps can be used instead.
Seems to this is too much resource and memory consumption implementation.
@FxKu wdyt about it?

Copy link
Member

Choose a reason for hiding this comment

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

I read about maps.Copy yesterday. We would not need this function then and could do it in the code. Sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Switched to use maps.Copy in 3fb0b42

owenthereal and others added 10 commits January 10, 2023 14:00
Introduce `masterServiceAnnotations` & `replicaServiceAnnotations` to the `Postgresql` CRD.
`masterServiceAnnotations` overrides `serviceAnnotations` for master role if not empty.
`replicaServiceAnnotations` overrides `serviceAnnotations` for replica role if not empty.
Existing definition of `serviceAnnotations` continue to work for backward compatibitlity when neither `masterServiceAnnotations` nor `replicaServiceAnnotations` is defined.

This closes zalando#1927
First, global config, then ServiceAnnotations overriding, then MasterServiceAnnotations and ReplicaServiceAnnotations.

This addresses
zalando#2161 (comment).
Co-authored-by: Felix Kunde <felix-kunde@gmx.de>
Co-authored-by: Felix Kunde <felix-kunde@gmx.de>
Co-authored-by: Felix Kunde <felix-kunde@gmx.de>
@owenthereal owenthereal requested review from dmvolod and FxKu and removed request for FxKu and dmvolod January 10, 2023 22:08
@FxKu
Copy link
Member

FxKu commented Jan 11, 2023

👍

1 similar comment
@idanovinda
Copy link
Member

👍

@FxKu FxKu merged commit 021ab07 into zalando:master Jan 11, 2023
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.

Load balancing service annotations are applying to services that do not have load balancing enabled
4 participants