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
Warning Rate limit exceeded@c3y1huang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (5)pkg/local/preflight/checker.go (5)
The code follows best practices with proper error handling and descriptive error messages.
The CoreDNS check is correctly placed before OS-specific checks.
The documentation clearly explains the purpose, behavior, and includes a reference to the related issue.
The early return skips checking ready replicas even when the desired replica count is correct. This could miss reporting cases where desired replicas are correct but ready replicas are insufficient.
Consider potential race condition in replica checks. There's a potential race condition between checking Spec.Replicas and Status.ReadyReplicas. Consider combining the warnings if both conditions fail to avoid missing states during deployment scaling. Consider this alternative implementation: - if deployment.Spec.Replicas == nil || *deployment.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
- }
-
- if deployment.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")
- return
- }
+ desiredReplicas := int32(2)
+ if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas < desiredReplicas {
+ local.collection.Log.Warn = append(local.collection.Log.Warn, "CoreDNS is set with fewer than 2 replicas; consider increasing replica count for high availability")
+ }
+ if deployment.Status.ReadyReplicas < desiredReplicas {
+ 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 deployment.Spec.Replicas != nil && *deployment.Spec.Replicas >= desiredReplicas && deployment.Status.ReadyReplicas >= desiredReplicas {
+ 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))
+ }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
|
Test results:
|
longhorn/longhorn-9752 Signed-off-by: Chin-Ya Huang <[email protected]>
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
| @@ -4,6 +4,8 @@ go 1.23 | |||
|
|
|||
| toolchain go1.23.3 | |||
|
|
|||
| replace github.com/longhorn/go-common-libs => github.com/c3y1huang/go-common-libs v0.0.0-20241114031240-e1117130cd3b | |||
There was a problem hiding this comment.
TODO: update after dependent vendor PR merged.
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 |
longhorn/longhorn-9752 Signed-off-by: Chin-Ya Huang <[email protected]>
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
77362e7
to
af34ac2
Compare
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
| func (local *Checker) checkCoreDNS() { | ||
| logrus.Info("Checking if CoreDNS has multiple replicas") | ||
|
|
||
| deployment, err := commonkube.GetDeployment(local.kubeClient, metav1.NamespaceSystem, consts.AppNameCoreDNS) |
There was a problem hiding this comment.
AppNameCoreDNS is coredns. If the k8s cluster is bootstrapped by rke2, the coredns deployment is rke2-coredns-rke2-coredns, is the check still valid?
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