Refactor GCEClient: wrap ValidConfigs in an interface so that unit te… #130

spew · 2018-05-03T15:58:22Z

…sts using machineactuator.go can inject their Configs instead of having a dependency on machined_setup_configs.yaml

What this PR does / why we need it:
This PR enables unit tests to control the MachineSetupConfig used by machineactuator.go without needing to write out custom files. In addition, it was previously possible to pass in an invalid "configListPath" to machineactuator.go. When doing so, the machineactuator.go would encounter nil pointer errors when calling methods that required a valid SetupConfig. The Create(...) method has been updated to require it.

Note that there is some similar code in both,

gce-machine-controller/main.go
deploy/deploy.go

In one we are using glog.fatalF, in the other glog.Exit, this is done because it is matching the convention currently in use in that file. If this is something that should be reconciled and standardized happy to follow up in another commit.

Release note:

NONE

@kubernetes/kube-deploy-reviewers

k8s-ci-robot · 2018-05-03T15:58:37Z

Hi @spew. Thanks for your PR.

I'm waiting for a kubernetes or kubernetes-sigs 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.

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.

krousey · 2018-05-03T20:28:42Z

cloud/google/cmd/gce-machine-controller/main.go

+	if err != nil {
+		glog.Fatalf("Could not create config watch: %v", err)
+	}
+	machineSetupConfigs, err := configWatch.ValidConfigs()


This cannot be done here. It needs to be done on every time a machine is created. The file's contents are allowed to change during runtime and needs to be re-parsed.

Got it, I was not understanding the "on-cluster" context in which this code runs (inside of gce-machine-controller). I will update with a version that encapsulates the refreshing logic.

spew · 2018-05-03T21:19:42Z

Posted an update where the config methods (GetYaml, GetImage, GetMetadata) have been changed to encapsulate loading the config. This will enable consumers of the ConfigWatch to gain access to those methods without knowing about the reload functionality. And it will allow for tests to mock those methods.

kcoronado

Just one comment, otherwise lgtm

kcoronado · 2018-05-04T16:16:22Z

cloud/google/machineactuator.go

