feat: implement stagedUpdateRun initialization #975
Conversation
jwtty
force-pushed
the
stagerun-init
branch
3 times, most recently
from
d8825f2 to
5022c97
Compare
jwtty
changed the title
jwtty
force-pushed
the
stagerun-init
branch
3 times, most recently
from
6727c44 to
f2d71b3
Compare
| if toBeUpdatedBindings, toBeDeletedBindings, err = r.initialize(ctx, &updateRun); err != nil { | ||
| klog.ErrorS(err, "Failed to initialize the clusterStagedUpdateRun", "clusterStagedUpdateRun", runObjRef) | ||
| // errInitializedFailed cannot be retried. | ||
| if errors.Is(err, errInitializedFailed) { |
There was a problem hiding this comment.
we also should not retry if the error is "unexpected"
There was a problem hiding this comment.
I believe I wrapped all terminating errors including unexpected ones as errInitializedFailed. I think adding another case would make things more complicated.
There was a problem hiding this comment.
oh, I miss read the code. However, since the outer error is always failedToInitialize, can we unwrap when we return the error. It might be easier to classify the error and create alert/monitor that way, especially now we emit error types like user error, unexpected or API error.
There was a problem hiding this comment.
hmmm but this error is not returned/reported in the controller, it's written to the clusterStagedUpdateRun condition.
jwtty
force-pushed
the
stagerun-init
branch
2 times, most recently
from
d318d7a to
ad9bda9
Compare
| if toBeUpdatedBindings, toBeDeletedBindings, err = r.initialize(ctx, &updateRun); err != nil { | ||
| klog.ErrorS(err, "Failed to initialize the clusterStagedUpdateRun", "clusterStagedUpdateRun", runObjRef) | ||
| // errInitializedFailed cannot be retried. | ||
| if errors.Is(err, errInitializedFailed) { |
There was a problem hiding this comment.
oh, I miss read the code. However, since the outer error is always failedToInitialize, can we unwrap when we return the error. It might be easier to classify the error and create alert/monitor that way, especially now we emit error types like user error, unexpected or API error.
| }) | ||
| }) | ||
|
|
||
| func validateFailedInitCondition(ctx context.Context, updateRun *placementv1alpha1.ClusterStagedUpdateRun, message string) { |
There was a problem hiding this comment.
no need to add this in this PR but I think it will be good to also verify the type of error (user error, unexpected, or api error) in this function.
There was a problem hiding this comment.
This one is related to above discussion. The initialization errors are not returned by the reconciler but just written to the initialization condition. The controller returns nil for these errors to avoid retrying. We can only check the error by checking the condition.
Description of your changes
Implement the initialization stage of the
clusterStagedUpdateRuncontroller.Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer