-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
[OSPP]Support Kubernetes ConfigMap for Apollo java, golang client #79 #318
base: feature/develop_5.0
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce enhancements for managing application configurations with Kubernetes ConfigMaps. A new Changes
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 (
|
感谢您提出Pull Request,我会尽快Review。我会在1-2日内进行查看或者回复,如果遇到节假日可能会处理较慢,敬请谅解。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
store/configMap/store_test.go (2)
52-99
: Great test coverage for the happy path scenario!The
TestStore_LoadConfigMap
function is well-structured and covers the successful loading of a ConfigMap. It initializes the necessary dependencies, creates a sample ConfigMap with comprehensive test data, and makes appropriate assertions to verify the expected behavior of theLoadConfigMap
method.To further enhance the test coverage, consider adding more test cases to cover error scenarios and edge cases, such as:
- Loading a non-existent ConfigMap
- Handling errors during ConfigMap retrieval
- Dealing with invalid or malformed ConfigMap data
101-135
: Excellent test coverage for the ConfigMap write operation!The
TestStore_WriteConfigMap
function is well-structured and covers the successful writing of a ConfigMap. It initializes the necessary dependencies, prepares the test data, and makes appropriate assertions to verify the expected behavior of theWriteConfigMap
method.To further improve the test coverage, consider adding more test cases to handle error scenarios and edge cases, such as:
- Writing a ConfigMap with invalid data
- Handling errors during ConfigMap creation or update
- Dealing with permission issues or other Kubernetes-related errors
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
Files selected for processing (15)
- client.go (2 hunks)
- component/remote/async.go (1 hunks)
- env/config/config.go (2 hunks)
- extension/configmap.go (1 hunks)
- extension/configmap_test.go (1 hunks)
- go.mod (1 hunks)
- start.go (1 hunks)
- start_test.go (1 hunks)
- storage/repository.go (2 hunks)
- storage/repository_test.go (1 hunks)
- store/configMap/manager.go (1 hunks)
- store/configMap/manager_test.go (1 hunks)
- store/configMap/store.go (1 hunks)
- store/configMap/store_test.go (1 hunks)
- store/configmap.go (1 hunks)
Additional context used
golangci-lint
client.go
53-53: cannot use &configMap.Store{…} (value of type *configMap.Store) as store.ConfigMapHandler value in argument to extension.SetConfigMapHandler: *configMap.Store does not implement store.ConfigMapHandler (wrong type for method LoadConfigMap)
have LoadConfigMap(*config.ApolloConfig, string) (*config.ApolloConfig, error)
want LoadConfigMap(string) (*config.ApolloConfig, error)(typecheck)
Additional comments not posted (29)
store/configmap.go (1)
22-27
: LGTM!The
ConfigMapHandler
interface provides a clean abstraction for managing configuration maps. The method signatures are well-defined, and the comments provide clear explanations of the method behaviors. The code follows Go conventions and best practices.extension/configmap.go (2)
25-27
: LGTM!The
SetConfigMapHandler
function correctly sets the globalconfigMapHandler
variable to the providedinConfigMapHandler
parameter. The function is exported, which is appropriate as it is intended to be used by other packages to set theconfigMapHandler
.
30-32
: LGTM!The
GetConfigMapHandler
function correctly returns the globalconfigMapHandler
variable. The function is exported, which is appropriate as it is intended to be used by other packages to get theconfigMapHandler
.extension/configmap_test.go (4)
26-27
: LGTM!The
TestConfigMapHandler
struct is correctly defined. It appears to be a test stub for theConfigMapHandler
interface.
29-31
: LGTM!The
LoadConfigMap
method is correctly defined on theTestConfigMapHandler
struct. The method signature matches theConfigMapHandler
interface. Returningnil
values is appropriate for a test stub.
33-35
: LGTM!The
WriteConfigMap
method is correctly defined on theTestConfigMapHandler
struct. The method signature matches theConfigMapHandler
interface. Returning anil
value is appropriate for a test stub.
38-44
: LGTM!The
TestSetConfigMapHandler
function is a well-defined test case. It correctly tests the functionality of setting and retrieving theConfigMapHandler
. The assertion ensures that the handler is properly set.store/configMap/store.go (2)
24-27
: LGTM!The
Store
struct definition looks good. Using dependency injection for theK8sManager
is a nice approach to make the code more testable.
38-44
: LGTM!The
WriteConfigMap
method looks good. It properly uses theAppID
,Cluster
, andNamespaceName
to uniquely identify the ConfigMap and the configuration key, which is consistent with theLoadConfigMap
method. The error handling is also done correctly.start.go (6)
30-35
: LGTM!The changes to
SetSignature
are limited to comment formatting and do not alter the functionality. The nil check ensures that only a validauth
component is set.
37-42
: LGTM!The changes to
SetBackupFileHandler
are limited to comment formatting and do not alter the functionality. The nil check ensures that only a validfile
component is set.
44-49
: New feature looks good!The new
SetConfigMapHandler
function enables setting a custom ConfigMap persistence component, which aligns with the PR objective of integrating Kubernetes ConfigMap as a new persistent storage solution. The nil check ensures that only a validconfigMapHandler
is set.
51-56
: LGTM!The changes to
SetLoadBalance
are limited to comment formatting and do not alter the functionality. The nil check ensures that only a validloadBalance
component is set.
58-63
: LGTM!The changes to
SetLogger
are limited to comment formatting and do not alter the functionality. The nil check ensures that only a validloggerInterface
is set.
Line range hint
65-70
: LGTM!The changes to
SetCache
are limited to comment formatting and do not alter the functionality. The nil check ensures that only a validcacheFactory
is set.store/configMap/manager_test.go (2)
30-55
: LGTM!The test function
TestK8sManager_SetConfigMap
is well-written and covers the happy path scenario of setting a ConfigMap using theK8sManager
. It follows a clear arrange-act-assert pattern, uses a fake Kubernetes client for testing, and makes comprehensive assertions to verify the expected behavior.
57-82
: LGTM!The test function
TestK8sManager_GetConfigMap
is well-written and covers the scenario of retrieving a ConfigMap using theK8sManager
. It follows a clear arrange-act-assert pattern, uses a fake Kubernetes client with a pre-existing ConfigMap for testing, and makes assertions to verify that the returned configurations match the expected values.go.mod (1)
1-67
: LGTM! The changes to thego.mod
file align with the PR objectives.The updates to the
go.mod
file, including the addition of Kubernetes-related dependencies, testing libraries, and configuration management libraries, are consistent with the PR objectives of implementing Kubernetes ConfigMap support and enhancing the module's capabilities.The updated Go version from
1.13
to1.22.0
may introduce new features and improvements that could benefit the module.Please ensure that the new dependencies are compatible with the updated Go version to avoid any potential integration issues. You can verify this by running the following commands:
If there are no errors or compatibility issues, the changes to the
go.mod
file should be good to go.Verification successful
Go module dependencies look good and compatible
The analysis of the
go.mod
file shows that the dependencies and Go version are up-to-date and compatible. Here are the key points:
- Go version 1.22.0 is specified, which is recent and supported.
- Direct dependencies, including Kubernetes-related packages, are at consistent versions.
- No version conflicts were detected among the dependencies.
The changes to the
go.mod
file align well with the PR objectives and don't introduce any apparent compatibility issues.While the static analysis shows no issues, it's recommended that you perform a final manual verification in your development environment to ensure everything works as expected, especially regarding the Kubernetes integration and new dependencies.
Scripts executed
The following scripts were executed for the analysis:
Script:
go mod tidy go build ./... go test ./...
Length of output: 256
Script:
#!/bin/bash # Display the content of go.mod file cat go.mod # Extract and display the Go version echo "Go version:" grep "^go " go.mod # Extract and display direct dependencies echo "Direct dependencies:" sed -n '/^require (/,/^)$/p' go.mod | grep -v "^require (" | grep -v "^)$" # Check for any potential version conflicts echo "Checking for potential version conflicts:" grep -P "v\d+\.\d+\.\d+" go.mod | sort | uniq -dLength of output: 5634
store/configMap/manager.go (3)
42-57
: LGTM!The
GetK8sManager
function correctly initializes the singletonK8sManager
instance using in-cluster configuration. It handles errors appropriately and ensures thread-safe lazy initialization usingsync.Once
.
60-86
: LGTM!The
SetConfigMap
method correctly converts theApolloConfig
data to JSON and creates or updates a Kubernetes ConfigMap. It handles errors appropriately and sets a timeout for the API operations.
89-112
: LGTM!The
GetConfigMap
method correctly retrieves a ConfigMap, extracts JSON data for the given key, and unmarshals it into theConfigurations
field of anApolloConfig
struct. It handles errors appropriately and sets a timeout for the API operation.component/remote/async.go (1)
150-158
: LGTM!The changes introduce support for loading configurations from ConfigMaps as a fallback mechanism. The logic correctly prioritizes loading from the local backup file over the ConfigMap. This aligns with the PR objective of using ConfigMaps for persistent storage in Kubernetes environments.
The error handling is maintained by logging any issues encountered during the loading process.
env/config/config.go (3)
44-54
: LGTM!The addition of the new fields
IsBackupConfigToConfigMap
andConfigMapNamespace
to theAppConfig
struct, along with the clarifying comment for the existingIsBackupConfig
field, looks good. The default value offalse
forIsBackupConfigToConfigMap
ensures that this feature is opt-in, and theConfigMapNamespace
field provides flexibility in specifying the namespace for the ConfigMap.
81-86
: LGTM!The addition of the
GetIsBackupConfigToConfigMap
getter method to theAppConfig
struct looks good. The method name follows the Go naming convention, and it provides a way to retrieve the value of theIsBackupConfigToConfigMap
field.
88-90
: LGTM!The addition of the
GetConfigMapNamespace
getter method to theAppConfig
struct looks good. The method name follows the Go naming convention, and it provides a way to retrieve the value of theConfigMapNamespace
field.start_test.go (1)
228-246
: LGTM!The code changes introduce a new
testConfigMapHandler
struct and its placeholder methodsLoadConfigMap
andWriteConfigMap
, which serve as a foundation for future implementation of loading and writing configurations to Kubernetes ConfigMaps.The
TestSetConfigMapHandler
test function ensures that the ConfigMap handler can be set and retrieved correctly, providing adequate coverage for the newly introducedSetConfigMapHandler
function.The code changes are well-structured, follow the existing coding style and conventions, and align with the larger effort to integrate Kubernetes ConfigMaps into the agollo package for configuration management.
client.go (1)
23-23
: LGTM!The import statement is correct and necessary for the changes introduced in this file.
storage/repository_test.go (1)
61-61
: LGTM!The TODO comment is a good reminder for a planned test enhancement. It does not alter any functionality at this time.
storage/repository.go (1)
Line range hint
456-492
: LGTM!The changes to the
UpdateApolloConfig
function look good:
- The new conditional check for writing the configuration to a ConfigMap follows the existing pattern for writing backup files.
- The
WriteConfigMap
method from theConfigMapHandler
is invoked with the appropriate parameters.- The changes maintain the existing code structure, style, and conventions.
store/configMap/store.go
Outdated
// TODO 在这里把json转为ApolloConfig, 但ReleaseKey字段会丢失 | ||
config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address the TODO comment about losing the ReleaseKey
.
The comment indicates that the ReleaseKey
field is lost when converting the JSON to ApolloConfig
. This seems like an important piece of information that shouldn't be discarded. Please investigate this further and ensure that the ReleaseKey
is properly handled.
store/configMap/store.go
Outdated
configMapName := config.AppID | ||
key := config.Cluster + "+" + config.NamespaceName | ||
// TODO 在这里把json转为ApolloConfig, 但ReleaseKey字段会丢失 | ||
config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle the error returned from GetConfigMap
.
The error returned from GetConfigMap
is being ignored. It's a good practice to always handle errors. If an error occurs while retrieving the ConfigMap, it should be returned from the LoadConfigMap
method so that the caller can handle it appropriately.
Modify the code to handle the error:
-config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key)
+config.Configurations, err := c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key)
+if err != nil {
+ return nil, err
+}
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) | |
config.Configurations, err := c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) | |
if err != nil { | |
return nil, err | |
} |
client.go
Outdated
@@ -49,6 +50,7 @@ const separator = "," | |||
func init() { | |||
extension.SetCacheFactory(&memory.DefaultCacheFactory{}) | |||
extension.SetLoadBalance(&roundrobin.RoundRobin{}) | |||
extension.SetConfigMapHandler(&configMap.Store{configMap.GetK8sManager()}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type mismatch error in LoadConfigMap
method signature.
The static analysis tool has correctly identified a type mismatch error in the LoadConfigMap
method implemented by configMap.Store
. The method signature does not match the one expected by the store.ConfigMapHandler
interface:
have LoadConfigMap(*config.ApolloConfig, string) (*config.ApolloConfig, error)
want LoadConfigMap(string) (*config.ApolloConfig, error)
This error needs to be fixed to ensure the correct functionality of the configuration map handler.
Do you want me to suggest the necessary changes to fix this type mismatch error or open a GitHub issue to track this task?
Tools
golangci-lint
53-53: cannot use &configMap.Store{…} (value of type *configMap.Store) as store.ConfigMapHandler value in argument to extension.SetConfigMapHandler: *configMap.Store does not implement store.ConfigMapHandler (wrong type for method LoadConfigMap)
have LoadConfigMap(*config.ApolloConfig, string) (*config.ApolloConfig, error)
want LoadConfigMap(string) (*config.ApolloConfig, error)(typecheck)
store/configmap.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the directory should be named configmap instead of camelCase,camelCase is not a common mode of naming
store/configmap.go
Outdated
type ConfigMapHandler interface { | ||
WriteConfigMap(config *config.ApolloConfig, configMapNamespace string) error | ||
// LoadConfigMap 直接读出json,封装为*config.ApolloConfig | ||
LoadConfigMap(configMapNamespace string) (*config.ApolloConfig, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the config parameter missed?According to ur code, the structure of Store should be Implement ConfigMapHandler interface, But it is incorrect that the method of LoadConfigMap sign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I changed it to LoadConfigMap(appConfig config.AppConfig, configMapNamespace string) (*config.ApolloConfig, error)
component/remote/async.go
Outdated
if appConfig.GetIsBackupConfig() { | ||
c, err = extension.GetFileHandler().LoadConfigFile(appConfig.BackupConfigPath, appConfig.AppID, namespace) | ||
} else if appConfig.GetIsBackupConfigToConfigMap() { | ||
c, err = extension.GetConfigMapHandler().LoadConfigMap(appConfig.ConfigMapNamespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe should be LoadConfigMap(c, appConfig.ConfigMapNamespace)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
store/configmap.go (2)
25-25
: Translate comment to English for consistency.The comment for the LoadConfigMap method is in Chinese. For better international collaboration and consistency, consider translating it to English.
Suggested translation:
// LoadConfigMap reads JSON directly and encapsulates it into *config.ApolloConfig
1-1
: Consider renaming the parent directory to "configmap".A previous review comment suggested naming the directory "configmap" instead of using camelCase. While this doesn't affect the code directly, following consistent naming conventions across the project is important for maintainability.
Consider moving this file to a directory named "configmap" if other files in the project follow this naming convention.
store/configMap/store.go (2)
18-20
: LGTM! Consider adding K8s client-go import.The package name and import look good. However, since this package deals with Kubernetes ConfigMaps, you might need to import the Kubernetes client-go library. Consider adding:
import ( "k8s.io/client-go/kubernetes" )This will likely be needed for the
K8sManager
implementation.
1-49
: Add documentation and consider additional improvements.Overall, this implementation provides a good foundation for managing Apollo configurations using Kubernetes ConfigMaps. However, there are several areas for improvement:
Add documentation for the
Store
struct and its methods. Use GoDoc style comments to describe the purpose, parameters, and return values of each exported entity.Implement proper error handling and logging throughout the file.
Add concurrency protection using mutexes as suggested in previous comments.
Consider adding unit tests for this file to ensure the correctness of the implementation.
Implement the
K8sManager
interface and its methods, ensuring they handle Kubernetes API interactions correctly.Add a constructor function for the
Store
struct to ensure proper initialization:func NewStore(k8sManager *K8sManager) *Store { return &Store{ K8sManager: k8sManager, mu: sync.RWMutex{}, } }These improvements will enhance the reliability, maintainability, and usability of this package.
store/configMap/store_test.go (3)
50-50
: Consider elaborating on the TODO comment or creating specific issues.While it's good to acknowledge the need for more boundary tests, it would be more actionable to either:
- Elaborate on what specific boundary cases need testing, or
- Create separate GitHub issues for each boundary case that needs to be tested.
This will help ensure that these additional tests are not overlooked and can be properly tracked and implemented in the future.
52-96
: LGTM: TestStore_LoadConfigMap is well-structured. Consider improving error handling.The test function is well-written and covers the basic functionality of loading a ConfigMap. It properly sets up the test environment using a fake clientset and verifies the loaded configuration.
A minor suggestion for improvement:
Consider adding an assertion for the error case when creating the ConfigMap. Replace lines 73-74 with:
_, err = clientset.CoreV1().ConfigMaps(configMapNamespace).Create(context.TODO(), configMap, metav1.CreateOptions{}) assert.NoError(t, err, "Failed to create test ConfigMap")This will provide a more informative error message if the ConfigMap creation fails.
98-132
: LGTM: TestStore_WriteConfigMap is well-structured. Consider improving consistency with LoadConfigMap test.The test function is well-written and covers the basic functionality of writing to a ConfigMap. It properly sets up the test environment using a fake clientset and verifies the written configuration.
A suggestion for improvement:
For consistency with the
TestStore_LoadConfigMap
function, consider using anAppConfig
struct when callingWriteConfigMap
. This would make the test setup more similar between the two functions. You could modify theWriteConfigMap
call as follows:appConfig := config.AppConfig{ AppID: apolloConfig.AppID, Cluster: apolloConfig.Cluster, NamespaceName: apolloConfig.NamespaceName, } err = store.WriteConfigMap(apolloConfig, appConfig, configMapNamespace)This assumes that the
WriteConfigMap
method signature is updated to accept anAppConfig
parameter, which would be a good practice for consistency across the package.component/remote/async.go (1)
150-159
: Implementation of Kubernetes ConfigMap support looks good.The changes successfully implement the support for Kubernetes ConfigMap as a new persistent storage solution, which aligns with the PR objectives. The order of precedence (local file over ConfigMap) is logical and well-commented.
Consider updating the
LoadConfigMap
function call.As suggested in a previous review comment, you might want to update the
LoadConfigMap
function call to include theappConfig
parameter explicitly:c, err = extension.GetConfigMapHandler().LoadConfigMap(appConfig, appConfig.ConfigMapNamespace)This change would make the function call more explicit and potentially more consistent with other parts of the codebase.
Enhance error logging for ConfigMap loading.
Consider adding more specific error logging for the ConfigMap loading case:
if appConfig.GetIsBackupConfig() { c, err = extension.GetFileHandler().LoadConfigFile(appConfig.BackupConfigPath, appConfig.AppID, namespace) if err != nil { log.Errorf("Failed to load config from file: %v", err) } } else if appConfig.GetIsBackupConfigToConfigMap() { c, err = extension.GetConfigMapHandler().LoadConfigMap(appConfig, appConfig.ConfigMapNamespace) if err != nil { log.Errorf("Failed to load config from ConfigMap: %v", err) } }This change would provide more detailed error information, making it easier to diagnose issues specific to ConfigMap loading.
go.mod (1)
13-62
: Consider Cleaning Up Indirect DependenciesThe
require
block from lines 13 to 62 lists many indirect dependencies. Typically, indirect dependencies are managed automatically by the Go module system and do not need to be explicitly listed in thego.mod
file. Including them can clutter the file and make maintenance more challenging. Consider runninggo mod tidy
to remove unnecessary indirect dependencies and keep thego.mod
file concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (6)
- client.go (2 hunks)
- component/remote/async.go (1 hunks)
- go.mod (1 hunks)
- store/configMap/store.go (1 hunks)
- store/configMap/store_test.go (1 hunks)
- store/configmap.go (1 hunks)
🔇 Additional comments (7)
store/configmap.go (2)
18-20
: LGTM: Package declaration and imports are appropriate.The package name "store" aligns with the file location, and the import of the config package is relevant for the ConfigMapHandler interface.
22-27
: LGTM: ConfigMapHandler interface is well-defined.The interface name and methods are appropriate for the intended functionality. The method signatures look correct and align with the expected behavior for writing and loading config maps.
store/configMap/store.go (1)
29-41
:⚠️ Potential issueHandle the GetConfigMap error and address the ReleaseKey issue.
- The error from
GetConfigMap
is currently being ignored. This could lead to silent failures. Please handle this error:config.Configurations, err := c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) if err != nil { return nil, err }
- The TODO comment indicates that the ReleaseKey is being lost. This is an important piece of information. Consider modifying the
GetConfigMap
method to return both the configurations and the ReleaseKey, then set it in theApolloConfig
:config.Configurations, config.ReleaseKey, err := c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) if err != nil { return nil, err }
- Don't forget to use the mutex for thread-safety:
func (c *Store) LoadConfigMap(appConfig config.AppConfig, configMapNamespace string) (*config.ApolloConfig, error) { c.mu.RLock() defer c.mu.RUnlock() // ... rest of the method }store/configMap/store_test.go (1)
1-48
: LGTM: Package declaration, imports, and constants are well-defined.The package name, imports, and constants are appropriately set up for the test file. The use of the fake Kubernetes clientset (
k8s.io/client-go/kubernetes/fake
) is a good practice for unit testing Kubernetes-related functionality.component/remote/async.go (1)
150-159
: Overall, the changes look good and align with the PR objectives.The implementation of Kubernetes ConfigMap support enhances the reliability and fault tolerance of configuration information, as intended. The code is well-structured and maintains consistency with the existing error handling patterns. The suggested improvements are minor and aimed at enhancing clarity and debugging capabilities.
client.go (1)
23-23
: LGTM: New import for ConfigMap support.The new import for the
configMap
package is correctly added and necessary for the new functionality being introduced.go.mod (1)
64-66
:⚠️ Potential issueUpdate the Go Version and Remove the Toolchain Directive
The
go.mod
file specifies Go version1.22.0
and includes atoolchain go1.22.5
directive. As of April 2024, the latest stable Go release is1.21.x
. Specifying a Go version or toolchain that is not yet released could lead to build errors or compatibility issues. Please update the Go version to the latest stable release and remove thetoolchain
directive if it's unnecessary.Apply the following changes:
- go 1.22.0 - toolchain go1.22.5 + go 1.21Likely invalid or redundant comment.
store/configMap/store.go
Outdated
// TODO 并发加锁 | ||
|
||
type Store struct { | ||
// 模块化依赖注入,便于测试 | ||
K8sManager *K8sManager | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address concurrency concerns and consider adding a mutex.
The TODO comment correctly identifies a potential concurrency issue. To ensure thread-safety, consider adding a mutex to the Store
struct:
import "sync"
type Store struct {
K8sManager *K8sManager
mu sync.RWMutex
}
Then, use this mutex in the LoadConfigMap
and WriteConfigMap
methods to protect shared resources.
The K8sManager
field is a good approach for dependency injection, which will facilitate testing.
store/configMap/store.go
Outdated
// WriteConfigMap write apollo config to configmap | ||
func (c *Store) WriteConfigMap(config *config.ApolloConfig, configMapNamespace string) error { | ||
// AppId作为configMap的name,cluster+namespace作为key, value为config | ||
configMapName := config.AppID | ||
key := config.Cluster + "+" + config.NamespaceName | ||
return c.K8sManager.SetConfigMap(configMapName, configMapNamespace, key, config) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance error handling and add concurrency protection.
The WriteConfigMap
method looks good overall, but consider the following improvements:
- Add error logging to provide more context:
err := c.K8sManager.SetConfigMap(configMapName, configMapNamespace, key, config)
if err != nil {
log.Printf("Failed to write ConfigMap %s in namespace %s: %v", configMapName, configMapNamespace, err)
return err
}
- Use the mutex for thread-safety:
func (c *Store) WriteConfigMap(config *config.ApolloConfig, configMapNamespace string) error {
c.mu.Lock()
defer c.mu.Unlock()
// ... rest of the method
}
- Consider adding a context parameter for better control over the operation's lifecycle:
func (c *Store) WriteConfigMap(ctx context.Context, config *config.ApolloConfig, configMapNamespace string) error {
// Use ctx in the SetConfigMap call
}
store/configMap/store_test.go
Outdated
appId = "testAppId" | ||
configMapNamespace = "testConfigMapNamespace" | ||
cluster = "testCluster" | ||
namespace = "testNamespace" | ||
) | ||
|
||
var testData = map[string]interface{}{ | ||
"stringKey": "stringValue", | ||
"intKey": 123, | ||
"boolKey": true, | ||
"sliceKey": []interface{}{1, 2, 3}, | ||
"mapKey": map[string]interface{}{ | ||
"nestedStringKey": "nestedStringValue", | ||
"nestedIntKey": 456, | ||
}, | ||
} | ||
|
||
// TODO 更多边界的测试 | ||
|
||
func TestStore_LoadConfigMap(t *testing.T) { | ||
// 初始化fake clientset | ||
clientset := fake.NewSimpleClientset() | ||
jsonData, err := json.MarshalIndent(testData, "", "") | ||
if err != nil { | ||
fmt.Println("Error marshalling map to JSON:", err) | ||
return | ||
} | ||
|
||
// 创建一个ConfigMap对象 | ||
configMap := &corev1.ConfigMap{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: appId, | ||
Namespace: configMapNamespace, | ||
}, | ||
Data: map[string]string{ | ||
cluster + "+" + namespace: string(jsonData), | ||
}, | ||
} | ||
|
||
// 使用fake clientset创建ConfigMap | ||
_, err = clientset.CoreV1().ConfigMaps(configMapNamespace).Create(context.TODO(), configMap, metav1.CreateOptions{}) | ||
assert.NoError(t, err) | ||
|
||
// 初始化Store,注入fake clientset | ||
store := Store{ | ||
K8sManager: &K8sManager{ | ||
clientSet: clientset, | ||
}, | ||
} | ||
|
||
var appConfig = config.AppConfig{ | ||
AppID: appId, | ||
NamespaceName: namespace, | ||
Cluster: cluster, | ||
} | ||
|
||
// 执行 | ||
loadedConfig, err := store.LoadConfigMap(appConfig, configMapNamespace) | ||
|
||
// 测试LoadConfigMap方法 | ||
assert.NoError(t, err) | ||
assert.NotNil(t, loadedConfig) | ||
assert.Equal(t, testData, loadedConfig.Configurations) | ||
} | ||
|
||
func TestStore_WriteConfigMap(t *testing.T) { | ||
// 初始化fake clientset | ||
clientset := fake.NewSimpleClientset() | ||
jsonData, err := json.MarshalIndent(testData, "", "") | ||
if err != nil { | ||
fmt.Println("Error marshalling map to JSON:", err) | ||
return | ||
} | ||
|
||
// 初始化Store,注入fake clientset | ||
store := Store{ | ||
K8sManager: &K8sManager{ | ||
clientSet: clientset, | ||
}, | ||
} | ||
|
||
// 反序列化到ApolloConfig | ||
apolloConfig := &config.ApolloConfig{} | ||
err = json.Unmarshal(jsonData, apolloConfig) | ||
assert.NoError(t, err) | ||
apolloConfig.AppID = appId | ||
apolloConfig.Cluster = cluster | ||
apolloConfig.NamespaceName = namespace | ||
|
||
// 测试WriteConfigMap方法 | ||
err = store.WriteConfigMap(apolloConfig, configMapNamespace) | ||
assert.NoError(t, err) | ||
|
||
// 验证ConfigMap是否被正确创建或更新 | ||
configMap, err := clientset.CoreV1().ConfigMaps(configMapNamespace).Get(context.TODO(), appId, metav1.GetOptions{}) | ||
assert.NoError(t, err) | ||
assert.NotNil(t, configMap) | ||
assert.Equal(t, testData, configMap.Data) | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Overall, good test coverage. Consider adding error cases and boundary tests.
The test file provides good coverage of the basic functionality for loading and writing ConfigMaps. The use of a fake Kubernetes clientset is a good practice for unit testing, ensuring isolation and reproducibility.
Suggestions for further improvement:
- Add test cases for error scenarios, such as:
- ConfigMap not found when loading
- Permission issues (if applicable in the fake clientset)
- Invalid data in the ConfigMap
- Implement the boundary tests mentioned in the TODO comment.
- Consider adding tests for concurrent access if the Store is intended to be used in a multi-goroutine environment.
- Add tests for any helper functions or methods that might be present in the actual Store implementation.
These additions would significantly enhance the robustness and reliability of the test suite.
client.go
Outdated
@@ -49,6 +50,7 @@ const separator = "," | |||
func init() { | |||
extension.SetCacheFactory(&memory.DefaultCacheFactory{}) | |||
extension.SetLoadBalance(&roundrobin.RoundRobin{}) | |||
extension.SetConfigMapHandler(&configMap.Store{K8sManager: configMap.GetK8sManager()}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Issue Found: configMap.Store
does not implement the required LoadConfigMap
method.
- The
Store
struct is missing theLoadConfigMap(string) (*config.ApolloConfig, error)
method required byextension.SetConfigMapHandler
. - Ensure that the
Store
struct implements all necessary methods to satisfy the expected interface.
🔗 Analysis chain
LGTM: ConfigMap handler setup.
The new line correctly sets up the ConfigMap handler using the extension.SetConfigMapHandler
function. This aligns with the PR objective of supporting Kubernetes ConfigMap for the Apollo client.
To ensure the ConfigMap functionality is properly integrated, please verify that:
- The
configMap.Store
struct implements the correct interface expected byextension.SetConfigMapHandler
. - The
configMap.GetK8sManager()
function is properly implemented and returns a valid Kubernetes manager.
You can run the following script to check the implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ConfigMap implementation
# Check if the Store struct implements the correct interface
ast-grep --lang go --pattern 'type Store struct { $$$
LoadConfigMap(string) (*config.ApolloConfig, error)
$$$
}'
# Check the GetK8sManager function implementation
ast-grep --lang go --pattern 'func GetK8sManager() $_ {
$$$
}'
Length of output: 992
go.mod
Outdated
k8s.io/api v0.30.1 | ||
k8s.io/apimachinery v0.30.1 | ||
k8s.io/client-go v0.30.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify the Versions of Kubernetes Dependencies
The go.mod
file specifies Kubernetes modules with version v0.30.1
:
k8s.io/api v0.30.1
k8s.io/apimachinery v0.30.1
k8s.io/client-go v0.30.1
As of April 2024, the latest stable versions of these modules are around v0.28.x
. Versions like v0.30.1
may not exist or might be pre-release. Using non-existent or unstable versions can lead to build failures or unexpected behavior. Please verify that these versions are correct and update them to valid, stable versions if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (14)
store/configmap/manager_test.go (3)
30-55
: LGTM:TestK8sManager_SetConfigMap
is well-structured.The test function correctly sets up a fake Kubernetes client, creates a
K8sManager
instance, and tests theSetConfigMap
method. It properly verifies both the successful creation of the ConfigMap and its content.Consider adding the following test cases to improve coverage:
- Test with an empty
ApolloConfig
.- Test updating an existing ConfigMap.
- Test error handling when the Kubernetes API call fails.
57-82
: LGTM:TestK8sManager_GetConfigMap
is well-implemented.The test function correctly sets up a fake Kubernetes client with a pre-existing ConfigMap, creates a
K8sManager
instance, and tests theGetConfigMap
method. It properly verifies both the successful retrieval of the ConfigMap and its content.Consider the following improvements:
- Add test cases for:
- Retrieving a non-existent ConfigMap
- Handling API errors
- Retrieving a ConfigMap with multiple key-value pairs
- Use
assert.Equal(t, "value", configurations["key"])
instead ofassert.Equal(t, configurations["key"], "value")
for consistency with Go's convention of putting the expected value first in assertions.
1-82
: Good test coverage, with room for improvement.The test file provides a solid foundation for testing the
K8sManager
struct. It covers the basic functionality of bothSetConfigMap
andGetConfigMap
methods using fake Kubernetes clients, which is a good practice for unit testing.To enhance the test suite, consider the following improvements:
- Add error handling tests for both methods (e.g., API failures, permission issues).
- Include edge cases such as empty configs, large configs, or configs with special characters.
- Test concurrent access to the ConfigMap if applicable to the use case.
- Add benchmarks for performance-critical operations if relevant.
- Consider using table-driven tests to cover multiple scenarios more efficiently.
These additions would provide more comprehensive coverage and increase confidence in the robustness of the
K8sManager
implementation.store/configmap/store_test.go (4)
50-50
: Consider adding more boundary tests as suggested by the TODO comment.It's good practice to include edge cases and boundary conditions in your tests. Consider adding tests for empty configurations, very large configurations, or configurations with special characters.
Would you like assistance in generating additional boundary test cases?
52-96
: LGTM: TestStore_LoadConfigMap covers basic functionality.The test effectively verifies the LoadConfigMap method using a fake Kubernetes clientset. It properly sets up the test environment and asserts the expected outcomes.
Consider adding the following test cases to improve coverage:
- Test with an empty ConfigMap.
- Test with a non-existent ConfigMap to verify error handling.
- Test with malformed JSON data in the ConfigMap.
Would you like assistance in implementing these additional test cases?
98-132
: LGTM: TestStore_WriteConfigMap covers basic functionality.The test effectively verifies the WriteConfigMap method using a fake Kubernetes clientset. It properly sets up the test environment, writes the configuration, and asserts the expected outcomes.
Consider adding the following test cases to improve coverage:
- Test writing an empty configuration.
- Test updating an existing ConfigMap.
- Test writing a very large configuration to check for any size limitations.
- Test error handling when the clientset fails to write the ConfigMap.
Would you like assistance in implementing these additional test cases?
1-132
: Overall, good test coverage with room for improvement.The tests in this file provide a solid foundation for verifying the functionality of loading and writing Kubernetes ConfigMaps. The use of a fake Kubernetes clientset is commendable for unit testing.
To further enhance the test suite:
- Implement the suggested additional test cases for both LoadConfigMap and WriteConfigMap functions.
- Add error handling tests to ensure robustness.
- Consider using table-driven tests to cover multiple scenarios efficiently.
- Add comments explaining the purpose of each test case for better maintainability.
These improvements will lead to a more comprehensive and maintainable test suite.
Would you like assistance in implementing any of these suggestions?
env/config/config.go (3)
44-54
: LGTM! Consider adding a comment forConfigMapNamespace
.The new fields
IsBackupConfigToConfigMap
andConfigMapNamespace
are well-aligned with the PR objectives of supporting Kubernetes ConfigMap. The default value forIsBackupConfigToConfigMap
maintains backward compatibility.Consider adding a comment for the
ConfigMapNamespace
field to clarify its purpose and default value, similar to howIsBackupConfigToConfigMap
is documented. For example:// ConfigMapNamespace specifies the Kubernetes namespace for ConfigMap. If empty, it defaults to "default". ConfigMapNamespace string `json:"configMapNamespace"`
88-93
: LGTM! Consider using a local variable for thread safety.The
GetConfigMapNamespace
method is well-implemented, providing a default value of "default" whenConfigMapNamespace
is empty. This aligns with Kubernetes conventions.For better thread safety, consider using a local variable instead of modifying the struct's state:
func (a *AppConfig) GetConfigMapNamespace() string { if a.ConfigMapNamespace == "" { return "default" } return a.ConfigMapNamespace }This approach prevents potential race conditions in concurrent scenarios.
Line range hint
44-93
: Overall, the changes effectively support Kubernetes ConfigMap integration.The modifications to the
AppConfig
struct and the addition of new methods align well with the PR objectives. The implementation provides a flexible way to enable ConfigMap backup and specify a custom namespace.Key points:
- The
IsBackupConfigToConfigMap
field allows users to opt-in to the new functionality.- The
ConfigMapNamespace
field provides flexibility in specifying the Kubernetes namespace.- The new getter methods follow existing conventions and provide clean access to the new fields.
These changes enhance the configuration management capabilities of the Apollo client, particularly in Kubernetes environments, addressing the issue of configuration persistence during server downtime or Pod restarts.
As you continue to develop this feature:
- Ensure that the ConfigMap integration is thoroughly tested in various Kubernetes scenarios.
- Consider adding documentation or examples demonstrating how to use these new configuration options effectively.
- Be mindful of backward compatibility and provide clear migration guidelines for users adopting this new feature.
storage/repository.go (1)
490-490
: Remove commented-out log statement.Please remove the commented-out log statement to maintain clean code.
- //log.Info("11111111开始写configmap缓存")
store/configmap/store.go (2)
33-33
: Translate code comments to EnglishThe comment on line 33 is in Chinese. For consistency and maintainability, please use English for code comments to ensure that all team members can understand.
Apply this diff to translate the comment:
-// TODO 在这里把json转为ApolloConfig, 但ReleaseKey字段会丢失, 影响大不大 +// TODO: Convert the JSON to ApolloConfig here, but the ReleaseKey field will be lost. Assess the impact.
44-44
: Translate code comments to EnglishThe comment on line 44 is in Chinese. Please use English for code comments for consistency.
Apply this diff to translate the comment:
-// AppId作为configMap的name,cluster+namespace作为key, value为config +// Use AppID as the ConfigMap name, cluster + namespace as the key, and config as the valuestore/configmap/manager.go (1)
94-94
: Remove debugging comments from the codeThe comment
// 新建不成功,测试能否解决
appears to be a debugging note. It's advisable to remove such comments to keep the codebase clean and professional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- client.go (2 hunks)
- env/config/config.go (2 hunks)
- storage/repository.go (2 hunks)
- store/configmap/manager.go (1 hunks)
- store/configmap/manager_test.go (1 hunks)
- store/configmap/store.go (1 hunks)
- store/configmap/store_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
store/configmap/manager_test.go (1)
1-28
: LGTM: File header and imports are appropriate.The license header, package name, and import statements are all correctly set up for the test file. The imports cover all necessary packages for Kubernetes testing and Apollo configuration.
store/configmap/store_test.go (2)
18-30
: LGTM: Package declaration and imports are appropriate.The package name
configmap
aligns with the file's purpose, and the imports cover all necessary libraries for Kubernetes ConfigMap testing.
32-48
: LGTM: Constants and test data are well-defined.The constants provide clear identifiers for the test environment, and the test data covers a good range of data types, including nested structures.
env/config/config.go (1)
81-86
: LGTM! The new getter method is well-implemented.The
GetIsBackupConfigToConfigMap
method is correctly implemented and follows the existing naming conventions in the file. It provides a clean way to access the newIsBackupConfigToConfigMap
field.client.go (3)
23-23
: LGTM: New import for ConfigMap support.The new import for the
configmap
package is correctly added and aligns with the PR objective of supporting Kubernetes ConfigMap for the Apollo client.
Line range hint
52-52
: LGTM: Global variable declaration for syncApolloConfig.The global variable
syncApolloConfig
is correctly declared and initialized usingremote.CreateSyncApolloConfig()
. This aligns with the synchronization functionality for Apollo configurations.
Line range hint
1-291
: Summary of changes and recommendations.The changes in this file successfully introduce support for Kubernetes ConfigMap in the Apollo client. The new import, global variable declaration, and conditional block in the
StartWithConfig
function are all implemented correctly and align with the PR objectives.However, there's one important point that requires attention:
- Ensure that the
configmap.Store
struct correctly implements all methods required by thestore.ConfigMapHandler
interface, especially theLoadConfigMap(string) (*config.ApolloConfig, error)
method. This was flagged in a previous review and needs to be verified to guarantee proper functionality of the ConfigMap feature.Once this point is addressed and verified, the implementation will be complete and ready for integration.
storage/repository.go (1)
Line range hint
456-493
: LGTM! Implementation aligns with PR objectives.The changes successfully implement the backup to Kubernetes ConfigMap functionality. The new feature is optional and executed asynchronously, which maintains the existing behavior while adding the desired functionality.
client.go
Outdated
if appConfig.IsBackupConfigToConfigMap { | ||
extension.SetConfigMapHandler(&configmap.Store{K8sManager: configmap.GetK8sManager()}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
ConfigMap Handler Missing LoadConfigMap
Method
The configmap.Store
struct does not implement the required LoadConfigMap(string) (*config.ApolloConfig, error)
method. Please implement this method to satisfy the store.ConfigMapHandler
interface and ensure proper ConfigMap handling.
🔗 Analysis chain
Verify the ConfigMap handler implementation.
The conditional block for setting up the ConfigMap handler is correctly added. However, there's a potential issue that needs to be addressed:
-
In a previous review, it was noted that the
configmap.Store
struct did not implement the requiredLoadConfigMap
method. Please ensure that this has been resolved. -
Verify that the
configmap.Store
struct now correctly implements all methods required by thestore.ConfigMapHandler
interface, especially theLoadConfigMap(string) (*config.ApolloConfig, error)
method.
To check the implementation, you can run the following script:
If the LoadConfigMap
method is not found or doesn't match the expected signature, please update the configmap.Store
struct accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ConfigMap implementation
# Check if the Store struct implements the correct interface
ast-grep --lang go --pattern 'type Store struct { $$$
LoadConfigMap(string) (*config.ApolloConfig, error)
$$$
}'
# Check the GetK8sManager function implementation
ast-grep --lang go --pattern 'func GetK8sManager() $_ {
$$$
}'
Length of output: 1036
store/configmap/store.go
Outdated
var config = &config.ApolloConfig{} | ||
configMapName := appConfig.AppID | ||
key := appConfig.Cluster + "+" + appConfig.NamespaceName | ||
// TODO 在这里把json转为ApolloConfig, 但ReleaseKey字段会丢失, 影响大不大 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address the TODO comment regarding JSON conversion
The TODO comment suggests that converting JSON to ApolloConfig
may result in the ReleaseKey
field being lost, which could significantly impact the application's functionality.
Would you like assistance in implementing a solution that preserves the ReleaseKey
field during JSON conversion? I can help modify the code to ensure that all necessary fields are retained.
store/configmap/store.go
Outdated
configMapName := appConfig.AppID | ||
key := appConfig.Cluster + "+" + appConfig.NamespaceName | ||
// TODO 在这里把json转为ApolloConfig, 但ReleaseKey字段会丢失, 影响大不大 | ||
config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle errors returned by GetConfigMap
The call to GetConfigMap
may return an error, which is currently being ignored. Proper error handling is important to avoid unexpected runtime issues.
Apply this diff to handle the error:
-config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key)
+config.Configurations, err := c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key)
+if err != nil {
+ return nil, err
+}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) | |
config.Configurations, err := c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) | |
if err != nil { | |
return nil, err | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest return the error, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyruslo, thank you for your suggestion. You're absolutely right; returning the error ensures that any issues are properly propagated to the caller. I'll update the code accordingly.
store/configmap/store.go
Outdated
type Store struct { | ||
K8sManager *K8sManager | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undefined type K8sManager
The Store
struct references K8sManager
, but K8sManager
is not defined or imported. Ensure that K8sManager
is properly defined in this package or imported from the correct package.
Apply this diff to add the missing import or definition:
import (
"github.com/apolloconfig/agollo/v4/env/config"
+ "path/to/your/k8smanager/package"
)
+// Make sure to define or import K8sManager
+type K8sManager struct {
+ // Define fields and methods for K8sManager
+}
Committable suggestion was skipped due to low confidence.
store/configmap/manager.go
Outdated
panic(err.Error()) | ||
} | ||
clientSet, err := kubernetes.NewForConfig(config) | ||
if err != nil { | ||
panic(err.Error()) | ||
} | ||
instance = &K8sManager{ | ||
clientSet: clientSet, | ||
mutex: sync.RWMutex{}, | ||
} | ||
}) | ||
return instance | ||
} | ||
|
||
// SetConfigMap 将map[string]interface{}转换为JSON字符串,并创建或更新ConfigMap | ||
func (m *K8sManager) SetConfigMap(configMapName string, configMapNamespace string, key string, config *config.ApolloConfig) error { | ||
m.mutex.Lock() // 加锁 | ||
defer m.mutex.Unlock() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
// 将ApolloConfig的配置信息数据转换为JSON字符串 | ||
jsonData, err := json.Marshal(config.Configurations) | ||
if err != nil { | ||
return fmt.Errorf("error marshaling data to JSON: %v", err) | ||
} | ||
|
||
jsonString := string(jsonData) | ||
log.Infof("准备configmap内容,json:%s", jsonString) | ||
// 尝试获取 ConfigMap,如果不存在则创建 | ||
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | ||
if errors.IsNotFound(err) { | ||
cm = &coreV1.ConfigMap{ | ||
ObjectMeta: metaV1.ObjectMeta{ | ||
Name: configMapName, | ||
Namespace: configMapNamespace, | ||
}, | ||
Data: map[string]string{ | ||
key: jsonString, | ||
}, | ||
} | ||
|
||
// 新建不成功,测试能否解决 | ||
if utf8.Valid(jsonData) { | ||
cm.Data[key] = jsonString | ||
} else { | ||
cm.BinaryData[key] = jsonData | ||
} | ||
|
||
//log.Infof("准备创建, cm:%s", cm) | ||
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Create(ctx, cm, metaV1.CreateOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("error creating ConfigMap: %v", err) | ||
} | ||
log.Infof("ConfigMap %s created in namespace %s", configMapName, configMapNamespace) | ||
} else if err != nil { | ||
return err | ||
} else { | ||
// ConfigMap 存在,更新数据 | ||
cm.Data[key] = jsonString | ||
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Update(ctx, cm, metaV1.UpdateOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("error updating ConfigMap: %v", err) | ||
} | ||
log.Infof("ConfigMap %s updated in namespace %s", configMapName, configMapNamespace) | ||
} | ||
return err | ||
} | ||
|
||
// GetConfigMap 从ConfigMap中获取JSON字符串,并反序列化为map[string]interface{} | ||
func (m *K8sManager) GetConfigMap(configMapName string, configMapNamespace string, key string) (map[string]interface{}, error) { | ||
m.mutex.RLock() // 加读锁 | ||
defer m.mutex.RUnlock() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
configMap, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | ||
if err != nil { | ||
return nil, fmt.Errorf("error getting ConfigMap: %v", err) | ||
} | ||
|
||
//log.Info("读取成功。,configmap的内容:", configMap.Data[key]) | ||
// 从ConfigMap中读取JSON数据 | ||
jsonData, ok := configMap.Data[key] | ||
if !ok { | ||
return nil, fmt.Errorf("key: %v not found in ConfigMap", key) | ||
} | ||
|
||
// 反序列化JSON配置信息到ApolloConfig的Configurations字段 | ||
// 可以考虑使用 configTemp := &config.ApolloConfig{} 接收 | ||
var configurations map[string]interface{} | ||
err = json.Unmarshal([]byte(jsonData), &configurations) | ||
if err != nil { | ||
return nil, fmt.Errorf("error unmarshaling JSON to map[string]interface{}: %v", err) | ||
} | ||
|
||
return configurations, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure consistent language usage in comments and log messages
The code contains comments and log messages in both Chinese and English. For better maintainability and wider accessibility, consider standardizing all comments and log messages to English.
store/configmap/manager.go
Outdated
"unicode/utf8" | ||
) | ||
|
||
// TODO 改为CAS更好,用版本号解决api server的并发 https://blog.csdn.net/boling_cavalry/article/details/128745382 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Implement optimistic concurrency control to handle concurrent updates
The current implementation does not handle concurrent updates to the ConfigMap, which may lead to conflicts or data loss. The TODO comment at line 36 suggests using CAS (Compare-And-Swap) with version numbers.
Consider utilizing the ResourceVersion
field of the ConfigMap for concurrency control:
// Before updating the ConfigMap
+oldResourceVersion := cm.ResourceVersion
// Update the ConfigMap data
cm.Data[key] = jsonString
+cm.ResourceVersion = oldResourceVersion
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Update(ctx, cm, metaV1.UpdateOptions{})
if err != nil {
return fmt.Errorf("error updating ConfigMap: %v", err)
}
This ensures that the update will fail if the ConfigMap has been modified since it was fetched.
Would you like assistance in fully implementing this concurrency control mechanism?
Also applies to: 111-116
store/configmap/manager.go
Outdated
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | ||
if errors.IsNotFound(err) { | ||
cm = &coreV1.ConfigMap{ | ||
ObjectMeta: metaV1.ObjectMeta{ | ||
Name: configMapName, | ||
Namespace: configMapNamespace, | ||
}, | ||
Data: map[string]string{ | ||
key: jsonString, | ||
}, | ||
} | ||
|
||
// 新建不成功,测试能否解决 | ||
if utf8.Valid(jsonData) { | ||
cm.Data[key] = jsonString | ||
} else { | ||
cm.BinaryData[key] = jsonData | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialize BinaryData map before assignment to prevent nil pointer dereference
When assigning to cm.BinaryData[key]
, the BinaryData
map may not be initialized, leading to a nil pointer dereference.
Apply this diff to initialize BinaryData
:
cm = &coreV1.ConfigMap{
ObjectMeta: metaV1.ObjectMeta{
Name: configMapName,
Namespace: configMapNamespace,
},
Data: map[string]string{},
+ BinaryData: map[string][]byte{},
}
// Remove debugging comment
-// 新建不成功,测试能否解决
if utf8.Valid(jsonData) {
cm.Data[key] = jsonString
} else {
cm.BinaryData[key] = jsonData
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | |
if errors.IsNotFound(err) { | |
cm = &coreV1.ConfigMap{ | |
ObjectMeta: metaV1.ObjectMeta{ | |
Name: configMapName, | |
Namespace: configMapNamespace, | |
}, | |
Data: map[string]string{ | |
key: jsonString, | |
}, | |
} | |
// 新建不成功,测试能否解决 | |
if utf8.Valid(jsonData) { | |
cm.Data[key] = jsonString | |
} else { | |
cm.BinaryData[key] = jsonData | |
} | |
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | |
if errors.IsNotFound(err) { | |
cm = &coreV1.ConfigMap{ | |
ObjectMeta: metaV1.ObjectMeta{ | |
Name: configMapName, | |
Namespace: configMapNamespace, | |
}, | |
Data: map[string]string{}, | |
BinaryData: map[string][]byte{}, | |
} | |
if utf8.Valid(jsonData) { | |
cm.Data[key] = jsonString | |
} else { | |
cm.BinaryData[key] = jsonData | |
} |
store/configmap/manager.go
Outdated
func GetK8sManager() *K8sManager { | ||
once.Do(func() { | ||
config, err := rest.InClusterConfig() | ||
if err != nil { | ||
panic(err.Error()) | ||
} | ||
clientSet, err := kubernetes.NewForConfig(config) | ||
if err != nil { | ||
panic(err.Error()) | ||
} | ||
instance = &K8sManager{ | ||
clientSet: clientSet, | ||
mutex: sync.RWMutex{}, | ||
} | ||
}) | ||
return instance | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using panic for error handling in production code
In the GetK8sManager()
function, errors are handled using panic
, which can cause the entire application to crash. It's recommended to return the error to the caller instead, allowing for graceful error handling and recovery.
Apply this diff to modify the function to return an error:
-func GetK8sManager() *K8sManager {
+func GetK8sManager() (*K8sManager, error) {
once.Do(func() {
config, err := rest.InClusterConfig()
if err != nil {
- panic(err.Error())
+ instance = nil
+ once = sync.Once{}
+ log.Errorf("Error creating in-cluster config: %v", err)
+ return
}
clientSet, err := kubernetes.NewForConfig(config)
if err != nil {
- panic(err.Error())
+ instance = nil
+ once = sync.Once{}
+ log.Errorf("Error creating Kubernetes client set: %v", err)
+ return
}
instance = &K8sManager{
clientSet: clientSet,
mutex: sync.RWMutex{},
}
})
- return instance
+ if instance == nil {
+ return nil, fmt.Errorf("failed to create K8sManager instance")
+ }
+ return instance, nil
}
You'll also need to update any calls to GetK8sManager()
to handle the returned error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
func GetK8sManager() *K8sManager { | |
once.Do(func() { | |
config, err := rest.InClusterConfig() | |
if err != nil { | |
panic(err.Error()) | |
} | |
clientSet, err := kubernetes.NewForConfig(config) | |
if err != nil { | |
panic(err.Error()) | |
} | |
instance = &K8sManager{ | |
clientSet: clientSet, | |
mutex: sync.RWMutex{}, | |
} | |
}) | |
return instance | |
} | |
func GetK8sManager() (*K8sManager, error) { | |
once.Do(func() { | |
config, err := rest.InClusterConfig() | |
if err != nil { | |
instance = nil | |
once = sync.Once{} | |
log.Errorf("Error creating in-cluster config: %v", err) | |
return | |
} | |
clientSet, err := kubernetes.NewForConfig(config) | |
if err != nil { | |
instance = nil | |
once = sync.Once{} | |
log.Errorf("Error creating Kubernetes client set: %v", err) | |
return | |
} | |
instance = &K8sManager{ | |
clientSet: clientSet, | |
mutex: sync.RWMutex{}, | |
} | |
}) | |
if instance == nil { | |
return nil, fmt.Errorf("failed to create K8sManager instance") | |
} | |
return instance, nil | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- client.go (2 hunks)
- store/configmap/manager.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client.go
🧰 Additional context used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
store/configmap/manager.go (3)
35-35
: Consider implementing CAS for better concurrency controlThe TODO comment suggests using Compare-And-Swap (CAS) with version numbers for better concurrency control. This would be a valuable improvement to prevent potential race conditions when updating ConfigMaps.
Would you like assistance in implementing this CAS mechanism using the
ResourceVersion
field of the ConfigMap?
88-88
: Ensure consistent language usage in comments and log messagesThe log message on this line is in Chinese, while other parts of the code use English. For better maintainability and wider accessibility, consider standardizing all comments and log messages to English.
135-135
: Remove or uncomment log messageThere's a commented-out log message on this line. If this log message is useful for debugging, consider uncommenting it. Otherwise, it's best to remove it to keep the code clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- client.go (2 hunks)
- store/configmap/manager.go (1 hunks)
- store/configmap/store.go (1 hunks)
- store/configmap/store_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client.go
- store/configmap/store.go
- store/configmap/store_test.go
🧰 Additional context used
🔇 Additional comments (3)
store/configmap/manager.go (3)
18-33
: LGTM: Package declaration and imports are appropriate.The package name
configmap
is suitable for the functionality, and all imported packages are relevant and used within the file.
46-71
: LGTM: Improved error handling in GetK8sManager()The error handling in
GetK8sManager()
has been improved by returning errors instead of panicking. This change allows for more graceful error handling by the caller.
1-151
: Overall implementation is solid with room for improvementsThe
K8sManager
implementation provides a good foundation for managing Kubernetes ConfigMaps in the Apollo client. Key strengths include:
- Proper error handling and thread safety considerations.
- Correct implementation of the singleton pattern.
- Clear separation of concerns between setting and getting ConfigMap data.
Areas for improvement:
- Implement CAS (Compare-And-Swap) for better concurrency control.
- Refine mutex usage in
SetConfigMap
for improved performance.- Handle non-UTF-8 data using
BinaryData
in ConfigMaps.- Check both
Data
andBinaryData
when retrieving ConfigMap data.- Ensure consistent language usage in comments and log messages.
Addressing these points will significantly enhance the robustness, efficiency, and maintainability of the code.
store/configmap/manager.go
Outdated
func (m *K8sManager) SetConfigMap(configMapName string, configMapNamespace string, key string, config *config.ApolloConfig) error { | ||
m.mutex.Lock() // 加锁 | ||
defer m.mutex.Unlock() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider refining mutex usage for better performance
The current implementation holds the mutex lock for the entire duration of the method, including I/O operations. This could potentially lead to performance issues. Consider narrowing the scope of the lock to protect only the critical sections:
func (m *K8sManager) SetConfigMap(configMapName string, configMapNamespace string, key string, config *config.ApolloConfig) error {
// Prepare data outside the lock
jsonData, err := json.Marshal(config.Configurations)
if err != nil {
return fmt.Errorf("error marshaling data to JSON: %v", err)
}
jsonString := string(jsonData)
m.mutex.Lock()
defer m.mutex.Unlock()
// Perform Kubernetes API operations here
// ...
}
This change would allow for better concurrency by reducing the time the lock is held.
store/configmap/manager.go
Outdated
Data: map[string]string{ | ||
key: jsonString, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle non-UTF-8 data in ConfigMap
The current implementation assumes all data can be stored as a string. However, some configuration data might not be valid UTF-8. Consider using BinaryData
for non-UTF-8 data:
cm := &coreV1.ConfigMap{
ObjectMeta: metaV1.ObjectMeta{
Name: configMapName,
Namespace: configMapNamespace,
},
Data: map[string]string{},
BinaryData: map[string][]byte{},
}
if utf8.Valid(jsonData) {
cm.Data[key] = jsonString
} else {
cm.BinaryData[key] = jsonData
}
This change ensures that all types of data can be stored correctly in the ConfigMap.
store/configmap/manager.go
Outdated
jsonData, ok := configMap.Data[key] | ||
if !ok { | ||
return nil, fmt.Errorf("key: %v not found in ConfigMap", key) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential BinaryData in ConfigMap
The current implementation only checks the Data
map of the ConfigMap. However, some data might be stored in BinaryData
. Consider checking both:
var jsonData string
if data, ok := configMap.Data[key]; ok {
jsonData = data
} else if binaryData, ok := configMap.BinaryData[key]; ok {
jsonData = string(binaryData)
} else {
return nil, fmt.Errorf("key: %v not found in ConfigMap", key)
}
This change ensures that all types of stored data can be retrieved correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
extension/configmap_test.go (3)
20-24
: Consider removing the dot import for gohamcrest.While the imports are generally appropriate, using a dot import (.) for the gohamcrest package is not recommended. It can lead to naming conflicts and make the code less readable.
Consider changing the import statement to:
- . "github.com/tevid/gohamcrest" + "github.com/tevid/gohamcrest"And update any usage of gohamcrest functions to include the package name, e.g.,
gohamcrest.Assert()
.
26-35
: LGTM: TestConfigMapHandler struct and methods are well-defined.The TestConfigMapHandler struct and its methods (LoadConfigMap and WriteConfigMap) are correctly implemented as stubs for testing purposes. The method signatures align with the expected ConfigMapHandler interface.
For improved clarity, consider adding a comment to explain that these are stub implementations for testing:
// TestConfigMapHandler is a stub implementation of ConfigMapHandler for testing purposes. type TestConfigMapHandler struct { } // LoadConfigMap is a stub implementation that always returns nil values. func (t *TestConfigMapHandler) LoadConfigMap(appConfig config.AppConfig, configMapNamespace string) (*config.ApolloConfig, error) { return nil, nil } // WriteConfigMap is a stub implementation that always returns nil. func (t *TestConfigMapHandler) WriteConfigMap(config *config.ApolloConfig, configMapNamespace string) error { return nil }
37-44
: LGTM: TestSetConfigMapHandler function is well-implemented.The test function correctly verifies the SetConfigMapHandler functionality by setting a handler and asserting that it can be retrieved.
Consider translating the Chinese comment to English for consistency with the rest of the codebase:
-// TestSetConfigMapHandler 测试 SetConfigMapHandler 函数 +// TestSetConfigMapHandler tests the SetConfigMapHandler functionstore/configmap/manager.go (2)
35-71
: Improved error handling, but consider enhancing concurrency controlThe
GetK8sManager
function now properly returns an error instead of usingpanic
, which is a good improvement. However, the TODO comment about using Compare-And-Swap (CAS) for better concurrency control is still present.Consider implementing optimistic concurrency control using the
ResourceVersion
field of the ConfigMap. This would help prevent conflicts in concurrent updates.Would you like assistance in implementing the CAS mechanism for better concurrency control?
1-150
: Ensure consistent language usage in comments and log messagesWhile the overall structure and implementation of the file are good, there's an inconsistency in the language used for comments and log messages. Some are in Chinese, while others are in English.
For better maintainability and wider accessibility, consider standardizing all comments and log messages to English. This will ensure consistency throughout the codebase and make it easier for non-Chinese speaking developers to understand and maintain the code.
For example, change:
log.Infof("准备configmap内容,json:%s", jsonString)to:
log.Infof("Preparing ConfigMap content, JSON: %s", jsonString)Please review all comments and log messages in the file and update them to use consistent English terminology.
start_test.go (2)
228-245
: LGTM! Consider adding error cases to the test.The implementation of
testConfigMapHandler
andTestSetConfigMapHandler
looks good. The test correctly verifies that the ConfigMap handler can be set and retrieved.To improve test coverage, consider adding test cases for error scenarios, such as:
- Attempting to set a nil handler
- Verifying behavior when the handler is not set
Here's a suggestion to enhance the test:
func TestSetConfigMapHandler(t *testing.T) { // Test default handler configMapHandler := extension.GetConfigMapHandler() Assert(t, configMapHandler, NotNilVal()) // Test setting a custom handler t2 := &testConfigMapHandler{} SetConfigMapHandler(t2) Assert(t, t2, Equal(extension.GetConfigMapHandler())) // Test setting nil handler (should not change the handler) SetConfigMapHandler(nil) Assert(t, t2, Equal(extension.GetConfigMapHandler())) }
228-245
: Summary: Changes align with PR objectivesThe addition of
testConfigMapHandler
andTestSetConfigMapHandler
is a good step towards supporting Kubernetes ConfigMap for the Apollo client. These changes provide a foundation for testing the ConfigMap functionality, which is crucial for ensuring the reliability and fault tolerance of configuration information in Kubernetes environments.To further strengthen this implementation:
- Consider adding more comprehensive tests that simulate real-world scenarios of loading and writing ConfigMaps.
- Ensure that the actual ConfigMap handler (not just the test stub) is implemented and tested thoroughly in the main code.
- Document the usage and benefits of the ConfigMap feature for users operating in Kubernetes environments.
storage/repository.go (1)
Line range hint
456-493
: LGTM! Consider adding error handling for ConfigMap writing.The changes look good and appropriately implement the backup to ConfigMap functionality. The use of a goroutine for asynchronous writing is a good practice for performance.
Consider adding error handling for the ConfigMap writing operation. Although it's running in a goroutine, it would be beneficial to log any errors that occur during the process. You could modify the code as follows:
if appConfig.GetIsBackupConfigToConfigMap() { // write configmap async - go extension.GetConfigMapHandler().WriteConfigMap(apolloConfig, appConfig.GetConfigMapNamespace()) + go func() { + err := extension.GetConfigMapHandler().WriteConfigMap(apolloConfig, appConfig.GetConfigMapNamespace()) + if err != nil { + log.Errorf("Failed to write to ConfigMap: %v", err) + } + }() }This change will ensure that any errors during the ConfigMap writing process are logged and can be monitored.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- client.go (2 hunks)
- component/remote/async.go (2 hunks)
- extension/configmap_test.go (1 hunks)
- start_test.go (1 hunks)
- storage/repository.go (2 hunks)
- store/configmap/manager.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client.go
- component/remote/async.go
🧰 Additional context used
🔇 Additional comments (4)
extension/configmap_test.go (2)
1-18
: LGTM: License and package declaration are correct.The Apache 2.0 license header is properly included, and the package is correctly declared as "extension".
1-44
: Overall, the ConfigMap test implementation looks good with minor suggestions for improvement.The
extension/configmap_test.go
file introduces a well-structured test suite for ConfigMap handling. It includes appropriate license information, necessary imports, a test struct (TestConfigMapHandler), and a test function (TestSetConfigMapHandler). The implementation correctly sets up stub methods for testing purposes and verifies the basic functionality of setting and retrieving a ConfigMapHandler.To further improve the file:
- Consider removing the dot import for the gohamcrest package.
- Add comments to clarify that TestConfigMapHandler is a stub implementation for testing.
- Translate the Chinese comment to English for consistency.
These minor adjustments will enhance the clarity and maintainability of the test suite.
store/configmap/manager.go (2)
1-33
: LGTM: Package declaration and imports are appropriateThe file is properly licensed, and the imports include all necessary packages for Kubernetes ConfigMap management and Apollo client integration.
1-150
: Overall assessment: Good implementation with room for improvementThe
store/configmap/manager.go
file successfully implements the functionality for managing Kubernetes ConfigMaps in the Apollo client. The code is well-structured and follows Go best practices. However, there are several areas where the implementation can be enhanced:
- Implement optimistic concurrency control using Compare-And-Swap (CAS) with the
ResourceVersion
field.- Refine mutex usage in the
SetConfigMap
method to improve performance.- Handle non-UTF-8 data in ConfigMaps using the
BinaryData
field.- Enhance the
GetConfigMap
method to handle data stored in bothData
andBinaryData
fields.- Ensure consistent use of English in comments and log messages throughout the file.
Addressing these points will significantly improve the robustness, efficiency, and maintainability of the code. Great work on the implementation so far!
@zouyx PTAL |
store/configmap/store.go
Outdated
|
||
// LoadConfigMap load ApolloConfig from configmap | ||
func (c *Store) LoadConfigMap(appConfig config.AppConfig, configMapNamespace string) (*config.ApolloConfig, error) { | ||
var config = &config.ApolloConfig{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable 'config' collides with imported package name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks a lot
store/configmap/store.go
Outdated
configMapName := appConfig.AppID | ||
key := appConfig.Cluster + "+" + appConfig.NamespaceName | ||
// TODO 在这里把json转为ApolloConfig, 但ReleaseKey字段会丢失, 影响大不大 | ||
config.Configurations, _ = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest return the error, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
store/configmap/store_test.go (4)
50-50
: Translate TODO comment and create an issue for tracking.The TODO comment is in Chinese, which may not be accessible to all contributors. Consider translating it to English. Additionally, it's a good practice to create a GitHub issue to track the need for more boundary tests.
Would you like me to translate the comment and create a GitHub issue for tracking the additional boundary tests?
52-97
: LGTM: TestStore_LoadConfigMap is well-structured. Consider adding more test cases.The test function effectively covers the main functionality of
LoadConfigMap
. It properly sets up the test environment, executes the method, and verifies the results.To improve test coverage:
- Add test cases for error conditions (e.g., non-existent ConfigMap, unmarshalling errors).
- Test with empty or malformed ConfigMap data.
- Verify behavior with different namespace and cluster combinations.
99-140
: LGTM: TestStore_WriteConfigMap is well-structured. Consider improvements and remove unused variable.The test function effectively covers the main functionality of
WriteConfigMap
. It properly sets up the test environment, executes the method, and verifies the results.Suggestions for improvement:
- Add test cases for error conditions (e.g., write failures, marshalling errors).
- Test with empty or special character configurations.
- Verify behavior when updating an existing ConfigMap.
Also, there's an unused error variable at line 103 that should be removed:
- var err error
1-140
: Good test coverage, but room for improvement in edge cases and test structure.Overall, the test file provides good coverage for the main functionality of
LoadConfigMap
andWriteConfigMap
. The use of fake Kubernetes clientsets is a good practice for isolated testing.Suggestions for improving the test suite:
- Add negative test cases and edge condition tests for both functions.
- Consider creating helper functions for common setup tasks to reduce code duplication.
- Implement setup and teardown methods if more complex test scenarios are needed in the future.
- Add tests for concurrent access if the Store is intended to be used in a multi-goroutine environment.
- Consider using table-driven tests for testing multiple scenarios efficiently.
These improvements will enhance the robustness and maintainability of the test suite.
store/configmap/manager.go (1)
1-150
: Good overall structure, consider improving language consistencyThe file implements a robust K8sManager for managing Kubernetes ConfigMaps with proper error handling and logging. The structure is clear with good separation of concerns.
To improve consistency and maintainability:
- Translate all Chinese comments to English.
- Ensure all log messages are in English.
- Consider adding more detailed comments explaining the purpose and behavior of each method, especially for edge cases and error scenarios.
These changes will make the code more accessible to a wider range of developers and easier to maintain in the long run.
store/configmap/store.go (4)
29-29
: Improve function comment to follow GoDoc conventionsThe comment for
LoadConfigMap
should start with the function name and be a full sentence. This enhances documentation readability and complies with GoDoc standards.Apply this diff to update the comment:
-// LoadConfigMap load ApolloConfig from configmap +// LoadConfigMap loads ApolloConfig from a ConfigMap.
35-35
: Translate the TODO comment to English for consistencyThe TODO comment is written in Chinese. To maintain consistency and accessibility for all contributors, please translate it into English.
Apply this diff to translate the comment:
-// TODO 在这里把json转为ApolloConfig, 但ReleaseKey字段会丢失, 影响大不大 +// TODO: Convert JSON to ApolloConfig here, but note that the ReleaseKey field might be lost. Assess the impact.
44-44
: Improve function comment to follow GoDoc conventionsThe comment for
WriteConfigMap
should begin with the function name and be a complete sentence to adhere to GoDoc standards.Apply this diff to update the comment:
-// WriteConfigMap write apollo config to configmap +// WriteConfigMap writes ApolloConfig to a ConfigMap.
46-46
: Translate the code comment to English for consistencyThe comment is currently in Chinese. Translating it to English will ensure that all team members can understand it.
Apply this diff to translate the comment:
-// AppId作为configMap的name,cluster-namespace作为key, 配置信息的JSON作为value +// Use AppID as the ConfigMap name, cluster-namespace as the key, and the configuration JSON as the value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- store/configmap/manager.go (1 hunks)
- store/configmap/manager_test.go (1 hunks)
- store/configmap/store.go (1 hunks)
- store/configmap/store_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- store/configmap/manager_test.go
🧰 Additional context used
🔇 Additional comments (3)
store/configmap/store_test.go (1)
1-48
: LGTM: Appropriate package, imports, and constants.The package name, imports, and constants are well-defined and relevant for the test file. Good job on using constants for reusable test values.
store/configmap/manager.go (2)
46-71
: LGTM: Robust singleton implementation with proper error handlingThe
GetK8sManager
function implements a thread-safe singleton pattern usingsync.Once
. It correctly handles errors by resetting the instance andsync.Once
on failure, allowing for potential retry attempts. The function now returns an error, which is a good practice for error propagation.
35-39
: 🛠️ Refactor suggestionConsider implementing optimistic concurrency control
The TODO comment suggests using CAS (Compare-And-Swap) with version numbers for better concurrency control. This is a good suggestion for production use to handle concurrent updates to the ConfigMap.
Also, consider translating the Chinese comment to English for consistency:
- mutex sync.RWMutex // 添加读写锁 + mutex sync.RWMutex // Add read-write lockTo verify the current implementation and its potential issues with concurrent updates, you can run the following script:
store/configmap/manager.go
Outdated
func (m *K8sManager) SetConfigMap(configMapName string, configMapNamespace string, key string, config *config.ApolloConfig) error { | ||
m.mutex.Lock() // 加锁 | ||
defer m.mutex.Unlock() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
// 将ApolloConfig的配置信息数据转换为JSON字符串 | ||
jsonData, err := json.Marshal(config.Configurations) | ||
if err != nil { | ||
return fmt.Errorf("error marshaling data to JSON: %v", err) | ||
} | ||
|
||
jsonString := string(jsonData) | ||
log.Infof("准备configmap内容,json:%s", jsonString) | ||
// 尝试获取 ConfigMap,如果不存在则创建 | ||
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | ||
if errors.IsNotFound(err) { | ||
cm = &coreV1.ConfigMap{ | ||
ObjectMeta: metaV1.ObjectMeta{ | ||
Name: configMapName, | ||
Namespace: configMapNamespace, | ||
}, | ||
Data: map[string]string{ | ||
key: jsonString, | ||
}, | ||
} | ||
|
||
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Create(ctx, cm, metaV1.CreateOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("error creating ConfigMap: %v", err) | ||
} | ||
log.Infof("ConfigMap %s created in namespace %s", configMapName, configMapNamespace) | ||
} else if err != nil { | ||
return fmt.Errorf("error getting ConfigMap: %v", err) | ||
} else { | ||
// ConfigMap 存在,更新数据 | ||
cm.Data[key] = jsonString | ||
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Update(ctx, cm, metaV1.UpdateOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("error updating ConfigMap: %v", err) | ||
} | ||
log.Infof("ConfigMap %s updated in namespace %s", configMapName, configMapNamespace) | ||
} | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize SetConfigMap for performance and robustness
While the implementation is generally correct, consider the following improvements:
-
Refine mutex usage:
The current implementation holds the mutex for the entire duration of the method, including I/O operations. Consider narrowing the scope of the lock to protect only the critical sections. -
Handle non-UTF-8 data:
UseBinaryData
for non-UTF-8 data to ensure all types of data can be stored correctly. -
Translate log message:
Translate the Chinese log message to English for consistency. -
Simplify error handling:
Thereturn err
at the end of the function is unnecessary as all error cases are already handled.
Here's a suggested refactoring:
func (m *K8sManager) SetConfigMap(configMapName string, configMapNamespace string, key string, config *config.ApolloConfig) error {
jsonData, err := json.Marshal(config.Configurations)
if err != nil {
return fmt.Errorf("error marshaling data to JSON: %v", err)
}
log.Infof("Preparing ConfigMap content, JSON: %s", string(jsonData))
m.mutex.Lock()
defer m.mutex.Unlock()
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{})
if errors.IsNotFound(err) {
cm = &coreV1.ConfigMap{
ObjectMeta: metaV1.ObjectMeta{
Name: configMapName,
Namespace: configMapNamespace,
},
Data: map[string]string{},
BinaryData: map[string][]byte{},
}
if utf8.Valid(jsonData) {
cm.Data[key] = string(jsonData)
} else {
cm.BinaryData[key] = jsonData
}
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Create(ctx, cm, metaV1.CreateOptions{})
if err != nil {
return fmt.Errorf("error creating ConfigMap: %v", err)
}
log.Infof("ConfigMap %s created in namespace %s", configMapName, configMapNamespace)
} else if err != nil {
return fmt.Errorf("error getting ConfigMap: %v", err)
} else {
if utf8.Valid(jsonData) {
cm.Data[key] = string(jsonData)
} else {
cm.BinaryData[key] = jsonData
}
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Update(ctx, cm, metaV1.UpdateOptions{})
if err != nil {
return fmt.Errorf("error updating ConfigMap: %v", err)
}
log.Infof("ConfigMap %s updated in namespace %s", configMapName, configMapNamespace)
}
return nil
}
This refactoring addresses all points mentioned above.
store/configmap/manager.go
Outdated
func (m *K8sManager) GetConfigMap(configMapName string, configMapNamespace string, key string) (map[string]interface{}, error) { | ||
m.mutex.RLock() // 加读锁 | ||
defer m.mutex.RUnlock() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
configMap, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | ||
if err != nil { | ||
return nil, fmt.Errorf("error getting ConfigMap: %v", err) | ||
} | ||
|
||
//log.Info("读取成功。,configmap的内容:", configMap.Data[key]) | ||
// 从ConfigMap中读取JSON数据 | ||
jsonData, ok := configMap.Data[key] | ||
if !ok { | ||
return nil, fmt.Errorf("key: %v not found in ConfigMap", key) | ||
} | ||
|
||
// 反序列化JSON配置信息到ApolloConfig的Configurations字段 | ||
// 可以考虑使用 configTemp := &config.ApolloConfig{} 接收 | ||
var configurations map[string]interface{} | ||
err = json.Unmarshal([]byte(jsonData), &configurations) | ||
if err != nil { | ||
return nil, fmt.Errorf("error unmarshaling JSON to map[string]interface{}: %v", err) | ||
} | ||
|
||
return configurations, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance GetConfigMap to handle BinaryData and improve code cleanliness
Consider the following improvements:
-
Handle BinaryData:
The current implementation only checks theData
map. Add support forBinaryData
to ensure all types of stored data can be retrieved. -
Remove or uncomment the log message:
Either remove the commented-out log message or uncomment it if it's needed for debugging.
Here's a suggested improvement:
func (m *K8sManager) GetConfigMap(configMapName string, configMapNamespace string, key string) (map[string]interface{}, error) {
m.mutex.RLock()
defer m.mutex.RUnlock()
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
configMap, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{})
if err != nil {
return nil, fmt.Errorf("error getting ConfigMap: %v", err)
}
var jsonData []byte
if data, ok := configMap.Data[key]; ok {
jsonData = []byte(data)
} else if binaryData, ok := configMap.BinaryData[key]; ok {
jsonData = binaryData
} else {
return nil, fmt.Errorf("key: %v not found in ConfigMap", key)
}
var configurations map[string]interface{}
err = json.Unmarshal(jsonData, &configurations)
if err != nil {
return nil, fmt.Errorf("error unmarshaling JSON to map[string]interface{}: %v", err)
}
return configurations, nil
}
This change ensures that the method can handle data stored in both Data
and BinaryData
fields of the ConfigMap.
store/configmap/store.go
Outdated
apolloConfig.Configurations, err = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) | ||
|
||
apolloConfig.AppID = appConfig.AppID | ||
apolloConfig.Cluster = appConfig.Cluster | ||
apolloConfig.NamespaceName = appConfig.NamespaceName | ||
return apolloConfig, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle errors before populating ApolloConfig
fields
If GetConfigMap
returns an error, populating apolloConfig
fields may lead to inconsistent state. Consider returning early when an error occurs to ensure reliability.
Apply this diff to handle the error:
apolloConfig.Configurations, err = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key)
+if err != nil {
+ return nil, err
+}
apolloConfig.AppID = appConfig.AppID
apolloConfig.Cluster = appConfig.Cluster
apolloConfig.NamespaceName = appConfig.NamespaceName
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
apolloConfig.Configurations, err = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) | |
apolloConfig.AppID = appConfig.AppID | |
apolloConfig.Cluster = appConfig.Cluster | |
apolloConfig.NamespaceName = appConfig.NamespaceName | |
return apolloConfig, err | |
apolloConfig.Configurations, err = c.K8sManager.GetConfigMap(configMapName, configMapNamespace, key) | |
if err != nil { | |
return nil, err | |
} | |
apolloConfig.AppID = appConfig.AppID | |
apolloConfig.Cluster = appConfig.Cluster | |
apolloConfig.NamespaceName = appConfig.NamespaceName | |
return apolloConfig, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
store/configmap/manager.go (2)
46-71
: LGTM! Consider consolidating error handlingThe implementation of the singleton pattern and error handling is well done. However, you could simplify the error handling by moving the common error handling logic to a separate function:
func resetInstanceOnError(err error, errMsg string) { instance = nil once = sync.Once{} log.Errorf("%s: %v", errMsg, err) } func GetK8sManager() (*K8sManager, error) { once.Do(func() { inClusterConfig, err := rest.InClusterConfig() if err != nil { resetInstanceOnError(err, "Error creating in-cluster config") return } clientSet, err := kubernetes.NewForConfig(inClusterConfig) if err != nil { resetInstanceOnError(err, "Error creating Kubernetes client set") return } instance = &K8sManager{ clientSet: clientSet, mutex: sync.RWMutex{}, } }) if instance == nil { return nil, fmt.Errorf("failed to create K8sManager instance") } return instance, nil }This refactoring reduces code duplication and improves maintainability.
1-146
: Overall good implementation with room for improvementThe K8sManager implementation is generally well-structured and handles ConfigMap operations effectively. Here are some final suggestions for improvement:
- Ensure all comments and log messages are in English for consistency and accessibility.
- Consider implementing optimistic concurrency control using ResourceVersion as suggested in the TODO comment.
- Add unit tests to verify the behavior of the K8sManager methods, especially error cases and edge conditions.
- Consider adding a method to delete ConfigMaps for completeness.
- Add documentation comments (godoc) for exported types and methods to improve code documentation.
Example of adding godoc:
// K8sManager handles operations on Kubernetes ConfigMaps. type K8sManager struct { // ... } // GetK8sManager returns a singleton instance of K8sManager. // It initializes the Kubernetes client using in-cluster configuration. func GetK8sManager() (*K8sManager, error) { // ... }These improvements will enhance the overall quality, maintainability, and usability of the K8sManager implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- go.mod (1 hunks)
- store/configmap/manager.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
store/configmap/manager.go
Outdated
// TODO 改为CAS更好,用版本号解决api server的并发 https://blog.csdn.net/boling_cavalry/article/details/128745382 | ||
type K8sManager struct { | ||
clientSet kubernetes.Interface | ||
mutex sync.RWMutex // 添加读写锁 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Translate comments to English and consider implementing CAS
- Please translate the TODO comment and the mutex comment to English for consistency and accessibility.
- The TODO suggests implementing Compare-and-Swap (CAS) with version numbers for better concurrency control. Consider implementing this suggestion to improve the robustness of concurrent operations on the ConfigMap.
Would you like assistance in implementing the CAS mechanism or translating the comments?
store/configmap/manager.go
Outdated
// SetConfigMap 将map[string]interface{}转换为JSON字符串,并创建或更新ConfigMap | ||
func (m *K8sManager) SetConfigMap(configMapName string, configMapNamespace string, key string, config *config.ApolloConfig) error { | ||
jsonData, err := json.Marshal(config.Configurations) | ||
jsonString := string(jsonData) | ||
if err != nil { | ||
return fmt.Errorf("error marshaling data to JSON: %v", err) | ||
} | ||
log.Infof("Preparing Configmap content,JSON: %s", jsonString) | ||
|
||
m.mutex.Lock() // 加锁 | ||
defer m.mutex.Unlock() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
// 尝试获取 ConfigMap,如果不存在则创建 | ||
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | ||
if errors.IsNotFound(err) { | ||
cm = &coreV1.ConfigMap{ | ||
ObjectMeta: metaV1.ObjectMeta{ | ||
Name: configMapName, | ||
Namespace: configMapNamespace, | ||
}, | ||
Data: map[string]string{ | ||
key: jsonString, | ||
}, | ||
} | ||
|
||
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Create(ctx, cm, metaV1.CreateOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("error creating ConfigMap: %v", err) | ||
} | ||
log.Infof("ConfigMap %s created in namespace %s", configMapName, configMapNamespace) | ||
} else if err != nil { | ||
return fmt.Errorf("error getting ConfigMap: %v", err) | ||
} else { | ||
// ConfigMap 存在,更新数据 | ||
cm.Data[key] = jsonString | ||
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Update(ctx, cm, metaV1.UpdateOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("error updating ConfigMap: %v", err) | ||
} | ||
log.Infof("ConfigMap %s updated in namespace %s", configMapName, configMapNamespace) | ||
} | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling, logging, and data handling in SetConfigMap
- Remove the unnecessary
return err
at the end of the function, as all error cases are already handled. - Translate the log message to English for consistency.
- Handle potential non-UTF-8 data by using
BinaryData
when appropriate. - Consider using
cm.ObjectMeta.ResourceVersion
for optimistic concurrency control.
Here's a suggested refactoring:
func (m *K8sManager) SetConfigMap(configMapName string, configMapNamespace string, key string, config *config.ApolloConfig) error {
jsonData, err := json.Marshal(config.Configurations)
if err != nil {
return fmt.Errorf("error marshaling data to JSON: %v", err)
}
log.Infof("Preparing ConfigMap content, JSON: %s", string(jsonData))
m.mutex.Lock()
defer m.mutex.Unlock()
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
cm, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{})
if errors.IsNotFound(err) {
cm = &coreV1.ConfigMap{
ObjectMeta: metaV1.ObjectMeta{
Name: configMapName,
Namespace: configMapNamespace,
},
Data: map[string]string{},
BinaryData: map[string][]byte{},
}
} else if err != nil {
return fmt.Errorf("error getting ConfigMap: %v", err)
}
if utf8.Valid(jsonData) {
cm.Data[key] = string(jsonData)
} else {
cm.BinaryData[key] = jsonData
}
if cm.ResourceVersion == "" {
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Create(ctx, cm, metaV1.CreateOptions{})
if err != nil {
return fmt.Errorf("error creating ConfigMap: %v", err)
}
log.Infof("ConfigMap %s created in namespace %s", configMapName, configMapNamespace)
} else {
_, err = m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Update(ctx, cm, metaV1.UpdateOptions{})
if err != nil {
return fmt.Errorf("error updating ConfigMap: %v", err)
}
log.Infof("ConfigMap %s updated in namespace %s", configMapName, configMapNamespace)
}
return nil
}
This refactoring addresses the issues mentioned and improves the overall robustness of the method.
store/configmap/manager.go
Outdated
// GetConfigMap 从ConfigMap中获取JSON字符串,并反序列化为map[string]interface{} | ||
func (m *K8sManager) GetConfigMap(configMapName string, configMapNamespace string, key string) (map[string]interface{}, error) { | ||
m.mutex.RLock() // 加读锁 | ||
defer m.mutex.RUnlock() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
defer cancel() | ||
|
||
configMap, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{}) | ||
if err != nil { | ||
return nil, fmt.Errorf("error getting ConfigMap: %v", err) | ||
} | ||
|
||
// 从ConfigMap中读取JSON数据 | ||
jsonData, ok := configMap.Data[key] | ||
if !ok { | ||
return nil, fmt.Errorf("key: %v not found in ConfigMap", key) | ||
} | ||
|
||
var configurations map[string]interface{} | ||
err = json.Unmarshal([]byte(jsonData), &configurations) | ||
if err != nil { | ||
return nil, fmt.Errorf("error unmarshaling JSON to map[string]interface{}: %v", err) | ||
} | ||
|
||
return configurations, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance GetConfigMap to handle BinaryData and improve error handling
Consider the following improvements:
- Handle potential BinaryData in the ConfigMap.
- Provide more specific error messages.
Here's a suggested refactoring:
func (m *K8sManager) GetConfigMap(configMapName string, configMapNamespace string, key string) (map[string]interface{}, error) {
m.mutex.RLock()
defer m.mutex.RUnlock()
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
configMap, err := m.clientSet.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metaV1.GetOptions{})
if err != nil {
if errors.IsNotFound(err) {
return nil, fmt.Errorf("ConfigMap %s not found in namespace %s: %v", configMapName, configMapNamespace, err)
}
return nil, fmt.Errorf("error getting ConfigMap %s in namespace %s: %v", configMapName, configMapNamespace, err)
}
var jsonData []byte
if data, ok := configMap.Data[key]; ok {
jsonData = []byte(data)
} else if binaryData, ok := configMap.BinaryData[key]; ok {
jsonData = binaryData
} else {
return nil, fmt.Errorf("key %s not found in ConfigMap %s in namespace %s", key, configMapName, configMapNamespace)
}
var configurations map[string]interface{}
if err := json.Unmarshal(jsonData, &configurations); err != nil {
return nil, fmt.Errorf("error unmarshaling JSON for key %s in ConfigMap %s in namespace %s: %v", key, configMapName, configMapNamespace, err)
}
return configurations, nil
}
This refactoring handles both Data and BinaryData in the ConfigMap and provides more specific error messages, improving the robustness and debuggability of the method.
这个 PR 发生了冲突. @dyx1234 是否能帮忙修复呢? 🙏 |
client.go
Outdated
@@ -114,6 +115,15 @@ func StartWithConfig(loadAppConfig func() (*config.AppConfig, error)) (Client, e | |||
return nil, err | |||
} | |||
|
|||
// set k8sManager before first sync | |||
if appConfig.IsBackupConfigToConfigMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么会会修改 client?我理解整体的框架应该是没有变化。
只是提供一个新的插件,通过文档告诉用户有这个插件可以选择。
我理解只需要按下方实现方式实现即可:
https://github.sheincorp.cn/zouyx/agollo_demo/tree/master/custom/file
如果是现在这种改法,可能会在用户升级的时候出现不兼容状态。
出现这种状态,可能会导致用户出现测试成本增加,或者生产故障。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我最初是在client文件的init里,作为插件初始化的。
但是那个位置无法获取用户的配置信息(*config.AppConfig)来判断是否初始化这个插件。如果盲目初始化插件会调用GetK8sManager方法,连接不到k8s的话会直接报错。所以挪到StartWithConfig这个方法里来了。
这样会有什么问题吗,如果用户没设置IsBackupConfigToConfigMap,就不进行这个插件的初始化
client.go
Outdated
@@ -49,7 +49,7 @@ const separator = "," | |||
func init() { | |||
extension.SetCacheFactory(&memory.DefaultCacheFactory{}) | |||
extension.SetLoadBalance(&roundrobin.RoundRobin{}) | |||
extension.SetFileHandler(&jsonFile.FileHandler{}) | |||
extension.AddFileHandler(&jsonFile.FileHandler{}, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么定义优先级为 2 ?其中意义是?
extension/file.go
Outdated
return fileHandler | ||
// GetFileHandlers 按优先级排序并返回所有的 FileHandler(priority 值越大,优先级越高) | ||
func GetFileHandlers() []file.FileHandler { | ||
sort.Slice(handlers, func(i, j int) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果优先级不能修改,为什么不是在 add 的时候排序好,而是在 get?
component/remote/async.go
Outdated
break | ||
} | ||
c, err = handler.LoadConfigFile(appConfig.BackupConfigPath, appConfig.AppID, namespace, cluster) | ||
if err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果 err 为空,但是读取的配置文件对象(c) 为空,需要继续尝试下一个 handler 吗?
按照之前同步的思想 maybe 是要的?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是要的,疏忽了。已优化这个循环的逻辑
configmap/manager.go
Outdated
inClusterConfig, err := rest.InClusterConfig() | ||
if err != nil { | ||
instance = nil | ||
once = sync.Once{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个方法在使用中实际会被多次调用吗?如果不会的话,这个方式是不是不会有效果?
同时,好像没有 test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
正常情况是只被调用一次。但用户使用方式错误的话可能会调用多次。有必要预防吗🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我理解这个初始化 k8s 对象,只会在初始化 agollo 这个 client 的时候用到。
- 从动作层面上分析:初始化这个动作只会是一次,而不会是多次。
- 从初始化失败的处理方式上分析:要不就是重启,要不就是改代码。
所以我理解这里应该不需要预防?
configmap/manager.go
Outdated
inClusterConfig, err := rest.InClusterConfig() | ||
if err != nil { | ||
instance = nil | ||
once = sync.Once{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我理解这个初始化 k8s 对象,只会在初始化 agollo 这个 client 的时候用到。
- 从动作层面上分析:初始化这个动作只会是一次,而不会是多次。
- 从初始化失败的处理方式上分析:要不就是重启,要不就是改代码。
所以我理解这里应该不需要预防?
extension/file.go
Outdated
var handlers = list.New() | ||
|
||
// AddFileHandler 添加一个 FileHandler 实现,并设定其优先级 | ||
func AddFileHandler(handler file.FileHandler, priority ...int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个 priority 为什么是个数组?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
一开始想着用户只设置一个实现的话,设为变长,用户可以不传。但不太好管理,我把它改回普通参数
if file != nil { | ||
extension.SetFileHandler(file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个动作不能变化,原来是 set 是替换的意思。
现在如果变成了 add 是增加的意思,只能通过增加方法的形式。
configmap/manager.go
Outdated
return instance, nil | ||
} | ||
|
||
inClusterConfig, err := rest.InClusterConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个还是得加once做单例防并发呀
extension/file.go
Outdated
// 添加新的 handler | ||
newHandler := HandlerWithPriority{ | ||
Handler: handler, | ||
priority: 10, // 默认优先级 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个搞成常量,在设置默认值的时候,引用对应的常量
start.go
Outdated
* extension.AddBackupFileHandler(myFileHandler, 11) | ||
* extension.AddConfigMapHandler(configMapHandler, 9) | ||
*/ | ||
func AddConfigMapHandler(configMap file.FileHandler, priority int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么会加两个一样的方法?意义是什么?
What's the purpose of this PR
解决Apollo客户端在Kubernetes环境下因服务端宕机或Pod重启导致配置信息文件丢失的问题。通过使用Kubernetes ConfigMap作为新的持久化存储方案,提高配置信息的可靠性和容错性。
Solve the problem of Apollo client configuration information files being lost due to server downtime or Pod restart in the Kubernetes environment. By using Kubernetes ConfigMap as a new persistent storage solution, the reliability and fault tolerance of configuration information are improved.
discussion apolloconfig/apollo#5210
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests