feat: Add label for the greptimedb services and controllers #223
Conversation
WalkthroughThe pull request introduces enhancements to the labeling of Kubernetes resources across multiple components, including Datanode, Flownode, Frontend, Meta, and Standalone. Each component's Changes
Assessment against linked issues
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
Documentation and Community
|
![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 (6)
controllers/greptimedbcluster/deployers/meta.go (1)
209-211: Consider extracting label generation to a common functionWhile the implementation is clean and consistent, consider extracting the label generation logic into a common function in the
CommonBuilderto ensure consistent label format across all components and reduce code duplication.Example implementation:
// In CommonBuilder func (b *CommonBuilder) GetComponentLabels() map[string]string { return map[string]string{ constant.GreptimeDBComponentName: common.ResourceName(b.Cluster.Name, b.ComponentKind), } }Also applies to: 244-246
controllers/greptimedbcluster/deployers/frontend.go (1)
166-168: Consider using MergeStringMap for deployment labels.While the label implementation is functionally correct, consider using
util.MergeStringMapto preserve any existing deployment labels, similar to how service labels are handled. This ensures consistency across the codebase and prevents potential loss of labels specified in the CRD.- Labels: map[string]string{ - constant.GreptimeDBComponentName: common.ResourceName(b.Cluster.Name, b.ComponentKind), - }, + Labels: util.MergeStringMap(b.Cluster.Spec.Frontend.Labels, map[string]string{ + constant.GreptimeDBComponentName: common.ResourceName(b.Cluster.Name, b.ComponentKind), + }),controllers/greptimedbcluster/deployers/datanode.go (4)
Line range hint
391-393: Update selector to match new label keyThe StatefulSet's selector must be updated to match the new label key.
Apply this diff to update the selector:
Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - constant.GreptimeDBComponentName: common.ResourceName(b.Cluster.Name, b.ComponentKind), + "greptime.io/component": common.ResourceName(b.Cluster.Name, b.ComponentKind), }, },
Line range hint
333-335: Update Service selector to match new label keyThe Service's selector must be updated to match the new label key for proper pod selection.
Apply this diff to update the selector:
Selector: map[string]string{ - constant.GreptimeDBComponentName: common.ResourceName(b.Cluster.Name, b.ComponentKind), + "greptime.io/component": common.ResourceName(b.Cluster.Name, b.ComponentKind), },
Line range hint
494-496: Update Pod template labels to match new label keyThe Pod template labels must be updated to match the new label key for proper selection by the Service and StatefulSet.
Apply this diff to update the labels:
podTemplateSpec.ObjectMeta.Labels = util.MergeStringMap(podTemplateSpec.ObjectMeta.Labels, map[string]string{ - constant.GreptimeDBComponentName: common.ResourceName(b.Cluster.Name, b.ComponentKind), + "greptime.io/component": common.ResourceName(b.Cluster.Name, b.ComponentKind), })
Line range hint
1-700: Consider adding unit tests for label changesThe changes to the label keys are significant and affect multiple resources. It would be beneficial to add unit tests to verify:
- Correct label propagation to all resources
- Proper selector matching between Service, StatefulSet, and Pods
- Backward compatibility with existing deployments
Would you like me to help generate the unit test code for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
controllers/greptimedbcluster/deployers/datanode.go(2 hunks)controllers/greptimedbcluster/deployers/flownode.go(2 hunks)controllers/greptimedbcluster/deployers/frontend.go(2 hunks)controllers/greptimedbcluster/deployers/meta.go(2 hunks)controllers/greptimedbstandalone/deployer.go(2 hunks)
🔇 Additional comments (8)
controllers/greptimedbcluster/deployers/meta.go (1)
209-211: Verify label format matches requirements
The PR objective specifies the label format should be greptime.io/component=${cluster}-${role}, but the current implementation uses a different format. Please verify that constant.GreptimeDBComponentName matches the required format.
✅ Verification successful
Let me gather more information about the ResourceName function implementation since the previous result didn't show its content.
Label format is correctly implemented
The implementation matches the required format greptime.io/component=${cluster}-${role}:
- The label key is defined as
app.greptime.io/componentinconstant.GreptimeDBComponentName - The label value is constructed by
common.ResourceName()which concatenates the cluster name and component kind with a hyphen (name + "-" + string(componentKind))
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the label format in constants and its usage
# Check the constant definition
rg -A 1 "GreptimeDBComponentName.*="
# Check the ResourceName function implementation
ast-grep --pattern 'func ResourceName($name, $kind) {
$$$
}'
Length of output: 445
Script:
#!/bin/bash
# Search for ResourceName function implementation
rg -A 5 "func ResourceName"
Length of output: 495
controllers/greptimedbcluster/deployers/frontend.go (2)
130-132: LGTM! Service labels implementation looks good.
The implementation correctly adds the component label while preserving existing service labels. The label format aligns with the PR objectives and maintains consistency with service selectors.
130-132: Verify consistent label implementation across components.
Let's verify that the label implementation is consistent across other components (datanode, meta, etc.) and that these labels are correctly used in selectors and filters.
Also applies to: 166-168
✅ Verification successful
Label implementation is consistent across components
The verification shows consistent label implementation across all components (frontend, meta, datanode, flownode):
- All components use
util.MergeStringMapto merge custom labels with the component identifier - The
constant.GreptimeDBComponentNamelabel is consistently used withcommon.ResourceName(b.Cluster.Name, b.ComponentKind) - Labels are properly used in corresponding selectors for services and pod templates
- The implementation is consistent in both standalone and cluster deployments
The current implementation in the frontend service follows the established pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check label implementation consistency across components
# Check label implementation in other component files
echo "Checking label implementation in other components..."
rg -A 2 "Labels.*GreptimeDBComponentName" controllers/greptimedbcluster/deployers/
# Check label usage in selectors
echo "Checking label usage in selectors..."
rg -A 2 "Selector.*GreptimeDBComponentName" controllers/greptimedbcluster/deployers/
Length of output: 369
Script:
#!/bin/bash
# Let's try a broader search to understand label implementations and usage
# Check all label implementations in deployers
echo "Checking all label implementations..."
rg "Labels.*MergeStringMap" controllers/greptimedbcluster/deployers/ -B 1 -A 2
# Check component name constant usage
echo "Checking component name constant usage..."
rg "constant\.GreptimeDBComponentName" -B 1 -A 2
# Check service label implementations
echo "Checking service label implementations..."
rg "Service\.Labels" controllers/greptimedbcluster/deployers/ -B 1 -A 2
# Check selector implementations
echo "Checking selector implementations..."
rg "LabelSelector" controllers/greptimedbcluster/deployers/ -B 1 -A 2
Length of output: 15222
controllers/greptimedbcluster/deployers/flownode.go (2)
133-135: LGTM: Service labels implementation is correct
The label implementation for the Service follows Kubernetes best practices and matches the PR objectives. The label format greptime.io/component=${cluster}-${role} is correctly implemented using common.ResourceName().
188-190: LGTM: StatefulSet labels implementation is correct
The label implementation for the StatefulSet maintains consistency with the Service labels and ensures proper pod selection through the StatefulSet's selector.
controllers/greptimedbstandalone/deployer.go (2)
247-249: LGTM! Verify StatefulSet label consistency.
The StatefulSet labeling implementation maintains consistency with the service labeling pattern.
Let's verify label consistency across resources:
✅ Verification successful
Let's also check the constant definition:
And verify the actual usage pattern:
Label consistency verified across resources
Based on the verification results, the label implementation is consistent across all resources:
- The label key
app.greptime.io/componentis defined as a constant incontrollers/constant/constant.go - The same labeling pattern is consistently used across:
- StatefulSet labels and selector
- Service labels
- Pod template labels
- All cluster components (frontend, datanode, meta, flownode)
- Standalone deployments
The implementation maintains consistency by using the same label key and value format (common.ResourceName(name, kind)) throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify label consistency between Services and StatefulSets
# Expected: Both resource types should use the same label format
# Compare label usage in Services and StatefulSets
echo "=== Services ==="
rg --type yaml 'kind:\s+Service' -A 10 | rg 'greptime.io/component'
echo "=== StatefulSets ==="
rg --type yaml 'kind:\s+StatefulSet' -A 10 | rg 'greptime.io/component'
Length of output: 233
Script:
#!/bin/bash
# Let's try a different approach to verify label consistency
# Search for label definitions in the codebase
echo "=== Searching for label definitions in constant package ==="
rg -l "constant\.GreptimeDBComponentName" --type go
echo -e "\n=== Checking label usage in deployer files ==="
fd deployer.go --type f --exec rg -C 5 "Labels.*map\[string\]string"
Length of output: 2301
Script:
#!/bin/bash
echo "=== Checking constant definition ==="
rg "GreptimeDBComponentName.*=" --type go
Length of output: 347
Script:
#!/bin/bash
echo "=== Checking actual label usage patterns ==="
rg "Labels.*map\[string\]string" -A 3 --type go
Length of output: 8984
193-195: LGTM! Verify service label propagation.
The service label implementation correctly merges existing labels with the new component label, maintaining backward compatibility while adding the new standardized labeling.
Let's verify the label propagation to services:
✅ Verification successful
Service label implementation is correct and consistent
The implementation is verified to be correct based on the following findings:
- The component label key is properly defined as a constant
app.greptime.io/component MergeStringMaputility function correctly preserves existing labels while adding new ones- The implementation is consistently used across standalone and cluster deployments
- The label value correctly uses the resource name format combining cluster name and kind
The changes maintain backward compatibility while standardizing component labeling across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that services get the new component label
# Expected: Services should have label "greptime.io/component=${cluster}-standalone"
# Search for service definitions with the new label
rg --type yaml 'kind:\s+Service' -A 10 | rg 'greptime.io/component'
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach to verify the service label implementation:
# 1. First, check the constant definition for the component label
rg "GreptimeDBComponentName\s*=\s*" -A 1
# 2. Check how the label is used in the codebase
rg "Labels.*MergeStringMap.*Component" -B 2 -A 2
# 3. Look for test cases that verify the label
rg --type go "Test.*Label.*Service" -A 10
Length of output: 303
Script:
#!/bin/bash
# Let's verify the implementation more thoroughly:
# 1. Check all usages of MergeStringMap with labels
rg "MergeStringMap.*Labels" -B 2 -A 2
# 2. Look for the actual implementation of MergeStringMap
ast-grep --pattern 'func MergeStringMap($$$) $_ {
$$$
}'
# 3. Check for any existing service label tests
rg -g "**/*_test.go" "Labels.*Service" -B 2 -A 2
Length of output: 4294
controllers/greptimedbcluster/deployers/datanode.go (1)
Line range hint 1-700: Verify impact on existing deployments
The label key changes might affect existing deployments during upgrades.
Let's verify the usage of the old label key in the codebase:
Close: #222
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor