-
Notifications
You must be signed in to change notification settings - Fork 47
✨Add an option to control initial rollout of HelmReleaseProxies #443
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
✨Add an option to control initial rollout of HelmReleaseProxies #443
Conversation
Signed-off-by: Chaitanya Kolluru <[email protected]>
Signed-off-by: Chaitanya Kolluru <[email protected]>
|
|
|
Welcome @chaitanyakolluru! |
|
Hi @chaitanyakolluru. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
Signed-off-by: Chaitanya Kolluru <[email protected]>
Signed-off-by: Chaitanya Kolluru <[email protected]>
9bd4698 to
d72d7f1
Compare
api/v1alpha1/helmchartproxy_types.go
Outdated
| c.Status.Conditions = conditions | ||
| } | ||
|
|
||
| func (c *HelmChartProxy) GetHelmReleaseProxyReadyCondition() *clusterv1.Condition { |
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 kind of function already implemented in Cluster API conditions package. Please use it instead of create.
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.
removed the fn and used Get() from cluster-api/util/conditions. Thanks.
…er-api's condition package. Signed-off-by: Chaitanya Kolluru <[email protected]>
| if err != nil { | ||
| return err | ||
| } | ||
| for _, hrpRltMeta := range clusterRolloutMeta { |
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.
Please note that the objects in the list are in a non-deterministic order, and processing them in batches during different reconciliation cycles may lead to unpredictable results. Are you sure this will be handled correctly?
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.
Introduced orderliness on rollout meta before processing to prevent inconsistencies with different reconciliations over a list of matching clusters. Thanks for bringing this up.
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.
Ok, good
| hrpReadyCond := conditions.Get(helmChartProxy, addonsv1alpha1.HelmReleaseProxiesReadyCondition) | ||
| // If HelmReleaseProxiesReadyCondition is false, reconcile existing | ||
| // HelmReleaseProxies and exit. | ||
| if hrpReadyCond != nil && (hrpReadyCond.Status == corev1.ConditionFalse) { |
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.
Can you utilize conditions.IsTrue() or conditions.IsFalse() here and later in the code.
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.
Used conditions.IsFalse() when checking for false status on the condition. I don't see other instance where conditions.IsTrue() can be used.
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.
Ok
| if hrpRltMeta.hrpExists { | ||
| continue | ||
| } | ||
|
|
||
| // Don't reconcile if the Cluster is being deleted | ||
| if !hrpRltMeta.cluster.DeletionTimestamp.IsZero() { | ||
| continue | ||
| } | ||
|
|
||
| err := r.reconcileForCluster(ctx, helmChartProxy, hrpRltMeta.cluster) | ||
| if err != nil { | ||
| return err | ||
| } |
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.
There are very similar code to the code in lines 244-255, except condition in hrpRltMeta.hrpExists. You can wrap it with function (probably anonymous) and use it here and upper.
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.
Moved cluster deletion timestamp check to within the reconcileForCluster() to make those sections look cleaner. Let me know if this works for you.
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.
Ok, good
Signed-off-by: Chaitanya Kolluru <[email protected]>
Signed-off-by: Chaitanya Kolluru <[email protected]>
…roxies Signed-off-by: Chaitanya Kolluru <[email protected]>
Signed-off-by: Chaitanya Kolluru <[email protected]>
| // creating HelmReleaseProxy resources for all matching clusters. | ||
| // e.g. an int (5) or percentage of count of total matching clusters (25%) | ||
| // +optional | ||
| RolloutStepSize *intstr.IntOrString `json:"rolloutStepSize,omitempty"` |
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.
Is it possible and currently processed changes to this field during recomcile, i.e. reset condition in case of reduce or increase it value?
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.
It depends on the list of matched clusters per the clusterSelector field in the spec. Once all HelmReleaseProxies have been spawned during initial rollout, condition HelmReleaseProxiesRolloutCompletedCondition is marked True and upon changes to either the clusterSelector or the list of clusters being managed, the next reconcile will notice the difference in count of clusters matched to the HelmReleaseProxies, sets condition HelmReleaseProxiesRolloutCompletedCondition to False and rolls them out according to the rolloutStepSize defined. Hope that answers the question.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chaitanyakolluru, dmvolod The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Could I get some eyes on this MR please. @mboersma @jackfrancis. |
|
|
||
| // reconcileForCluster will create or update a HelmReleaseProxy for the given cluster. | ||
| func (r *HelmChartProxyReconciler) reconcileForCluster(ctx context.Context, helmChartProxy *addonsv1alpha1.HelmChartProxy, cluster clusterv1.Cluster) error { | ||
| // Don't reconcile if the Cluster is being deleted |
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.
Do we need this change in this PR?
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.
Made this change as part of minimizing code reuse and moved the cluster deletion timestamp check to within the method instead.
| return nil | ||
| } | ||
|
|
||
| // getNnStringFor to retrieve the namespaced name as a string. |
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.
nit: prefer the more verbose getNamespacedNameStringFor
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.
updated name to getNamespacedNameStringFor
Signed-off-by: Chaitanya Kolluru <[email protected]>
Signed-off-by: Chaitanya Kolluru <[email protected]>
Signed-off-by: Chaitanya Kolluru <[email protected]>
Signed-off-by: Chaitanya Kolluru <[email protected]>
87e97d1 to
2d32f4d
Compare
|
@dmvolod @mboersma @jackfrancis made a couple of updates to how it decides to rollout the next batch of |
|
@chaitanyakolluru: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Closed in favor of #452 |
What this PR does / why we need it:
We do not have a way to control the initial rollout of
HelmReleaseProxiesfor every matching cluster as part of aHelmChartProxy. This change introduces a new field toHelmChartProxyspecrolloutStepSize, which can be used to stagger the initial rollout ofHelmReleaseProxiesbased on already existingHelmReleaseProxies's ready status.This change will allow for a controlled rollout strategy that comes into place only during initial rollout and will not impact other options for
ReconcileStrategy. A new status conditionHelmReleaseProxiesRolloutCompletedis added to denote initial rollout status ofHelmReleaseProxiesand is included when determining overall summaryReadystatus condition onHelmChartProxyresources. ForHelmChartProxiesnot using the rollout option,HelmReleaseProxiesRolloutCompletedstatus condition is added to the resource with a reason ofHelmReleaseProxiesRolloutUndefinedand a message that mentions not using the rollout option.Below are two
HelmChartProxyresource status conditions, one with and the other without rollout as an option:rolloutStepSize:rolloutStepSize: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 #