fix: use index number in clusterStagedUpdateRun and add ApprovalAccepted status to ApprovalRequests #1019
Conversation
jwtty
changed the title
jwtty
force-pushed
the
stagerun-index-fix
branch
3 times, most recently
from
2843cee to
79a44b9
Compare
jwtty
changed the title
jwtty
force-pushed
the
stagerun-index-fix
branch
6 times, most recently
from
dd0aad0 to
fe9aefe
Compare
| GenericFunc: func(ctx context.Context, e event.GenericEvent, q workqueue.RateLimitingInterface) { | ||
| klog.V(2).InfoS("Handling a clusterApprovalRequest generic event", "clusterApprovalRequest", klog.KObj(e.Object)) | ||
| handleClusterApprovalRequest(e.Object, q) | ||
| handleClusterApprovalRequest(e.ObjectOld, e.ObjectNew, q) |
There was a problem hiding this comment.
just curious, I wonder why removing the genricFunc? I put it there just for safety as I am not 100% sure what is considered "generic".
There was a problem hiding this comment.
genericFunc handles genericEvents, whose operation is unknown, e.g. triggered by a timer or events outside of the cluster: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/event#GenericEvent. Since I only want to trigger reconcile upon approval status change, I removed the genericFunc.
There was a problem hiding this comment.
I guess my question is "is it guaranteed that the status (and in other cases, the spec) of an object is not changed when the operation is unknown"? I haven't spent enough time in the code base to figure that out.
| // Make sure the cluster count in the policy snapshot has not changed. | ||
| if updateRun.Status.PolicyObservedClusterCount != clusterCount { | ||
| // PickAll policy case will be verified in validateStagesStatus. | ||
| if clusterCount != -1 && updateRun.Status.PolicyObservedClusterCount != clusterCount { |
There was a problem hiding this comment.
why adding the extra check since both will be -1 in the case of pickAll
There was a problem hiding this comment.
This fixes a regression introduced in PR #1014 . For pickAll policy, determinePolicySnapshot function returns -1 as clusterCount and the policyObservedClusterCount is updated to the total of scheduled bindings in collectScheduledBindings. So for the clusterCount validation here, I ignore the case where clusterCount == -1. The clustercount validation will be done later in the stage validation. Without this change, updateRun validation would fail with clusterCount mismatch error: "the cluster count initialized in the clusterStagedUpdateRun is outdated, latest: -1, recorded: 3 (total cluster count)"
There was a problem hiding this comment.
It's not ideal to assign values to PolicyObservedClusterCount in both determinePolicySnapshot and collectScheduledClusters functions. It will be good to add some comments (and maybe a TODO to refactor)
jwtty
force-pushed
the
stagerun-index-fix
branch
from
fe9aefe to
a38fb75
Compare
| // - "True": The request is approved. | ||
| ApprovalRequestConditionApproved ApprovalRequestConditionType = "Approved" | ||
|
|
||
| // ApprovalRequestConditionApprovalAccepted indicates if the approved approval request was accepted. |
There was a problem hiding this comment.
- jsonPath: .status.conditions[?(@.type=="ApprovalAccepted")].status
name: ApprovalAccepted
type: string
| GenericFunc: func(ctx context.Context, e event.GenericEvent, q workqueue.RateLimitingInterface) { | ||
| klog.V(2).InfoS("Handling a clusterApprovalRequest generic event", "clusterApprovalRequest", klog.KObj(e.Object)) | ||
| handleClusterApprovalRequest(e.Object, q) | ||
| handleClusterApprovalRequest(e.ObjectOld, e.ObjectNew, q) |
There was a problem hiding this comment.
I guess my question is "is it guaranteed that the status (and in other cases, the spec) of an object is not changed when the operation is unknown"? I haven't spent enough time in the code base to figure that out.
| // Make sure the cluster count in the policy snapshot has not changed. | ||
| if updateRun.Status.PolicyObservedClusterCount != clusterCount { | ||
| // PickAll policy case will be verified in validateStagesStatus. | ||
| if clusterCount != -1 && updateRun.Status.PolicyObservedClusterCount != clusterCount { |
There was a problem hiding this comment.
It's not ideal to assign values to PolicyObservedClusterCount in both determinePolicySnapshot and collectScheduledClusters functions. It will be good to add some comments (and maybe a TODO to refactor)
2 participants
Description of your changes
ApprovalAcceptedstatus in approvalRequestsFixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer