Short circuiting backoff #814

mshitrit · 2021-03-02T08:25:25Z

This PR is the implementation of this enhancement.
The main purpose is to allow create a large enough time frame for an admin to fix certain issues by delaying the remediation time for machines that are in a failed stated and has no node.

openshift-ci-robot · 2021-03-02T08:25:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign alexander-demichev after the PR has been reviewed.
You can assign the PR to them by writing /assign @alexander-demichev 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JoelSpeed

Changes look ok, I've added a bunch of suggestions. I'd like to see some more thorough unit testing of this feature before we merge though, at the moment it's not obvious that we have test cases in place that cover the new behaviour

pkg/apis/machine/v1beta1/machinehealthcheck_types.go

pkg/controller/machinehealthcheck/machinehealthcheck_controller.go

pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

elmiko

this mostly makes sense to me, just a question about the defaults.

pkg/apis/machine/v1beta1/machinehealthcheck_types.go

JoelSpeed

Because I went through this commit by commit, there may be some comments that are fixed by later commits.

Could you do a rebase and fix up the commits and commit messages so that the commits are atomic and have good descriptions

Please also add a decent description to the PR

No major blockers from my side though, mostly nits/ additional testing required

pkg/controller/machinehealthcheck/machinehealthcheck_controller.go

pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

pkg/apis/machine/v1beta1/machinehealthcheck_types.go

pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

elmiko · 2021-05-05T13:33:30Z

looks like this might need a rebase?

mshitrit · 2021-05-06T07:01:31Z

looks like this might need a rebase?

Hi @elmiko , any idea if you are planning to merge it soon ?
(I'd like to know whether to prioritize this)

elmiko · 2021-05-06T13:27:18Z

Hi @elmiko , any idea if you are planning to merge it soon ?

good question. given that we don't have a bugzilla associated with this, i'm guessing we are waiting for 4.8 master to open, if that's the case then you have a couple weeks before we could merge it. (cc @JoelSpeed )

JoelSpeed · 2021-05-06T13:41:25Z

Yep, this will have to wait until the 4.9 master branch opens towards the end of May

Signed-off-by: Michael Shitrit <mshitrit@redhat.com>

mshitrit · 2021-05-23T14:56:59Z

/retest

mshitrit · 2021-05-24T06:19:43Z

/retest

JoelSpeed · 2021-06-15T16:13:06Z

/bugzilla refresh
/retest

openshift-ci · 2021-06-15T16:13:07Z

@JoelSpeed: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh
/retest

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

I think this is pretty much ready to go apart from the default.

Would like to make sure we have support upstream for this too, let's make sure we are having that conversation

pkg/apis/machine/v1beta1/machinehealthcheck_types.go

install/0000_30_machine-api-operator_07_machinehealthcheck.crd.yaml

Signed-off-by: Michael Shitrit <mshitrit@redhat.com>

JoelSpeed · 2021-06-29T09:48:49Z

/lgtm

Would like to make sure we have support upstream for this too, let's make sure we are having that conversation

Would like to pursue this (at least start the conversation) before we merge this if possible. We have just under 4 weeks til feature freeze, I think we could probably get some consensus one way or another by then

mshitrit · 2021-06-29T10:59:50Z

/lgtm

Would like to make sure we have support upstream for this too, let's make sure we are having that conversation

Would like to pursue this (at least start the conversation) before we merge this if possible. We have just under 4 weeks til feature freeze, I think we could probably get some consensus one way or another by then

Adding Marc, I assume he'll be involved in necessary upstream process.
/cc @slintes

mshitrit · 2021-06-29T11:00:04Z

/retest

mshitrit · 2021-06-29T14:18:35Z

/retest

mshitrit · 2021-08-22T06:16:02Z

@JoelSpeed , @elmiko
Is this PR still relevant ?

JoelSpeed · 2021-08-22T18:29:59Z

As far as I'm aware, this is still an issue that we wanted to solve. As far as I know, nothing else has solved this issue just yet.

CC @slintes @beekhof

…backoff # Conflicts: # pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

openshift-ci · 2021-08-23T06:41:52Z

New changes are detected. LGTM label has been removed.

slintes · 2021-10-11T10:44:27Z

/test all

slintes · 2021-10-20T12:06:41Z

/retest

JoelSpeed · 2021-10-21T12:05:23Z

/approve

I think this is good and aligns with the enhancement document now. I'm keen to see if we can get agreement for this feature upstream as well. If we can hold off merging this for a little bit while we negotiate that, would be appreciated.

openshift-ci · 2021-10-21T12:05:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

elmiko

lgtm, +1 to what @JoelSpeed said

slintes · 2021-11-03T14:36:59Z

Leaving a link to the upstream discussiom here, I always have to search for it 😉
kubernetes-sigs/cluster-api#3106 (comment)

No activity since the last comment 2 weeks ago...

…ged_failing # Conflicts: # pkg/apis/machine/v1beta1/zz_generated.deepcopy.go # pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

Signed-off-by: Michael Shitrit <mshitrit@redhat.com>

openshift-ci · 2021-11-05T13:26:24Z

@mshitrit: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-workers-rhel7	`7855ff4`	link	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-vsphere	`5fdbfdb`	link	false	`/test e2e-vsphere`
ci/prow/e2e-vsphere-serial	`5fdbfdb`	link	true	`/test e2e-vsphere-serial`
ci/prow/e2e-vsphere-upgrade	`5fdbfdb`	link	false	`/test e2e-vsphere-upgrade`
ci/prow/e2e-vsphere-operator	`5fdbfdb`	link	false	`/test e2e-vsphere-operator`
ci/prow/e2e-gcp-operator	`5fdbfdb`	link	false	`/test e2e-gcp-operator`
ci/prow/e2e-aws-disruptive	`5fdbfdb`	link	false	`/test e2e-aws-disruptive`
ci/prow/e2e-metal-ipi-ovn-ipv6	`5fdbfdb`	link	false	`/test e2e-metal-ipi-ovn-ipv6`

Full PR test history. Your PR dashboard.

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.

mshitrit · 2021-11-23T08:21:19Z

Closing this PR since upstream is distinctly uninterested in this feature and the burden of maintaining the delta indefinitely was considered disproportionate to the benefits gained
Jira Link

openshift-ci-robot requested review from cynepco3hahue and paulfantom March 2, 2021 08:25

JoelSpeed reviewed Mar 3, 2021

View reviewed changes

elmiko reviewed Mar 16, 2021

View reviewed changes

pkg/apis/machine/v1beta1/machinehealthcheck_types.go Outdated Show resolved Hide resolved

zaneb mentioned this pull request Mar 19, 2021

MachineHealthCheck fights with MachineSet on invalid configuration #736

Closed

JoelSpeed reviewed Mar 23, 2021

View reviewed changes

mshitrit force-pushed the short_circuiting_backoff branch from 7855ff4 to bbbd72c Compare March 24, 2021 16:35

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2021

mshitrit force-pushed the short_circuiting_backoff branch from bbbd72c to 086f7e8 Compare May 9, 2021 07:17

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2021

functional changes: PR implementation + Unit Tests

f1e9ae5

Signed-off-by: Michael Shitrit <mshitrit@redhat.com>

mshitrit force-pushed the short_circuiting_backoff branch from 086f7e8 to f1e9ae5 Compare May 23, 2021 12:27

JoelSpeed reviewed Jun 28, 2021

View reviewed changes

pkg/apis/machine/v1beta1/machinehealthcheck_types.go Outdated Show resolved Hide resolved

install/0000_30_machine-api-operator_07_machinehealthcheck.crd.yaml Outdated Show resolved Hide resolved

Removing default value of FailedNodeStartupTimeout

e3d7784

Signed-off-by: Michael Shitrit <mshitrit@redhat.com>

openshift-ci bot assigned JoelSpeed Jun 29, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2021

openshift-ci bot requested a review from slintes June 29, 2021 10:59

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2021

Merge remote-tracking branch 'upstream/master' into short_circuiting_…

2d35d8e

…backoff # Conflicts: # pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 23, 2021

mshitrit mentioned this pull request Oct 14, 2021

short-circuiting-backoff openshift/enhancements#673

Merged

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2021

elmiko reviewed Oct 28, 2021

View reviewed changes

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 3, 2021

mshitrit added 2 commits November 5, 2021 12:48

Merge remote-tracking branch 'upstream/master' into short_circuit_mer…

e455a4a

…ged_failing # Conflicts: # pkg/apis/machine/v1beta1/zz_generated.deepcopy.go # pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

Merge fixes

5fdbfdb

Signed-off-by: Michael Shitrit <mshitrit@redhat.com>

mshitrit force-pushed the short_circuiting_backoff branch from 6dad461 to 5fdbfdb Compare November 5, 2021 11:04

mshitrit closed this Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Short circuiting backoff #814

Short circuiting backoff #814

mshitrit commented Mar 2, 2021 •

edited

Loading

openshift-ci-robot commented Mar 2, 2021

JoelSpeed left a comment

elmiko left a comment

JoelSpeed left a comment

elmiko commented May 5, 2021

mshitrit commented May 6, 2021

elmiko commented May 6, 2021

JoelSpeed commented May 6, 2021

mshitrit commented May 23, 2021

mshitrit commented May 24, 2021

JoelSpeed commented Jun 15, 2021

openshift-ci bot commented Jun 15, 2021

JoelSpeed left a comment

JoelSpeed commented Jun 29, 2021

mshitrit commented Jun 29, 2021

mshitrit commented Jun 29, 2021

mshitrit commented Jun 29, 2021

mshitrit commented Aug 22, 2021

JoelSpeed commented Aug 22, 2021

openshift-ci bot commented Aug 23, 2021

slintes commented Oct 11, 2021

slintes commented Oct 20, 2021

JoelSpeed commented Oct 21, 2021

openshift-ci bot commented Oct 21, 2021

elmiko left a comment

slintes commented Nov 3, 2021

openshift-ci bot commented Nov 5, 2021 •

edited

Loading

mshitrit commented Nov 23, 2021

Short circuiting backoff #814

Short circuiting backoff #814

Conversation

mshitrit commented Mar 2, 2021 • edited Loading

openshift-ci-robot commented Mar 2, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

elmiko commented May 5, 2021

mshitrit commented May 6, 2021

elmiko commented May 6, 2021

JoelSpeed commented May 6, 2021

mshitrit commented May 23, 2021

mshitrit commented May 24, 2021

JoelSpeed commented Jun 15, 2021

openshift-ci bot commented Jun 15, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed commented Jun 29, 2021

mshitrit commented Jun 29, 2021

mshitrit commented Jun 29, 2021

mshitrit commented Jun 29, 2021

mshitrit commented Aug 22, 2021

JoelSpeed commented Aug 22, 2021

openshift-ci bot commented Aug 23, 2021

slintes commented Oct 11, 2021

slintes commented Oct 20, 2021

JoelSpeed commented Oct 21, 2021

openshift-ci bot commented Oct 21, 2021

elmiko left a comment

Choose a reason for hiding this comment

slintes commented Nov 3, 2021

openshift-ci bot commented Nov 5, 2021 • edited Loading

mshitrit commented Nov 23, 2021

mshitrit commented Mar 2, 2021 •

edited

Loading

openshift-ci bot commented Nov 5, 2021 •

edited

Loading