@@ -186,6 +177,9 @@ func (gce *GCEClient) CreateMachineController(cluster *clusterv1.Cluster, initia
 }

 func (gce *GCEClient) Create(cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
+	if gce.machineSetupConfig == nil {


Do we need this nil check? There's a check in the gcp-deployer create command to ensure the given filepath isn't empty. There isn't one in gce-machine-controller's main method, but I think maybe the check should be there instead of here, unless there's a reason why we should be able to create a machine controller without the machine setup config.

You are correct that it would be simpler and we would not need the nil check if we required the machineSetupConfig to be a non-nil pointer in the NewMachineActuator(...) method. However, there are commands, such as Delete, that do not need the machineSetupConfig and do not have a valid configListPath and would pass in nil.

note that there are pluses and minuses to making it required (simplifies machineactuator.go code) vs making it optional (simplifies usage of machineactuator.go).

jessicaochen · 2018-05-04T18:03:20Z

cloud/google/cmd/gce-machine-controller/main.go

@@ -68,3 +74,10 @@ func main() {
 	c.RunAsync(shutdown)
 	select {}
 }
+
+func newConfigWatchOrNil() (*machinesetup.ConfigWatch, error) {


This main.go is for the pod machine controller. It should always have a non-empty path passed in.

Empty path is only for deployer tool use of machine actuator.

Got it, i will remove the check for the empty path since that is an invalid state and we would prefer an error.

jessicaochen · 2018-05-04T18:06:21Z

cloud/google/machineactuator.go

-		return err
-	}
-	yaml, err := machineSetupConfigs.GetYaml()
+	yaml, err := gce.machineSetupConfig.GetYaml()


Do we need a nil check before here too since we are trying to use the machineSetupConfig?

Yes we do, I will update this method. Also, I'll do a search and make sure I didn't miss any other locations. Thank you for noticing.

spew · 2018-05-04T18:55:44Z

Updated with the changes requested by @jessicaochen

jessicaochen

lgtm (leaving backslashing to krousey)

Remember to bump relevant image eg. gce-machine-controller after merging.

spew · 2018-05-07T18:38:32Z

Thanks, note @krousey this PR is now pending your LGTM.

krousey · 2018-05-07T20:48:58Z

cloud/google/machineactuator.go

 	if err != nil {
 		return err
 	}
 	imagePath := gce.getImagePath(image)

-	machineSetupMetadata, err := machineSetupConfigs.GetMetadata(configParams)
+	machineSetupMetadata, err := gce.machineSetupConfig.GetMetadata(configParams)


What guarantees do we have that the configuration has not updated between the time that we retrieved the image, and when we retrieved the metadata? Before there was a guarantee that we were operating on the same data for this create.

This is a problem and I'll need to make changes to avoid this from happening.

…sts using machineactuator.go can inject their Configs instead of having a dependency on machined_setup_configs.yaml

spew · 2018-05-08T21:24:01Z

Updated again with another level of interface indirection. The reason why I chose to take this route is it is difficult to control the behavior of the "ValidConfigs" object in config_types.go because it has all private fields.

However, we may want to go with that approach and I can do so -- it will end up with a bit more test cruft in the machinesetup package as the fake / test / mocked implementations sorta need to be in the machiensetup package.

krousey

Probably the least terrible option.

krousey · 2018-05-08T22:48:28Z

/lgtm

krousey · 2018-05-08T22:48:38Z

/approve

k8s-ci-robot · 2018-05-08T22:48:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krousey, spew

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [krousey]

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

medinatiger · 2018-05-09T17:39:53Z

/ok-to-test

Ignore invalid samples

Set the args in the manager_auth_proxy_patch.yaml

Resolves kubernetes-sigs#125 Change-Id: I270c2678dd2d9ee4984cf1d61e7c629997ea1da0

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 3, 2018

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 May 3, 2018

spew requested review from kcoronado, jessicaochen and krousey May 3, 2018 15:58

spew mentioned this pull request May 3, 2018

Add the ability to configure disk size and type for GCP clusters (Kube Deploy 638) #77

Merged

spew assigned krousey, kcoronado and jessicaochen May 3, 2018

krousey suggested changes May 3, 2018

View reviewed changes

spew force-pushed the gceclient-machine-setup-config branch from ade6992 to 419e96f Compare May 3, 2018 21:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 3, 2018

kcoronado reviewed May 4, 2018

View reviewed changes

jessicaochen suggested changes May 4, 2018

View reviewed changes

spew force-pushed the gceclient-machine-setup-config branch from 419e96f to c6d2dec Compare May 4, 2018 18:55

jessicaochen approved these changes May 7, 2018

View reviewed changes

krousey reviewed May 7, 2018

View reviewed changes

Refactor GCEClient: wrap ValidConfigs in an interface so that unit te…

f3e608f

…sts using machineactuator.go can inject their Configs instead of having a dependency on machined_setup_configs.yaml

spew force-pushed the gceclient-machine-setup-config branch from c6d2dec to f3e608f Compare May 8, 2018 21:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2018

krousey approved these changes May 8, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 9, 2018

k8s-ci-robot merged commit 7bad547 into kubernetes-sigs:master May 9, 2018

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#130 from chuckha/samples

cf8b32f

Ignore invalid samples

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#130 from chuckha/manager-args

2b1abc1

Set the args in the manager_auth_proxy_patch.yaml

jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020

Clean up the code post terraform provider removal (kubernetes-sigs#130)

68625c4

Resolves kubernetes-sigs#125 Change-Id: I270c2678dd2d9ee4984cf1d61e7c629997ea1da0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GCEClient: wrap ValidConfigs in an interface so that unit te… #130

Refactor GCEClient: wrap ValidConfigs in an interface so that unit te… #130

spew commented May 3, 2018 •

edited

Loading

k8s-ci-robot commented May 3, 2018

krousey May 3, 2018

spew May 3, 2018

spew commented May 3, 2018

kcoronado left a comment

kcoronado May 4, 2018

spew May 4, 2018

jessicaochen May 4, 2018

spew May 4, 2018

jessicaochen May 4, 2018

spew May 4, 2018

spew commented May 4, 2018

jessicaochen left a comment

spew commented May 7, 2018

krousey May 7, 2018

spew May 8, 2018

spew commented May 8, 2018

krousey left a comment

krousey commented May 8, 2018

krousey commented May 8, 2018

k8s-ci-robot commented May 8, 2018

medinatiger commented May 9, 2018

Refactor GCEClient: wrap ValidConfigs in an interface so that unit te… #130

Refactor GCEClient: wrap ValidConfigs in an interface so that unit te… #130

Conversation

spew commented May 3, 2018 • edited Loading

k8s-ci-robot commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spew commented May 3, 2018

kcoronado 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spew commented May 4, 2018

jessicaochen left a comment

Choose a reason for hiding this comment

spew commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spew commented May 8, 2018

krousey left a comment

Choose a reason for hiding this comment

krousey commented May 8, 2018

krousey commented May 8, 2018

k8s-ci-robot commented May 8, 2018

medinatiger commented May 9, 2018

spew commented May 3, 2018 •

edited

Loading