🌱 Deprecate obsolete errors pkg

[enxebre]

The /errors package has its origin in when capi providers were machineActuators that needed to vendor core capi to function. There's no usage recommendations and value is questionable since we moved to CRDs and conditions for interoperability between core and providers. I think we should deprecate it and if there's any use case relying on it we should support it via conditions

What this PR does / why we need it:

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

[killianmuldoon]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 175a6cc5d9ea0c04b43946305ac3f18b0d933267

[fabriziopandini]

Am I wrong or MachineError is still used in the machine controller?

[enxebre]

Am I wrong or MachineError is still used in the machine controller?

I can't find any MachineError struct usage. The type MachineStatusError is used for the failureReason

cluster-api/api/v1beta1/machine_types.go

Line 173 in ada2764

FailureReason *capierrors.MachineStatusError `json:"failureReason,omitempty"`

cluster-api/internal/controllers/machine/machine_controller_phases.go

Line 110 in ada2764

machineStatusError := capierrors.MachineStatusError(failureReason)

We also use the InvalidConfigurationMachineError wrapper here

cluster-api/internal/controllers/machine/machine_controller_phases.go

Line 271 in ada2764

m.Status.FailureReason = ptr.To(capierrors.InvalidConfigurationMachineError)

These are known importers https://pkg.go.dev/sigs.k8s.io/cluster-api/errors?tab=importedby.

I think we can keep MachineStatusError type and deprecate and drop all the others unused wrappers and funcs. Or still deprecate MachineStatusError as well but don't drop it until next api bump reconsideration.

[fabriziopandini]

Double checked this and you are correct.
PR lgtm after rebase

[fabriziopandini]

/area util

(note, no rush at all to get this into the 1.8 release)

[enxebre]

rebased now cc @fabriziopandini

[fabriziopandini]

This should go in migrations/v1.9-to-v1.9.md now 😅

[enxebre]

Thanks Fabrizio, updated the doc.

[sbueringer]

I think it would be good to provide some more context.

If I understand this correctly we are basically deprecating and eventually removing the types used for FailureReason.

This means that providers have to stop using them in their API types and instead use conditions correct? (can we give them some more details what they should do instead & that the concept of "terminal failure" will go away)

(a deprecation without some hints about what to do instead is just not ideal)

[enxebre]

Updated! Let me know if that works.

[sbueringer]

The Errors package was created when capi provider implementation was running as machineActuators that needed to vendor core capi to function. There is no usage recommendations today

Just to avoid misunderstandings (and to ensure we're talking about the same thing)

MachineStatusError & ClusterStatusError are types used in our API and they are both used to read the corresponding fields from InfrastructureCluster & InfrastructureMachine (e.g.:

cluster-api/internal/controllers/machine/machine_controller_phases.go

Lines 105 to 111 in 4714c48

	failureReason, failureMessage, err := external.FailuresFrom(obj)
	if err != nil {
	return external.ReconcileOutput{}, err
	}
	if failureReason != "" {
	machineStatusError := capierrors.MachineStatusError(failureReason)
	m.Status.FailureReason = &machineStatusError

)

So as of right now, they are even part of our contract (vs. "no usage recommendation").
But this will be deprecated with #10897 and then removed with v1beta2.

Correct?

[sbueringer]

(cc @fabriziopandini)

[enxebre]

That's what I'm proposing yes. The whole package mostly got obsolete and unused when we move to CRDs, conditions and continuous reconciliation flow vs create/update. An even more as we move towards dropping existing terminal failure signal in favour of fleshed out conditions now.
If we need to have specific machine error types for v1beta2 I think we should redefine then when the time comes, but those will most likely come as conditions reasons. My intent with this PR is mainly to avoid confusion and perpetuating a dead end pattern e.g. https://github.com/kubernetes-sigs/cluster-api/pull/10360/files

[sbueringer]

Sounds good!

[fabriziopandini]

+1

[sbueringer]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: dba0e84689985dacf194c6f09e86dbe07d5c1be9

[fabriziopandini]

/lgtm
/approve

[k8s-ci-robot]

[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:

• OWNERS [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fabriziopandini]

/test pull-cluster-api-verify-main

[fabriziopandini]

@enxebre not 100% why the job is failing, might be the PR needs a rebase 🤔

[sbueringer]

It needs a fix for: Didn't find developer/providers/migrations/v1.8-to-v1.9.md in SUMMARY.md

[sbueringer]

Thx!
/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 131f2a9865480ad313c3b9e1c04f950ecc53717e