Skip to content

Add support for node annotation to disable node fencing. #333

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
Closed
emesika wants to merge 1 commit into from
Closed

Add support for node annotation to disable node fencing. #333

emesika wants to merge 1 commit into from

Conversation

@emesika
Copy link

@emesika emesika commented Jun 24, 2019

Implementation for the requirement to disable fencing using a special label set on the machine object

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 24, 2019

Hi @emesika. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 24, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 24, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found here.

Details 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

@emesika
Copy link
Author

emesika commented Jun 24, 2019

To do check list

  1. add unit tests
  2. add functional tests

@vikaschoudhary16 vikaschoudhary16 mentioned this pull request Jul 5, 2019
Closed
@vikaschoudhary16
Copy link
Contributor

vikaschoudhary16 commented Jul 5, 2019

@emesika PTAL openshift/cluster-api#48

enxebre
enxebre reviewed Jul 10, 2019
View reviewed changes
pkg/controller/machinehealthcheck/machinehealthcheck_controller.go Outdated
const (
machineAnnotationKey = "machine.openshift.io/machine"
ownerControllerKind = "MachineSet"
disableFencingLabel = "machine.openshift.io/healthcheck-disabled"
Copy link
Member

@enxebre enxebre Jul 10, 2019

Choose a reason for hiding this comment

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

if anything at all this should be an annotation

Copy link
Contributor

@beekhof beekhof Jul 11, 2019

Choose a reason for hiding this comment

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

@emesika ack. we'll also change to healthchecking.openshift.io/disabled

Do you have any other concerns about the concept in general?

Copy link
Member

@enxebre enxebre Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

this will also need at least unit tests. Can you please share a gdoc with all the rationale annotations you think you need, use cases and scenarious for the baremetal story?
If you want to opt out why don't just remove that machine from the MHC target or remove the MHC resource?

Copy link
Member

@enxebre enxebre Jul 11, 2019

Choose a reason for hiding this comment

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

Is there any reason why this can't be in the remediation controller for now?

Copy link
Contributor

@beekhof beekhof Jul 16, 2019

Choose a reason for hiding this comment

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

Generally its better that the work is not scheduled if we don't want it to take place. Otherwise it looks like there is work to do when there isn't, which can look messy and confuse admins.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019
@beekhof
Copy link
Contributor

beekhof commented Jul 16, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 16, 2019

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot 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 Jul 16, 2019
@emesika emesika changed the title Add support for node label to disable node fencing. Add support for node annotation to disable node fencing. Jul 16, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 21, 2019
@emesika
Add support for node annotation to disable node fencing
7c3c17f 
Adding "healthchecking.openshift.io/disabled=true" annotation to the machine object will
disable fencing on that machine until annotation is set again to false.

Provided with basic unit tests.
@beekhof
Copy link
Contributor

beekhof commented Jul 25, 2019

/retest

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 25, 2019

@emesika: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/unit 7c3c17f link /test unit
ci/prow/govet 7c3c17f link /test govet
ci/prow/images 7c3c17f link /test images
ci/prow/e2e-aws-operator 7c3c17f link /test e2e-aws-operator
ci/prow/e2e-aws 7c3c17f link /test e2e-aws
ci/prow/e2e-aws-upgrade 7c3c17f link /test e2e-aws-upgrade
ci/jenkins/integration 7c3c17f link /test integration

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.

Details

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.

@yanirq yanirq mentioned this pull request Sep 3, 2019
Closed
@enxebre
Copy link
Member

enxebre commented Nov 4, 2019

Closing due to inactivity. Please create a proposal against https://github.com/openshift/enhancements if still relevant

@enxebre enxebre closed this Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@enxebre enxebre enxebre left review comments

@paulfantom paulfantom Awaiting requested review from paulfantom

@spangenberg spangenberg Awaiting requested review from spangenberg

+1 more reviewer

@beekhof beekhof beekhof left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@beekhof beekhof

Labels

needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@emesika @openshift-ci-robot @vikaschoudhary16 @beekhof @enxebre