-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return true when PowerVS network is already created during ReconcileNetwork #1931
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -840,7 +840,7 @@ func (s *PowerVSClusterScope) ReconcileNetwork() (bool, error) { | |||
if networkID != nil { | |||
s.V(3).Info("Found PowerVS network in IBM Cloud", "id", networkID) | |||
s.SetStatus(infrav1beta2.ResourceTypeNetwork, infrav1beta2.ResourceReference{ID: networkID, ControllerCreated: ptr.To(false)}) | |||
return false, nil | |||
return true, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will happen when network is there not not in ready state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking for not ready state for DHCP server? If so we never check for ready state of DHCP server when ever user provides an existing DHCP server name, We try to formulate the possible network name from DHCP server name : https://github.com/Karthik-K-N/cluster-api-provider-ibmcloud/blob/3eb2b524c65167565c595d01e475497b066ef22a/cloud/scope/powervs_cluster.go#L884-L891
I think we should check for DHCP server state as well but not sure there will be condition where DHCP server is not ready but still has network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @Amulyam24 this ReconcileNetwork is not following the format of other reconcile function, In every other we expect requeue and error but here we are looking for DHCPactive or err, Thats the main reason for this uncaught error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @Amulyam24 this ReconcileNetwork is not following the format of other reconcile function, In every other we expect requeue and error but here we are looking for DHCPactive or err, Thats the main reason for this uncaught error.
@Karthik-K-N, this was changed recently via #1869
cc @dharaneeshvrd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are not requeuing based on ReconcileNetwork return values, made the changes to set NetworkReady condition based on DHCP server active state returned from ReconcileNetwork.
Missed to handle the existing DHCP server logic, with this change it would return true and NetworkReady can be set to true.
Agree and we should check the state of DHCP server and decide the DHCP server is ok or not based on that.
Because underneath its just a VM, there is a possibility for network to be created and VM can go into bad state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then shall I create a issue to track this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref. #1933
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Amulyam24, 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
When an existing DHCP server is passed such as
The cluster provisioning is stuck due to
Network or LoadBalancer still not ready, requeuing
. This is not right as the network already exists and is ready.Hence, return true when PowerVS network is already created and is active.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
/area provider/ibmcloud
Release note: