pass instanceMetadata to updateNodeAddress to reduce api calls #93284

gongguan · 2020-07-21T09:08:22Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Pass instanceMetadata to updateNodeAddress to reduce api calls.

Which issue(s) this PR fixes:
Ref #90652

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

gongguan · 2020-07-21T10:47:09Z

/retest

gongguan · 2020-07-21T10:52:33Z

/assign @andrewsykim @wojtek-t

wojtek-t · 2020-07-21T12:43:07Z

LGTM, but unfortunately we are in code-freeze now and this needs to wait until we start accepting 1.20 PRs.

I would also like @andrewsykim to take a look.

dims · 2020-07-27T10:58:39Z

/uncc
/milestone v1.20

staging/src/k8s.io/legacy-cloud-providers/gce/gce.go

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

gongguan · 2020-07-29T14:01:04Z

/retest

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

gongguan · 2020-07-29T15:00:02Z

/retest

andrewsykim · 2020-09-03T13:28:23Z

Sorry for the delay @gongguan -- I'll review this soon

gongguan · 2020-09-03T13:40:40Z

Sorry for the delay @gongguan -- I'll review this soon

No sorry, there is something wrong I'll fix it now, you can review after ci passed.

gongguan · 2020-09-04T01:43:09Z

/retest

gongguan · 2020-09-04T01:50:28Z

@andrewsykim ready for review now :)

andrewsykim

LGTM, minor comment about error logging

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

gongguan · 2020-09-15T03:10:34Z

/test pull-kubernetes-e2e-kind-ipv6

gongguan · 2020-09-15T03:39:25Z

LGTM, minor comment about error logging

@andrewsykim addressed and rebased, PTAL.

andrewsykim

Sorry, had one more comment

andrewsykim · 2020-09-15T16:23:41Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

