Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP #22

Karthik-K-N · 2022-06-23T08:10:03Z

Main intention of this PR is to set the NodeInternalIP for machine so that the cluster auto approver will approve the server csr requests

With DHCP server in place , The IP of the machine needs to fetched from dhcp server lease using the VM mac address
Also a cache implementation is added to avoid frequent calls to DHCP server

Add unit test cases

openshift-ci · 2022-06-23T08:10:20Z

@Karthik-K-N: This pull request references Bugzilla bug 2100033, which is invalid:

expected the bug to target the "4.11.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP

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.

Karthik-K-N · 2022-06-23T08:12:02Z

@mkumatag @dharaneeshvrd @Prajyot-Parab Please review.

mkumatag · 2022-06-23T08:50:05Z

@Karthik-K-N there is an already cache library written here in the https://github.com/kubernetes/client-go/blob/master/tools/cache/expiration_cache.go package which supports the TTL as well, wondering if we can use that here directly.

This one in particular https://github.com/kubernetes/client-go/blob/8dfe88aeffaac2cd6aba895db8fccfb51baf770d/tools/cache/expiration_cache.go#L203

Karthik-K-N · 2022-06-23T09:02:39Z

@Karthik-K-N there is an already cache library written here in the https://github.com/kubernetes/client-go/blob/master/tools/cache/expiration_cache.go package which supports the TTL as well, wondering if we can use that here directly.

This one in particular https://github.com/kubernetes/client-go/blob/8dfe88aeffaac2cd6aba895db8fccfb51baf770d/tools/cache/expiration_cache.go#L203

Sure, Will check that out

openshift-ci · 2022-06-23T12:56:31Z

@Karthik-K-N: This pull request references Bugzilla bug 2100033, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (pdsilva@redhat.com), skipping review request.

In response to this:

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP

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.

Karthik-K-N · 2022-06-23T12:57:10Z

Updated with changes to use cache functions from client-go. This PR is ready for PR

mkumatag

can we add some unit tests around this as well?

pkg/actuators/machine/instances.go

mkumatag · 2022-06-23T14:23:46Z

pkg/actuators/machine/reconciler.go

@@ -20,6 +21,8 @@ const (
 	requeueAfterSeconds      = 20
 	requeueAfterFatalSeconds = 180
 	masterLabel              = "node-role.kubernetes.io/master"
+	// CacheTTL is duration of time to store the vm ip in cache
+	CacheTTL = time.Duration(15) * time.Minute


why is this 15mins? we need to understand what's the lease period for the dhcp IP address and set the tll accordingly

and this can be moved to that new file caching.go

pkg/actuators/machine/reconciler.go

mkumatag · 2022-06-23T14:45:58Z

pkg/actuators/machine/reconciler.go

+			klog.Infof("found DHCP server with ID %s for network ID %s", *server.Network.ID, *networkID)
+			dhcpServerDetails, err = r.powerVSClient.GetDHCPServerByID(*server.ID)
+			if err != nil || dhcpServerDetails == nil {
+				errStr := fmt.Errorf("failed to get DHCP server details for with DHCP server ID: %s error: %v ", *server.Network.ID, err)


There is chance of NPE exception when err == nil and dhcpServerDetails == nil

pkg/actuators/machine/reconciler.go

openshift-ci · 2022-06-24T08:56:09Z

@Karthik-K-N: This pull request references Bugzilla bug 2100033, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (pdsilva@redhat.com), skipping review request.

In response to this:

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP

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.

mkumatag

one minor nit, otherwise lgtm

pkg/actuators/machine/caching.go

openshift-ci · 2022-06-24T10:00:21Z

@Karthik-K-N: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

mkumatag

/lgtm

openshift-ci · 2022-06-24T10:02:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Karthik-K-N, mkumatag

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~~ [mkumatag]

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

openshift-ci · 2022-06-24T10:05:31Z

@Karthik-K-N: All pull requests linked via external trackers have merged:

openshift/machine-api-provider-powervs#22

Bugzilla bug 2100033 has been moved to the MODIFIED state.

In response to this:

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP

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.

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 23, 2022

openshift-ci bot requested a review from mkumatag June 23, 2022 08:10

Karthik-K-N force-pushed the update-ip branch from 053661e to 86e9598 Compare June 23, 2022 12:55

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 23, 2022

Karthik-K-N force-pushed the update-ip branch from 86e9598 to 2dee45e Compare June 23, 2022 13:12

mkumatag requested changes Jun 23, 2022

View reviewed changes

openshift-ci bot assigned mkumatag Jun 23, 2022

mkumatag requested changes Jun 24, 2022

View reviewed changes

pkg/actuators/machine/caching.go Outdated Show resolved Hide resolved

Fetch VM IP from DHCP server

da14713

Karthik-K-N force-pushed the update-ip branch from 936ae6c to da14713 Compare June 24, 2022 09:45

mkumatag approved these changes Jun 24, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2022

openshift-ci bot merged commit f52a8d4 into openshift:main Jun 24, 2022

Karthik-K-N deleted the update-ip branch June 24, 2022 10:09

mkumatag mentioned this pull request Jun 27, 2022

Populate the Ip addresses for the vms deployed in the DHCP based network for the PowerVS platform kubernetes-sigs/cluster-api-provider-ibmcloud#723

Closed

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP #22

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP #22

Karthik-K-N commented Jun 23, 2022 •

edited

openshift-ci bot commented Jun 23, 2022

Karthik-K-N commented Jun 23, 2022

mkumatag commented Jun 23, 2022 •

edited

Karthik-K-N commented Jun 23, 2022

openshift-ci bot commented Jun 23, 2022

Karthik-K-N commented Jun 23, 2022

mkumatag left a comment

mkumatag Jun 23, 2022

mkumatag Jun 23, 2022

mkumatag Jun 23, 2022

openshift-ci bot commented Jun 24, 2022

mkumatag left a comment

openshift-ci bot commented Jun 24, 2022

mkumatag left a comment

openshift-ci bot commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP #22

Bug 2100033: Fetch VM IP from DHCP server to set NodeInternalIP #22

Conversation

Karthik-K-N commented Jun 23, 2022 • edited

openshift-ci bot commented Jun 23, 2022

Karthik-K-N commented Jun 23, 2022

mkumatag commented Jun 23, 2022 • edited

Karthik-K-N commented Jun 23, 2022

openshift-ci bot commented Jun 23, 2022

Karthik-K-N commented Jun 23, 2022

mkumatag left a comment

Choose a reason for hiding this comment

mkumatag Jun 23, 2022

Choose a reason for hiding this comment

mkumatag Jun 23, 2022

Choose a reason for hiding this comment

mkumatag Jun 23, 2022

Choose a reason for hiding this comment

openshift-ci bot commented Jun 24, 2022

mkumatag left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 24, 2022

mkumatag left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

Karthik-K-N commented Jun 23, 2022 •

edited

mkumatag commented Jun 23, 2022 •

edited