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

MGMT-6437: Redirecting cluster metrics to the correct Telemeter server. #1773

Merged
merged 1 commit into from
May 25, 2021
Merged

MGMT-6437: Redirecting cluster metrics to the correct Telemeter server. #1773

merged 1 commit into from
May 25, 2021

Conversation

ybettan
Copy link
Member

@ybettan ybettan commented May 20, 2021

Currently all clusters send metrics to the default Telemeter server
which is the prod instance of Telemeter in the cloud.

This is wrong for 2 reasons:

  1. integration/stage clusters statistics shouldn't be a part of
    production dashboards.

  2. integration/stage service is configured to work against integration/stage
    AMS therefore, for those clusters, when metrics reaches prod
    Telemeter, they will fail on authN anyway.

This change make sure each env send it metrics to the correct server or
being disabled.

Signed-off-by: Yoni Bettan [email protected]

@ybettan ybettan changed the title MGMT-6437: Redirecting cluster metrics to the correct Telemeter server. [WIP] MGMT-6437: Redirecting cluster metrics to the correct Telemeter server. May 20, 2021
@openshift-ci openshift-ci bot requested review from eranco74 and ori-amizur May 20, 2021 10:02
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 20, 2021
@ybettan
Copy link
Member Author

ybettan commented May 20, 2021 •
edited
Loading

Couldn't find an easy way to test it further than unit-tests.

@ybettan
Copy link
Member Author

ybettan commented May 20, 2021

/retest
https://coreos.slack.com/archives/C014N2VLTQE/p1621509027027000?thread_ts=1621446988.016700&cid=C014N2VLTQE

@ybettan
Copy link
Member Author

ybettan commented May 20, 2021

/retest

@ybettan
Copy link
Member Author

ybettan commented May 20, 2021

/assign @eranco74

@ybettan ybettan force-pushed the redirect-clusters-metrics branch from 3e20d2f to 1393e58 Compare May 23, 2021 07:08
@ybettan ybettan changed the title [WIP] MGMT-6437: Redirecting cluster metrics to the correct Telemeter server. MGMT-6437: Redirecting cluster metrics to the correct Telemeter server. May 23, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2021
eranco74
eranco74 reviewed May 23, 2021
View reviewed changes
internal/cluster/cluster.go Outdated Show resolved Hide resolved
internal/network/manifests_generator.go Outdated Show resolved Hide resolved
internal/network/manifests_generator.go Show resolved Hide resolved
internal/network/manifests_generator.go Outdated Show resolved Hide resolved
romfreiman
romfreiman requested changes May 24, 2021
View reviewed changes
internal/network/manifests_generator.go Outdated Show resolved Hide resolved
internal/network/manifests_generator.go Outdated Show resolved Hide resolved
@ybettan ybettan force-pushed the redirect-clusters-metrics branch 5 times, most recently from cd0330a to aecd7a3 Compare May 24, 2021 12:39
eranco74
eranco74 reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@eranco74 eranco74 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@ybettan ybettan force-pushed the redirect-clusters-metrics branch from aecd7a3 to b5cec47 Compare May 24, 2021 14:02
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 24, 2021
@ybettan ybettan force-pushed the redirect-clusters-metrics branch from b5cec47 to b8fc6b4 Compare May 24, 2021 14:20
@eranco74
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2021
@ybettan
Copy link
Member Author

ybettan commented May 24, 2021

/retest

1 similar comment
@ybettan
Copy link
Member Author

ybettan commented May 24, 2021

/retest

@ybettan
Copy link
Member Author

ybettan commented May 24, 2021

We can see in the service that the custom manifest was created:

$ curl -s $(minikube service assisted-service -n assisted-installer --url)/api/assisted-install/v1/clusters/c12773ed-5bcf-407c-8b4b-bf23808b9c86/manifests | jq '.'
[
  {
    "file_name": "masters-chrony-configuration.yaml",
    "folder": "openshift"
  },
  {
    "file_name": "redirect-telemeter.yaml",
    "folder": "openshift"
  },
  {
    "file_name": "workers-chrony-configuration.yaml",
    "folder": "openshift"
  }
]

In the created cluster we can see that the ConfigMap was applied with the correct URL:

$ oc --kubeconfig kubeconfig describe cm/cluster-monitoring-config -n openshift-monitoring
Name:         cluster-monitoring-config
Namespace:    openshift-monitoring
Labels:       <none>
Annotations:  <none>

Data
====
config.yaml:
----
telemeterClient:
  telemeterServerURL: https://dummy.com

Events:  <none>

@ybettan
MGMT-6437: Redirecting cluster metrics to the correct Telemeter server.
1300142 
Currently all clusters send metrics to the default Telemeter server
which is the prod instance of Telemeter in the cloud.

This is wrong for 2 reasons:

1. integration/stage clusters statistics shouldn't be a part of
   production dashboards.

2. integration/stage service is configured to work against integration/stage
   AMS therefore, for those clusters, when metrics reaches prod
   Telemeter, they will fail on authN anyway.

This change make sure each env send it metrics to the correct server or
being disabled.

Signed-off-by: Yoni Bettan <[email protected]>
@ybettan ybettan force-pushed the redirect-clusters-metrics branch from b8fc6b4 to 1300142 Compare May 25, 2021 03:43
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 25, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2021
@openshift-ci
Copy link

openshift-ci bot commented May 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gamli75, ybettan

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

@openshift-merge-robot openshift-merge-robot merged commit af46cb3 into openshift:master May 25, 2021
@ybettan ybettan deleted the redirect-clusters-metrics branch May 25, 2021 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eranco74 eranco74 eranco74 left review comments

@romfreiman romfreiman romfreiman requested changes

@gamli75 gamli75 gamli75 approved these changes

@ori-amizur ori-amizur Awaiting requested review from ori-amizur

Assignees

@romfreiman romfreiman

@eranco74 eranco74

@gamli75 gamli75

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ybettan @openshift-bot @eranco74 @romfreiman @gamli75 @openshift-merge-robot