Skip to content

DEVOPS-2689-extend-k-8-s-operator-support-to-include-stateful-sets #43

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

Merged

Conversation

imeliran
Copy link
Contributor

No description provided.

@imeliran imeliran requested a review from moshiko-s May 4, 2025 14:52
eliranb added 3 commits May 13, 2025 15:05
…in helpers.go for consistency with recent changes
…adName and WorkloadType fields, replacing legacy DeploymentName usage. Update indexers and error handling for improved clarity and maintainability.
@moshiko-s moshiko-s removed their request for review May 14, 2025 06:20
eliranb added 9 commits May 14, 2025 11:40
…oad status and field names, replacing DeploymentStatus with WorkloadStatus and updating related assertions for consistency with recent refactorings.
…ted example StatefulSet YAML files for LightrunJavaAgent.
…eferences with Workload and WorkloadType, enhancing clarity and consistency in the LightrunJavaAgent configuration.
… WorkloadName field description to specify that it can refer to either Deployment or StatefulSet, improving understanding for users.
…pecify that it can refer to either Deployment or StatefulSet, enhancing documentation consistency across the LightrunJavaAgent configuration.
…e and statefulSetName with workloadName and workloadType, enhancing clarity and consistency in documentation. Adjust related documentation to reflect these changes and improve user understanding.
…umentation to recommend using WorkloadName and WorkloadType instead for improved clarity and consistency.
…es, updating documentation to recommend using WorkloadName and WorkloadType instead for improved clarity and consistency.
…f the file to ensure proper rendering and adherence to markdown standards.
@imeliran imeliran requested a review from moshiko-s May 14, 2025 12:30

var lightrunJavaAgentList agentv1beta.LightrunJavaAgentList
// Combine results
agents.Items = append(agents.Items, newAgents.Items...)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that someone creates a new Custom resource with workloadName and Type and leave the old CR and it will cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for instance:

  • deployment: example-deploment
  • workload cr: example-workload-cr
  • deployment cr: example-deployment-cr

both workload cr and deployment cr are trying to patch example-deploment . one out of them will succeed to patch and other will throw ReconcileFailed with error deployment already patched:
image

}
}

func (r *LightrunJavaAgentReconciler) determineWorkloadType(lightrunJavaAgent *agentv1beta.LightrunJavaAgent) (agentv1beta.WorkloadType, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should support 2 configurations:

  1. deploymentName is set
  2. workloadName and Type are set
    if both are set you fail return an error.

if deploymentName is set you should log a warning that it's deprecated in favor of workloadName and workloadType.

If we agree you can add:
var isDeploymentConfigured bool = spec.DeploymentName != "";
var isWorkloadConfigured bool = spec.WorkloadName != "" || spec.WorkloadType != "";

if both are true/false you return an error.

return spec.WorkloadType, nil
}

// === Case 3: Misconfigured — Both names are set ===
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant case IMO, see comment above

// Fall back to legacy field if WorkloadName isn't set
deploymentName = lightrunJavaAgent.Spec.DeploymentName
}

Copy link
Contributor

Choose a reason for hiding this comment

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

let's add check if deploymentName is empty and return a specialized error


// reconcileStatefulSet handles the reconciliation logic for StatefulSet workloads
func (r *LightrunJavaAgentReconciler) reconcileStatefulSet(ctx context.Context, lightrunJavaAgent *agentv1beta.LightrunJavaAgent, namespace string) (ctrl.Result, error) {
log := r.Log.WithValues("lightrunJavaAgent", lightrunJavaAgent.Name, "statefulSet", lightrunJavaAgent.Spec.WorkloadName)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a check if workloadName is empty

@imeliran imeliran requested a review from moshiko-s May 20, 2025 08:30
}

//+kubebuilder:object:root=true
//+kubebuilder:subresource:status
//+kubebuilder:resource:shortName=lrja
//+kubebuilder:printcolumn:priority=0,name=Deployment,type=string,JSONPath=".spec.deploymentName",description="Deployment name",format=""
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove this column 😬 and see if we can fill the "Workload" column with deploymenName if using it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As agreed we will remove this column completely

@@ -17,6 +17,10 @@ spec:
scope: Namespaced
versions:
- additionalPrinterColumns:
- description: Deployment name
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -211,6 +215,8 @@ spec:
- type
type: object
type: array
deploymentStatus:
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Contributor Author

@imeliran imeliran May 21, 2025

Choose a reason for hiding this comment

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

it is generated by kubebuilder according to

type LightrunJavaAgentStatus struct {
	LastScheduleTime *metav1.Time       `json:"lastScheduleTime,omitempty"`
	Conditions       []metav1.Condition `json:"conditions,omitempty"`
	WorkloadStatus   string             `json:"workloadStatus,omitempty"`
	DeploymentStatus string             `json:"deploymentStatus,omitempty"`
}

we need to keep DeploymentStatus so we will be able to update Status field accordingly

@@ -86,31 +86,30 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re

func (r *LightrunJavaAgentReconciler) determineWorkloadType(lightrunJavaAgent *agentv1beta.LightrunJavaAgent) (agentv1beta.WorkloadType, error) {
spec := lightrunJavaAgent.Spec
// Check which configuration approach is being used
var isDeploymentConfigured bool = spec.DeploymentName != ""
var isWorkloadConfigured bool = spec.WorkloadName != "" || spec.WorkloadType != ""
Copy link
Contributor

Choose a reason for hiding this comment

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

should be &&


// === Case 1: Legacy only — DeploymentName only ===
if spec.DeploymentName != "" && spec.WorkloadName == "" && spec.WorkloadType == "" {
if isDeploymentConfigured && !isWorkloadConfigured {
Copy link
Contributor

Choose a reason for hiding this comment

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

based on previous suggestion you can simplify to:
if isDeploymentConfigured

return agentv1beta.WorkloadTypeDeployment, nil
}

// === Case 2: New fields — WorkloadName + WorkloadType ===
if spec.DeploymentName == "" && spec.WorkloadName != "" {
if !isDeploymentConfigured && isWorkloadConfigured {
Copy link
Contributor

Choose a reason for hiding this comment

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

based on previous suggestion you can remove the condition here and assume isWorkloadConfigured

eliranb added 6 commits May 21, 2025 11:06
…t functions by including workload names for better debugging clarity.
…Status to streamline CRD definitions and enhance clarity.
…CRDs and example configurations to enhance clarity and maintain consistency across definitions.
… new configuration validation for deploymentName and workloadName, enhancing clarity in failure conditions.
…dation for LightrunJavaAgent. Enhance error handling by ensuring only one configuration method (legacy or new) is used, and provide clearer error messages for invalid configurations.
…etermination for LightrunJavaAgent. Simplify logic by removing unnecessary checks and enhancing clarity in configuration handling.
@imeliran imeliran merged commit 2be38da into main May 22, 2025
2 checks passed
@imeliran imeliran deleted the DEVOPS-2689-extend-k-8-s-operator-support-to-include-stateful-sets branch May 22, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants