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

Helm: add ping HTTP and UDP annotations into chart #1520

Merged
merged 2 commits into from
May 11, 2020

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented May 7, 2020

Add ability to change Ping service annotations.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation

/kind feature

/kind hotfix

What this PR does / Why we need it:
Add an ability to set annotations for UDP and HTTP Ping services.
helm upgrade --install agones ./install/helm/agones/ --set agones.ping.http.annotations."external-dns.alpha.kubernetes.io/hostname"="agones-http-ping.example.com"

Which issue(s) this PR fixes:

Closes #1491

Special notes for your reviewer:

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 299cf1af-d525-41fe-a83a-4a40477f9d23

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 00d58ca4-c421-4052-aed5-4259b26137c3

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@aLekSer
Copy link
Collaborator Author

aLekSer commented May 7, 2020

Tests timeout after 10 minutes:

panic: test timed out after 10m0s

goroutine 2937 [running]:
testing.(*M).startAlarm.func1()
	/usr/local/go/src/testing/testing.go:1377 +0x11c
created by time.goFunc
	/usr/local/go/src/time/sleep.go:168 +0x52

goroutine 1 [chan receive, 9 minutes]:
testing.tRunner.func1(0xc0003a0500)
	/usr/local/go/src/testing/testing.go:885 +0x2ee
testing.tRunner(0xc0003a0500, 0xc00019dd40)
	/usr/local/go/src/testing/testing.go:913 +0x1bc
testing.runTests(0xc000375420, 0x2a6cd60, 0x28, 0x28, 0x0)
	/usr/local/go/src/testing/testing.go:1200 +0x522
testing.(*M).Run(0xc000424e80, 0x0)
	/usr/local/go/src/testing/testing.go:1117 +0x300
main.main()
	_testmain.go:122 +0x224

goroutine 21 [chan receive]:
k8s.io/klog.(*loggingT).flushDaemon(0x2a7a180)
	/go/src/agones.dev/agones/vendor/k8s.io/klog/klog.go:975 +0xae
created by k8s.io/klog.init.0
	/go/src/agones.dev/agones/vendor/k8s.io/klog/klog.go:404 +0x9b

goroutine 36 [chan receive, 9 minutes]:
testing.(*T).Run(0xc0003a0600, 0x1cf6d6e, 0x19, 0x1d759a8, 0xc000450718)
	/usr/local/go/src/testing/testing.go:961 +0x68a
agones.dev/agones/pkg/gameservers.TestControllerSyncGameServer(0xc0003a0600)
	/go/src/agones.dev/agones/pkg/gameservers/controller_test.go:58 +0x6c
testing.tRunner(0xc0003a0600, 0x1d75ae0)
	/usr/local/go/src/testing/testing.go:909 +0x19a
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:960 +0x652

@aLekSer
Copy link
Collaborator Author

aLekSer commented May 7, 2020

E2E test fail:

--- FAIL: TestAllocator (1.23s)
    allocator_test.go:61: deleting pods failed: pods "agones-allocator-978fd597-ntr5b" not found

@markmandel
Copy link
Member

@TBBle just checking to make sure this PR is doing what you are looking for?

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 0208648f-226d-4f6c-82d9-ae07e6ce9b09

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 47ac060f-437f-43a2-838f-eab88e54ec16

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1520/head:pr_1520 && git checkout pr_1520
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-6dcf274

@@ -95,6 +95,10 @@ metadata:
chart: {{ template "agones.chart" . }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
{{- if .Values.agones.ping.http.annotations }}
annotations:
{{ toYaml .Values.agones.ping.http.annotations | indent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should only be indent 4 (i.e. one level deeper than the annotations key), but it's only a minor thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me update and see if it would generate proper annotations.

@TBBle
Copy link
Contributor

TBBle commented May 8, 2020

Yeah, looks right to me. Pretty much exactly what I expected. Thanks @aLekSer

@google-oss-robot
Copy link

@TBBle: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@TBBle
Copy link
Contributor

TBBle commented May 8, 2020

While you're doing the changes, it might be worth making the same change for the allocator's service. It should be the same thing, adding an annotations field next to the serviceType field, and passing it through toYaml in the service template.

I wouldn't take that as a blocker for this PR, to be clear. Just if you get back to it before it's approved for merge.

@@ -119,6 +123,10 @@ metadata:
chart: {{ template "agones.chart" . }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
{{- if .Values.agones.ping.udp.annotations }}
annotations:
{{ toYaml .Values.agones.ping.udp.annotations | indent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, indent 4 if you come back to this.

@aLekSer
Copy link
Collaborator Author

aLekSer commented May 11, 2020

The command to test:

helm install ../install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-6dcf274 \ 
--set agones.ping.http.annotations."external-dns\.alpha\.kubernetes\.io/hostname"="agones-http-ping\.example\.com" \ 
--set agones.ping.udp.annotations."external-dns\.alpha\.kubernetes\.io/hostname"="agones-udp-ping\.example\.com" \ 
--set agones.allocator.http.annotations."external-dns\.alpha\.kubernetes\.io/hostname"="agones-http-allocator\.example.com"                    

Result:

kubectl describe svc agones-allocator  -n agones-system                                                                                                               
Name:                     agones-allocator
Namespace:                agones-system
Labels:                   app=agones
                          chart=agones-1.6.0
                          component=allocator
                          heritage=Tiller
                          release=agones
Annotations:              external-dns.alpha.kubernetes.io/hostname: agones-http-allocator.example.com        

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: aec45b54-0416-4b38-9700-b86dcb2abcac

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1520/head:pr_1520 && git checkout pr_1520
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-ccd4d23

@google-oss-robot
Copy link

@TBBle: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@markmandel markmandel added area/operations Installation, updating, metrics etc kind/feature New features for Agones labels May 11, 2020
@markmandel markmandel added this to the 1.6.0 milestone May 11, 2020
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Good to go!

@markmandel markmandel merged commit e6a511e into googleforgames:master May 11, 2020
@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, markmandel, TBBle

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

ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
* Helm: add ping HTTP and UDP annotations into chart

Add ability to change Ping service annotations.

* Helm: add annotations to Allocator API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/operations Installation, updating, metrics etc kind/feature New features for Agones lgtm size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support annotations for ping services in the Helm chart
5 participants