pods: setupConfig: merge config from config store #889

petertseng · 2017-07-17T23:43:58Z

No need to review yet, just letting Travis build this because I'm too lazy to do it locally nor make my own fork do Travis.

Advice requested on the question "When do the pod labels get set? Will they be available when the preparer is installing the pod?"

petertseng · 2017-07-18T00:08:49Z

"When do the pod labels get set? Will they be available when the preparer is installing the pod?"

By the RC. It's fine, they'll be available.

Next question. Given that a pod has some labels L, with which subset of those labels should we query for a matching pod cluster?

Hard code. Doesn't seem sensible. The RC doesn't hard code the labels it applies (Right?)
Config. I guess that works
Take the current labels, subtract pod_id and replication_controller_id, use that. Trying to be clever but require zero config. Brittle if an unexpected label gets added.

petertseng · 2017-07-19T00:20:44Z

Oooookay, I think we now merge the config in on install time. If the code works on the first try.

petertseng

I think we are good to go on this, but I haven't bothered doing watches yet.

Note the TODOs and advise whether something should be done about them!

petertseng · 2017-07-19T23:26:07Z

bin/p2-install-hook/main.go

@@ -53,7 +53,9 @@ func main() {
 	pod := hookFactory.NewHookPod(manifest.ID())

 	// for now use noop verifier in this CLI
-	err = pod.Install(manifest, auth.NopVerifier(), artifact.NewRegistry(*registryURI, uri.DefaultFetcher, osversion.DefaultDetector))
+	// TODO: should this have config store/labeler to merge hook configs?


a TODO I was concerned about, but less so

petertseng · 2017-07-19T23:26:13Z

bin/p2-launch/main.go

@@ -88,7 +88,8 @@ func main() {
 	podFactory := pods.NewFactory(*podRoot, types.NodeName(*nodeName), fetcher, "")
 	pod := podFactory.NewLegacyPod(manifest.ID())

-	err = pod.Install(manifest, auth.NopVerifier(), artifact.NewRegistry(*artifactRegistryURL, fetcher, osversion.DefaultDetector))
+	// TODO: config store/labeler? Otherwise, p2-launch ignores the config store, which doesn't seem ideal.


a TODO I was concerned about, more so

hmmmmmmmmmmmmmmmmmmmmmm yeah we're really hampering p2-launch here because now the manifest isn't all the info you need to run a pod

Passing nil arguments to a function we control is a smell. Can you write a function with a more descriptive name and a more correct arity?

petertseng · 2017-07-19T23:36:21Z

I need #896 for this. I should probably just rebase off that branch...

petertseng · 2017-07-19T23:38:28Z

temporarily cherry picking it in to make sure the build would pass with it, I didn't rebase directly because #896 is itself out of date, but maybe it's not missing any commit I need, so maybe not necessary

petertseng · 2017-07-20T06:23:26Z

Hmm. Install and InstallWithConfig? I wouldn't want to imply that Install installs without config though.

This will allow labeling config values with information that can be used to identify the pod cluster that the config value corresponds to.

Golang's JSON marshal doesn't support arbitrary types as keys, probably because JSON itself does not support this.

Will be useful when we are merging configs

The type of map used by pkg/manifest is map[interface{}]interface{} so it behooves us to match that. The map[string]interface{} change was to support JSON marshalling of config stanzas, which seems less important now than it once did.

This will make it easier to initialize a config store

This will allow callers to switch on the special error type the labels library returns when there are no labels for a type (NoLabelsFound)

We intend to backfill config in which case it will become an error

petertseng · 2017-07-26T02:09:44Z

--- FAIL: TestDeleteConfigTxn (0.44s)
consultest.go:98: starting Consul server with port: 33739
store_test.go:279: Expected to receive an error when fetching deleted configuration. Got: {map[] }
consultest.go:111: stopping Consul server with port: 33739
FAIL
FAIL github.com/square/p2/pkg/store/consul/configstore 0.551s

petertseng · 2017-07-26T02:09:59Z

--- FAIL: TestDeleteConfig (0.00s)
store_test.go:171: Expected to receive an error when fetching deleted configuration. Got: {map[] }

petertseng · 2017-07-26T02:10:41Z

2017/07/26 01:54:55 Bootstrapping complete
2017/07/26 01:54:58 Status of preparer: p2-preparer OK
2017/07/26 01:54:59 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:3000: getsockopt: connection refused"; Reconnecting to {localhost:3000 }
2017/07/26 01:55:00 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:3000: getsockopt: connection refused"; Reconnecting to {localhost:3000 }
2017/07/26 01:55:02 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:3000: getsockopt: connection refused"; Reconnecting to {localhost:3000 }
2017/07/26 01:55:04 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:3000: getsockopt: connection refused"; Reconnecting to {localhost:3000 }
2017/07/26 01:55:08 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:3000: getsockopt: connection refused"; Reconnecting to {localhost:3000 }
label-store-server tail:
2017-07-26_01:55:08.48305 time="2017-07-26T01:55:08Z" level=info msg="Listening tcp on port 3000" Counter=0 PID=11962

petertseng

Hey there. It looks like this is pretty old and we haven't had a need to roll it out widely. As a result I'd like to close it to get it out of the queue.

petertseng · 2018-08-29T18:17:28Z

pkg/store/consul/configstore/store.go

+	"encoding/json"
+	"path"
+
+	"github.com/square/p2/pkg/util" // TODO this is wrong


I no longer remember why this is wrong

petertseng force-pushed the pc-config branch from feab516 to 99ae534 Compare July 18, 2017 00:05

petertseng force-pushed the pc-config branch 5 times, most recently from 851e310 to 61a6eb6 Compare July 19, 2017 00:19

petertseng force-pushed the pc-config branch 2 times, most recently from 8b0d8e5 to 9e40554 Compare July 19, 2017 23:18

petertseng changed the title ~~WIP: merge configs from PC~~ pods: setupConfig: merge config from config store Jul 19, 2017

petertseng commented Jul 19, 2017

View reviewed changes

petertseng force-pushed the pc-config branch from 9e40554 to a040163 Compare July 19, 2017 23:37

petertseng force-pushed the pc-config branch from a040163 to ee42a20 Compare July 19, 2017 23:39

mpuncel approved these changes Jul 19, 2017

View reviewed changes

petertseng force-pushed the pc-config branch from 840cb5b to ddb9570 Compare July 20, 2017 06:22

petertseng force-pushed the pc-config branch 4 times, most recently from 3a8c280 to 56131d4 Compare July 24, 2017 21:58

Sean Sorrell and others added 7 commits July 25, 2017 10:55

Introduce the Config Store and an interface for its ops

2c9e4e0

Add label tree constant for config

392f512

This will allow labeling config values with information that can be used to identify the pod cluster that the config value corresponds to.

WIP: add a test, but not a very good test

b468a0d

implementation and test for putconfig

5a70f02

Versions are numbers, treat them as such

2fc0f65

Implementation for Delete on Config Store

e872123

correct a YAML import

94ada2e

Sean Sorrell and others added 14 commits July 25, 2017 10:55

labels! They are here and they can be searched on.

9f859a7

formatting directives unto the formatting directive throne

0fc47f3

change config's type to string -> interface{}

748b7eb

Golang's JSON marshal doesn't support arbitrary types as keys, probably because JSON itself does not support this.

add transactional versions of PutConfig and DeleteConfig

3f6506b

change ConsulKV interface's Get function to match existing functions

8af148c

add tests for transaction based PutConfigTxn and DeleteConfigTxn

50c3856

manifest: Allow writing any config, not just manifest's

e5018c7

Will be useful when we are merging configs

manifest: Add function to merge two configs

5291cb9

check some missed errors and fix travis build/test

2b44bd4

Effectively revert a change made in square#890

27401d2

The type of map used by pkg/manifest is map[interface{}]interface{} so it behooves us to match that. The map[string]interface{} change was to support JSON marshalling of config stanzas, which seems less important now than it once did.

Use KVCAS operation to check-and-set config

633c3ec

Namespace config keys underneath configs/ tree in consul

1a43aca

Reduce interface size of labels applicator in config store

d6101d1

This will make it easier to initialize a config store

Pass error from GetMatches through in FindWhereLabeled()

4f02763

This will allow callers to switch on the special error type the labels library returns when there are no labels for a type (NoLabelsFound)

mpuncel force-pushed the hackweek branch from dde1255 to 4f02763 Compare July 25, 2017 17:56

Sean Sorrell and others added 3 commits July 25, 2017 11:46

This isn't an error case yet

986f784

We intend to backfill config in which case it will become an error

preparer: pass labeler and configStore to Install -> setupConfig

08f71dc

pods: setupConfig: merge config from config store

5f10bf2

petertseng force-pushed the pc-config branch from 56131d4 to 5f10bf2 Compare July 25, 2017 18:46

mpuncel force-pushed the hackweek branch from 4f02763 to 576d267 Compare August 2, 2017 18:01

petertseng commented Aug 29, 2018

View reviewed changes

petertseng closed this Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pods: setupConfig: merge config from config store #889

pods: setupConfig: merge config from config store #889

petertseng commented Jul 17, 2017

petertseng commented Jul 18, 2017

petertseng commented Jul 19, 2017

petertseng left a comment

petertseng Jul 19, 2017

petertseng Jul 19, 2017

mpuncel Jul 19, 2017

rudle Jul 19, 2017

petertseng commented Jul 19, 2017

petertseng commented Jul 19, 2017

petertseng commented Jul 20, 2017

petertseng commented Jul 26, 2017

petertseng commented Jul 26, 2017

petertseng commented Jul 26, 2017

petertseng left a comment

petertseng Aug 29, 2018

pods: setupConfig: merge config from config store #889

pods: setupConfig: merge config from config store #889

Conversation

petertseng commented Jul 17, 2017

petertseng commented Jul 18, 2017

petertseng commented Jul 19, 2017

petertseng left a comment

Choose a reason for hiding this comment

petertseng Jul 19, 2017

Choose a reason for hiding this comment

petertseng Jul 19, 2017

Choose a reason for hiding this comment

mpuncel Jul 19, 2017

Choose a reason for hiding this comment

rudle Jul 19, 2017

Choose a reason for hiding this comment

petertseng commented Jul 19, 2017

petertseng commented Jul 19, 2017

petertseng commented Jul 20, 2017

petertseng commented Jul 26, 2017

petertseng commented Jul 26, 2017

petertseng commented Jul 26, 2017

petertseng left a comment

Choose a reason for hiding this comment

petertseng Aug 29, 2018

Choose a reason for hiding this comment