Add utility functions for new status check #260
Conversation
lllamnyp
force-pushed
the
feat/new-status-check-pt2
branch
from
6993adf to
cb2a88f
Compare
lllamnyp
force-pushed
the
feat/new-status-check-pt3
branch
from
d9036b3 to
4b22a70
Compare
WalkthroughThe changes introduce several modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EtcdClusterReconciler
participant KubernetesAPI
User->>EtcdClusterReconciler: Request to manage Etcd cluster
EtcdClusterReconciler->>KubernetesAPI: Patch or create object
alt Object not found
KubernetesAPI-->>EtcdClusterReconciler: Not Found
EtcdClusterReconciler->>KubernetesAPI: Create new object
end
KubernetesAPI-->>EtcdClusterReconciler: Object updated/created
EtcdClusterReconciler-->>User: Response with status
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lllamnyp
force-pushed
the
feat/new-status-check-pt3
branch
4 times, most recently
from
672010f to
34f88e6
Compare
lllamnyp
changed the title
lllamnyp
requested review from
sircthulhu,
Kirill-Garbar and
sergeyshevch
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
internal/controller/etcdcluster_controller.go (3)
666-677: Implementation looks good, but remove the TODO andnolint:unusedcomments when the function is used.The
patchOrCreateObjectfunction follows a common pattern for patching or creating Kubernetes objects in a consistent manner. The implementation looks correct.However, please remove the TODO comment and the
nolint:unusedcomment when the function is integrated into the codebase to avoid confusion and maintain code cleanliness.
679-692: Complete the implementation and remove the TODO,nolintcomments, and panic statement.The
createClusterFromScratchfunction is intended to create a new EtcdCluster from scratch, but the implementation is currently incomplete. Please consider the following suggestions:
Remove the TODO and
nolint:unparam,unusedcomments when the function is fully implemented and integrated into the codebase to avoid confusion and maintain code cleanliness.Complete the implementation of the function by adding the necessary logic to create the EtcdCluster from scratch. This may involve creating additional resources, updating the cluster status, etc.
Remove the panic statement at the end of the function and replace it with appropriate error handling and return statements based on the desired behavior of the function.
Ensure that the function returns a meaningful
ctrl.Resultand error based on the outcome of the cluster creation process.
694-698: Implement the logic to scale up the EtcdCluster from zero replicas.The
scaleUpFromZerofunction is intended to scale up an EtcdCluster from zero replicas, but the implementation is currently missing. Please consider the following suggestions:
Remove the TODO and
nolint:unusedcomments when the function is fully implemented and integrated into the codebase to avoid confusion and maintain code cleanliness.Implement the necessary logic to scale up the EtcdCluster from zero replicas. This may involve updating the desired replicas, creating or updating resources, and handling any necessary reconciliation tasks.
Remove the panic statement and replace it with appropriate error handling and return statements based on the desired behavior of the function.
Ensure that the function returns a meaningful
ctrl.Resultand error based on the outcome of the scale-up process.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- internal/controller/etcdcluster_controller.go (2 hunks)
- internal/controller/factory/pvc.go (2 hunks)
- internal/controller/factory/statefulset.go (1 hunks)
- internal/controller/observables.go (3 hunks)
Additional comments not posted (11)
internal/controller/observables.go (6)
5-6: LGTM!Commenting out unused imports is a good practice to keep the code clean.
9-10: LGTM!The new import statement for "github.com/aenix-io/etcd-operator/api/v1alpha1" is added correctly. Commenting out the unused import statement for "github.com/aenix-io/etcd-operator/pkg/set" is a good practice to keep the code clean.
30-30: LGTM!The new field
instanceof type*v1alpha1.EtcdClusteris added correctly to theobservablesstruct. As per the AI-generated summary, this field likely enhances the operator's ability to manage and observe the state of Etcd clusters.
33-33: LGTM!The new field
endpointsof type[]stringis added correctly to theobservablesstruct. As per the AI-generated summary, this field is likely a placeholder for storing endpoint information related to the Etcd cluster. The field is marked as unused, indicating potential future use.
37-37: LGTM!The new field
pvcsof type[]corev1.PersistentVolumeClaimis added correctly to theobservablesstruct. As per the AI-generated summary, this change clarifies the purpose of this field within the struct. The field is marked as unused, indicating potential future use.
77-78: LGTM!The method
_()has been renamed todesiredReplicas()correctly. As per the AI-generated summary, the new name improves code readability and maintainability. The method's logic remains unchanged.internal/controller/factory/pvc.go (2)
33-39: LGTM!The
PVCLabelsfunction correctly combines the default pod labels with the custom labels specified in the cluster's volume claim template. This allows for flexible labeling of PVCs based on the cluster configuration.
49-57: LGTM!The
PVCsfunction correctly retrieves a list of PVCs matching the labels generated byPVCLabels. It uses the Kubernetes client to list the PVCs in the specified context and returns the items or an error if the operation fails.internal/controller/factory/statefulset.go (2)
44-47: Placeholder function for future implementation.The
TemplateStatefulSetfunction is currently a placeholder that raises a panic. Please remember to implement this function when the corresponding functionality is ready.
49-59: LGTM!The new
PodLabelshelper function is a great addition that centralizes the label creation logic for pods. This enhances code readability and maintainability by reducing duplication and making it easier to maintain consistency across the codebase.internal/controller/etcdcluster_controller.go (1)
94-94: LGTM!Initializing
state.instancewith the current instance of the EtcdCluster resource is a good change that aligns with the goal of updating theobservablesstruct for better state observation. This allows the reconciliation logic to access the desired state of the EtcdCluster resource.
lllamnyp
force-pushed
the
feat/new-status-check-pt3
branch
from
34f88e6 to
5e22c32
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
internal/controller/observables.go (4)
5-6: Consider removing commented-out imports if they are no longer needed.There are several commented-out import statements in the file. If these imports are no longer needed, it's best to remove them entirely rather than leaving them as comments. This helps keep the codebase clean and easier to maintain.
If you anticipate needing these imports in the near future, consider adding a TODO comment explaining why they're kept and when they might be needed.
Also applies to: 9-10
26-37: Approved: New fields added toobservablesstruct. Consider adding explanatory comments.The new fields
instance,endpoints, andpvcshave been added to theobservablesstruct, which aligns with the PR objectives of introducing utility functions for new status checks.To improve code readability and maintainability:
- Consider adding comments explaining the purpose of each new field, especially for
instanceandendpoints.- For the fields marked with
nolint:unused, consider adding a TODO comment explaining when these fields are expected to be used in the future.Example:
type observables struct { // instance holds the reference to the EtcdCluster being reconciled instance *v1alpha1.EtcdCluster // ... other fields ... // endpoints represents the list of etcd endpoints // TODO: This field will be used in future implementations for endpoint management endpoints []string //nolint:unused // ... other fields ... }This will help other developers understand the intended use of these fields in the context of the ongoing feature development.
Line range hint
77-87: Approved: NewdesiredReplicas()method added. Consider clarifying the TODO comment.The new
desiredReplicas()method has been added to theobservablesstruct. The logic looks correct for determining the desired number of replicas based on the first non-nil member list inetcdStatuses.To improve clarity and maintainability:
- Consider expanding the TODO comment to explain the intended future use of this method and any pending implementation details.
- You might want to add a comment explaining the logic of the method, especially why it returns the length of the first non-nil member list.
Example:
// TODO: Implement full functionality for determining desired replicas. // This method will be used in the reconciliation loop to determine the // desired number of replicas for the EtcdCluster. // nolint:unused func (o *observables) desiredReplicas() int { // Return the number of members in the first non-nil member list // This assumes that all member lists should be consistent in a healthy cluster if o.etcdStatuses != nil { for i := range o.etcdStatuses { if o.etcdStatuses[i].memberList != nil { return len(o.etcdStatuses[i].memberList.Members) } } } return 0 }This will help other developers understand the purpose and current limitations of this method.
Line range hint
1-87: Overall structure is consistent, but consider adding more context for future development.The file structure is consistent and aligns well with the PR objectives of laying groundwork for future features. The mix of fully implemented methods (like
setClusterID()andinSplitbrain()) and placeholder functionality (likedesiredReplicas()) is appropriate for this stage of development.To improve the file's maintainability and guide future development:
- Consider adding a file-level comment explaining the current state of development and the overall purpose of this file in the context of the EtcdCluster operator.
- For each TODO comment or
nolintdirective, try to provide more context about what needs to be done and why. This will help developers (including your future self) understand the intended direction of the code.- If possible, create issues or link to existing issues for each TODO item to track the remaining work.
Example file-level comment:
/* Package controller implements reconciliation logic for EtcdCluster resources. This file contains the observables struct and associated methods, which are used to gather and process information about the current state of an EtcdCluster. Some functionality is still in development, as indicated by TODO comments. Related to feature #181: [Brief description of the feature] */These additions will help maintain a clear roadmap for the ongoing development of this package.
internal/controller/factory/pvc.go (1)
49-57: LGTM! Well-implemented function for retrieving PVCs with a minor suggestion.The
PVCsfunction is implemented correctly and efficiently. It uses thePVCLabelsfunction to generate labels for filtering and the Kubernetes client to list matching PVCs. The error handling and return values are appropriate.Consider enhancing the error handling to provide more context:
func PVCs(ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, cli client.Client) ([]corev1.PersistentVolumeClaim, error) { labels := PVCLabels(cluster) pvcs := corev1.PersistentVolumeClaimList{} err := cli.List(ctx, &pvcs, client.MatchingLabels(labels)) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to list PVCs for cluster %s/%s: %w", cluster.Namespace, cluster.Name, err) } return pvcs.Items, nil }This change would make debugging easier by providing more information about which cluster was involved when an error occurs.
internal/controller/factory/statefulset.go (1)
Line range hint
1-605: Summary of changes in statefulset.go
- A new
TemplateStatefulSetfunction has been added, but it's currently unimplemented.- The
CreateOrUpdateStatefulSetfunction has been improved to use a centralizedPodLabelsfunction for label management.These changes align with the PR objectives of introducing utility functions for new status checks. The
TemplateStatefulSetfunction, once implemented, will likely contribute to the groundwork for future functionality.Consider the following recommendations:
- Implement the
TemplateStatefulSetfunction in this PR if it's crucial for the current objectives.- If
TemplateStatefulSetis for future use, add a comment explaining its intended purpose and when it will be implemented.- Update the PR description or add a comment in the code to explain how these changes contribute to the new status check functionality mentioned in the PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- internal/controller/etcdcluster_controller.go (2 hunks)
- internal/controller/factory/pvc.go (2 hunks)
- internal/controller/factory/statefulset.go (1 hunks)
- internal/controller/observables.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (7)
internal/controller/observables.go (1)
Line range hint
1-87: Summary: Changes align well with PR objectives and lay groundwork for future functionality.The modifications in this file successfully introduce the groundwork for new utility functions related to EtcdCluster status checks and management, as outlined in the PR objectives. Key points:
- The new
instancefield in theobservablesstruct allows for holding a reference to an EtcdCluster, which is crucial for the planned functionality.- The placeholder
desiredReplicas()method aligns with the goal of supporting various stages in the reconciliation loop, particularly for determining cluster size.- The changes are preparatory and don't alter existing functionality, maintaining the current behavior of the operator.
These additions set the stage for implementing the functions mentioned in the PR objectives, such as
createClusterFromScratchandscaleUpFromZero, in future development iterations.The approach of introducing these changes incrementally is commendable, as it allows for gradual implementation and testing of new features without disrupting existing functionality.
internal/controller/factory/pvc.go (2)
33-39: LGTM! Well-implemented function for PVC label generation.The
PVCLabelsfunction is implemented correctly and efficiently. It combines default pod labels with any additional labels specified in the EtcdCluster's volume claim template. The use of the existingPodLabelsfunction and the idiomatic approach to merging maps in Go are particularly noteworthy.
Line range hint
33-57: New PVC utility functions align well with PR objectives.The addition of
PVCLabelsandPVCsfunctions enhances the capability to manage PersistentVolumeClaims for EtcdClusters. These changes align well with the PR objectives of introducing utility functions for new status checks.To ensure these new functions are properly integrated, let's verify their usage across the codebase:
This will help confirm that the new functions are being utilized as intended in other parts of the codebase.
✅ Verification successful
PVC utility functions are correctly added.
The newly introduced
PVCLabelsandPVCsfunctions are properly integrated withininternal/controller/factory/pvc.go. No usages of these functions are found elsewhere in the codebase, which is expected for new additions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new PVC utility functions # Test: Search for usage of PVCLabels function echo "Searching for PVCLabels usage:" rg --type go "PVCLabels\(" --glob '!internal/controller/factory/pvc.go' # Test: Search for usage of PVCs function echo "Searching for PVCs usage:" rg --type go "PVCs\(" --glob '!internal/controller/factory/pvc.go'Length of output: 274
Script:
#!/bin/bash # Description: Search for invocations of PVCLabels and PVCs functions using ast-grep # Search for PVCLabels function calls echo "Searching for PVCLabels function calls:" ast-grep --lang go --pattern 'CallExpr:func: "PVCLabels"($_)' . # Search for PVCs function calls echo "Searching for PVCs function calls:" ast-grep --lang go --pattern 'CallExpr:func: "PVCs"($_)' .Length of output: 294
internal/controller/factory/statefulset.go (2)
67-67: LGTM: Improved label management usingPodLabelsfunction.The change to use the
PodLabelsfunction for setting theLabelsfield inpodMetadatais a good improvement. It centralizes the label creation logic, enhancing maintainability and adhering to the DRY principle.
44-47:⚠️ Potential issueImplement or remove the
TemplateStatefulSetfunction.The
TemplateStatefulSetfunction is currently a placeholder with a panic statement. This could lead to runtime errors if called elsewhere in the codebase.Consider one of the following actions:
- Implement the function if it's needed for the current PR objectives.
- Remove the function if it's not immediately necessary.
- If the implementation is planned for a future PR, replace the panic with a more informative error message.
Example of a more informative error:
func TemplateStatefulSet() *appsv1.StatefulSet { return nil, errors.New("TemplateStatefulSet: not yet implemented") }To ensure this function is not called elsewhere, run:
✅ Verification successful
Remove the
TemplateStatefulSetfunction as it's not used.The
TemplateStatefulSetfunction is currently defined but not called anywhere in the codebase. Removing this unimplemented placeholder will help avoid potential confusion and reduce dead code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to TemplateStatefulSet rg "TemplateStatefulSet\(" --type goLength of output: 130
internal/controller/etcdcluster_controller.go (2)
94-94: Assignment ofinstancetostate.instanceis appropriateThe line correctly initializes
state.instancewith the currentinstance, which is necessary for subsequent state management.
668-676: FunctionpatchOrCreateObjecthandles object patching and creation effectivelyThe
patchOrCreateObjectmethod correctly attempts to patch the object and falls back to creation if the object does not exist. The use ofclient.Applyandclient.ForceOwnershipensures that the desired state is applied properly.
This is part of a series of stacked PRs. The previous PR in the stack is #259. The next PR is #265.
This commit includes some changes for feature #181. It adds skeletons for utility functions that represent various steps in the reconciliation loop, e.g.
createClusterFromScratchif it is determined, that no resources for the EtcdCluster exist yet, orscaleUpFromZero, when a cluster was at one point scaled down and temporarily shut down, but the data is still being stored. Care is taken not to alter existing functionality yet.Summary by CodeRabbit
New Features
Bug Fixes
observablesstruct for better readability.Documentation