Wire cloud-provider to kube-controller-manager #65

ironcladlou · 2018-10-30T22:17:20Z

ironcladlou · 2018-10-30T22:17:26Z

/hold

ironcladlou · 2018-10-30T22:18:08Z

pkg/operator/observe_config_test.go

+	"k8s.io/client-go/rest"
+)
+
+func TestObserveClusterConfig(t *testing.T) {


Test needs refactored to use tables, not going to bother until we're consuming data from the right place

ironcladlou · 2018-10-30T22:18:45Z

pkg/operator/observe_config.go

+	//  data:
+	//   install-config:
+	//     platform:
+	//       aws: {}


This isn't the right place to get cloud provider info from; I'm just using it for testing.

juanvallejo · 2018-10-30T22:42:50Z

pkg/operator/observe_config.go

+	}
+
+	installConfigYaml, ok := clusterConfig.Data["install-config"]
+	if !ok {


check for the length here, in case of an empty string

Not sure it matters given we check for deserialization errors

Not sure it matters given we check for deserialization errors

In preference to an ok. That's fairly standard in the overall kube codebase. Using len is more standard

juanvallejo · 2018-10-30T22:43:40Z

pkg/operator/observe_config.go

+	//       aws: {}
+	cloudProvider := ""
+	platform, ok := installConfig["platform"].(map[string]interface{})
+	if !ok {


check for empty values as well

Does it matter given we check for unmarshaling errors?

Oops, previous comment applies to another line... what do you mean re: empty value here? if the type conversion works I have a map to work with

abhinavdahiya · 2018-10-30T22:59:18Z

pkg/operator/observe_config.go

+	if !ok {
+		return observedConfig, nil
+	}
+	installConfig := map[string]interface{}{}


would like to see if we can vendor installer's type rather than this map[string]interface{}, install-config has Name func on platform see here that can be used to make decisions.

would like to see if we can vendor installer's type rather than this map[string]interface{}, install-config has Name func on platform see here that can be used to make decisions.

Once the type is promoted to openshift/api we can do that, but I don't want to start having different operators depend on each other for types. If you do that, we'll end up wedged as we try to rebase to various levels of kube.

Yeah, this is a pervasive issue that cuts across all my team's operators (and now this one). Not going to solve it with this PR.

deads2k · 2018-10-31T15:27:43Z

pkg/operator/observe_config.go

+// cluster-config-v1 in order to configure kube-controller-manager's cloud
+// provider.
+func observeClusterConfig(kubeClient kubernetes.Interface, clientConfig *rest.Config, observedConfig map[string]interface{}) (map[string]interface{}, error) {
+	clusterConfig, err := kubeClient.CoreV1().ConfigMaps("kube-system").Get("cluster-config-v1", metav1.GetOptions{})


I don't see the namespace scoped informer being plumbed to the control loop to watch for events.

Fixed (I think)

pkg/operator/observe_config.go

deads2k · 2018-10-31T15:30:20Z

pkg/operator/observe_config.go

+	//  extendedArguments:
+	//    cloud-provider:
+	//	  - "name"
+	if len(cloudProvider) > 0 {


if we returned above, is this if still needed or can we do it unconditionally?

ironcladlou · 2018-10-31T21:47:01Z

Just to be totally clear, the current PR is a WIP; I'm inferring cloud provider config from some existing cluster config that happens to be useful but which isn't really designed to be cloud provider config. Before this is mergeable we might need an installer PR to introduce cloud provider stuff to the cluster config and then refactor this code to use the new purposefully introduced cloud provider config.

ironcladlou · 2018-11-02T19:22:54Z

/hold cancel

ironcladlou · 2018-11-02T19:23:13Z

Spoke with @deads2k, decided to use the current cluster config available to use to get started.

deads2k · 2018-11-05T14:51:16Z

/approve
/assign @juanvallejo

ironcladlou · 2018-11-05T14:59:17Z

Squashed

juanvallejo · 2018-11-05T15:41:58Z

pkg/operator/observe_config.go

+		return observedConfig, nil
+	}
+	if err != nil {
+		return observedConfig, err


nit: in the future, might be good to just explicitly return nil here for observedConfig

juanvallejo · 2018-11-05T15:44:01Z

pkg/operator/observe_config.go

+	err = yaml.Unmarshal([]byte(installConfigYaml), &installConfig)
+	if err != nil {
+		glog.Warningf("Unable to parse install-config: %s", err)
+		return observedConfig, nil


any particular reason why we would not return the error here?

juanvallejo · 2018-11-05T15:49:50Z

/lgtm

openshift-ci-robot · 2018-11-05T15:50:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ironcladlou, juanvallejo

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

ironcladlou · 2018-11-05T15:50:32Z

/retest

ironcladlou · 2018-11-05T16:40:58Z

e2e failure is reminiscent of openshift/installer#606

openshift-bot · 2018-11-05T16:58:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

ironcladlou · 2018-11-05T18:12:28Z

/retest

ironcladlou · 2018-11-05T19:10:43Z

This is what I get for rebasing. 😭

/retest

openshift-bot · 2018-11-05T20:59:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

ironcladlou · 2018-11-05T21:57:13Z

/retest

ironcladlou · 2018-11-07T00:13:52Z

From my testing I'm pretty sure this also fixed openshift/installer#530.

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2018

openshift-ci-robot requested review from mfojtik and sttts October 30, 2018 22:17

ironcladlou commented Oct 30, 2018

View reviewed changes

juanvallejo reviewed Oct 30, 2018

View reviewed changes

abhinavdahiya reviewed Oct 30, 2018

View reviewed changes

deads2k reviewed Oct 31, 2018

View reviewed changes

pkg/operator/observe_config.go Show resolved Hide resolved

deads2k reviewed Oct 31, 2018

View reviewed changes

ironcladlou mentioned this pull request Oct 31, 2018

LoadBalancer Services are not provisioned on cloud platforms openshift/installer#577

Closed

openshift-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 Oct 31, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2018

Wire cloud-provider to kube-controller-manager

b96bfb0

ironcladlou force-pushed the cloud-provider-wiring branch from b3886a0 to b96bfb0 Compare November 5, 2018 14:59

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2018

juanvallejo reviewed Nov 5, 2018

View reviewed changes

juanvallejo approved these changes Nov 5, 2018

View reviewed changes

openshift-ci-robot assigned juanvallejo Nov 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2018

openshift-merge-robot merged commit 05509a1 into openshift:master Nov 5, 2018

ironcladlou mentioned this pull request Nov 7, 2018

seeing reported 'Failed to get AWS Cloud Provider' when trying to create pvc openshift/installer#530

Closed

Wire cloud-provider to kube-controller-manager #65

Wire cloud-provider to kube-controller-manager #65

Uh oh!

Conversation

ironcladlou commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ironcladlou commented Oct 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Oct 31, 2018

Uh oh!

ironcladlou commented Nov 2, 2018

Uh oh!

ironcladlou commented Nov 2, 2018

Uh oh!

deads2k commented Nov 5, 2018

Uh oh!

ironcladlou commented Nov 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Nov 5, 2018

Uh oh!

openshift-ci-robot commented Nov 5, 2018

Uh oh!

ironcladlou commented Nov 5, 2018

Uh oh!

ironcladlou commented Nov 5, 2018

Uh oh!

openshift-bot commented Nov 5, 2018

Uh oh!

ironcladlou commented Nov 5, 2018

Uh oh!

ironcladlou commented Nov 5, 2018

Uh oh!

openshift-bot commented Nov 5, 2018

Uh oh!

ironcladlou commented Nov 5, 2018

Uh oh!

ironcladlou commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

ironcladlou commented Oct 30, 2018 •

edited

Loading

deads2k Nov 1, 2018 •

edited

Loading