-	if err != nil {
-		return nil, err
+	var nodeModifiers []nodeModifier
+	if node.Spec.ProviderID != providerID {


I think this check needs to be updated to

if node.Spec.ProviderID == "" && providerID != "" {

otherwise we might try to set an existing node.Spec.ProviderID to "". What do you think?

Done as your suggestion, it is more legible.

otherwise we might try to set an existing node.Spec.ProviderID to "". What do you think?

No, in line 454(getProviderID function), providerID will be set as node.Spec.ProviderID if node.Spec.ProviderID not nil. That means node.Spec.ProviderID not equal to providerID only occurs when node.Spec.ProviderID not set.

No, in line 454(getProviderID function), providerID will be set as node.Spec.ProviderID if node.Spec.ProviderID not nil. That means node.Spec.ProviderID not equal to providerID only occurs when node.Spec.ProviderID not set.

Good point

andrewsykim · 2020-09-16T01:55:41Z

/retest

gongguan · 2020-09-16T02:27:12Z

should set TCP CLOSE_WAIT timeout [Privileged] failed.

/test pull-kubernetes-e2e-kind-ipv6

andrewsykim · 2020-09-16T03:43:33Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

+		instanceMetadata, err := cnc.getInstanceNodeAddresses(ctx, &nodes.Items[i])
+		if err != nil {
+			klog.Errorf("Error getting instance metadata for node addresses: %v", err)
+			return


this should probably be continue?

You're right, fixed.

andrewsykim · 2020-09-16T03:45:17Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

-	nodeModifiers, err := cnc.getNodeModifiersFromCloudProvider(ctx, curNode)
+	providerID, err := cnc.getProviderID(ctx, curNode)
+	if err != nil {
+		utilruntime.HandleError(fmt.Errorf("failed to initialize node %s at cloudprovider: %v", node.Name, err))


This error message should probably be specific to getting the providerID:

utilruntime.HandleError(fmt.Errorf("failed to get provider ID for node %s: %v", node.Name, err))

andrewsykim · 2020-09-16T03:46:17Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

+
+	instanceMetadata, err := cnc.getInstanceMetadata(ctx, providerID, curNode)
+	if err != nil {
+		utilruntime.HandleError(err)


Wrap this error similar to other calls to HandleError here

utilruntime.HandleError(fmt.Errorf("failed to get instance metadata for node %s: %v", node.Name, err))

…tadata to updateNodeAddress

andrewsykim

/approve
/lgtm

Thanks @gongguan

k8s-ci-robot · 2020-09-16T14:56:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, gongguan

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:

~~staging/src/k8s.io/cloud-provider/OWNERS~~ [andrewsykim]

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

k8s-ci-robot requested review from dims and vishh July 21, 2020 09:09

k8s-ci-robot assigned andrewsykim and wojtek-t Jul 21, 2020

k8s-ci-robot removed the request for review from dims July 27, 2020 10:58

k8s-ci-robot added this to the v1.20 milestone Jul 27, 2020

andrewsykim reviewed Jul 27, 2020

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/gce/gce.go Outdated Show resolved Hide resolved

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go Show resolved Hide resolved

gongguan force-pushed the instancemeta branch from 4b7e7c6 to 54c5342 Compare July 29, 2020 02:37

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 Jul 29, 2020

gongguan force-pushed the instancemeta branch 3 times, most recently from 71b43dd to 6d2052e Compare July 29, 2020 09:28

andrewsykim reviewed Jul 29, 2020

View reviewed changes

gongguan force-pushed the instancemeta branch from 6d2052e to bdb2593 Compare July 29, 2020 14:55

gongguan force-pushed the instancemeta branch from a862791 to 8ed2f64 Compare September 3, 2020 09:57

gongguan force-pushed the instancemeta branch 2 times, most recently from 28c7f7e to 53aba78 Compare September 3, 2020 14:05

justaugustus modified the milestones: v1.20-phase-feature, v1.20 Sep 8, 2020

andrewsykim reviewed Sep 10, 2020

View reviewed changes

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2020

gongguan force-pushed the instancemeta branch from 53aba78 to ad8297e Compare September 15, 2020 01:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2020

andrewsykim reviewed Sep 15, 2020

View reviewed changes

gongguan force-pushed the instancemeta branch from ad8297e to e0b2dce Compare September 16, 2020 01:24

andrewsykim reviewed Sep 16, 2020

View reviewed changes

reduce cloud api calls in cloud-node-controller by passing instanceMe…

9678d4f

…tadata to updateNodeAddress

gongguan force-pushed the instancemeta branch from e0b2dce to 9678d4f Compare September 16, 2020 04:43

andrewsykim reviewed Sep 16, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2020

k8s-ci-robot merged commit 9621ac6 into kubernetes:master Sep 16, 2020

gongguan deleted the instancemeta branch September 16, 2020 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pass instanceMetadata to updateNodeAddress to reduce api calls #93284

pass instanceMetadata to updateNodeAddress to reduce api calls #93284

gongguan commented Jul 21, 2020 •

edited

gongguan commented Jul 21, 2020

gongguan commented Jul 21, 2020

wojtek-t commented Jul 21, 2020

dims commented Jul 27, 2020

gongguan commented Jul 29, 2020

gongguan commented Jul 29, 2020

andrewsykim commented Sep 3, 2020

gongguan commented Sep 3, 2020

gongguan commented Sep 4, 2020

gongguan commented Sep 4, 2020

andrewsykim left a comment

gongguan commented Sep 15, 2020

gongguan commented Sep 15, 2020

andrewsykim left a comment

andrewsykim Sep 15, 2020

gongguan Sep 16, 2020 •

edited

andrewsykim Sep 16, 2020

andrewsykim commented Sep 16, 2020

gongguan commented Sep 16, 2020

andrewsykim Sep 16, 2020

gongguan Sep 16, 2020

andrewsykim Sep 16, 2020 •

edited

gongguan Sep 16, 2020

andrewsykim Sep 16, 2020

gongguan Sep 16, 2020

andrewsykim left a comment

k8s-ci-robot commented Sep 16, 2020

pass instanceMetadata to updateNodeAddress to reduce api calls #93284

pass instanceMetadata to updateNodeAddress to reduce api calls #93284

Conversation

gongguan commented Jul 21, 2020 • edited

gongguan commented Jul 21, 2020

gongguan commented Jul 21, 2020

wojtek-t commented Jul 21, 2020

dims commented Jul 27, 2020

gongguan commented Jul 29, 2020

gongguan commented Jul 29, 2020

andrewsykim commented Sep 3, 2020

gongguan commented Sep 3, 2020

gongguan commented Sep 4, 2020

gongguan commented Sep 4, 2020

andrewsykim left a comment

Choose a reason for hiding this comment

gongguan commented Sep 15, 2020

gongguan commented Sep 15, 2020

andrewsykim left a comment

Choose a reason for hiding this comment

andrewsykim Sep 15, 2020

Choose a reason for hiding this comment

gongguan Sep 16, 2020 • edited

Choose a reason for hiding this comment

andrewsykim Sep 16, 2020

Choose a reason for hiding this comment

andrewsykim commented Sep 16, 2020

gongguan commented Sep 16, 2020

andrewsykim Sep 16, 2020

Choose a reason for hiding this comment

gongguan Sep 16, 2020

Choose a reason for hiding this comment

andrewsykim Sep 16, 2020 • edited

Choose a reason for hiding this comment

gongguan Sep 16, 2020

Choose a reason for hiding this comment

andrewsykim Sep 16, 2020

Choose a reason for hiding this comment

gongguan Sep 16, 2020

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 16, 2020

gongguan commented Jul 21, 2020 •

edited

gongguan Sep 16, 2020 •

edited

andrewsykim Sep 16, 2020 •

edited