Force retry when adoption fails

[stbenjam]

Currently when adoption fails, we use RegistrationError but this gets
cleared any time registration succeeds. We need a separate signal for
adoption failure, which allows us to force retry the adoption again.

Fixes #697

[zaneb]

I've been thinking about alternative names that could avoid exposing to the user the implementation detail that after re-registration in the provisioned state, we call Ironic's adopt API.
The best I've come up with is "provisioned registration error" which is admittedly terrible. Any other ideas?

[stbenjam]

/test-integration

[zaneb]

This LGTM.
@dhellmann do you have any opinion on the name of this error?

[andfasano]

It also LGTM

[dhellmann]

This LGTM.
@dhellmann do you have any opinion on the name of this error?

The only time this error would come up is if the pod is rescheduled and we have to restore Ironic's state? That feels like we're exposing an internal error to the user. Can they even do anything about it?

I haven't followed the discussion, but is there a technical reason to not use RegistrationError? Maybe we have to be able to tell the 2 errors apart inside the BMO code to trigger the retry in the right way?

[stbenjam]

This LGTM.
@dhellmann do you have any opinion on the name of this error?

The only time this error would come up is if the pod is rescheduled and we have to restore Ironic's state? That feels like we're exposing an internal error to the user. Can they even do anything about it?

I haven't followed the discussion, but is there a technical reason to not use RegistrationError? Maybe we have to be able to tell the 2 errors apart inside the BMO code to trigger the retry in the right way?

Yes. Registration previously cleared out all error messages, I've refactored it to specifically look for RegistrationError instead, and introduced AdoptionError when there's a failure to adopt. Currently if adoption fails we never retry because the registration state clears all errors out.

[dhellmann]

Do we have a corresponding state to indicate that a host is being adopted? I'm trying to understand if this new error condition exposes a concept to the user that we haven't previously expected them to know about. If not, then I might just call it something like InternalError. That's pretty vague, but doesn't leave the user wondering why a host is showing an error when they didn't do anything to it.

Since we don't really want to expose the error to the user, it would be even better if we could use ironic's status directly, but I don't think we use that pattern anywhere else.

[zaneb]

Yeah, so it's kind of a philosophical question.
Adoption is not mentioned anywhere else in the public API, so this is a new and potentially confusing piece of information to users. (If 'adoption' means anything to users, it's probably the thing that the ExternallyProvisioned flag does.)

Ideally the fact that Ironic keeps a bunch of state in a relational database that occasionally gets deleted and needs to be regenerated should be something that users never need to know about. However, if something goes wrong with that part, then we need a way of recording it. Arguably we do already expose this to the user in the sense that a registration error can occur at any time, not just in the Registration state. But having an adoption error seems worse because of the complete meaninglessness of the name outside of the implementation detail that we use in Ironic to accomplish it. Nevertheless we must have some way of distinguishing errors in adoption from errors in registration to fix the bug.

My thought process behind provisioned registration error was basically that adoption is the second half of the registration process that only occurs when the Host is already provisioned. I think that's a productive line of enquiry even though that particular name is pretty so-so.

[andfasano]

The idea of InternalError looks appealing, as it could be of some use in future - at the same time it risks to be abused being so generic, whereas the provisioned registration error is definitely more granular and strictly scoped for a very specific kind of error. Anyhow, by looking at the current Provisioner design, where many actions could depend on ErrorType (the force bool), I'd say that we're moving into a direction closer to having more granular errors (maybe in future we could introduce a SubErrorType?) rather than generic.
The name provisioned registration error doesn't sound too bad to me (and I don't have better alternatives), so I think we could proceed in that direction.

[stbenjam]

Thanks all, I've updated the message to the proposed name. Please take another look

/test-integration

[zaneb]

/approve
Will leave this open until tomorrow in case somebody else has a better idea for the name.

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, stbenjam, zaneb

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [stbenjam,zaneb]

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

[furkatgofurov7]

This also LGTM.
I do not have either better alternative for the name, but provisioned registration error sounds reasonable.

[furkatgofurov7]

This needs a rebase though.

/cc @stbenjam

[metal3-io-bot]

@furkatgofurov7: GitHub didn't allow me to request PR reviews from the following users: stbenjam.

Note that only metal3-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

This needs a rebase though.

/cc @stbenjam

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.

[stbenjam]

Thanks, rebased!

/test-integration

[zaneb]

/lgtm