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

Bug 1731222: unwarranted SARs on foreground GC of objects with owner refs #102

Conversation

gabemontero
Copy link
Contributor

@gabemontero gabemontero commented Apr 30, 2020

See https://coreos.slack.com/archives/CB48XQ4KZ/p1588008887095400 for history

/assign @deads2k

@deads2k - I could not find any official guideline on how to (a presumably temporary until a rebase pulls it in) copy of k8s in lieu of vendoring; please advise if code changes / commit wording etc. need modification

/assign @dmage

@dmage if possible minimally pls make sure this change catches your instance of the problem

@openshift/openshift-team-developer-experience @bparees FYI

/hold

since the upstream PR kubernetes/kubernetes#90534 has not merged yet ... however, if during review we think it is just a matter of when I'll unhold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2020
@openshift-ci-robot
Copy link

@gabemontero: An error was encountered adding this pull request to the external tracker bugs for bug 1731222 on the Bugzilla server at https://bugzilla.redhat.com:

JSONRPC error 32000: There was an error reported for a GitHub REST call. URL: https://api.github.com/repos/openshift/openshift-apiserver/pulls/102 Error: 404 Not Found at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 107. at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 107. eval {...} called at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 94 Bugzilla::Extension::ExternalBugs::Type::GitHub::_do_rest_call('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x560cf8...', 'https://api.github.com/repos/openshift/openshift-apiserver/pu...', 'GET') called at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 58 Bugzilla::Extension::ExternalBugs::Type::GitHub::get_data('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x560cf8...', 'Bugzilla::Extension::ExternalBugs::Bug=HASH(0x560cf9524158)') called at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/Bug.pm line 298 eval {...} called at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/Bug.pm line 298 Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x560cf9524158)', 1) called at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/Bug.pm line 121 Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x560cf955b828)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 873 Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x560cf8a2d568)', 'HASH(0x560cf8fde518)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21 Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x560cf8fde518)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1170 Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x560cf956b2e8)') called at /loader/0x560cee30c048/Bugzilla/Extension/ExternalBugs/WebService.pm line 84 Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x560cf937b7c0)') called at (eval 2198) line 1 eval ' $procedure->{code}->($self, @params) ;' called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 220 JSON::RPC::Legacy::Server::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x560cf92a0ef8)') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 295 Bugzilla::WebService::Server::JSONRPC::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x560cf92a0ef8)') called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 126 JSON::RPC::Legacy::Server::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 70 Bugzilla::WebService::Server::JSONRPC::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/jsonrpc.cgi line 31 ModPerl::ROOT::Bugzilla::ModPerl::ResponseHandler::var_www_html_bugzilla_jsonrpc_2ecgi::handler('Apache2::RequestRec=SCALAR(0x560cf92a10f0)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 eval {...} called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 ModPerl::RegistryCooker::run('Bugzilla::ModPerl::ResponseHandler=HASH(0x560cf8c42840)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173 ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x560cf8c42840)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32 ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x560cf92a10f0)') called at /var/www/html/bugzilla/mod_perl.pl line 139 Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x560cf92a10f0)') called at (eval 2198) line 0 eval {...} called at (eval 2198) line 0
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1731222: copy/use kubernetes/kubernetes#90534 for IsMu…

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.

@gabemontero
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 30, 2020
@openshift-ci-robot
Copy link

@gabemontero: This pull request references Bugzilla bug 1731222, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

@gabemontero
Copy link
Contributor Author

note for reviewers - I've confirmed this changes is not needed prior to 4.5, at least for templates

I confirmed all the way back to 4.2

@gabemontero
Copy link
Contributor Author

/retest

Copy link
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

Nit on the commit & PR title:

  1. Reference the bug in the title line of the git commit
  2. Leave a shorter title that makes sense to a customer

You should continue to reference the upstream PR in the details of the commit.

@gabemontero gabemontero force-pushed the copy-upstream-k8s-k8s-pull-90534 branch from 6556e17 to 418f0ce Compare May 1, 2020 18:19
@gabemontero gabemontero changed the title Bug 1731222: copy/use https://github.com/kubernetes/kubernetes/pull/90534 for IsMu… Bug 1731222: unwarranted SARs on foreground GC of objects with owner refs May 1, 2020
@openshift-ci-robot
Copy link

@gabemontero: This pull request references Bugzilla bug 1731222, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1731222: unwarranted SARs on foreground GC of objects with owner refs

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.

@gabemontero
Copy link
Contributor Author

Nit on the commit & PR title:

1. Reference the bug in the title line of the git commit

2. Leave a shorter title that makes sense to a customer

You should continue to reference the upstream PR in the details of the commit.

both PR title and commit updated @adambkaplan

@gabemontero
Copy link
Contributor Author

all green tests again @deads2k @dmage @adambkaplan

@openshift-ci-robot
Copy link

@gabemontero: This pull request references Bugzilla bug 1731222, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1731222: unwarranted SARs on foreground GC of objects with owner refs

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.

@gabemontero
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2020
@deads2k
Copy link
Contributor

deads2k commented May 8, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 8, 2020
@deads2k
Copy link
Contributor

deads2k commented May 8, 2020

/hold

move the package to where you use it please

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 8, 2020
@gabemontero gabemontero force-pushed the copy-upstream-k8s-k8s-pull-90534 branch from 418f0ce to 894f585 Compare May 8, 2020 15:19
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 8, 2020
@gabemontero
Copy link
Contributor Author

per slack conversation with @deads2k we put versions of the new mutating GC check in both packages

@gabemontero
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 8, 2020
@deads2k
Copy link
Contributor

deads2k commented May 8, 2020

/lgtm
/hold cancel

this duplication makes sense because these are logically distinct resources that are unlikely to live together long term. The upstream reference and the reflective test ensure they will stay in sync. This also eliminates a k/k dep. Good PR.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, gabemontero

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 4c982be into openshift:master May 8, 2020
@openshift-ci-robot
Copy link

@gabemontero: Some pull requests linked via external trackers have merged: openshift/openshift-apiserver#102. The following pull requests linked via external trackers have not merged:

In response to this:

Bug 1731222: unwarranted SARs on foreground GC of objects with owner refs

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.

@gabemontero gabemontero deleted the copy-upstream-k8s-k8s-pull-90534 branch May 8, 2020 17:04
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. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. 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