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

UPSTREAM: <carry>: Fix to avoid REST API calls at log level 2. #15934

Merged

Conversation

aveshagarwal
Copy link
Contributor

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1484095. Its a regression in origin master (3.7). @sjenning

This PR is carry because the previous PR to 3.6 was carry too (#13844) due to uncertainty around upstream PR (kubernetes/kubernetes#40933).

@stevekuznetsov @deads2k Also is something broken in the rebase process or any related scripts that previous carry PR (#13844) was dropped? Not to blame anyone, just informing as I am wondering if there are other carry PRs that got dropped and we are not aware causing regressions in future, so that if something is broken (process or script), can be fixed.

@openshift-merge-robot openshift-merge-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 23, 2017
@deads2k
Copy link
Contributor

deads2k commented Aug 23, 2017

This was not accidentally lost. It was intentionally removed since it was rejected upstream and directions for suppressing in openshift without upstream patching provided: #15660 .

@aveshagarwal
Copy link
Contributor Author

@deads2k thanks.

@aveshagarwal
Copy link
Contributor Author

@deads2k Do you know how to use glog's vmodule with openshift?

@deads2k
Copy link
Contributor

deads2k commented Aug 28, 2017

@deads2k Do you know how to use glog's vmodule with openshift?

It's --logspec= right?

@aveshagarwal
Copy link
Contributor Author

@deads2k nice, thanks.

@sjenning
Copy link
Contributor

sjenning commented Sep 15, 2017

/assign @eparis

/retest

@eparis
Copy link
Member

eparis commented Sep 15, 2017

thank you switching from upstream to carry.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, eparis

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2017
@eparis
Copy link
Member

eparis commented Sep 15, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16089, 16305, 16219, 15934, 16366)

@openshift-merge-robot openshift-merge-robot merged commit 428befa into openshift:master Sep 16, 2017
@deads2k
Copy link
Contributor

deads2k commented Sep 22, 2017

thank you switching from upstream to carry.
/lgtm

@eparis Why are we carrying patch for this instead of updating our deployment mechanism to pass the flags designed to keep exactly this sort of message under control? It's needless debt to tote around.

@mfojtik
Copy link
Contributor

mfojtik commented Sep 22, 2017

+1 this is just increasing our tech debt. @eparis can we revert this and fix it using vmodule?

@soltysh
Copy link
Member

soltysh commented Sep 22, 2017

+1 we cannot increase the amount of our carry patches, especially if a reasonable workaround is available. Carries should by always our last resort.

@eparis
Copy link
Member

eparis commented Sep 22, 2017

I agree that carries are horrible. I'm still fighting upstream. Agreeing with me there would be a better solution to this problem. V(2) is the standard logging level and should not contain spammy debug.

kubernetes/kubernetes#40933

We will not ship a product with this spam at V(2). If you would like to overrule me please talk to @smarterclayton

If the answer is vmodule instead of this 1 liner that means either

  1. The end user (of which we have literally hundreds) must get this right. This is an absolute non-starter.
    or
  2. The openshift ansible team should deal with this. BUT that is actually saying that they must tote this tech debt instead of the master team. Two major problems with that. (a) They will not know if the location of the log spam moved. Only the team doing a rebase will know is this moves and needs updated. and (b) we have (sadly) many customers who don't use openshift-ansible. Thus they digress to case 1, which is a non-starter.

If there is a better way than this 1 liner I'm fine with that, but I admit that any such solution will suffer from the problems of 2(b).

I like to believe that I recognize that carries are the worst thing we do/have and have rarely said we need to do it. But I am saying that this time. Making our product usable and maintainable by our users will always be my top priority. Even if it sometimes means more work for us.

@deads2k
Copy link
Contributor

deads2k commented Sep 22, 2017

We will not ship a product with this spam at V(2). If you would like to overrule me please talk to @smarterclayton

This needs to be agreed to as a strong goal in the upstream product or you will be chasing these across the entire stack and just hoping a local approver agrees with you. If @smarterclayton agrees with you, you two should give us a place we can point to in the community repo that gives a frequency of particular v(2) messages.

The end user (of which we have literally hundreds) must get this right. This is an absolute non-starter.
or
The openshift ansible team should deal with this. BUT that is actually saying that they must tote this tech debt instead of the master team. Two major problems with that. (a) They will not know if the location of the log spam moved. Only the team doing a rebase will know is this moves and needs updated. and (b) we have (sadly) many customers who don't use openshift-ansible. Thus they digress to case 1, which is a non-starter.

Or you choose to centralize all of your logging patches using a vmodule defaulting/reseting on the flag we're already propogating through via a shim. Making it obvious in one place, all the messages we're having trouble with, eliminating any carry conflicts, and avoiding special-case patches that require ongoing maintenance.

@eparis
Copy link
Member

eparis commented Sep 22, 2017

The vmodule suggestion has similar but worse carry problems since it won't conflict and you won't know you need to update it. It suffers from the same issues as 2(a).

@aveshagarwal aveshagarwal deleted the master-rhbz-1484095 branch June 15, 2018 15:20
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants