search

kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.52k stars 1.3k forks source link

Add KubeadmControlPlanePhase to CRD #10704

Closed sivchari closed 2 months ago

sivchari commented 4 months ago

What would you like to be added (User Story)?

I suggest to add KubeadmControlPlanePhase to CRD. Currently, Cluster and MachineDeployment has individual phase, but KCP doesn't have it. KCP is same as MachineDeployment, because ControlPlane is machine, too.

Detailed Description

TBW

Anything else you would like to add?

No response

Label(s) to be applied

/kind feature One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels.

k8s-ci-robot commented 4 months ago

This issue is currently awaiting triage.

If CAPI contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
fabriziopandini commented 4 months ago

@sivchari which phases do you envision for KCP? how they are defined?

sivchari commented 4 months ago

I want Scaling Up, Scaling Down and Running phase. This phase is changed following and changing by replicas and other field.

chrischdi commented 4 months ago

You should be able to take a look at the ResizedCondition of a KCP:

https://github.com/kubernetes-sigs/cluster-api/blob/88905d4349f793db458b719e9844ba7960ed2004/controlplane/kubeadm/api/v1beta1/condition_consts.go#L61

It is getting set here:

https://github.com/kubernetes-sigs/cluster-api/blob/88905d4349f793db458b719e9844ba7960ed2004/controlplane/kubeadm/internal/controllers/status.go#L65-L86

And from quickly reading it, the condition should either be:

sivchari commented 3 months ago

I didn't know this condition so far, thanks. Then why does MachineDeployment has conditions and phase ?

chrischdi commented 3 months ago

Good question, maybe searching the PRs which introduced the one or the other or the ones for the proposals and reading through them may have context on that.

fabriziopandini commented 2 months ago

I was looking into API conventions and they are stating:

Some resources in the v1 API contain fields called phase, and associated message, reason, and other status fields. The pattern of using phase is deprecated. Newer API types should use conditions instead. Phase was essentially a state-machine enumeration field, that contradicted system-design principles and hampered evolution, since adding new enum values breaks backward compatibility. Rather than encouraging clients to infer implicit properties from phases, we prefer to explicitly expose the individual conditions that clients need to monitor. Conditions also have the benefit that it is possible to create some conditions with uniform meaning across all resource types, while still exposing others that are unique to specific resource types. See #7856 for more details and discussion.

So I think we should close this issue because we should align to API conventions (that' means also dropping existing phase fields from other resources in a future API version)

/close

k8s-ci-robot commented 2 months ago

@fabriziopandini: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/10704#issuecomment-2212462940): >I was looking into API conventions and they are stating: > >> Some resources in the v1 API contain fields called phase, and associated message, reason, and other status fields. The pattern of using phase is deprecated. Newer API types should use conditions instead. Phase was essentially a state-machine enumeration field, that contradicted [system-design principles](https://git.k8s.io/design-proposals-archive/architecture/principles.md#control-logic) and hampered evolution, since [adding new enum values breaks backward compatibility](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md). Rather than encouraging clients to infer implicit properties from phases, we prefer to explicitly expose the individual conditions that clients need to monitor. Conditions also have the benefit that it is possible to create some conditions with uniform meaning across all resource types, while still exposing others that are unique to specific resource types. See [#7856](http://issues.k8s.io/7856) for more details and discussion. > >So I think we should close this issue because we should align to API conventions (that' means also dropping existing phase fields from other resources in a future API version) > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.