add configmap based config #928

cadmuxe · 2019-11-05T21:26:12Z

No description provided.

k8s-ci-robot · 2019-11-05T21:26:20Z

Hi @cadmuxe. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-11-05T21:26:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cadmuxe
To complete the pull request process, please assign freehan
You can assign the PR to them by writing /assign @freehan in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/cmconfig/controller_test.go

cadmuxe · 2019-11-05T21:28:05Z

cmd/glbc/main.go

 		time.Sleep(30 * time.Second)
+		return


Should we add IsStopped functions to all the controllers and do a check in here?

cadmuxe · 2019-11-05T21:28:51Z

/cc @freehan

freehan

just some, will continue

freehan · 2019-11-05T21:39:24Z

pkg/cmconfig/config.go

+// Config holds configmap based configurations.
+type Config struct {
+	EnableASM                   bool
+	ASMServiceNEGSkipNamespaces string // should be common seperated namespaces.


Use list of strings?

freehan · 2019-11-05T21:40:58Z

cmd/glbc/main.go

 	stopCh := make(chan struct{})
+	cmConfig := cmConifgController.GetConfig()
+	if cmConifgController.IsConfigMapConfigModeOn() {


need to flag gate this behavior. Let me think about it.

I added the flag gate to the controller. if it's enabled, it will try to load the configmap and watch it.
if disabled, it will just return the default config structure.
The idea is, even this feature is disabled, we want to get the default config. and I don't want to put the get default config out from the controller.

freehan · 2019-11-07T19:04:08Z

/ok-to-test

cmd/glbc/main.go

freehan · 2019-11-07T19:17:48Z

cmd/glbc/main.go

 	lbc := controller.NewLoadBalancerController(ctx, stopCh)

 	fwc := firewalls.NewFirewallController(ctx, flags.F.NodePortRanges.Values())

+	if ctx.EnableASMConfigMapConfig {


move to below ctx.Init()

I moved it to the below of lbc := controller.NewLoadBalancerController(ctx, stopCh)
Because the RegisterInformer need to call lbc.Stop.

pkg/context/context.go

pkg/cmconfig/controller.go

freehan · 2019-11-07T19:24:43Z

pkg/context/context.go

+		if cmConfig.EnableASM {
+			dynamicClient, err := dynamic.NewForConfig(ctx.KubeConfig)
+			if err != nil {
+				klog.Fatalf("Failed to create kubernetes dynamic client: %v", err)


throw an event to the Config Map

pkg/context/context.go

cmd/glbc/main.go

pkg/cmconfig/controller.go

freehan · 2019-11-07T22:59:17Z

pkg/cmconfig/controller.go

+}
+
+// GetConfig returns the internal Config
+func (c *ConfigMapConfigController) GetConfig() Config {


I would recommend this return (Config, boolean)

It the controller was created with NewConfigMapConfigController, the c.currentConfig would never be null. Which means, if it's null, we probably want the program fail directly.

Imaging we change this to (Config, boolean), and in the case that it return (config, false).
What should the caller to doe?

fail the program [ same as now]

make a new Config which use the default value. [ I want to put the make default logic in the NewConfigMapConfigController function but not anywhere else. ]

freehan · 2019-11-08T18:31:28Z

cmd/glbc/main.go

 	go app.RunHTTPServer(ctx.HealthCheck)

 	if !flags.F.LeaderElection.LeaderElect {
 		runControllers(ctx)
 		return
 	}

+	leaderElectionCtx, leaderElectionCancel := context.WithCancel(context.Background())
 	electionConfig, err := makeLeaderElectionConfig(leaderElectKubeClient, ctx.Recorder(flags.F.LeaderElection.LockObjectNamespace), func() {
 		runControllers(ctx)


@bowei suggested exiting the program directly. after runController exits. That is more reliable.

https://gobyexample.com/exit

yeah, agree.

pkg/cmconfig/controller.go

freehan · 2019-11-09T01:07:35Z

pkg/cmconfig/config.go

+}
+
+// LoadValue loads configs from a map, it will ignore any unknow/unvalid field.
+func (c *Config) LoadValue(m map[string]string, recordWarning func(msg string)) {


I think this function may be too complex.

Check out this one, it is very simple and straightforward.
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/gce/gce_clusterid.go#L234

agree, the initial idea was keeping this generic.
Updated.

docs/deploy/resources/configmap-based-config.yaml

freehan · 2019-11-09T01:09:37Z

pkg/cmconfig/controller.go

+}
+
+// GetConfig returns the internal Config
+func (c *ConfigMapConfigController) GetConfig() Config {


pkg/cmconfig/controller.go

pkg/cmconfig/config.go

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 5, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 5, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2019

k8s-ci-robot requested review from bowei and thockin November 5, 2019 21:26

cadmuxe commented Nov 5, 2019

View reviewed changes

k8s-ci-robot requested a review from freehan November 5, 2019 21:28

freehan reviewed Nov 5, 2019

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 6, 2019

cadmuxe marked this pull request as ready for review November 7, 2019 01:06

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2019

add configmap based config

e6e9522

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 7, 2019

freehan reviewed Nov 7, 2019

View reviewed changes

cmd/glbc/main.go Outdated Show resolved Hide resolved

freehan reviewed Nov 7, 2019

View reviewed changes

freehan self-assigned this Nov 8, 2019

freehan reviewed Nov 8, 2019

View reviewed changes

address code review

5eaf60d

freehan reviewed Nov 9, 2019

View reviewed changes

updates for code review

b55280f

freehan reviewed Nov 11, 2019

View reviewed changes

pkg/cmconfig/controller.go Show resolved Hide resolved

pkg/cmconfig/config.go Show resolved Hide resolved

pkg/cmconfig/config.go Show resolved Hide resolved

pkg/cmconfig/config.go Show resolved Hide resolved

cadmuxe closed this Nov 11, 2019

cadmuxe mentioned this pull request Nov 12, 2019

add configmap based config #935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add configmap based config #928

add configmap based config #928

cadmuxe commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

cadmuxe Nov 5, 2019

cadmuxe commented Nov 5, 2019

freehan left a comment

freehan Nov 5, 2019

freehan Nov 5, 2019

cadmuxe Nov 5, 2019

freehan commented Nov 7, 2019

freehan Nov 7, 2019

cadmuxe Nov 8, 2019

freehan Nov 7, 2019

freehan Nov 7, 2019

cadmuxe Nov 8, 2019

freehan Nov 9, 2019

freehan Nov 8, 2019

cadmuxe Nov 8, 2019

freehan Nov 9, 2019

cadmuxe Nov 11, 2019

freehan Nov 9, 2019

add configmap based config #928

add configmap based config #928

Conversation

cadmuxe commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

Choose a reason for hiding this comment

cadmuxe commented Nov 5, 2019

freehan left a 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

freehan commented Nov 7, 2019

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