Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove legacy kata config status parts #333

Merged
merged 7 commits into from
Aug 21, 2023
Merged

Remove legacy kata config status parts #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac3e774
introduce status.kataNodes.readyNodeCount for backwards compatibility
pmores Jul 11, 2023
817e7c0
migrate KataConfig CRD's additionalPrinterColumns to new status fields
pmores Jul 11, 2023
2fb12e5
remove legacy status fields usage from uninstallation flow
pmores Jul 11, 2023
383f96b
remove legacy status fields usage from installation flow
pmores Jul 11, 2023
9747a9b
remove legacy status fields usage from envtest
pmores Jul 12, 2023
dce7aef
stop updating legacy parts of KataConfig.status
pmores Jul 11, 2023
6a0a6de
remove legacy structures from KataConfig.status definition
pmores Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
111 changes: 8 additions & 103 deletions api/v1/kataconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,21 +51,6 @@ type KataConfigStatus struct {
// RuntimeClass is the name of the runtime class used in CRIO configuration
RuntimeClass string `json:"runtimeClass"`

// TotalNodesCounts is the total number of worker nodes targeted by this CR
TotalNodesCount int `json:"totalNodesCount"`

// InstallationStatus reflects the status of the ongoing kata installation
// +optional
InstallationStatus KataInstallationStatus `json:"installationStatus,omitempty"`

// UnInstallationStatus reflects the status of the ongoing kata uninstallation
// +optional
UnInstallationStatus KataUnInstallationStatus `json:"unInstallationStatus,omitempty"`

// Upgradestatus reflects the status of the ongoing kata upgrade
// +optional
Upgradestatus KataUpgradeStatus `json:"upgradeStatus,omitempty"`

// +optional
KataNodes KataNodesStatus `json:"kataNodes,omitempty"`

Expand All @@ -85,9 +70,9 @@ type KataConfigStatus struct {
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
// +kubebuilder:resource:path=kataconfigs,scope=Cluster
// +kubebuilder:printcolumn:name="InProgress",type=string,JSONPath=".status.installationStatus.IsInProgress",description="Status of Kata runtime installation"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not mark these fields as deprecated and leave them in? We don't know what tools people have written that rely on these fields, i.e. the .status section to me is part of the API.

We could even add logging for the case that any of the fields are non-empty (unlikely that another software has the rights to update it, but possible). After a while we can bump the version and remove the fields. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's generally the correct way to handle this.

Admittedly I'd still tend to remove the fields now. I suspect there's little automation in use that relies on the old status. The combined status with new and old parts is fairly confusing. Migration from old to new status is fairly easy and mechanical (migrations so far were apparently easy and without problems), the migration guide is included in the #330 cover letter and it's been a couple of weeks since the need to migrate has been announced.

If folks feel more comfortable taking the slow path I wouldn't block it though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can change our automation to check both.
-o=jsonpath='{.status.conditions[?(@.type=="InProgress")].status}' is the new one
-o=jsonpath={.status.installationStatus.IsInProgress}{.status.unInstallationStatus.inProgress.status} is the current one

As long as ToLower() != false, I know things are not in progress

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbuskey I think that checking the new one should be enough by now.

Copy link
Contributor

@jensfr jensfr Jul 31, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay. I think we don't know who's using it and in what way (we were surprised a few times already. We may break users that we are not even aware of. To be honest I think we should take the slow path. What do others think? @bpradipt @snir911 @cpmeadors ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm considering these aspects when providing my view on this PR.
We are adding/improving new features - peer-pods, CoCo which will increase the adoption and thereby make it relativey more harder to make this sort of a change.
Further, this change will only be available in newer versions, and is only affecting the install/uninstall of the operator with no impact to the workloads. Aso it'll be documented so that users relying on earlier status field can make suitable changes to use the newer fields.

Given the above aspects I'm in favour of getting this merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmores I don't have any comments on the code.
lgtm on the code front.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a more general note, although it seems in this case that we can opt to have both the old and new stuff running alongside, this doesn't seem to be true in general. Consider the upcoming change of KataConfig.status.runtimeClass to an array to support runtimeclasses other than plain kata. How would we go about it, add the array as runtimeClassNew first and then rename it back to runtimeClass? What about users who'd ignore the runtimeClassNew addition and continued to use the single-value runtimeClass?

I'm afraid there's no avoiding breaking users who don't pay attention, unfortunately. For users who do, they've had a month now to adapt to the change.

Also, I think we should be pragmatic. As @bpradipt says, changes will be harder when we have a lot of users. If we know already we need to change something, we should consider doing it as fast as possible since it will only get harder in the future I'm afraid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with @pmores and @bpradipt. Better to get rid of this now while we don't have too many users and migration is easy. The gain on code maintenance outperforms the inconvenience for users IMHO.

// +kubebuilder:printcolumn:name="Completed",type=integer,JSONPath=".status.installationStatus.completed.completedNodesCount",description="Number of nodes with Kata runtime installed"
// +kubebuilder:printcolumn:name="Total",type=integer,JSONPath=".status.totalNodesCount",description="Total number of nodes"
// +kubebuilder:printcolumn:name="InProgress",type=string,JSONPath=".status.conditions[?(@.type=='InProgress')].status",description="Status of Kata runtime installation"
// +kubebuilder:printcolumn:name="Completed",type=integer,JSONPath=".status.kataNodes.readyNodeCount",description="Number of nodes with Kata runtime installed"
// +kubebuilder:printcolumn:name="Total",type=integer,JSONPath=".status.kataNodes.nodeCount",description="Total number of nodes"
// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=".metadata.creationTimestamp",description="Age of the KataConfig Custom Resource"
type KataConfig struct {
metav1.TypeMeta `json:",inline"`
Expand All @@ -112,91 +97,6 @@ func init() {
SchemeBuilder.Register(&KataConfig{}, &KataConfigList{})
}

// KataInstallationStatus reflects the status of the ongoing kata installation
type KataInstallationStatus struct {
// InProgress reflects the status of nodes that are in the process of kata installation
InProgress KataInstallationInProgressStatus `json:"inprogress,omitempty"`

// IsInProgress reflects the current state of installing or not installing
IsInProgress corev1.ConditionStatus `json:"IsInProgress,omit"`

// Completed reflects the status of nodes that have completed kata installation
Completed KataConfigCompletedStatus `json:"completed,omitempty"`

// Failed reflects the status of nodes that have failed kata installation
Failed KataFailedNodeStatus `json:"failed,omitempty"`
}

// KataInstallationInProgressStatus reflects the status of nodes that are in the process of kata installation
type KataInstallationInProgressStatus struct {
// InProgressNodesCount reflects the number of nodes that are in the process of kata installation
InProgressNodesCount int `json:"inProgressNodesCount,omitempty"`
// IsInProgress reflects if installation is still in progress
IsInProgress bool `json:"isInProgress,omitempty"`
// +optional
BinariesInstalledNodesList []string `json:"binariesInstallNodesList,omitempty"`
}

// KataConfigCompletedStatus reflects the status of nodes that have completed kata operation
type KataConfigCompletedStatus struct {
// CompletedNodesCount reflects the number of nodes that have completed kata operation
CompletedNodesCount int `json:"completedNodesCount,omitempty"`

// CompletedNodesList reflects the list of nodes that have completed kata operation
// +optional
CompletedNodesList []string `json:"completedNodesList,omitempty"`
}

// KataFailedNodeStatus reflects the status of nodes that have failed kata operation
type KataFailedNodeStatus struct {
// FailedNodesCount reflects the number of nodes that have failed kata operation
FailedNodesCount int `json:"failedNodesCount,omitempty"`
FailedReason string `json:"failedNodesReason,omitempty"`

// FailedNodesList reflects the list of nodes that have failed kata operation
// +optional
FailedNodesList []FailedNodeStatus `json:"failedNodesList,omitempty"`
}

// KataUnInstallationStatus reflects the status of the ongoing kata uninstallation
type KataUnInstallationStatus struct {
// InProgress reflects the status of nodes that are in the process of kata uninstallation
InProgress KataUnInstallationInProgressStatus `json:"inProgress,omitempty"`

// Completed reflects the status of nodes that have completed kata uninstallation
Completed KataConfigCompletedStatus `json:"completed,omitempty"`

// Failed reflects the status of nodes that have failed kata uninstallation
Failed KataFailedNodeStatus `json:"failed,omitempty"`

// Stores an error message if any. Note that this is currently meant for a single
// failure source when kata uninstallation is blocked by existing kata-based pods, so
// handling of this field in the controller code is correspondingly simple. A review
// might be necessary if this field were ever to store messages coming from another
// source.
ErrorMessage string `json:"errorMessage,omitempty"`
}

// KataUnInstallationInProgressStatus reflects the status of nodes that are in the process of kata installation
type KataUnInstallationInProgressStatus struct {
InProgressNodesCount int `json:"inProgressNodesCount,omitempty"`
IsInProgress corev1.ConditionStatus `json:"status"`
// +optional
BinariesUnInstalledNodesList []string `json:"binariesUninstallNodesList,omitempty"`
}

// KataUpgradeStatus reflects the status of the ongoing kata upgrade
type KataUpgradeStatus struct {
}

// FailedNodeStatus holds the name and the error message of the failed node
type FailedNodeStatus struct {
// Name of the failed node
Name string `json:"name"`
// Error message of the failed node reported by the installation daemon
Error string `json:"error"`
}

type KataNodesStatus struct {
// Number of cluster nodes that have kata installed on them including
// those queued for installation and currently installing, though
Expand All @@ -205,6 +105,11 @@ type KataNodesStatus struct {
// +optional
NodeCount int `json:"nodeCount"`

// Number of cluster nodes that have kata installed on them and are
// currently ready to run kata workloads.
// +optional
ReadyNodeCount int `json:"readyNodeCount"`

// +optional
Installed []string `json:"installed,omitempty"`
// +optional
Expand Down