-
Notifications
You must be signed in to change notification settings - Fork 2
Adding netbox support for EgressIP for OcpSandbox #146
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
base: main
Are you sure you want to change the base?
Conversation
internal/models/ocp_sandbox.go
Outdated
| "egressIPs": []string{strings.Split(egressIPAvailable, "/")[0]}, | ||
| "namespaceSelector": map[string]any{ | ||
| "matchLabels": map[string]any{ | ||
| "guid": guid, // The label selector for the Keycloak realm |
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.
can we use service_uuid instead?
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.
Shouldn't we add some logic in the Candidate loop to check:
- if the annotation egressip is there
- make sure the cluster has an availableIP
- otherwise => move to the next one
?
| egressIPAvailable, err = netbox.RequestIP(selectedCluster.NetboxApiUrl, selectedCluster.NetboxToken, rnew.ServiceUuid) | ||
| log.Logger.Info("selectedCluster", "egressip", egressIPAvailable) | ||
| if err != nil { | ||
| log.Logger.Error("Error creating EgressIP", "error", err) |
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.
So this is non-fatal?
| if ok && egressIP != "" { | ||
| err = netbox.ReleaseIP(cluster.NetboxApiUrl, cluster.NetboxToken, egressIP+"/"+deletens.Labels["egressNetmask"]) | ||
| if err != nil { | ||
| log.Logger.Error("Error deleting egressIP on netbox", egressIP, err) |
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.
we should make this fatal, no?
Releasing the IP should work, otherwise consider the delete as fail, and don't proceed. Retry later
Otherwise it's going to leak IPs?
Problem is if something goes wrong here with the netboxapi, then namespaces aren't deleted
Not sure how we can address that?
Ordering a OcpSandbox, sandbox will do:
If connection or if there is any error getting the Egressip, is ignored and throwing error to the log.
Idea EgressIP is not, at this point, blocking the provisioning.