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

feat: add linode UUID as a metadata to each node #162

Merged
merged 6 commits into from
Jan 5, 2024

Conversation

shanduur
Copy link
Contributor

@shanduur shanduur commented Jan 4, 2024

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

A host UUID was added to the Linode Instance in a client change: linode/linodego#284

Apply this UUID from the CCM to expose the underlying host uniqueness, can improve scheduling and placement decisions for resources deployed on clusters with CCM installed.

cloud/linode/annotations.go Outdated Show resolved Hide resolved
linodeClient.SetBaseURL(validatedURL.String())

version := ""
matches := regexp.MustCompile(`/v\d+`).FindAllString(parsedURL.Path, -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still required after the linodego update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same logic as in the linode-cosi-driver and linode-blockstorage-csi-driver, added there just so we have similar code paths in all 3 components.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a dumb question, but why dont we make a linodeClient.ParseURL which does this logic and sets the base url and version for you. this would move all this logic to the linodego and we could reuse it right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lgarber-akamai @zliang-akamai for visibility on my dumb questions

Copy link
Member

Choose a reason for hiding this comment

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

Hi @luthermonson, I think you are right. Let us discuss it and get back to you next week.

Copy link

@lgarber-akamai lgarber-akamai Jan 16, 2024

Choose a reason for hiding this comment

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

Sorry for the late follow-up! I think this is a good idea and I've created a ticket internally to get this implemented ^

cloud/linode/node_controller.go Outdated Show resolved Hide resolved
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.

looks good - agree with uuid -> host-uuid change.

cloud/linode/annotations.go Outdated Show resolved Hide resolved
@shanduur shanduur force-pushed the feat/metadata branch 2 times, most recently from f1fc47f to d8f8b67 Compare January 4, 2024 19:02
@shanduur shanduur requested a review from srust January 4, 2024 19:02
Signed-off-by: Mateusz Urbanek <[email protected]>
@shanduur shanduur requested a review from zliang-akamai January 5, 2024 09:14
@shanduur shanduur merged commit 5103be6 into linode:main Jan 5, 2024
4 checks passed
@shanduur shanduur deleted the feat/metadata branch January 5, 2024 20:05
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.

7 participants