Skip to content
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

UPSTREAM: <carry>: openshift: prioritise search by Provider ID #100

Conversation

frobware
Copy link

Change findMachineByNodeProviderID() to first match using ProviderID value. If there is no match (because actuator does not set the value on the machine object) then fallback to matching based on the "machine" annotation from the node object (should it exist). By falling back to the "machine" annotation on the node object we don't break compatibility with the behaviour in OpenShift 4.1.0.

frobware added 2 commits May 31, 2019 10:09
Index machines using machine.Spec.ProviderID.
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 31, 2019
Copy link
Member

@ingvagabund ingvagabund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits

frobware added 2 commits May 31, 2019 11:16
…r ID

Change findMachineByNodeProviderID() to first match using ProviderID
values. If there is no match (because actuator does not set the value
on the machine object) then fallback to matchin based on the "machine"
annotation from the node object.

Update unit tests to reflect new behaviour.
Add new test that verifies that machines without a ProviderID value
can still be found if there is a "machine" annotation on the node
object.
@frobware frobware force-pushed the find-machine-by-node-provider-id-v3 branch from 6aa254b to 8894561 Compare May 31, 2019 10:16
@ingvagabund
Copy link
Member

/approve

@frobware
Copy link
Author

/hold

Need to also search by ProviderID first in machineSetNodeNames.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2019
@frobware frobware changed the title UPSTREAM: <carry>: openshift: search for machines by Provider ID value first UPSTREAM: <carry>: openshift: search by Provider ID value first May 31, 2019
@frobware frobware changed the title UPSTREAM: <carry>: openshift: search by Provider ID value first UPSTREAM: <carry>: openshift: prioritise search by Provider ID May 31, 2019
@frobware
Copy link
Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2019
When mapping from a machine to a node, prioritize the search based on
ProviderID, falling back to using machine.Status.NodeRef.Name. This
value is populated by the nodelink-controller.
@frobware frobware force-pushed the find-machine-by-node-provider-id-v3 branch from 76599f4 to 47af784 Compare May 31, 2019 16:10
@ingvagabund
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2019
@frobware
Copy link
Author

frobware commented Jun 1, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit 6c23d61 into openshift:master Jun 1, 2019
frobware added a commit to frobware/autoscaler that referenced this pull request Jun 3, 2019
alvaroaleman pushed a commit to kubermatic/autoscaler that referenced this pull request Jun 4, 2019
alvaroaleman pushed a commit to kubermatic/autoscaler that referenced this pull request Jun 4, 2019
frobware added a commit to frobware/autoscaler that referenced this pull request Jun 5, 2019
frobware added a commit to frobware/autoscaler that referenced this pull request Jun 11, 2019
frobware added a commit to frobware/autoscaler that referenced this pull request Jun 12, 2019
frobware added a commit to frobware/autoscaler that referenced this pull request Aug 27, 2019
frobware added a commit to frobware/autoscaler that referenced this pull request Sep 11, 2019
frobware added a commit to frobware/autoscaler that referenced this pull request Feb 4, 2020
enxebre pushed a commit to enxebre/autoscaler that referenced this pull request Feb 28, 2020
frobware added a commit to frobware/autoscaler that referenced this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants