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

OCPBUGSM-30853: Change default telemeter server of installed clusters to prod. #1988

Merged
merged 1 commit into from Jun 14, 2021

Conversation

ybettan
Copy link
Contributor

@ybettan ybettan commented Jun 14, 2021

Description

Until now, in the service, we changed the default behavior of openshift,
which is to always send cluster metrics to prod-telemeter-server unless
changed by the user, and we:
    * left untouched clusters created by cloud prod env
    * redirected to telemeter-stage stage clusters
    * redirected to dummy-url all other clusters

The issue with this approach is that all prod clusters that aren't
created in the cloud, operator clusters for example,  will fail to
deliver telemetry.

Instead, we will now default to prod-telemeter instead of dummy-url
unless we know better:
    * left untouched clusters created by ALL prod envs (not just cloud)
    * redirect to telemeter-stage stage clusters
    * redirect to dummy-url integration clusters

Signed-off-by: Yoni Bettan <ybettan@redhat.com>

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

Please, select one or more if needed:

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

It's recommended to take a few extra minutes to provide more information about
how this code was tested. Here are some questions that may be worth answering:

  • Should this PR be tested by the reviewer?
  • Is this PR relying on CI for an e2e test run?
  • Should this PR be tested in a specific environment?
  • Any logs, screenshots, etc that can help with the review process?

Manual system tests:

I have run test-infra locally and we can see that no manifest was created to redirect the metrics (we defaulted to prod):

$ curl -s $(minikube service assisted-service -n assisted-installer --url)/api/assisted-install/v1/clusters/baa05cab-8b82-41e0-aef2-6771e350c2d2/manifests | jq '.'
[
  {
    "file_name": "50-masters-chrony-configuration.yaml",
    "folder": "openshift"
  },
  {
    "file_name": "50-workers-chrony-configuration.yaml",
    "folder": "openshift"
  }
]

Also, in the created cluster, we can see that the ConfigMap for redirection wasn't created:

$ oc --kubeconfig kubeconfig describe cm/cluster-monitoring-config -n openshift-monitoring
Error from server (NotFound): configmaps "cluster-monitoring-config" not found

Assignees

Please, add one or two reviewers that could help review this PR.

/assign @
/assign @

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • Reviewers have been listed
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@openshift-ci
Copy link

openshift-ci bot commented Jun 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2021
@ybettan ybettan changed the title CPBUGSM-30853: Defaulting metrics from all installed clusters to prod… CPBUGSM-30853: Defaulting metrics from all installed clusters to prod telemeter. Jun 14, 2021
@ybettan ybettan force-pushed the redirect-telemeter branch 4 times, most recently from 8199375 to 73c9e62 Compare June 14, 2021 06:46
@ybettan ybettan changed the title CPBUGSM-30853: Defaulting metrics from all installed clusters to prod telemeter. OCPBUGSM-30853: Change default telemeter server of installed clusters to prod. Jun 14, 2021
stageServiceBaseURL = "https://api.stage.openshift.com"
integrationServiceBaseURL = "https://api.integration.openshift.com"
stageTelemeterURL = "https://infogw.api.stage.openshift.com"
dummyURL = "https://dummy.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, but how about using stuff from .invalid TLD instead of domain that is registered to someone somewhere? It should not matter much until we decide to send there (accidentally or not) some data that we should not

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 don't see any reason why not to use https://dummy.invalid.
Anyway, I want to keep this PR minimal as possible - only the bug fix.
What you see is only the indention change but this wasn't inserted in this PR.
@mkowalski

Thanks for your review BTW, I have learned something new :)

Copy link
Member

Choose a reason for hiding this comment

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

@mkowalski @ybettan can one of you open an issue to track this? Or a PR to fix it?
I think this is a great point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have opened #1993 for that

Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks

… to prod.

Until now, in the service, we changed the default behavior of openshift,
which is to always send cluster metrics to prod-telemeter-server unless
changed by the user, and we:
    * left untouched clusters created by cloud prod env
    * redirected to telemeter-stage stage clusters
    * redirected to dummy-url all other clusters

The issue with this approach is that all prod clusters that aren't
created in the cloud, operator clusters for example,  will fail to
deliver telemetry.

Instead, we will now default to prod-telemeter instead of dummy-url
unless we know better:
    * left untouched clusters created by ALL prod envs (not just cloud)
    * redirect to telemeter-stage stage clusters
    * redirect to dummy-url integration clusters

Signed-off-by: Yoni Bettan <ybettan@redhat.com>
@ybettan
Copy link
Contributor Author

ybettan commented Jun 14, 2021

/assign @eranco74
FYI @nshidlin

@ybettan
Copy link
Contributor Author

ybettan commented Jun 14, 2021

/retest

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 Jun 14, 2021
@openshift-bot
Copy link
Contributor

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 06d15ca into openshift:master Jun 14, 2021
@ybettan ybettan deleted the redirect-telemeter branch June 15, 2021 06:52
ybettan added a commit to ybettan/assisted-service that referenced this pull request Jun 15, 2021
… to prod. (openshift#1988)

Until now, in the service, we changed the default behavior of openshift,
which is to always send cluster metrics to prod-telemeter-server unless
changed by the user, and we:
    * left untouched clusters created by cloud prod env
    * redirected to telemeter-stage stage clusters
    * redirected to dummy-url all other clusters

The issue with this approach is that all prod clusters that aren't
created in the cloud, operator clusters for example,  will fail to
deliver telemetry.

Instead, we will now default to prod-telemeter instead of dummy-url
unless we know better:
    * left untouched clusters created by ALL prod envs (not just cloud)
    * redirect to telemeter-stage stage clusters
    * redirect to dummy-url integration clusters

Signed-off-by: Yoni Bettan <ybettan@redhat.com>
ybettan added a commit to ybettan/assisted-service that referenced this pull request Jun 15, 2021
… to prod. (openshift#1988)

Until now, in the service, we changed the default behavior of openshift,
which is to always send cluster metrics to prod-telemeter-server unless
changed by the user, and we:
    * left untouched clusters created by cloud prod env
    * redirected to telemeter-stage stage clusters
    * redirected to dummy-url all other clusters

The issue with this approach is that all prod clusters that aren't
created in the cloud, operator clusters for example,  will fail to
deliver telemetry.

Instead, we will now default to prod-telemeter instead of dummy-url
unless we know better:
    * left untouched clusters created by ALL prod envs (not just cloud)
    * redirect to telemeter-stage stage clusters
    * redirect to dummy-url integration clusters

Signed-off-by: Yoni Bettan <ybettan@redhat.com>
ybettan added a commit to ybettan/assisted-service that referenced this pull request Jun 15, 2021
… prod. (openshift#1988)

Until now, in the service, we changed the default behavior of openshift,
which is to always send cluster metrics to prod-telemeter-server unless
changed by the user, and we:
    * left untouched clusters created by cloud prod env
    * redirected to telemeter-stage stage clusters
    * redirected to dummy-url all other clusters

The issue with this approach is that all prod clusters that aren't
created in the cloud, operator clusters for example,  will fail to
deliver telemetry.

Instead, we will now default to prod-telemeter instead of dummy-url
unless we know better:
    * left untouched clusters created by ALL prod envs (not just cloud)
    * redirect to telemeter-stage stage clusters
    * redirect to dummy-url integration clusters

Signed-off-by: Yoni Bettan <ybettan@redhat.com>
openshift-merge-robot pushed a commit that referenced this pull request Jun 17, 2021
… prod. (#1988) (#2003)

Until now, in the service, we changed the default behavior of openshift,
which is to always send cluster metrics to prod-telemeter-server unless
changed by the user, and we:
    * left untouched clusters created by cloud prod env
    * redirected to telemeter-stage stage clusters
    * redirected to dummy-url all other clusters

The issue with this approach is that all prod clusters that aren't
created in the cloud, operator clusters for example,  will fail to
deliver telemetry.

Instead, we will now default to prod-telemeter instead of dummy-url
unless we know better:
    * left untouched clusters created by ALL prod envs (not just cloud)
    * redirect to telemeter-stage stage clusters
    * redirect to dummy-url integration clusters

Signed-off-by: Yoni Bettan <ybettan@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants