Bug 1852959: Add ProviderID #83
Conversation
openshift-ci-robot
added
the
bugzilla/severity-high
|
@hardys: This pull request references Bugzilla bug 1852959, 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
In response to this:
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-robot
added
the
bugzilla/valid-bug
openshift-ci-robot
added
the
approved
|
@hardys: This pull request references Bugzilla bug 1852959, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
1 similar comment
|
@hardys: This pull request references Bugzilla bug 1852959, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
|
WIP as this doesn't yet set the node ProviderID - I'm not certain of the best approach there, is it reasonable to just wait for the node reference to appear on the machine, then write the ProviderID in the actuator update when that happens, e.g same place this is currently setting the machine ProviderID? Also not yet tested but feedback welcome. |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
The cluster autoscaler expects this to be set on both machines and nodes, and typically the cluster-api provider will set the value in the spec for the machine e.g https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/actuators/machine/reconciler.go#L280 In the baremetal case though there's no "provider" to set the value on the node, so we also update that correspondingly https://bugzilla.redhat.com/show_bug.cgi?id=1852959
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| log.Printf("Not setting Node ProviderID - Node does not yet exist for Machine %s", machine.Name) | ||
| return nil |
There was a problem hiding this comment.
What causes the controller to try again if the node isn't there? Does the controller watch Nodes as well as Machines?
There was a problem hiding this comment.
It does watch Nodes, looks for the machine annotation on the Node, and queues the corresponding Machine for reconciliation. The annotation itself gets added by the nodelink controller.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhellmann, hardys, mhrivnak 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 |
|
@hardys: All pull requests linked via external trackers have merged: openshift/cluster-api-provider-baremetal#83. Bugzilla bug 1852959 has been moved to the MODIFIED state. In response to this:
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. |
🏃 refactor code to replace bmm by m3m
The cluster autoscaler expects this to be set on both machines and nodes, and typically the cluster-api provider will set the
value in the spec for the machine e.g
https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/actuators/machine/reconciler.go#L280
In the baremetal case though there's no "provider" to set the value on the node, so we also update that correspondingly (TODO)
https://bugzilla.redhat.com/show_bug.cgi?id=1852959