Upstream: <880>: openshift: Record machine sync error event#24
Conversation
openshift-ci-robot
added
the
size/XS
| return reconcile.Result{}, errors.Wrap(err, "failed to update machine set status") | ||
| } | ||
|
|
||
| if syncErr != nil { |
There was a problem hiding this comment.
Why do we only handle this after the updateMachineSetStatus bit? Are those always tied together this way? If so, should that just be a part of syncReplicas?
There was a problem hiding this comment.
MachinesetStatus and events are two mechanisms through which user can get to know about current state. Even if syncReplicas is facing error, i thought it would help if we let the stats updated in the machinesetstatus.
There was a problem hiding this comment.
From a purely pattern perspective, why do we not check for a syncErr straight after we call:
syncErr := r.syncReplicas(machineSet, filteredMachines)There was a problem hiding this comment.
If calling syncReplicas can fail why do we not fail fast when this is true?
There was a problem hiding this comment.
There was a problem hiding this comment.
Discussed this offline. I think it might make sense to just move the updateMachineSetStatus call to syncReplicas if we always want to update the status while syncing. Not a big deal though, and this merged as-is upstream, so LGTM.
There was a problem hiding this comment.
In which case I would say add those exact words as to why we delay checking for success/failure on this call.
There was a problem hiding this comment.
so that machineset status status could get updated and along with error message in the events, user could have latest stats.
|
@vikaschoudhary16 commit message still looks broken could you please update? |
vikaschoudhary16
force-pushed
the
syncerrms-4.0
branch
2 times, most recently
from
b2e1142 to
2f510ca
Compare
vikaschoudhary16
force-pushed
the
syncerrms-4.0
branch
from
2f510ca to
734154e
Compare
| return reconcile.Result{}, errors.Wrap(err, "failed to update machine set status") | ||
| } | ||
|
|
||
| if syncErr != nil { |
There was a problem hiding this comment.
Discussed this offline. I think it might make sense to just move the updateMachineSetStatus call to syncReplicas if we always want to update the status while syncing. Not a big deal though, and this merged as-is upstream, so LGTM.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
openshift-merge-robot
merged commit 34fb0d0
into
openshift:openshift-4.0-cluster-api-0.0.0-alpha.4
6 participants
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 #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: