🌱 MHC marks machines for remediation with conditions

[benmoss]

What this PR does / why we need it:
Moves MHC controller and MachineSet to use conditions instead of MHC deleting them directly. This is the first part of #2976

@vincepri @fabriziopandini

[fabriziopandini]

Thanks @benmoss for this PR! Great to see conditions in action!

[vincepri]

/retitle 🌱 MHC marks machines for remediation with conditions

[vincepri]

@benmoss What's the status of this PR?

[benmoss]

It's waiting for approval and lgtm

[vincepri]

Let's sync up with @fabriziopandini on where to move and how to name conditions constants, there is also the other comment I made about moving the tests in the other package without the need to create a new envtest.

[benmoss]

Yeah, I still disagree with that comment. The controllers tests are a mess in part because of the mix of envtest and unit tests going on there. I think we should keep full integration tests like these separate.

[vincepri]

I'd like to better understand the benefit of having multiple separate suites. There is some downsides I can see with this approach if it grows past a certain point, for example getting a new envtest up takes quite a bit of time and having multiple of them can greatly increase the job's time.

Envtest doesn't come for free either, on my machine which isn't affected by other running jobs (like in prow) spinning up etcd and api-server takes 3-400% CPU on start and it settles betwen 2-300% during tests; we can potentially cause flakes or other resource-starvation problems in prow by creating more than necessary.

[benmoss]

So for one by marking these as a separate suite we can address the resource consumption issues. I've been running this test using the USE_EXISTING_CLUSTER environment variable with this test and it works and cleans up all the resources that it creates. However other controller tests don't seem to behave nicely and fail when run with this flag, I haven't looked into why, but probably there just isn't any cleanup logic in them. But on CI we could use an external cluster for these, rather than something that has to share resources with prow.

But even still the broader point is that these are slow tests and are fully integrated against the apiserver and etcd. They are a different kind of test, so they should be represented that way. I'm fine to move this into a separate issue. I tried moving this test into the MachineSet test suite but I think I have to convert it into a Ginkgo test for it to work.

[vincepri]

I tried moving this test into the MachineSet test suite but I think I have to convert it into a Ginkgo test for it to work.

In machinehealthcheck_controller_test.go, we can add these tests under var _ = Describe("MachineHealthCheck Reconciler", func() { or, probably even better, create a new top level Describe.

[benmoss]

This looks like a failure due to the MHC controller and the MachineSet controller clobbering each other's condition patches and the MHC controller ultimately winning. Not sure why we weren't seeing it before.

/hold

[benmoss]

Just out of curiosity if it passes, it seems to not fail that often on my machine but maybe it's related to the number of cores or something.

/test pull-cluster-api-test

[vincepri]

@benmoss Let's try to get this merged today if possible

[vincepri]

LGTM pending squash

[benmoss]

@vincepri it has been squished

[vincepri]

@benmoss mind rebasing on master if you haven't already? Seems some of the tests are failing.

[benmoss]

Seeing this failure even after rebase

• Failure [10.128 seconds]
Patch Helper
/home/mossity/workspace/cluster-api/util/patch/patch_test.go:38
  Should patch conditions
  /home/mossity/workspace/cluster-api/util/patch/patch_test.go:101
    on a clusterv1.Cluster object
    /home/mossity/workspace/cluster-api/util/patch/patch_test.go:149
      should recover if there is a resolvable conflict, incl. patch spec and status [It]
      /home/mossity/workspace/cluster-api/util/patch/patch_test.go:226

      Timed out after 10.000s.
      Expected
          <bool>: false
      to be true

      /home/mossity/workspace/cluster-api/util/patch/patch_test.go:270

opened #3178

[k8s-ci-robot]

@benmoss: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	2c77beb	link	/test pull-cluster-api-apidiff

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.

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.

[vincepri]

/lgtm

[vincepri]

IIRC this can probably be removed now, but we can do in a follow-up

cc @fabriziopandini