feat: check CoreDNS #120
feat: check CoreDNS #120
Conversation
WalkthroughThe pull request introduces several changes, including an update to the Changes
Assessment against linked issues
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
Documentation and Community
|
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
2 times, most recently
from
038cb0c to
9678468
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
go.mod (1)
Line range hint
3-3: Verify Go Version CompatibilityThe module specifies Go 1.23, which seems incorrect as the latest stable version is Go 1.22. This might be a typo or an unstable version.
Please update to use a stable Go version (e.g.,
go 1.22).pkg/local/preflight/checker.go (1)
471-502: Consider refactoring for better maintainability
- Define a constant for minimum required replicas.
- Consolidate the replica checks and logging for better flow.
+const minRequiredCoreDNSReplicas = 2 func (local *Checker) checkCoreDNS() { logrus.Info("Checking if CoreDNS has multiple replicas") - daemonSet, err := commonkube.GetDeployment(local.kubeClient, metav1.NamespaceSystem, consts.AppNameCoreDNS) + deployment, err := commonkube.GetDeployment(local.kubeClient, metav1.NamespaceSystem, consts.AppNameCoreDNS) if err != nil { - local.collection.Log.Error = append(local.collection.Log.Warn, errors.Wrap(err, "failed to check CoreDNS").Error()) + local.collection.Log.Error = append(local.collection.Log.Error, errors.Wrap(err, "failed to check CoreDNS").Error()) return } - if daemonSet.Spec.Replicas == nil || *daemonSet.Spec.Replicas < 2 { - local.collection.Log.Warn = append(local.collection.Log.Warn, "CoreDNS is set with fewer than 2 replicas; consider increasing replica count for high availability") - return - } + desiredReplicas := int32(0) + if deployment.Spec.Replicas != nil { + desiredReplicas = *deployment.Spec.Replicas + } - if daemonSet.Status.ReadyReplicas < 2 { - local.collection.Log.Warn = append(local.collection.Log.Warn, "CoreDNS has fewer than 2 ready replicas; some replicas may not be running or ready") + if desiredReplicas < minRequiredCoreDNSReplicas { + local.collection.Log.Warn = append(local.collection.Log.Warn, + fmt.Sprintf("CoreDNS is set with %d replicas; consider increasing replica count to at least %d for high availability", + desiredReplicas, minRequiredCoreDNSReplicas)) } - local.collection.Log.Info = append(local.collection.Log.Info, fmt.Sprintf("CoreDNS is set with %d replicas and %d ready replicas", *daemonSet.Spec.Replicas, daemonSet.Status.ReadyReplicas)) + if deployment.Status.ReadyReplicas < minRequiredCoreDNSReplicas { + local.collection.Log.Warn = append(local.collection.Log.Warn, + fmt.Sprintf("CoreDNS has only %d ready replicas; some replicas may not be running or ready", + deployment.Status.ReadyReplicas)) + } + + local.collection.Log.Info = append(local.collection.Log.Info, + fmt.Sprintf("CoreDNS is set with %d replicas and %d ready replicas", + desiredReplicas, deployment.Status.ReadyReplicas)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(1 hunks)pkg/consts/preflight.go(1 hunks)pkg/local/preflight/checker.go(3 hunks)pkg/remote/preflight/checker.go(1 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go(1 hunks)vendor/modules.txt(2 hunks)
🔇 Additional comments (9)
pkg/consts/preflight.go (1)
7-8: LGTM! Well-integrated constant addition.
The new AppNameCoreDNS constant follows the established naming convention and uses the correct lowercase value that matches Kubernetes naming standards. The constant is properly grouped with other app names and will integrate well with the CoreDNS preflight checks.
vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go (1)
1-11: LGTM: Clean package structure with appropriate imports
The package structure is well-organized with all necessary imports for Kubernetes client operations and logging.
go.mod (1)
7-7:
Security & Maintainability Concern: Using Personal Fork as Dependency
Replacing github.com/longhorn/go-common-libs with a personal fork raises several concerns:
- Security: Personal forks may not undergo the same security review and testing as the main repository
- Maintainability: Forks can become stale or unmaintained
- Visibility: No documentation explaining why the fork is necessary
Please clarify:
- Why is the fork necessary?
- Are the changes in the fork planned to be contributed back to the main repository?
- What is the long-term maintenance plan for the forked dependency?
pkg/remote/preflight/checker.go (3)
168-168: LGTM with testing reminder
The RBAC permission change looks good and is necessary for the CoreDNS check functionality. Please ensure that:
- The change is tested with various RBAC configurations
- The CoreDNS check works as expected in both success and failure scenarios
168-168: Verify minimal RBAC permissions for CoreDNS check
The addition of "deployments" permission is aligned with the PR objective to check CoreDNS replicas. However, let's verify if we can further restrict the scope:
- Consider namespace-specific permission instead of cluster-wide
- Verify if we need both "daemonsets" and "deployments" permissions
#!/bin/bash
# Description: Check CoreDNS deployment location and usage of daemonsets permission
# Check where CoreDNS is deployed
echo "Checking CoreDNS deployment namespace:"
rg -l "kind:\s+Deployment.*name:\s+coredns"
# Check if daemonsets permission is still needed
echo "Checking daemonset usage in code:"
rg "daemonsets?" --type go -B 2 -A 2168-168: Verify complete implementation of CoreDNS check
While the RBAC permission is added here, let's verify the implementation of the actual CoreDNS check:
✅ Verification successful
Let's gather more information about the CoreDNS implementation by examining the identified files.
CoreDNS replica check implementation is complete and well-implemented
The implementation is properly in place with:
- Complete
checkCoreDNS()function inpkg/local/preflight/checker.go - Proper constant definition in
pkg/consts/preflight.go - Well-documented function that checks both desired and ready replicas
- Appropriate warning messages for both cases (<2 replicas specified or <2 ready replicas)
- Proper error handling and logging
The RBAC permissions added in the review match the implementation's requirements for accessing deployments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify implementation of CoreDNS replica check
# Look for CoreDNS-related code
echo "Searching for CoreDNS check implementation:"
rg -l "CoreDNS" --type go
# Look for deployment replica check logic
echo "Searching for deployment replica check logic:"
ast-grep --pattern 'func $CHECK_FUNC($ARGS) {
$$$
Replicas $$$
$$$
}'
Length of output: 356
Script:
#!/bin/bash
# Check CoreDNS-related code in the identified files
echo "Content of pkg/consts/preflight.go with CoreDNS context:"
rg -A 5 -B 5 "CoreDNS" pkg/consts/preflight.go
echo -e "\nContent of pkg/local/preflight/checker.go with CoreDNS context:"
rg -A 10 -B 10 "CoreDNS" pkg/local/preflight/checker.go
# Look for deployment replica checks in these specific files
echo -e "\nLooking for deployment/replica checks:"
rg -A 5 -B 5 "deployment.*replica|replica.*deployment" pkg/local/preflight/checker.go
Length of output: 3909
pkg/local/preflight/checker.go (2)
56-64: LGTM: Proper Kubernetes client initialization
The initialization of the Kubernetes client is well-structured with appropriate error handling and wrapping.
180-180: LGTM: Appropriate placement of CoreDNS check
The CoreDNS check is correctly placed before OS-specific checks since it's a cluster-level verification.
vendor/modules.txt (1)
136-136: Justify the use of a personal fork instead of the official library.
I notice that the PR replaces github.com/longhorn/go-common-libs with a personal fork github.com/c3y1huang/go-common-libs. While this might be necessary for development, there are several concerns:
- Security: Using a personal fork instead of the official library could introduce security risks.
- Maintenance: Personal forks may not be maintained long-term.
- Best Practices: It's generally recommended to contribute changes back to the original repository rather than maintaining a separate fork.
Could you please:
- Explain why the fork is necessary
- Consider contributing the changes back to the original repository
- Document the specific changes made in the fork
Let's verify the changes in the forked repository:
Also applies to: 931-931
vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go
Outdated
Show resolved
Hide resolved
|
Test results:
|
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
from
9678468 to
86d0859
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
from
86d0859 to
dc7a477
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
pkg/local/preflight/checker.go (2)
472-502: Consider using constants for replica threshold.The magic number "2" is used multiple times. Consider defining it as a constant for better maintainability.
Add this at the package level:
+const minCoreDNSReplicas = 2Then use it in the checks:
-if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas < 2 { +if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas < minCoreDNSReplicas { -if deployment.Status.ReadyReplicas < 2 { +if deployment.Status.ReadyReplicas < minCoreDNSReplicas {
501-501: Consider security implications of logging deployment details.The log message exposes information about the cluster configuration. Consider making this a debug-level log or removing specific numbers.
-local.collection.Log.Info = append(local.collection.Log.Info, fmt.Sprintf("CoreDNS is set with %d replicas and %d ready replicas", *deployment.Spec.Replicas, deployment.Status.ReadyReplicas)) +local.collection.Log.Info = append(local.collection.Log.Info, "CoreDNS has sufficient replicas and is ready")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(1 hunks)pkg/consts/preflight.go(1 hunks)pkg/local/preflight/checker.go(3 hunks)pkg/remote/preflight/checker.go(1 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go(1 hunks)vendor/modules.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- pkg/consts/preflight.go
- pkg/remote/preflight/checker.go
- vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go
- vendor/modules.txt
🔇 Additional comments (2)
pkg/local/preflight/checker.go (2)
56-64: LGTM! Clean and straightforward client initialization.
The simplified Kubernetes client initialization with proper error handling looks good.
180-180: LGTM! Good placement of CoreDNS check.
The CoreDNS check is appropriately placed before OS-specific checks.
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
2 times, most recently
from
7780ea8 to
77362e7
Compare
|
Hi @c3y1huang |
This PR is proposing to add the CoreDNS check to the |
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
from
77362e7 to
af34ac2
Compare
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
from
af34ac2 to
430891e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
pkg/local/preflight/checker.go (2)
476-476: Fix typo in comment-// "kube-app: kube-dns" lable from the kubeClient and checks the +// "kube-app: kube-dns" label from the kubeClient and checks the🧰 Tools
🪛 GitHub Check: codespell
[failure] 476-476:
lable ==> label, ladle, labile, able
472-511: Consider alternative DNS providersThe function assumes CoreDNS/KubeDNS is the only DNS provider. Consider:
- Adding support for other DNS providers
- Making the check optional or configurable
- Adding documentation about supported DNS configurations
This is particularly important as the PR objectives mention this check is part of fixing DR volume reattachment issues, which might occur with different DNS setups.
🧰 Tools
🪛 GitHub Check: codespell
[failure] 476-476:
lable ==> label, ladle, labile, able
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/consts/preflight.go(1 hunks)pkg/local/preflight/checker.go(3 hunks)pkg/remote/preflight/checker.go(1 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go(2 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/kubernetes.go(2 hunks)vendor/modules.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- go.mod
- pkg/consts/preflight.go
- pkg/remote/preflight/checker.go
- vendor/modules.txt
🧰 Additional context used
🪛 GitHub Check: codespell
pkg/local/preflight/checker.go
[failure] 476-476:
lable ==> label, ladle, labile, able
🔇 Additional comments (9)
vendor/github.com/longhorn/go-common-libs/kubernetes/kubernetes.go (3)
6-7: LGTM: Import statements are correctly added
The new imports for metav1 and labels are properly organized and necessary for the label selector functionality.
29-33: LGTM: Clean implementation of label selector creation
The function is well-implemented, using standard Kubernetes API methods and providing a clean interface for creating label selectors.
29-33: Consider upstream contribution instead of maintaining a fork
This change is currently implemented in a forked version of go-common-libs (c3y1huang/go-common-libs instead of longhorn/go-common-libs). Consider contributing this utility function back to the upstream repository to avoid maintaining a separate fork.
Let's verify if this function exists in the upstream repository:
✅ Verification successful
Let me verify the PR details to make a more informed decision.
Let me check the actual content of the PR to verify if it contains the same implementation.
PR #87 already contains this implementation
The exact same NewLabelSelectorFromMap function implementation is already being contributed upstream through PR #87 (longhorn/go-common-libs#87) by the same author. The PR is currently open and under review. No need to create a separate contribution.
- The implementation in PR fix(deps): update patch digest dependencies to v0.31.1 (master) #87 matches exactly:
func NewLabelSelectorFromMap(labels map[string]string) (labels.Selector, error) { return metav1.LabelSelectorAsSelector(&metav1.LabelSelector{ MatchLabels: labels, }) }
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if similar functionality exists in the upstream repository
# and if there are any existing PRs for this feature
# Check if the function exists in the upstream repository
gh repo clone longhorn/go-common-libs /tmp/upstream-lib
cd /tmp/upstream-lib
git grep -l "NewLabelSelectorFromMap"
# Check for any related open PRs
gh pr list -R longhorn/go-common-libs --search "NewLabelSelectorFromMap"
Length of output: 394
Script:
#!/bin/bash
# Check the PR content to see if it's the same implementation
gh pr view 87 -R longhorn/go-common-libs
Length of output: 889
Script:
#!/bin/bash
# Get the PR diff to check the implementation
gh pr diff 87 -R longhorn/go-common-libs | grep -A 10 "NewLabelSelectorFromMap"
Length of output: 648
vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go (3)
10-10: LGTM: Import addition is appropriate
The added import for labels package is necessary for the new label selector functionality.
Line range hint 15-23: Consider improving context handling and input validation
The existing implementation could be improved as previously suggested:
- Accept a context parameter instead of using context.Background()
- Add validation for input parameters
- Add proper error handling for nil client
This is especially important as this function may be used in the CoreDNS check implementation.
26-44: Verify usage in CoreDNS check implementation
Since these functions are being added as part of implementing the CoreDNS check feature, we should verify their usage pattern.
✅ Verification successful
Based on the search results, I can now generate the final response:
Functions are used correctly in CoreDNS check implementation
The ListDeployments function is used appropriately in pkg/local/preflight/checker.go to list CoreDNS deployments in the kube-system namespace. The search results show no other direct callers of these functions in the codebase, and the implementation aligns with the CoreDNS check requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how these functions are used in the CoreDNS check implementation
echo "Searching for usage of GetDeployment and ListDeployments in CoreDNS check..."
rg -A 5 "GetDeployment|ListDeployments" --type go
# Check if there are any other callers that would need to be updated
echo "Checking for other potential callers..."
ast-grep --pattern 'func $name($$$) {
$$$
GetDeployment($$$)
$$$
}'
ast-grep --pattern 'func $name($$$) {
$$$
ListDeployments($$$)
$$$
}'
Length of output: 4143
pkg/local/preflight/checker.go (3)
56-64: LGTM: Clean Kubernetes client initialization
The implementation follows good practices with proper error handling and clear error messages.
180-180: LGTM: Consistent with existing check pattern
The integration of checkKubeDNS() follows the same pattern as other checks in the Run method.
500-503: 🛠️ Refactor suggestion
Remove early return to ensure complete check
The early return after checking replica count prevents checking the ready replicas when the desired count is insufficient. This could miss reporting cases where both the desired and ready replica counts are problematic.
if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas < 2 {
local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("Kube DNS %q is set with fewer than 2 replicas; consider increasing replica count for high availability", deployment.Name))
- return
}Likely invalid or redundant comment.
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
from
430891e to
38b989b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
vendor/github.com/longhorn/go-common-libs/kubernetes/kubernetes.go (1)
29-33: Add documentation and nil checkWhile the implementation is correct, consider these improvements:
- Add godoc comments explaining the function's purpose and parameters
- Add nil check for the input map
Consider this implementation:
+// NewLabelSelectorFromMap creates a labels.Selector from a map of label key-value pairs. +// It's used to filter Kubernetes resources based on their labels. +// Parameters: +// - labels: A map of label key-value pairs +// Returns: +// - labels.Selector: A selector that can be used to filter resources +// - error: An error if the selector creation fails func NewLabelSelectorFromMap(labels map[string]string) (labels.Selector, error) { + if labels == nil { + return nil, errors.New("labels map cannot be nil") + } return metav1.LabelSelectorAsSelector(&metav1.LabelSelector{ MatchLabels: labels, }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/consts/preflight.go(1 hunks)pkg/local/preflight/checker.go(3 hunks)pkg/remote/preflight/checker.go(1 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go(2 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/kubernetes.go(2 hunks)vendor/modules.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- go.mod
- pkg/consts/preflight.go
- pkg/local/preflight/checker.go
- pkg/remote/preflight/checker.go
- vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go
- vendor/modules.txt
🔇 Additional comments (2)
vendor/github.com/longhorn/go-common-libs/kubernetes/kubernetes.go (2)
6-7: LGTM: Import statements are correctly added
The new imports are necessary for the label selector functionality and follow Go conventions.
Line range hint 1-33: Verify implications of using forked dependency
The AI summary indicates this file is from a forked repository (c3y1huang/go-common-libs instead of longhorn/go-common-libs). This change requires careful consideration:
- Ensure the fork is maintained and synchronized with upstream
- Verify the security implications of using a forked dependency
- Document the reason for using the fork
|
Test Results:
|
longhorn/longhorn-9752 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-9752 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-9752 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
9752-dr-volume-faulted-after-node-reboot
branch
from
38b989b to
1d91c76
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/local/preflight/checker.go (2)
483-483: Rename function to match the component being checkedThe function name
checkKubeDNSdoesn't match the actual component being checked (CoreDNS). Consider renaming for clarity.-func (local *Checker) checkKubeDNS() { +func (local *Checker) checkCoreDNS() {
484-509: Standardize terminology in log messagesThe log messages inconsistently use "CoreDNS" and "Kube DNS". Standardize on "CoreDNS" throughout the function for consistency.
- logrus.Info("Checking if CoreDNS has multiple replicas") + logrus.Info("Checking if CoreDNS deployment has multiple replicas") deployments, err := commonkube.ListDeployments(local.kubeClient, metav1.NamespaceSystem, map[string]string{consts.KubeAppLabel: consts.KubeAppValueDNS}) if err != nil { - local.collection.Log.Error = append(local.collection.Log.Error, fmt.Sprintf("Failed to list Kube DNS with label %s=%s: %v", consts.KubeAppLabel, consts.KubeAppValueDNS, err)) + local.collection.Log.Error = append(local.collection.Log.Error, fmt.Sprintf("Failed to list CoreDNS deployment with label %s=%s: %v", consts.KubeAppLabel, consts.KubeAppValueDNS, err)) return } if len(deployments.Items) != 1 { - local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("Found %d deployments with label %s=%s; expected 1", len(deployments.Items), consts.KubeAppLabel, consts.KubeAppValueDNS)) + local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("Found %d CoreDNS deployments with label %s=%s; expected 1", len(deployments.Items), consts.KubeAppLabel, consts.KubeAppValueDNS)) return } deployment := deployments.Items[0] if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas < 2 { - local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("Kube DNS %q is set with fewer than 2 replicas; consider increasing replica count for high availability", deployment.Name)) + local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("CoreDNS deployment %q is set with fewer than 2 replicas; consider increasing replica count for high availability", deployment.Name)) } if deployment.Status.ReadyReplicas < 2 { - local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("Kube DNS %q has fewer than 2 ready replicas; some replicas may not be running or ready", deployment.Name)) + local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("CoreDNS deployment %q has fewer than 2 ready replicas; some replicas may not be running or ready", deployment.Name)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/consts/preflight.go(1 hunks)pkg/local/preflight/checker.go(3 hunks)pkg/remote/preflight/checker.go(1 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go(2 hunks)vendor/github.com/longhorn/go-common-libs/kubernetes/kubernetes.go(2 hunks)vendor/modules.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- pkg/consts/preflight.go
- pkg/remote/preflight/checker.go
- vendor/github.com/longhorn/go-common-libs/kubernetes/kubernetes.go
- vendor/modules.txt
🔇 Additional comments (6)
vendor/github.com/longhorn/go-common-libs/kubernetes/deployment.go (2)
8-9: LGTM!
The added import for labels package is necessary for the new functionality and follows Go import conventions.
37-55: 🛠️ Refactor suggestion
Improve robustness of ListDeployments implementation
The implementation needs the following improvements:
- Accept a context parameter instead of using context.Background()
- Add input validation for kubeClient and namespace
Apply this diff to improve the implementation:
-func ListDeployments(kubeClient kubeclient.Interface, namespace string, labelSelectors map[string]string) (*appsv1.DeploymentList, error) {
+func ListDeployments(ctx context.Context, kubeClient kubeclient.Interface, namespace string, labelSelectors map[string]string) (*appsv1.DeploymentList, error) {
+ if kubeClient == nil {
+ return nil, fmt.Errorf("kubeClient cannot be nil")
+ }
+ if namespace == "" {
+ return nil, fmt.Errorf("namespace cannot be empty")
+ }
+
log := logrus.WithFields(logrus.Fields{
"kind": "Deployment",
"namespace": namespace,
"labelSelectors": labelSelectors,
})
log.Trace("Listing resources")
var err error
selector := labels.Everything()
if len(labelSelectors) != 0 {
selector, err = NewLabelSelectorFromMap(labelSelectors)
if err != nil {
return nil, err
}
}
- return kubeClient.AppsV1().Deployments(namespace).List(context.Background(), metav1.ListOptions{LabelSelector: selector.String()})
+ return kubeClient.AppsV1().Deployments(namespace).List(ctx, metav1.ListOptions{LabelSelector: selector.String()})
}pkg/local/preflight/checker.go (4)
56-64: LGTM: Kubernetes client initialization looks good!
The initialization follows best practices with proper error handling and wrapping.
180-180: LGTM: Appropriate placement of DNS check!
The DNS check is correctly placed before OS-specific checks as it's a critical, OS-independent verification.
486-490:
Enhance DNS deployment detection for different Kubernetes distributions
As noted in previous discussions, the current label selector might not work with all Kubernetes distributions (e.g., RKE2). Consider implementing a more flexible approach.
- deployments, err := commonkube.ListDeployments(local.kubeClient, metav1.NamespaceSystem, map[string]string{consts.KubeAppLabel: consts.KubeAppValueDNS})
+ // Try different known label combinations
+ labelSelectors := []map[string]string{
+ {consts.KubeAppLabel: consts.KubeAppValueDNS},
+ {"app": "coredns"},
+ {"k8s-app": "coredns"},
+ }
+
+ var deployments *appsv1.DeploymentList
+ var err error
+
+ for _, selector := range labelSelectors {
+ deployments, err = commonkube.ListDeployments(local.kubeClient, metav1.NamespaceSystem, selector)
+ if err == nil && len(deployments.Items) > 0 {
+ break
+ }
+ }499-502: 🛠️ Refactor suggestion
Remove early return to ensure complete status check
The early return after checking desired replicas prevents checking the ready replicas status, which could provide additional useful information even when the desired replica count is insufficient.
if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas < 2 {
local.collection.Log.Warn = append(local.collection.Log.Warn, fmt.Sprintf("Kube DNS %q is set with fewer than 2 replicas; consider increasing replica count for high availability", deployment.Name))
- return
}
|
@mergify backport v1.7.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9752
What this PR does / why we need it:
Check if CoreDNS has more than 2 ready replicas.
Special notes for your reviewer:
NoneAdditional documentation or context
None