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

Two bugfixes in installTunneler #76741

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@logicalhan
Copy link
Contributor

commented Apr 17, 2019

Two bug fixes:

  1. We should at least log something out if we fail to register our health check instead of pretending it didn't happen
  2. If we're going to go through the trouble of making a metric, we should probably actually register it. Also, I've gone ahead and deleted the deprecated version of the metric since no one can be broken by changing a metric that they never see.

What type of PR is this?

/kind bug
/kind cleanup

NONE

/sig api-machinery instrumentation

Two bug fixes: (1) at least log something out if we fail to register …
…our health check, (2) actually register a prometheus metric. I delete the deprecated metric in this block because there isn't any point to it, since no one can be broken by changing a metric that doesn't get collected
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: logicalhan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: sttts

If they are not already assigned, you can assign the PR to them by writing /assign @sttts in a comment when ready.

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

@k8s-ci-robot k8s-ci-robot requested review from luxas and roberthbailey Apr 17, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

/test pull-kubernetes-e2e-gke

@logicalhan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/retest

@logicalhan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/test pull-kubernetes-e2e-gke

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

/skip

@logicalhan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/test pull-kubernetes-e2e-gke

2 similar comments
@logicalhan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/test pull-kubernetes-e2e-gke

@logicalhan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/test pull-kubernetes-e2e-gke

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

job is persistently failing across PRs - https://prow.k8s.io/job-history/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-e2e-gke

I'd like to see this tested on a setup where it is exercised in some form before merge, just let me know when that has been done

@logicalhan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

job is persistently failing across PRs - https://prow.k8s.io/job-history/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-e2e-gke

I'd like to see this tested on a setup where it is exercised in some form before merge, just let me know when that has been done

For sure, I will ping you once it is tested.

@cheftako

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

So we are fixing a deprecated metric on a deprecated feature. The metric seems to be more validating that wait.Until is firing correctly and not that the sync worked. If the sync fails it will still update the last sync metric. Given that this metric was never registered it can be safely deleted and I think that is our best option. (If the feature were not deprecated I would suggest we actually fix the metric before registering it)

@logicalhan logicalhan force-pushed the logicalhan:install-tunneler-bugs branch from 5e90fd3 to bb6feed Apr 29, 2019

@logicalhan logicalhan force-pushed the logicalhan:install-tunneler-bugs branch from bb6feed to 85bcd4f Apr 29, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@logicalhan: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke d010494 link /test pull-kubernetes-e2e-gke

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@cheftako

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

/test pull-kubernetes-verify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.