Bug 1823667: UPSTREAM: <carry>: Compare against minSize in deleteNodes() #148

enxebre · 2020-05-04T10:24:31Z

When calling deleteNodes() we should fail early if the operation could delete nodes below the nodeGroup minSize().

This is one in a series of PR to mitigate kubernetes#3104
Once we got all of them merged we'll put a PR upstream.

openshift-ci-robot · 2020-05-04T10:27:33Z

@enxebre: An error was encountered adding this pull request to the external tracker bugs for bug 1823667 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/kubernetes-autoscaler/pulls/148 Error: 403 Forbidden at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 107. at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 107. eval {...} called at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 94 Bugzilla::Extension::ExternalBugs::Type::GitHub::_do_rest_call('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x55fed3...', 'https://api.github.com/repos/openshift/kubernetes-autoscaler/...', 'GET') called at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 58 Bugzilla::Extension::ExternalBugs::Type::GitHub::get_data('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x55fed3...', 'Bugzilla::Extension::ExternalBugs::Bug=HASH(0x55fed3a0b460)') called at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/Bug.pm line 298 eval {...} called at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/Bug.pm line 298 Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x55fed3a0b460)', 1) called at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/Bug.pm line 121 Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x55fed31bad68)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 873 Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x55fed2f5c6e8)', 'HASH(0x55fed2f9c338)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21 Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x55fed2f9c338)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1170 Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x55fed2fe9f10)') called at /loader/0x55fec87b60d0/Bugzilla/Extension/ExternalBugs/WebService.pm line 84 Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x55fed39b35b8)') called at (eval 2157) 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(0x55fed35ca900)') 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(0x55fed35ca900)') 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(0x55fed39b12b8)') 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(0x55fed34c4f20)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173 ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x55fed34c4f20)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32 ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x55fed39b12b8)') called at /var/www/html/bugzilla/mod_perl.pl line 139 Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x55fed39b12b8)') called at (eval 2157) line 0 eval {...} called at (eval 2157) line 0
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1823667: UPSTREAM: : Compare against minSize in deleteNodes()

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.

JoelSpeed · 2020-05-04T10:55:52Z

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go

@@ -92,6 +92,7 @@ func (ng *nodegroup) IncreaseSize(delta int) error {
 // Implementation required.
 func (ng *nodegroup) DeleteNodes(nodes []*corev1.Node) error {
 	// Step 1: Verify all nodes belong to this node group.
+


Is this new line intentional?

This is resolved, the above code wasn't present when I first reviewed

JoelSpeed · 2020-05-04T10:59:46Z

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go

-		return fmt.Errorf("unable to delete %d machines in %q, machine replicas are < 0 ", len(nodes), ng.Id())
-	} else if replicas-int32(len(nodes)) == 0 && !ng.scalableResource.CanScaleFromZero() {
-		return fmt.Errorf("unable to delete %d machines in %q, machine replicas are 0", len(nodes), ng.Id())
+	if replicas-int32(len(nodes)) <= int32(ng.MinSize()) {


I think this should be < rather than <= shouldn't it?

Eg, if I have 3 replicas and want to remove 2, with min size 1, this should be allowed but 3-2 <= 1 is true

JoelSpeed

/approve

elmiko

this lgtm, holding on label for Joel's question

JoelSpeed · 2020-05-05T09:02:22Z

My comment was resolved, LGTM on @elmiko's behalf

/lgtm

enxebre · 2020-05-05T09:23:38Z

/bugzilla refresh

openshift-ci-robot · 2020-05-05T09:23:42Z

@enxebre: This pull request references Bugzilla bug 1823667, 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.

openshift-ci-robot · 2020-05-05T12:00:16Z

New changes are detected. LGTM label has been removed.

openshift-ci-robot · 2020-05-05T12:00:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JoelSpeed
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found 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-robot · 2020-05-05T12:07:16Z

@enxebre: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/unit	`9d397d2`	link	`/test unit`

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.

openshift-ci-robot requested review from alexander-demicev and JoelSpeed May 4, 2020 10:24

enxebre changed the title ~~UPSTREAM: <carry>: Compare against minSize in deleteNodes()~~ Bug 1823667: UPSTREAM: <carry>: Compare against minSize in deleteNodes() May 4, 2020

JoelSpeed reviewed May 4, 2020

View reviewed changes

enxebre force-pushed the deleteNodes-check branch 4 times, most recently from 7323be7 to 2d24858 Compare May 4, 2020 11:07

JoelSpeed reviewed May 4, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2020

elmiko approved these changes May 4, 2020

View reviewed changes

openshift-ci-robot assigned JoelSpeed May 5, 2020

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

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label May 5, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 5, 2020

openshift-ci-robot mentioned this pull request May 5, 2020

Bug 1823667: UPSTREAM: <carry>: Get replicas always from API server #147

Merged

enxebre closed this May 5, 2020

enxebre force-pushed the deleteNodes-check branch from 2d24858 to 9d397d2 Compare May 5, 2020 12:00

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 5, 2020

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2020

enxebre mentioned this pull request May 5, 2020

Bug 1823667: UPSTREAM: <carry>: Compare against minSize in deleteNodes() #150

Merged

openshift-ci-robot mentioned this pull request May 21, 2020

Bug 1823667: UPSTREAM: <carry>: Add mutex to DeleteNodes #149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1823667: UPSTREAM: <carry>: Compare against minSize in deleteNodes() #148

Bug 1823667: UPSTREAM: <carry>: Compare against minSize in deleteNodes() #148

enxebre commented May 4, 2020 •

edited

Loading

openshift-ci-robot commented May 4, 2020

JoelSpeed May 4, 2020

JoelSpeed May 5, 2020

JoelSpeed May 4, 2020

JoelSpeed left a comment

elmiko left a comment

JoelSpeed commented May 5, 2020

enxebre commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

Bug 1823667: UPSTREAM: <carry>: Compare against minSize in deleteNodes() #148

Bug 1823667: UPSTREAM: <carry>: Compare against minSize in deleteNodes() #148

Conversation

enxebre commented May 4, 2020 • edited Loading

openshift-ci-robot commented May 4, 2020

JoelSpeed May 4, 2020

Choose a reason for hiding this comment

JoelSpeed May 5, 2020

Choose a reason for hiding this comment

JoelSpeed May 4, 2020

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

JoelSpeed commented May 5, 2020

enxebre commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

enxebre commented May 4, 2020 •

edited

Loading