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

Metrics: add namespace to distinguish fleets with the same name #1585

Merged
merged 5 commits into from
May 29, 2020

Conversation

akremsa
Copy link
Contributor

@akremsa akremsa commented May 26, 2020

What type of PR is this?

/kind bug

What this PR does / Why we need it:
Fleet metrics from different namespaces are split.

Which issue(s) this PR fixes:
Closes #1501

Special notes for your reviewer:
Screenshot 2020-05-26 at 18 10 11

gameservers_count:
image
Prometheus:
image

@akremsa akremsa changed the title Metrics: add namespace to distinguish same name fleets Metrics: add namespace to distinguish fleets with the same name May 26, 2020
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 03f3f49d-a19b-4da2-985b-f640a185031c

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

@aLekSer
Copy link
Collaborator

aLekSer commented May 26, 2020

Seems that github breaks website check again:

for i in {1..5}; \
	do echo "Html Test: Attempt $i" && \
	  docker run --rm -t -e "TERM=xterm-256color" -v /workspace/build//.config/gcloud:/root/.config/gcloud -v ~/.kube/:/root/.kube -v ~/.helm:/root/.helm -v /workspace:/go/src/agones.dev/agones -v /workspace/build//.gomod:/go/pkg/mod -v /workspace/build//.gocache:/root/.cache/go-build  agones-build:ae232b533a bash -c \
		"mkdir -p /tmp/website && cp -r /go/src/agones.dev/agones/site/public /tmp/website/site && htmltest -c /go/src/agones.dev/agones/site/htmltest.yaml /tmp/website" && \
break || sleep 60; done
Html Test: Attempt {1..5}
...
 Non-OK status: 429 --- site/docs/installation/creating-cluster/aks/index.html --> https://github.com/dgkanatsios/AksNodePublicIPController
site/docs/installation/creating-cluster/minikube/index.html
  Non-OK status: 429 --- site/docs/installation/creating-cluster/minikube/index.html --> https://github.com/kubernetes/minikube#requirements
========================================================================
✘✘✘ failed in 45.759002885s
133 errors in 109 documents

And TestLocalSDKServerPlayerConnectAndDisconnect is flaky.

       localsdk_test.go:444: 
            	Error Trace:	localsdk_test.go:444
            	Error:      	Should be true
            	Test:       	TestLocalSDKServerPlayerConnectAndDisconnect/test_mode_on,_no_filePath
            	Messages:   	Player should not exist yet
        localsdk_test.go:448: 
            	Error Trace:	localsdk_test.go:448
            	Error:      	Not equal: 
            	            	expected: 1
            	            	actual  : 0
            	Test:       	TestLocalSDKServerPlayerConnectAndDisconnect/test_mode_on,_no_filePath
        localsdk_test.go:634: 
            	Error Trace:	localsdk_test.go:634
            	            				localsdk_test.go:455
            	Error:      	timeout on receiving messages
            	Test:       	TestLocalSDKServerPlayerConnectAndDisconnect/test_mode_on,_no_filePath

I am able to reproduce similar with go test -v -parallel 10

@aLekSer
Copy link
Collaborator

aLekSer commented May 26, 2020

@akremsa Please include metrics/controller_test.go which is failing:


    controller_test.go:71: 
        	Error Trace:	controller_test.go:71
        	            				controller_test.go:250
        	Error:      	Should be true
        	Test:       	TestControllerFleetAutoScalerState
        	Messages:   	no TimeSeries found with labels: [first-fleet name-switch default]

@markmandel
Copy link
Member

Html Test: Attempt {1..5}

It's so weird. Locally, it will do 5 retries (shell sees $i as 1, then 2 then 3, etc). On Cloud Build, it sees $i as {1..5} - I'm not sure why.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 84b5ce43-2807-4859-8553-4c192b89a69f

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d3d0c44c-357b-4c07-886c-cefd0911f693

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/1585/head:pr_1585 && git checkout pr_1585
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-4d502c0

@akremsa
Copy link
Contributor Author

akremsa commented May 28, 2020

@aLekSer is right. I was checking gameservers_count metric and found that gs amount is different for each namespace. Fixed in the latest commit. @markmandel please check.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 8b21ab52-9f4a-48e2-bbb4-54a26629a5f4

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 0520c2d4-c6d3-4d75-924b-7948ec581de1

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/1585/head:pr_1585 && git checkout pr_1585
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-eb20357

Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

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

Left some small comments. Overall looks promising.

make none to be a constant


removed redundant output used for debug
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a0e95e54-4cd2-4db8-a683-51152f9d3aa3

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/1585/head:pr_1585 && git checkout pr_1585
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-8c0702f

@akremsa akremsa requested a review from aLekSer May 28, 2020 16:34
Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

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

Now this PR looks nice.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akremsa, aLekSer
To complete the pull request process, please assign roberthbailey
You can assign the PR to them by writing /assign @roberthbailey in a comment when ready.

The full list of commands accepted by this bot can be found 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-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 97a858d2-421d-4dbb-9ea7-8bb0aea909f5

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/1585/head:pr_1585 && git checkout pr_1585
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-34e3b32

@markmandel markmandel added area/operations Installation, updating, metrics etc kind/feature New features for Agones labels May 29, 2020
@markmandel markmandel added this to the 1.7.0 milestone May 29, 2020
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 9cbd3d86-cfae-4dad-b95f-0dc3662faa0e

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/1585/head:pr_1585 && git checkout pr_1585
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-c6a3c22

@markmandel markmandel merged commit 024af01 into googleforgames:master May 29, 2020
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
…leforgames#1585)

* Added namespace to recordFleetReplicas

Co-authored-by: Alexander Apalikov <alexander.apalikov@globant.com>
Co-authored-by: Mark Mandel <markmandel@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operations Installation, updating, metrics etc kind/feature New features for Agones size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics: add namespace to distinguish same name fleets
6 participants