🌱 Add desired number column to KCP, MD, MS, MachinePool

[Bo0km4n]

What this PR does / why we need it:
I want to look the desired replicas number of KCP's machines when kubectl get kcp.
So I added print column desired replicas of KubeadmControlPlane.

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 #6165

[k8s-ci-robot]

Welcome @Bo0km4n!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @Bo0km4n. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

[killianmuldoon]

Thanks for this @Bo0km4n! I see at the moment we have:

KubeadmControlPlane

NAME             CLUSTER    INITIALIZED   API SERVER AVAILABLE   REPLICAS   READY   UPDATED   UNAVAILABLE   AGE   VERSION
cloister-9l2n7   cloister   true          true                   3          3       3         0             24h   v1.21.2

MachineDeployments:

NAME                           CLUSTER    REPLICAS   READY   UPDATED   UNAVAILABLE   PHASE     AGE   VERSION
cloister-linux-workers-jd4qq   cloister   3          3       3         0             Running   24h   v1.21.2

MachineSets:

NAME                                              CLUSTER      REPLICAS   READY   AVAILABLE   AGE   VERSION
my-cluster-default-worker-topo-bjn28-66767779f7   my-cluster   1                              21h   v1.22.0

Kubernetes deployment:

NAMESPACE                           NAME                                            READY   UP-TO-DATE   AVAILABLE   AGE
capd-system                         capd-controller-manager                         1/1     1            1           25h

Kubernetes daemonset:

NAMESPACE       NAME         DESIRED   CURRENT   READY   UP-TO-DATE   AVAILABLE   NODE SELECTOR            AGE
kube-system     kindnet      1         1         1       1            1           <none>                   25h

I think if we're going to add desired to our KCP machines we should do the same for machineDeployments.

But I think there's a broader question of whether we want to align more with the Kubernetes is doing things here (and how wide is too wide 😄 ) We could also implement an -o wide option which I don't think we have anywhere (not certain how hard that would be to implement)

[Bo0km4n]

@killianmuldoon Thanks. I will add also desired number column to MachinDeployment.
Then, I researched about -o wide option to print desired number when enable wide option, ref: https://book.kubebuilder.io/reference/generating-crd.html#additional-printer-columns

We can set priority param to comment such as +kubebuilder:printcolumn:name=...priority=10 .
Kubernetes's official docs say as below:

Columns with priority 0 are shown in standard view.
Columns with priority greater than 0 are shown only in wide view.

So I want to add column like aboves with priority param to MachineDeployment and KubeadmControlPlane, what do you think about ?

[killianmuldoon]

Thanks for looking into that! I'd like to get a few more folks' insight before getting you to do the implementation. @fabriziopandini @sbueringer @ykakarap WDYT?

[k8s-triage-robot]

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

[ykakarap]

Thanks for looking into that! I'd like to get a few more folks' insight before getting you to do the implementation. @fabriziopandini @sbueringer @ykakarap WDYT?

Would be nice to have Desired, but I do agree that the number of columns might be getting too large already.

[Bo0km4n]

If you worry about number of colums is too large, I suggest to combine 'desired' column with 'ready' column
like as below:

READY
1/3 # .status.readyReplicas / .spec.replicas

[fabriziopandini]

I +1 to make this consistent across all the CAPI objects and use Kubernetes as a reference.
However, if the resulting change is invasive or it implies changing existing fields/field names this most probably qualifies as API change and it should go in the next API version

[sbueringer]

If you worry about number of colums is too large, I suggest to combine 'desired' column with 'ready' column like as below:

READY
1/3 # .status.readyReplicas / .spec.replicas

IIRC the last time I did a deep-dive into kubectl about how that works, the result was that it's a client-side implementation in kubectl and impossible to do with CRDs.

But it would be very nice if we can achieve it

[sbueringer]

Here is the implementation for Deployments: https://github.com/kubernetes/kubernetes/blob/cd91e59f7c351fce47c064a5162c2cb79075159c/pkg/printers/internalversion/printers.go#L2007-L2012

But maybe there is a way to configure that behavior in a CRD that I'm not aware of.

[Bo0km4n]

IIRC the last time I did a deep-dive into kubectl about how that works, the result was that it's a client-side implementation in kubectl and impossible to do with CRDs.

I was looking into how to print column such as 1/3, and you were right, we can't implementation above format in client-side.
So I think we have only one option to use priority column and -o wide option

if the resulting change is invasive or it implies changing existing fields/field names this most probably qualifies as API change and it should go in the next API version

Is changing print-column included in breaking change api-version ?

[ykakarap]

Is changing print-column included in breaking change api-version ?

If you are just adding new columns without changing any of the existing columns then I don't think it will be considered a breaking change.

[Bo0km4n]

If you are just adding new columns without changing any of the existing columns then I don't think it will be considered a breaking change.

In this PR, I would like to add only desired print-column to KCP and MachineDeployment to v1beta1.
I will don''t change some parameter of CRD.

[ykakarap]

In this PR, I would like to add only desired print-column to KCP and MachineDeployment to v1beta1. I will don''t change some parameter of CRD.

This might be okay. I will let @fabriziopandini confirm.

[fabriziopandini]

In this PR, I would like to add only desired print-column to KCP and MachineDeployment to v1beta1. I will don''t change
some parameter of CRD.

This might be okay. I will let @fabriziopandini confirm.

If we are achieving consistency across all the CAPI objects and we are in line with Kubernetes by adding this column I'm ok, but as far as I remember showing the desired state vs the current state is a broader problem (see e.g #5332 as linked by @sbueringer in the issue).
I don't want to block, but I would like to be sure we are moving in the right direction and with a plan; can we add something to the issue/here comparing all the CAPI resources with replica counts?

[killianmuldoon]

@fabriziopandini We have MachineDeployments, MachineSets, KubeadmControlPlane here #6164 (review) .

Is there something missing that has replicas? (I'm assuming we can leave MachinePools out of the conversation for now)

[sbueringer]

Looks like the current state is:

• KCP, MD:
  • Replicas => .status.replicas
  • Ready => .status.readyReplicas
  • Updated => .status.updatedReplicas
  • Unavailable => .status.unavailableReplicas
• MS
  • Replicas => .status.replicas
  • Ready => .status.readyReplicas
  • Available => .status.availableReplicas
• MachinePools
  • Replicas => .status.replicas

[ykakarap]

Looks like the current state is:

• KCP, MD:

  • Replicas => .status.replicas
  • Ready => .status.readyReplicas
  • Updated => .status.updatedReplicas
  • Unavailable => .status.unavailableReplicas

• MS

  • Replicas => .status.replicas
  • Ready => .status.readyReplicas
  • Available => .status.availableReplicas

• MachinePools

  • Replicas => .status.replicas

Since the Replicas field in all of the types comes from status I think we should be fine with adding a Desired Replicas (better names are welcome :) ) columns in all of them and names and its meaning will remain consistent across all the types.

[Bo0km4n]

Can I add Desired column to KCP, MD, MS, MachinePools ?

[sbueringer]

Sounds good to me. Relevant prior art for me is the Daemonset and ReplicaSet DESIRED column.

Given that we consider the columns APIs and there is no way to deprecate them, I would really like to hear opinions of other maintainers before going ahead with this (or at least before merging).
@CecileRobertMichon @enxebre @fabriziopandini @vincepri

[fabriziopandini]

@sbueringer @killianmuldoon thanks for investigating this cross the board!

Let's wait for some more feedback before acting; my two cents:

• +1 to add the desired column to all the objects (this PR)
• let's open an API-change issue for milestone Next, tracking rename of replicas to current so we can achieve consistency with Kubernetes DeamonSets when we are going to rev the next API version
• let's open an API-change issue for milestone Next, tracking the differences between other replica-related columns in our API types; this probably requires some more investigation but the current situation is not ideal...

[fabriziopandini]

@Bo0km4n thank you for your patience, really appreaciated it!
if you can add the Desired column to KCP, MD, MS, MachinePools than we can get this merged

[Bo0km4n]

@fabriziopandini Thanks, I fixed it.

[sbueringer]

Suggested change

	// +kubebuilder:printcolumn:name="Replicas",type="integer",JSONPath=".status.replicas",description="Total number of non-terminated machines targeted by this machineset"
	// +kubebuilder:printcolumn:name="Desired",type=integer,JSONPath=".spec.replicas",description="Total number of machines desired by this machineset",priority=10
	// +kubebuilder:printcolumn:name="Desired",type=integer,JSONPath=".spec.replicas",description="Total number of machines desired by this machineset",priority=10
	// +kubebuilder:printcolumn:name="Replicas",type="integer",JSONPath=".status.replicas",description="Total number of non-terminated machines targeted by this machineset"

Let's use the same order in all CRDs

[sbueringer]

Also in MachinePool

[sbueringer]

/retitle 🌱 Add desired number column to KCP, MD, MS, MachinePool

(to make the GitHub check happy)

[enxebre]

Thanks!
/ok-to-test
/lgtm

I'm +1 to this and follow up with "current". I can't think of any drawback while it provides clear and consistent output.

[killianmuldoon]

Thanks @Bo0km4n - looking good! More general question - should we remove the priority=10 with a future release to bring this fully into line with Daemonsets? I'd like to document it on #6238 if so.

[sbueringer]

Hm does priority 10 mean it's only visible with --wide currently?

[killianmuldoon]

Hm does priority 10 mean it's only visible with --wide currently?

Exactly

[sbueringer]

Hm does priority 10 mean it's only visible with --wide currently?

Exactly

I wonder if we should add this column without priority. Doesn't seem that useful if it's not visible in most cases

[killianmuldoon]

It could be an idea to use -o wide as a way to 'deprecate' and add columns between API revisions, and document in the migration guide before making the change to the default output in the next version.
We could document this as the process for changing our output columns, given we don't currently use -o wide anywhere. @fabriziopandini @enxebre WDYT?

[sbueringer]

I really don't like to use -o wide as a way to show/hide deprecated columns. It's purpose is that we're able to define which columns should only be shown with wide.

I think we shouldn't overthink the deprecation :). Clear documentation has to be enough for things like changing columns of CRDs.

[fabriziopandini]

/lgtm
/approve
@sbueringer @killianmuldoon Let's move the discussion about weight or any pending issue into #6238, this is already a consistent step in the right direction and we can iterate.

thanks @Bo0km4n for your first contribution to CAPI 🥳

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

• OWNERS [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

/retest