Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
metal3-io-bot
added
the
size/XL
| return nil | ||
| } | ||
|
|
||
| // Update updates a machine and is invoked by the Machine Controller. |
|
|
||
| // Update updates a machine and is invoked by the Machine Controller. | ||
| func (m *HostManager) Update(ctx context.Context) error { | ||
| m.Log.Info("Updating machine") |
There was a problem hiding this comment.
Please find and replace all cases of "machine" here
There was a problem hiding this comment.
And this should be V(1) too unless we detect that actual changes are needed
| return hideConflictError(err) | ||
| } | ||
|
|
||
| // transient rebootAnnotation was successfully transmitted. We can delete it on HostClaim. |
There was a problem hiding this comment.
This is a bit unfortunate: this way it's hard to track when reboot is actually executed. But I don't see a good way out.
There was a problem hiding this comment.
Yes and if the HostClaim patch fails we may even reboot twice in rare cases.
| m.SetConditionHostToFalse( | ||
| metal3api.AssociatedCondition, metal3api.MissingBareMetalHostReason, | ||
| "BareMetalHost associated to the claim not found") | ||
| return errors.New("BareMetalHost not found") |
There was a problem hiding this comment.
I"m curious what the expected remediation is. Is a user supposed to delete and re-create the claim? Or is the controller supposed to provide another BMH?
There was a problem hiding this comment.
In the consumerRef case status.BaremetalHost is reset to nil, so the association logic is performed again (because in the controller introduced later the defer patch will be done as in m3m controller). This was not done here and I have added it.
| } | ||
| err := m.client.Get(ctx, key, &bmh) | ||
| if k8serrors.IsNotFound(err) { | ||
| m.Log.Info("Annotated host not found", "bmh", bmhRef.Name, "bmhNamespace", bmhRef.Namespace) |
| if !consumerRefMatches(bmh.Spec.ConsumerRef, hostClaim) { | ||
| m.Log.Info("The consumer ref does not point to the hostClaim", "consumerRef", bmh.Spec.ConsumerRef) | ||
| hostClaim.Status.BareMetalHost = nil | ||
| return nil, ErrNoBMH |
There was a problem hiding this comment.
Similar question here: what's the expected action in this case? Especially since we only provide a generic error message (which is understandable)?
| sourceRef *corev1.SecretReference, | ||
| namespace string, | ||
| hostName string, | ||
| ) (*corev1.SecretReference, error) { |
There was a problem hiding this comment.
This call should exit early if the target namespace is the same as the source namespace (a non-multi-tenant case).
| conditions.SetMirrorCondition(bmh, m.HostClaim, metal3api.ProvisionedCondition) | ||
| m.SetConditionHostToTrue(metal3api.AssociatedCondition, metal3api.BareMetalHostAssociatedReason) | ||
|
|
||
| if !equality.Semantic.DeepEqual(m.HostClaim.Status, hostOld) { |
There was a problem hiding this comment.
And this is where logging something would actually be useful
| bmh.Annotations = map[string]string{} | ||
| } | ||
|
|
||
| if syncReboot(m.HostClaim.Annotations, bmh.Annotations) { |
There was a problem hiding this comment.
We should also sync the detachment annotation (also maybe without the "force" flag being introduced in #2955 - i.e. sanitize the value). Detaching is commonly used in my world (e.g. by ACM) to prevent Ironic from further touching the deployed instance.
There was a problem hiding this comment.
@dtantsur I have not modified this part yet. I want to be sure that we want to give control of the Detach annotation to the customer.... An analogy of hostclaim is taking a taxi rather than renting/owning a car: you do not have the keys. With detach, you ask the taxi driver to throw away the keys.... but you still do not have them. As it is done, I do not see a way of sharing the right to detach by both the customer and the infra owner (no consumerOverride).
There was a problem hiding this comment.
We could have a knob to enable/disable it per HDP. But it's quite important for users to be able isolate their instance from Ironic's power management. It also allows reducing the load on Ironic.
I guess we'd need to re-attach on HostClaim deletion.
| } | ||
|
|
||
| if err != nil { | ||
| m.SetConditionHostToFalse( |
There was a problem hiding this comment.
Maybe don't update the condition on conflict?
Update ensures the synchronization between HostClaim and BareMetalHost. * Copy HostClaim secrets to BareMetalHost namespace * Ensure propagation of reboot annotations * Synchronize hostclaim status (mirrors BareMetalHost conditions) Co-authored-by: Pierre Crégut <pierre.cregut@orange.com> Co-authored-by: Laurent Roussarie <laurent.roussarie@orange.com> Signed-off-by: Pierre Crégut <pierre.cregut@orange.com>
pierrecregut
force-pushed
the
feature/hostclaim-update
branch
from
af309eb to
8a95ab0
Compare
3 participants
What this PR does / why we need it:
Update ensures the synchronization between HostClaim and BareMetalHost.
Checklist: