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.

Sign up for GitHub

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

Jump to bottom

Add ability to add annotations and labels to JobManager service #320

Merged
merged 3 commits into from
Mar 21, 2022
Merged

Add ability to add annotations and labels to JobManager service #320

merged 3 commits into from
Mar 21, 2022

Conversation

Sudokamikaze
Copy link
Contributor

@Sudokamikaze Sudokamikaze commented Mar 9, 2022 •
edited
Loading

Issue ref: #319

Result of patch:

  • Changes to example cluster:
   jobManager:
+    accessScope: External
+    ServiceAnnotations:
+      service.beta.kubernetes.io/aws-load-balancer-internal: "true"
  • Describe output from service:
$ kubectl describe svc flink-illiatest-jobmanager -n flink-job
Name:                     flink-illiatest-jobmanager
Namespace:                flink-job
Labels:                   app=flink
                          cluster=flink-illiatest
                          component=jobmanager
                          flinkoperator.k8s.io/revision-name=flink-illiatest-67bd68c554
Annotations:              service.beta.kubernetes.io/aws-load-balancer-internal: true

Image I built to test feature: https://hub.docker.com/repository/docker/sudokamikaze/flinktests built on latest master HEAD + my changes

@Sudokamikaze Sudokamikaze force-pushed the JobManagerSVC-Annotations branch from 8e140a0 to 9dd6364 Compare March 10, 2022 16:41
@Sudokamikaze
Copy link
Contributor Author

@regadas Hi, I know it's open-source project and I really appreciate your & team works on this project
Could you please check this PR/Issue when you have free time? Thank you in advance

regadas
regadas reviewed Mar 18, 2022
View reviewed changes
Copy link
Contributor

@regadas regadas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the late reply and Thanks for doing this!

apis/flinkcluster/v1beta1/flinkcluster_types.go Outdated Show resolved Hide resolved
@Sudokamikaze Sudokamikaze force-pushed the JobManagerSVC-Annotations branch from 9dd6364 to 7aa04ba Compare March 18, 2022 14:40
@Sudokamikaze Sudokamikaze requested a review from regadas March 18, 2022 14:41
regadas
regadas reviewed Mar 18, 2022
View reviewed changes
controllers/flinkcluster/flinkcluster_converter.go Outdated
@@ -292,6 +292,7 @@ func newJobManagerService(flinkCluster *v1beta1.FlinkCluster) *corev1.Service {
switch jobManagerSpec.AccessScope {
case v1beta1.AccessScopeCluster:
jobManagerService.Spec.Type = corev1.ServiceTypeClusterIP
jobManagerService.Annotations = jobManagerSpec.ServiceAnnotations
Copy link
Contributor

Choose a reason for hiding this comment

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

We could set the annotations earlier right in L#276

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

regadas
regadas reviewed Mar 18, 2022
View reviewed changes
apis/flinkcluster/v1beta1/flinkcluster_types.go
@@ -195,6 +195,9 @@ type JobManagerSpec struct {
// Currently `VPC, External` are only available for GKE.
AccessScope string `json:"accessScope,omitempty"`

// _(Optional)_ Define JobManager Service annotations for configuration.
ServiceAnnotations map[string]string `json:"ServiceAnnotations,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add ServiceLabels as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re this change, should I merge labels in this way:

var serviceLabels = mergeLabels(podLabels, jobManagerSpec.ServiceLabels, getRevisionHashLabels(&flinkCluster.Status.Revision))

Or should I exclusively define var.serviceLabels to jobManagerSpec.ServiceLabels ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or should I exclusively define var.serviceLabels to jobManagerSpec.ServiceLabels ?

Yeah, I think we should go with that and not inherit the pod labels

Copy link
Contributor

Choose a reason for hiding this comment

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

We should still set the revision hash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 1bfa0f9
Had to remove few pre-defined lines from tests though

@Sudokamikaze Sudokamikaze changed the title Add ability to add annotations to JobManager service Add ability to add annotations and labels to JobManager service Mar 18, 2022
@Sudokamikaze Sudokamikaze requested a review from regadas March 21, 2022 10:50
regadas
regadas approved these changes Mar 21, 2022
View reviewed changes
Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

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

Thanks @Sudokamikaze for pushing this.

@regadas regadas merged commit 2749ad0 into spotify:master Mar 21, 2022
@Sudokamikaze Sudokamikaze mentioned this pull request Mar 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@regadas regadas regadas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Sudokamikaze @regadas