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.

Sign up for GitHub

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

Jump to bottom

✨ Allow users to disable MHC NodeStartupTimeout #4471

Merged
merged 1 commit into from
May 19, 2021
Merged

✨ Allow users to disable MHC NodeStartupTimeout #4471

merged 1 commit into from
May 19, 2021

Conversation

JoelSpeed
Copy link
Contributor

What this PR does / why we need it:
This add a new explicit 0 second option for the MHC NodeStartupTimeout which allows users to disable the functionality provided by NodeStartupTimeout. This will allow use cases where customers want to explicitly just have MHC with condition based health checks.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4468

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 13, 2021
@JoelSpeed JoelSpeed mentioned this pull request Apr 13, 2021
Closed
elmiko
elmiko reviewed Apr 13, 2021
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

generally makes sense to me, added a nit and a question

api/v1alpha4/machinehealthcheck_types.go Outdated Show resolved Hide resolved
config/crd/bases/cluster.x-k8s.io_machinehealthchecks.yaml Outdated Show resolved Hide resolved
controllers/machinehealthcheck_targets.go Outdated Show resolved Hide resolved
controllers/machinehealthcheck_targets.go Outdated Show resolved Hide resolved
vincepri
vincepri requested changes Apr 13, 2021
View reviewed changes
api/v1alpha4/machinehealthcheck_types.go Show resolved Hide resolved
controllers/machinehealthcheck_targets.go Outdated Show resolved Hide resolved
controllers/machinehealthcheck_targets.go Outdated Show resolved Hide resolved
controllers/machinehealthcheck_targets.go Outdated Show resolved Hide resolved
@elmiko
Copy link
Contributor

elmiko commented Apr 13, 2021

something else i had wondered, is there a doc update we can include with this patch?

elmiko
elmiko approved these changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

thanks for the updates Joel, looking good to me
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 14, 2021
elmiko
elmiko approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

updates looks nice to me
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2021
@sbueringer
Copy link
Member

/retest
(known flake, fix PR is open)

vincepri
vincepri reviewed Apr 19, 2021
View reviewed changes
controllers/machinehealthcheck_targets.go Outdated Show resolved Hide resolved
controllers/machinehealthcheck_targets.go Outdated Show resolved Hide resolved
api/v1alpha4/machinehealthcheck_types.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2021
@JoelSpeed JoelSpeed force-pushed the disable-node-startup-timeout branch from a27d3b1 to d0e1d89 Compare May 7, 2021 15:10
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2021
@JoelSpeed
Copy link
Contributor Author

We had mentioned about plumbing metav1.Durations through all of the healthcheck stuff, I've added this in the last commit, not entirely sure if it makes things better or not though 🤔

vincepri
vincepri reviewed May 10, 2021
View reviewed changes
api/v1alpha4/common_types.go Outdated Show resolved Hide resolved
controllers/machinehealthcheck_controller.go Outdated Show resolved Hide resolved
api/v1alpha4/machinehealthcheck_types.go Show resolved Hide resolved
@fabriziopandini
Copy link
Member

changes Loog god to me, pending squash.
It would also help if addressed comment get resolved so we can check if something is still pending

@JoelSpeed
Allow users to disable MHC NodeStartupTimeout
2be6be2 
This add a new explicit 0 second option for the MHC NodeStartupTimeout
which allows users to disable the functionality provided by
NodeStartupTimeout. This will allow use cases where customers want to
explicitly just have MHC with condition based health checks.
@JoelSpeed JoelSpeed force-pushed the disable-node-startup-timeout branch from 8f50908 to 2be6be2 Compare May 19, 2021 12:55
@JoelSpeed
Copy link
Contributor Author

Thanks @fabriziopandini, I've resolved all of the comments that have been addressed and squashed, I don't think there's anything left to fix

elmiko
elmiko approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2021
@fabriziopandini
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2021
@k8s-ci-robot k8s-ci-robot merged commit 06f58a6 into kubernetes-sigs:master May 19, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@michaelgugino michaelgugino michaelgugino left review comments

@sbueringer sbueringer sbueringer left review comments

@vincepri vincepri vincepri requested changes

@enxebre enxebre enxebre left review comments

@detiber detiber detiber left review comments

@elmiko elmiko elmiko approved these changes

Assignees

@elmiko elmiko

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

Add option to disable NodeStartupTimeout on MachineHealthCheck
10 participants
@JoelSpeed @elmiko @sbueringer @fabriziopandini @k8s-ci-robot @detiber @enxebre @vincepri @michaelgugino @CecileRobertMichon