Skip to content

Conversation

@n1r1
Copy link
Contributor

@n1r1 n1r1 commented Oct 11, 2020

Modify machine health checks documentation to include an option to trigger power-cycle instead of machine deletion.
This is only relevant for IPI baremetal clusters and starting from OCP 4.5

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 11, 2020
@n1r1
Copy link
Contributor Author

n1r1 commented Oct 11, 2020

cc @JoelSpeed @beekhof

@vikram-redhat
Copy link
Contributor

@kalexand-rh can you take a look? @n1r1 which version does it apply to?

@n1r1
Copy link
Contributor Author

n1r1 commented Oct 13, 2020

@vikram-redhat
4.5 + (i.e 4.5, 4.6)

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Added a couple of suggestions to improve the wording, but otherwise I think this is good

Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

I think this is a great addition, but I think the information would be more effective in different modules.

@n1r1 n1r1 force-pushed the baremetal-machine-healthcheck branch from a89c53e to d7597a8 Compare October 14, 2020 13:11
@n1r1
Copy link
Contributor Author

n1r1 commented Oct 14, 2020

Thank you both for the feedback.
I made the changes per your suggestions.

@JoelSpeed
Copy link
Contributor

LGTM

@n1r1 n1r1 requested a review from kalexand-rh October 26, 2020 07:03
@n1r1 n1r1 force-pushed the baremetal-machine-healthcheck branch from d7597a8 to c273425 Compare November 4, 2020 08:13
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 4, 2020
Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

@n1r1, thank you! I have a few more suggestions, but this is really good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<2>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalexand-rh is this suggestion still relevant per our last discussion?

If I'll remove this, we will have <2> below explaining about the annotation without any reference in the yaml.
let me know if you want to remove it anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

@n1r1, I think it might be clearest to provide two yaml sections. Will you PTAL at the commit that I've added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalexand-rh thanks.
Looks good to me

@n1r1
Copy link
Contributor Author

n1r1 commented Nov 4, 2020

Thanks @kalexand-rh
I'm not sure I understand the last suggestions.

The annotation is not related to the matchLabel sections.
Here's a full example with that annotation, if it helps to clarify things:

apiVersion: machine.openshift.io/v1beta1
kind: MachineHealthCheck
metadata:
  name: example 
  namespace: openshift-machine-api
  annotations:
    machine.openshift.io/remediation-strategy: external-baremetal
spec:
  selector:
    matchLabels:
      machine.openshift.io/cluster-api-machine-role: <role> 
      machine.openshift.io/cluster-api-machine-type: <role> 
      machine.openshift.io/cluster-api-machineset: <cluster_name>-<label>-<zone> 
  unhealthyConditions:
  - type:    "Ready"
    timeout: "300s" 
    status: "False"
  - type:    "Ready"
    timeout: "300s" 
    status: "Unknown"
  maxUnhealthy: "40%" 

@n1r1 n1r1 force-pushed the baremetal-machine-healthcheck branch from c273425 to 856c3db Compare November 30, 2020 06:47
@n1r1 n1r1 requested a review from kalexand-rh November 30, 2020 07:01
@n1r1 n1r1 force-pushed the baremetal-machine-healthcheck branch from 856c3db to 9a5ce86 Compare December 2, 2020 14:02
@n1r1 n1r1 force-pushed the baremetal-machine-healthcheck branch from 9a5ce86 to cf39b18 Compare December 20, 2020 14:54
@n1r1 n1r1 force-pushed the baremetal-machine-healthcheck branch from cf39b18 to 6ef0960 Compare January 25, 2021 06:25
@netlify
Copy link

netlify bot commented Jan 27, 2021

Deploy preview for osdocs ready!

Built with commit 4116786

https://deploy-preview-26321--osdocs.netlify.app

Modify machine health checks documentation to include an option to trigger power-cycle instead of machine deletion.
This is only relevant for IPI baremetal clusters and starting from OCP 4.5

Signed-off-by: Nir <[email protected]>
@kalexand-rh
Copy link
Contributor

/cherrypick enterprise-4.5

@kalexand-rh
Copy link
Contributor

/cherrypick enterprise-4.6

@kalexand-rh
Copy link
Contributor

/cherrypick enterprise-4.7

@openshift-cherrypick-robot

@kalexand-rh: new pull request created: #29380

Details

In response to this:

/cherrypick enterprise-4.5

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-cherrypick-robot

@kalexand-rh: new pull request created: #29381

Details

In response to this:

/cherrypick enterprise-4.6

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-cherrypick-robot

@kalexand-rh: new pull request created: #29382

Details

In response to this:

/cherrypick enterprise-4.7

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.5 branch/enterprise-4.6 branch/enterprise-4.7 size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants