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

Adds new -type and -uplink flags to register command #61

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

nkinkade
Copy link
Contributor

@nkinkade nkinkade commented Nov 28, 2024

This allows Autojoin clients to specify these v2.Registration fields and pass them to the Autojoin API as part of the registration process.


This change is Reviewable

This allows Autojoin clients to specify these v2.Registration fields and pass
them to the Autojoin API as part of the registration process.
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 12060924129

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.142%

Totals Coverage Status
Change from base Build 12059416068: 0.0%
Covered Lines: 1215
Relevant Lines: 1238

💛 - Coveralls

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @nkinkade)


cmd/register/main.go line 43 at r1 (raw file):

	ipv4        = flagx.StringFile{}
	ipv6        = flagx.StringFile{}
	machineType = flag.String("type", "", "The type of machine: physical or virtual")

Do you want these to be flagx.StringFiles also?

Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)


cmd/register/main.go line 43 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Do you want these to be flagx.StringFiles also?

I couldn't think of a use case. For M-Lab-managed VMs, there is no current metadata file that has anything about the fact that the machine is a VM, nor the uplink speed. For M-Lab-managed machines, for now only virtual machines will be using the Autojoin API, and it is easy enough to set the -type flag of the autonode-register container to "virtual". As far as the uplink speed, as far as I know the only way to understanding the uplink speed is to read the GCE documentation. I presume the same is true for other cloud providers.

Are you imagining a case where those values would be written to a file by write-metadata.sh or similar and then read in by the register container?

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Thank you for this addition!!

:lgtm:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained


cmd/register/main.go line 43 at r1 (raw file):

Previously, nkinkade wrote…

I couldn't think of a use case. For M-Lab-managed VMs, there is no current metadata file that has anything about the fact that the machine is a VM, nor the uplink speed. For M-Lab-managed machines, for now only virtual machines will be using the Autojoin API, and it is easy enough to set the -type flag of the autonode-register container to "virtual". As far as the uplink speed, as far as I know the only way to understanding the uplink speed is to read the GCE documentation. I presume the same is true for other cloud providers.

Are you imagining a case where those values would be written to a file by write-metadata.sh or similar and then read in by the register container?

Thank you - I wasn't thinking through the specific cases. In the future, I could imagine if we want to use register for the physical nodes too. But, this doesn't need to be added now.

Copy link
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained


cmd/register/main.go line 43 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Thank you - I wasn't thinking through the specific cases. In the future, I could imagine if we want to use register for the physical nodes too. But, this doesn't need to be added now.

I definitely have in mind investigating the use of Autojoin for physical machines too. Offloading DNS management from siteinfo seems desirable, and would be one less dependency on mlabconfig.py, which feels like another legacy piece of software that we rely on heavily today.

@nkinkade nkinkade merged commit 380ac06 into main Nov 28, 2024
8 checks passed
@nkinkade nkinkade deleted the sandbox-kinkade branch November 28, 2024 01:50
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.

3 participants