🐛 Change MachineHealthCheck.spec.unhealthyConditions to optional #9774

mcbenjemaa · 2023-11-27T10:12:02Z

What this PR does / why we need it:

The MHC API doesn't allow creating a health check for a missing node only,
it requires you to create fake conditions.

This PR, make the spec.unhealthyConditions to allow creating MHC for missing nodes.

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 #9762

/area machinehealthcheck

k8s-ci-robot · 2023-11-27T10:12:12Z

Hi @mcbenjemaa. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

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.

sbueringer · 2023-11-27T15:03:22Z

/ok-to-test

mcbenjemaa · 2023-11-28T08:54:36Z

/retest

enxebre · 2023-11-28T12:06:14Z

Can we please include a test case for a healthCheckTarget with a MHC with empty conditions in TestHealthCheckTargets?
Can we please also include a test case for empty conditions within TestMachineHealthCheck_Reconcile in addition to the existing "when a Machine's Node has gone away"

internal/controllers/machinehealthcheck/machinehealthcheck_targets_test.go

enxebre · 2023-12-04T08:51:57Z

Thanks! lgtm after addressing https://github.com/kubernetes-sigs/cluster-api/pull/9774/files#r1413546275 and squashing commits.

sbueringer · 2023-12-08T13:59:25Z

/test pull-cluster-api-test-main

sbueringer · 2023-12-08T14:00:05Z

Looks like you have to run make generate-go-openapi. (or make the change here manually: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/9774/pull-cluster-api-verify-main/1732354796511301632)

api/v1beta1/machinehealthcheck_types.go

internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go

internal/webhooks/cluster_test.go

internal/webhooks/machinehealthcheck.go

mcbenjemaa · 2023-12-13T13:03:57Z

Looks like you have to run make generate-go-openapi. (or make the change here manually: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/9774/pull-cluster-api-verify-main/1732354796511301632)

Looks like make generate-go-openapi does nothing?

sbueringer · 2023-12-14T10:36:51Z

Looks like you have to run make generate-go-openapi. (or make the change here manually: prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/9774/pull-cluster-api-verify-main/1732354796511301632)

Looks like make generate-go-openapi does nothing?

We had an issue in the past with running this when the repository was outside of the "GOPATH" location: #6526

But as far as I know we got this resolved.

On my Ubuntu Machine I end up with the following diff after running this target:

-                               Required: []string{"clusterName", "selector", "unhealthyConditions"},
+                               Required: []string{"clusterName", "selector"},

(same as in Prow)

mcbenjemaa · 2023-12-14T13:42:58Z

@sbueringer I'm on Macos, I tried to generate in the GOPATH but still not working.

this what i got:

GOBIN=/Users/mbenjemaa/workspace/go/src/github.com/mcbenjemaa/cluster-api/hack/tools/bin ./scripts/go_install.sh k8s.io/kube-openapi/cmd/openapi-gen openapi-gen 5e7f5fd
** Generating openapi schema for types in ./api/v1beta1 **
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassPatch,Definitions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Patches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassStatus,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassStatusVariable,Definitions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Enum
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Required
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineDeploymentVariables,Overrides
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckClass,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckSpec,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckStatus,Targets
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachinePoolClass,FailureDomains
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachinePoolTopology,FailureDomains
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachinePoolVariables,Overrides
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,NetworkRanges,CIDRBlocks
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchDefinition,JSONPatches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchSelectorMatchMachineDeploymentClass,Names
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchSelectorMatchMachinePoolClass,Names
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,Topology,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersClass,MachineDeployments
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersClass,MachinePools
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersTopology,MachineDeployments
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersTopology,MachinePools
API rule violation: names_match,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,XPreserveUnknownFields
** Generating openapi schema for types in ./exp/runtime/hooks/api/v1alpha1 **
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,DiscoverVariablesResponse,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesRequest,Items
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesRequest,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesRequestItem,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesResponse,Items
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesResponseItem,Patch
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequest,Items
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequest,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequestItem,Variables

sbueringer · 2023-12-14T16:35:28Z

$ make generate-go-openapi
GOBIN=/Users/buringerst/code/src/sigs.k8s.io/cluster-api/hack/tools/bin ./scripts/go_install.sh k8s.io/kube-openapi/cmd/openapi-gen openapi-gen 5e7f5fd
go: downloading k8s.io/kube-openapi v0.0.0-20220413171646-5e7f5fdc6da6
go: downloading github.com/mailru/easyjson v0.0.0-20190626092158-b2ccc519800e
make[1]: *** No rule to make target `clean-generated-openapi-definitions'.  Stop.
** Generating openapi schema for types in ./api/v1beta1 **
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassPatch,Definitions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Patches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassStatus,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassStatusVariable,Definitions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Enum
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Required
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineDeploymentVariables,Overrides
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckClass,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckSpec,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckStatus,Targets
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachinePoolClass,FailureDomains
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachinePoolTopology,FailureDomains
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachinePoolVariables,Overrides
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,NetworkRanges,CIDRBlocks
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchDefinition,JSONPatches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchSelectorMatchMachineDeploymentClass,Names
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchSelectorMatchMachinePoolClass,Names
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,Topology,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersClass,MachineDeployments
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersClass,MachinePools
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersTopology,MachineDeployments
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersTopology,MachinePools
API rule violation: names_match,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,XPreserveUnknownFields
make[1]: *** No rule to make target `clean-generated-openapi-definitions'.  Stop.
** Generating openapi schema for types in ./exp/runtime/hooks/api/v1alpha1 **
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,DiscoverVariablesResponse,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesRequest,Items
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesRequest,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesRequestItem,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesResponse,Items
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesResponseItem,Patch
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequest,Items
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequest,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequestItem,Variables

$ pwd
/Users/buringerst/code/src/sigs.k8s.io/cluster-api

$ echo $GOPATH
/Users/buringerst/code

$ git diff
diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go
index 073de884b..fe9c4584d 100644
--- a/api/v1beta1/zz_generated.openapi.go
+++ b/api/v1beta1/zz_generated.openapi.go
@@ -2282,7 +2282,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_MachineHealthCheckSpec(ref common.
                                                },
                                        },
                                },
-                               Required: []string{"clusterName", "selector", "unhealthyConditions"},
+                               Required: []string{"clusterName", "selector"},
                        },
                },
                Dependencies: []string{

This time on Apple M2

mcbenjemaa · 2023-12-18T14:47:10Z

ready

sbueringer · 2024-01-25T15:17:41Z

I would take another look once the open findings are addressed

mcbenjemaa · 2024-03-07T12:05:15Z

@fabriziopandini will you take another look here

chrischdi

two small typo nits (which already did exist before) :-)

As of that I'm lgtm for this change 👍

Thanks for working on this! 🎉

internal/webhooks/machinehealthcheck_test.go

test/framework/machinehealthcheck_helpers.go

internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go

internal/controllers/machinehealthcheck/machinehealthcheck_targets_test.go

chrischdi · 2024-04-05T10:35:55Z

/assign @sbueringer

sbueringer · 2024-04-05T10:51:46Z

@mcbenjemaa Sorry for the delay, and thank you very much for seeing this through and the patience!!!

/lgtm
/approve

k8s-ci-robot · 2024-04-05T10:51:52Z

LGTM label has been added.

Git tree hash: 530ba36bee19420c227bb63b5ae2dda53307498a

k8s-ci-robot · 2024-04-05T10:51:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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 requested review from jackfrancis and richardcase November 27, 2023 10:12

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 27, 2023

mcbenjemaa force-pushed the mhc-conditions branch from 38033f6 to a846e32 Compare November 28, 2023 09:07

mcbenjemaa force-pushed the mhc-conditions branch from a846e32 to bb5dae1 Compare December 3, 2023 15:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 3, 2023

mcbenjemaa force-pushed the mhc-conditions branch from bb5dae1 to ffcfd8d Compare December 3, 2023 15:48

enxebre reviewed Dec 4, 2023

View reviewed changes

internal/controllers/machinehealthcheck/machinehealthcheck_targets_test.go Outdated Show resolved Hide resolved

mcbenjemaa mentioned this pull request Dec 5, 2023

REQUEST: New membership for mcbenjemaa kubernetes/org#4606

Closed

9 tasks

mcbenjemaa force-pushed the mhc-conditions branch from dd27342 to fdd1e38 Compare December 6, 2023 11:01

sbueringer reviewed Dec 8, 2023

View reviewed changes

mcbenjemaa force-pushed the mhc-conditions branch from fdd1e38 to 79fc2ad Compare December 13, 2023 13:14

mcbenjemaa force-pushed the mhc-conditions branch from b0343ee to 32764cc Compare February 20, 2024 16:07

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2024

mcbenjemaa added 10 commits March 20, 2024 18:25

Change MachineHealthCheck.spec.UnhealthyConditions to optional

1b635eb

Remove unnecessary leading newline

99abd84

Add test cases for mhc without conditions

9bd8d9b

Fix MHC empty conditions test

ba42c2a

Address Reviews

a63af41

Generate openapi

001cd80

Fix test cases

35fa1ff

Adjust test mhc override

34a4412

Address reviews

ab1c9d9

Update Test Machine's Node without conditions

7c72666

mcbenjemaa force-pushed the mhc-conditions branch from 32764cc to 7c72666 Compare March 20, 2024 17:33

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2024

Fix linting

f890eed

chrischdi reviewed Mar 27, 2024

View reviewed changes

internal/webhooks/machinehealthcheck_test.go Outdated Show resolved Hide resolved

internal/webhooks/machinehealthcheck_test.go Outdated Show resolved Hide resolved

sbueringer reviewed Apr 5, 2024

View reviewed changes

review fixes

bd2fb40

k8s-ci-robot assigned sbueringer Apr 5, 2024

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 5, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2024

k8s-ci-robot merged commit b7b53ca into kubernetes-sigs:main Apr 5, 2024
22 checks passed

k8s-ci-robot added this to the v1.8 milestone Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Change MachineHealthCheck.spec.unhealthyConditions to optional #9774

🐛 Change MachineHealthCheck.spec.unhealthyConditions to optional #9774

mcbenjemaa commented Nov 27, 2023

k8s-ci-robot commented Nov 27, 2023

sbueringer commented Nov 27, 2023

mcbenjemaa commented Nov 28, 2023

enxebre commented Nov 28, 2023

enxebre commented Dec 4, 2023

sbueringer commented Dec 8, 2023

sbueringer commented Dec 8, 2023

mcbenjemaa commented Dec 13, 2023

sbueringer commented Dec 14, 2023

mcbenjemaa commented Dec 14, 2023

sbueringer commented Dec 14, 2023

mcbenjemaa commented Dec 18, 2023

sbueringer commented Jan 25, 2024

mcbenjemaa commented Mar 7, 2024

chrischdi left a comment

chrischdi commented Apr 5, 2024

sbueringer commented Apr 5, 2024

k8s-ci-robot commented Apr 5, 2024

k8s-ci-robot commented Apr 5, 2024

🐛 Change MachineHealthCheck.spec.unhealthyConditions to optional #9774

🐛 Change MachineHealthCheck.spec.unhealthyConditions to optional #9774

Conversation

mcbenjemaa commented Nov 27, 2023

k8s-ci-robot commented Nov 27, 2023

sbueringer commented Nov 27, 2023

mcbenjemaa commented Nov 28, 2023

enxebre commented Nov 28, 2023

enxebre commented Dec 4, 2023

sbueringer commented Dec 8, 2023

sbueringer commented Dec 8, 2023

mcbenjemaa commented Dec 13, 2023

sbueringer commented Dec 14, 2023

mcbenjemaa commented Dec 14, 2023

sbueringer commented Dec 14, 2023

mcbenjemaa commented Dec 18, 2023

sbueringer commented Jan 25, 2024

mcbenjemaa commented Mar 7, 2024

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi commented Apr 5, 2024

sbueringer commented Apr 5, 2024

k8s-ci-robot commented Apr 5, 2024

k8s-ci-robot commented Apr 5, 2024