-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
handler: Retry enactment update #1215
Conversation
) | ||
|
||
func Update(cli client.Client, key types.NamespacedName, statusSetter func(*nmstate.NodeNetworkConfigurationEnactmentStatus)) error { | ||
logger := log.WithValues("enactment", key.Name) | ||
return retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
return retry.OnError(retry.DefaultRetry, allErrors, func() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a comment explaining how temporary network unavailability needs to be accounted for when handling errors. Is this the only place where this problem can happen? Or is more generic handling needed? Isn't connectivity verified before this update is attempted? Or is this when we try to update after rollback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jcaamano, AFAIR you were working on some bug when this was discovered. Do you have a Jira handle? We would simply link it in the commit so that whoever is interested at any time would git blame
and read what's needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was something @qinqon tried out in context of https://issues.redhat.com//browse/OPNET-268
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcaamano @mkowalski added some comments to explain it.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkowalski 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:
Approvers can indicate their approval by writing |
At some scenarios it takes some retries to update the enactment status if the network is not totally settle. This change add some retries to enactment status update. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
14b4b10
to
7932fb6
Compare
/lgtm |
At some scenarios it takes some retries to update the enactment status if the network is not totally settle. This change add some retries to enactment status update. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
At some scenarios it takes some retries to update the enactment status if the network is not totally settle. This change add some retries to enactment status update. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
At some scenarios it takes some retries to update the enactment status if the network is not totally settle. This change add some retries to enactment status update. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
At some scenarios it takes some retries to update the enactment status if the network is not totally settle. This change add some retries to enactment status update. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
At some scenarios it takes some retries to update the enactment status if the network is not totally settle. This change add some retries to enactment status update. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
Is this a BUG FIX or a FEATURE ?:
/kind bug
What this PR does / why we need it:
At some scenarios it takes some retries to update the enactment status if the network is not totally settle. This change add some retries to enactment status update.