⚠️ Stop using deprecated replica counters in controllers #12149
Conversation
k8s-ci-robot
added
area/cluster
fabriziopandini
changed the title
fabriziopandini
force-pushed
the
stop-using-deprecated-replica-counters-in-controllers
branch
from
46ae46b to
62d3e53
Compare
| func (i *Int32) Set(obj *unstructured.Unstructured, value int64) error { | ||
| if err := unstructured.SetNestedField(obj.UnstructuredContent(), value, i.path...); err != nil { | ||
| func (i *Int32) Set(obj *unstructured.Unstructured, value int32) error { | ||
| if err := unstructured.SetNestedField(obj.UnstructuredContent(), int64(value), i.path...); err != nil { |
There was a problem hiding this comment.
is this conversion to int64 correct?
i only ask because of my lack of familiarity with this function, and i had noted all the other conversions from int64 -> int32.
There was a problem hiding this comment.
this conversion is required because SetNestedField only handles int64 values, so we are up-converting before calling this method
There was a problem hiding this comment.
ahh, got it. i thought it might be something like that but when i looked at the SetNestedField function decl i thought it took an interface{} there. thanks!
There was a problem hiding this comment.
It takes an interface{} but uses DeepCopyJSONValue internally
// DeepCopyJSONValue deep copies the passed value, assuming it is a valid JSON representation i.e. only contains
// types produced by json.Unmarshal() and also int64.
// bool, int64, float64, string, []interface{}, map[string]interface{}, json.Number and nil
| func (i *Int32) Set(obj *unstructured.Unstructured, value int64) error { | ||
| if err := unstructured.SetNestedField(obj.UnstructuredContent(), value, i.path...); err != nil { | ||
| func (i *Int32) Set(obj *unstructured.Unstructured, value int32) error { | ||
| if err := unstructured.SetNestedField(obj.UnstructuredContent(), int64(value), i.path...); err != nil { |
There was a problem hiding this comment.
It takes an interface{} but uses DeepCopyJSONValue internally
// DeepCopyJSONValue deep copies the passed value, assuming it is a valid JSON representation i.e. only contains
// types produced by json.Unmarshal() and also int64.
// bool, int64, float64, string, []interface{}, map[string]interface{}, json.Number and nil
| @@ -540,7 +539,7 @@ func calculateStatus(allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet | |||
| V1Beta1: &clusterv1.MachineDeploymentV1Beta1DeprecatedStatus{ | |||
| Conditions: conditions, | |||
| UpdatedReplicas: mdutil.GetActualReplicaCountForMachineSets([]*clusterv1.MachineSet{newMS}), | |||
There was a problem hiding this comment.
l.554 Q: Should we really still use v1beta1 counters to calculate .status.phase?
There was a problem hiding this comment.
Would be great if we can make the phase and conditions line up (e.g. for scaling, not sure if more)
There was a problem hiding this comment.
No we should not.
Is that ok if I will address this in a follow up PR, I have point in my todo list to look into old UpdateStatus funcs (e.g. calculateV1Beta1Status in MD,MS - reconcileV1Beta1Status ) and move out everything not deprecated, like .status.phase.
There was a problem hiding this comment.
Absolutely fine to address in a follow-up, just noticed it here during review
fabriziopandini
added
the
tide/merge-method-squash
|
@fabriziopandini: 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. |
|
/lgtm /hold |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 445fdbc0b582e1f525968ee58b0cc0c5dcc072e6
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
k8s-ci-robot
added
the
approved
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
As defined in https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md, stop using Deprecated replica counters in controllers.
Use new replica counters instead
Which issue(s) this PR fixes:
Rif #11947
/area cluster
/area machinedeployment
/area machineset
/area machine