Refresh ConfigMap when data change #59

linglingye001 · 2024-07-15T05:05:23Z

No description provided.

RichardChen820 · 2024-07-22T03:21:11Z

internal/controller/appconfigurationprovider_controller.go

@@ -51,9 +51,10 @@ type AzureAppConfigurationProviderReconciler struct {
 type ReconciliationState struct {
 	Generation                              int64
 	ConfigMapResourceVersion                *string
-	SentinelETags                           map[acpv1.Sentinel]*azcore.ETag


I thought the Sentinel Etags is still required to be noted, we still need to check etags of the sentinels, if sentinals are set and they are not changes, we should not refresh the configmap

Yes, if user set sentinels, they would be cached in KeyValueEtags.

RichardChen820 · 2024-07-24T07:31:55Z

internal/controller/processor.go

@@ -288,8 +316,16 @@ func (processor *AppConfigurationProviderProcessor) Finish() (ctrl.Result, error
 		processor.ReconciliationState.ExistingSecretReferences = processor.Settings.SecretReferences
 	}

+	if len(processor.RefreshOptions.updatedKeyValueETags) > 0 {


How abou tusing nil check instead of length check? If not updated, return nil.

RichardChen820 · 2024-07-24T07:41:46Z

internal/loader/settings_client.go

 }

 type SentinelSettingsClient struct {
 	sentinel        acpv1.Sentinel
 	etag            *azcore.ETag
 	refreshInterval string
 }
+type SelectorSettingsClient struct {


nit: add an extra line before.

This one is not addressed

RichardChen820 · 2024-07-24T07:47:08Z

internal/loader/settings_client.go

-	selectors []acpv1.Selector
+type EtagSettingsClient struct {
+	etags       map[acpv1.Selector][]*azcore.ETag
+	etagChanged bool


I don't expect to have this field in EtagSettingsClient, if the etag is not changed, just return nil etgas is fine, if not just return non-nil etags

RichardChen820 · 2024-07-24T07:52:36Z

internal/loader/settings_client.go


 type SettingsClient interface {
-	GetSettings(ctx context.Context, client *azappconfig.Client) ([]azappconfig.Setting, error)
+	GetSettings(ctx context.Context, client *azappconfig.Client) ([]azappconfig.Setting, map[acpv1.Selector][]*azcore.ETag, error)


I don't really like such return types in an interface method, what if we need more returns in the future? Could we create a new type for them?

RichardChen820 · 2024-07-24T07:59:26Z

internal/loader/settings_client.go

@@ -11,26 +11,78 @@ import (

 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/data/azappconfig"
-	"k8s.io/klog/v2"


Why do this change?

RichardChen820 · 2024-07-24T08:05:50Z

internal/loader/configuration_setting_loader.go

@@ -354,6 +331,56 @@ func (csl *ConfigurationSettingLoader) CheckAndRefreshSentinels(
 	return sentinelChanged, refreshedETags, nil
 }

+func (csl *ConfigurationSettingLoader) CheckPageETags(ctx context.Context, provider *acpv1.AzureAppConfigurationProvider, eTags map[acpv1.Selector][]*azcore.ETag) (bool, error) {


provider *acpv1.AzureAppConfigurationProvider parameter is not used

internal/loader/configuration_setting_loader.go

RichardChen820 · 2024-07-24T08:35:28Z

Makefile

-undeploy: ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
-	$(KUSTOMIZE) build config/default | kubectl delete --ignore-not-found=$(ignore-not-found) -f -
-
-.PHONY: helm-generate


Let's keep these two items.

I mean the helm-generate and helm-push

RichardChen820 · 2024-07-24T08:39:38Z

api/v1/azureappconfigurationprovider_types.go

@@ -53,7 +53,7 @@ type AzureAppConfigurationProviderStatus struct {
 // RefreshStatus defines last refresh time of configmap and secret when dynamic feature is enabled
 type RefreshStatus struct {
 	LastKeyVaultReferenceRefreshTime metav1.Time `json:"lastKeyVaultReferenceRefreshTime,omitempty"`
-	LastSentinelBasedRefreshTime     metav1.Time `json:"lastSentinelBasedRefreshTime,omitempty"`
+	LastKeyValueRefreshTime          metav1.Time `json:"lastKeyValueRefreshTime,omitempty"`


Please test the upgrade scenario to see whether the field change in CRD would break upgrade scenario, You can try install 1.3.1 first then upgrade to this version to see if it block anything.

RichardChen820 · 2024-07-24T08:41:23Z

go.mod

@@ -82,6 +82,7 @@ require (
 	gopkg.in/yaml.v3 v3.0.1
 	k8s.io/api v0.30.1
 	k8s.io/apiextensions-apiserver v0.30.1 // indirect
+	k8s.io/klog v1.0.0


Why we need the klog v1?

RichardChen820 · 2024-07-24T08:50:27Z

internal/controller/processor.go

@@ -65,12 +68,15 @@ func (processor *AppConfigurationProviderProcessor) PopulateSettings(existingCon
 }

 func (processor *AppConfigurationProviderProcessor) processFullReconciliation() error {
+	processor.ReconciliationState.KeyValueETags, processor.ReconciliationState.FeatureFlagETags = initializeEtags(processor.Provider)


What is the reason that initializeEtags is required?

RichardChen820 · 2024-07-24T08:55:57Z

internal/loader/settings_client.go

-func (s *SentinelSettingsClient) GetSettings(ctx context.Context, client *azappconfig.Client) ([]azappconfig.Setting, error) {
+func (s *EtagSettingsClient) GetSettings(ctx context.Context, client *azappconfig.Client) ([]azappconfig.Setting, map[acpv1.Selector][]*azcore.ETag, error) {
+	nullString := "\x00"
+	for filter, pageEtags := range s.etags {


Could we check the etags is nil or not? if it's nil, just need to return non-nil etags

Do you mean check s.etags? What scenario will it be nil?

RichardChen820 · 2024-07-25T06:50:19Z

config/crd/bases/azconfig.io_azureappconfigurationproviders.yaml

@@ -345,9 +347,3 @@ spec:
    storage: true
    subresources:
      status: {}
-status:


I don't expect to remove this section

It's auto-generated by make command. What's the influence of removing this section?

RichardChen820 · 2024-07-25T08:05:08Z

internal/loader/configuraiton_setting_loader_test.go

@@ -5,6 +5,7 @@ package loader

 import (
 	acpv1 "azappconfig/provider/api/v1"
+	v1 "azappconfig/provider/api/v1"


Duplicate with line 7

RichardChen820 · 2024-07-25T08:17:03Z

internal/loader/settings_client.go

 	nullString := "\x00"
+	settingsToReturn := make([]azappconfig.Setting, 0)


settings is fine

RichardChen820 · 2024-07-25T08:18:53Z

internal/loader/settings_client.go

 	nullString := "\x00"
+	settingsToReturn := make([]azappconfig.Setting, 0)
+	refreshedEtags := make(map[acpv1.Selector][]*azcore.ETag)


pageEtags is fine

RichardChen820 · 2024-07-25T08:20:43Z

internal/loader/settings_client.go

@@ -72,15 +133,19 @@ func (s *SelectorSettingsClient) GetSettings(ctx context.Context, client *azappc
 				Fields:      azappconfig.AllSettingFields(),
 			}
 			pager := client.NewListSettingsPager(selector, nil)
+			latestEtags := make([]*azcore.ETag, 0)

 			for pager.More() {
 				page, err := pager.NextPage(ctx)
 				if err != nil {
 					return nil, err
 				} else if len(page.Settings) > 0 {


Would this check be problematic? What if it returns an empty setting list?

Yes, it should check if page.Settings == nil, when empty setting returned, pageEtag is not empty.

RichardChen820 · 2024-07-25T08:25:49Z

internal/loader/settings_client.go

+	}
+
+	// no change in the settings, return nil
+	return nil, nil


GetSetting() should never return nil without any error, I would suggest return nil etags and settings rather than nil response.

RichardChen820 · 2024-07-25T08:43:12Z

internal/controller/processor.go

 	// Get the latest key value settings
 	existingConfigMapSettings := &existingConfigMap.Data
 	if processor.Settings.ConfigMapSettings != nil {
 		existingConfigMapSettings = &processor.Settings.ConfigMapSettings
 	}
+
+	if provider.Spec.Configuration.Refresh.Monitoring != nil {


Could we put the line164- line178 right before the line 158, seems don't need to read the existing data if etag checking decides not refresh

* draft * update * update etag at finish stage * resolve conflicts * check etags before loading settings * update test * update * go mod tidy * update * update

linglingye001 marked this pull request as ready for review July 17, 2024 08:08

RichardChen820 reviewed Jul 22, 2024

View reviewed changes

linglingye001 added 4 commits July 22, 2024 16:32

draft

3dd2b7a

update

8715d7e

update etag at finish stage

050889d

resolve conflicts

3143464

linglingye001 force-pushed the uesr/linglingye/pageEtag branch from b67c1b9 to 3143464 Compare July 22, 2024 09:13

RichardChen820 changed the base branch from release/v2/preview to release/v2/stable July 22, 2024 15:12

linglingye001 added 2 commits July 24, 2024 14:13

check etags before loading settings

4d8f376

update test

3cc5a20

RichardChen820 reviewed Jul 24, 2024

View reviewed changes

internal/loader/configuration_setting_loader.go Show resolved Hide resolved

RichardChen820 reviewed Jul 24, 2024

View reviewed changes

RichardChen820 reviewed Jul 25, 2024

View reviewed changes

linglingye001 added 2 commits July 25, 2024 15:03

update

1eefe26

go mod tidy

f99f2e2

RichardChen820 reviewed Jul 25, 2024

View reviewed changes

linglingye001 added 2 commits July 25, 2024 17:17

update

64fa268

update

9f960e5

RichardChen820 approved these changes Jul 29, 2024

View reviewed changes

linglingye001 merged commit f364a5a into release/v2/stable Jul 29, 2024
3 checks passed

linglingye001 deleted the uesr/linglingye/pageEtag branch July 29, 2024 08:13

linglingye001 mentioned this pull request Oct 25, 2024

Feature Flags are loaded randomly in the ConfigMap #75

Open

linglingye001 added a commit that referenced this pull request Nov 8, 2024

Refresh ConfigMap when data change (#59)

4ffc67d

* draft * update * update etag at finish stage * resolve conflicts * check etags before loading settings * update test * update * go mod tidy * update * update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh ConfigMap when data change #59

Refresh ConfigMap when data change #59

linglingye001 commented Jul 15, 2024

RichardChen820 Jul 22, 2024 •

edited

Loading

linglingye001 Jul 22, 2024

RichardChen820 Jul 24, 2024

RichardChen820 Jul 24, 2024

RichardChen820 Jul 25, 2024

RichardChen820 Jul 24, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024

RichardChen820 Jul 24, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024

RichardChen820 Jul 24, 2024

RichardChen820 Jul 24, 2024

RichardChen820 Jul 24, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024

RichardChen820 Jul 24, 2024

linglingye001 Jul 25, 2024

RichardChen820 Jul 25, 2024

linglingye001 Jul 25, 2024

RichardChen820 Jul 25, 2024

RichardChen820 Jul 25, 2024

RichardChen820 Jul 25, 2024

RichardChen820 Jul 25, 2024

linglingye001 Jul 25, 2024

RichardChen820 Jul 25, 2024

RichardChen820 Jul 25, 2024

		nullString := "\x00"
		settingsToReturn := make([]azappconfig.Setting, 0)

Refresh ConfigMap when data change #59

Refresh ConfigMap when data change #59

Conversation

linglingye001 commented Jul 15, 2024

RichardChen820 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

RichardChen820 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jul 22, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024 •

edited

Loading

RichardChen820 Jul 24, 2024 •

edited

Loading