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

fix: set host-uuid correctly on non LKE clusters #167

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

tchinmai7
Copy link
Contributor

Fixes a bug where the host-uuid was not set correctly for non-LKE clusters.

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

Copy link
Contributor

@srust srust left a comment

Choose a reason for hiding this comment

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

This begs the question - if it works with or without a linode provider ID, what is the linode provider ID useful for.

It appears the answer is sentry only. Is that your understanding as well?

@tchinmai7
Copy link
Contributor Author

This begs the question - if it works with or without a linode provider ID, what is the linode provider ID useful for.

It appears the answer is sentry only. Is that your understanding as well?

The lookup using providerID is much faster than the lookup by name - see instances.go linodeByName - it does a O(n) search among all available nodes, while linodeByID does a O(1) map read.

I left the code as is, to take the efficiency hit only in non-LKE clusters

@@ -233,17 +233,6 @@ func TestMalformedProviders(t *testing.T) {

client := NewMockClient(ctrl)

t.Run("fails on malformed providerID", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove? can't fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no "malformed provider ID" anymore, its either a linode-id or a label that we can look up - since we mock the linodeGo response, I didn't feel like it was a useful test to see if a string existed in a list (that we mock)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not what the test does, it doesn't test anything in the linodego response - it checks that we error out in case there's an incorrect spec value on the Node object. The new codebase just ignores this issue entirely and I don't find that to be correct behaviour.

@tchinmai7 tchinmai7 merged commit c9c1f4a into linode:main Jan 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants