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

fingerprint kernel architecture name #13182

Merged
merged 9 commits into from
Jun 2, 2022
Merged

fingerprint kernel architecture name #13182

merged 9 commits into from
Jun 2, 2022

Conversation

shantanugadgil
Copy link
Contributor

first attempt at classic cpu architecture names while fingerprinting the cpu.

ref: #13181

Note as mentioned in the issue, I am fine if folks can come up with a name different that "classic" :)

@shantanugadgil
Copy link
Contributor Author

@angrycub @lgfa29 @jrasell

Any inputs here? I am not sure what/why the tests are failing!

@lgfa29
Copy link
Contributor

lgfa29 commented Jun 1, 2022

Vercel requires an explicit approval when deploying code from PRs, not a test failure 😄

I will double check with team with regards to the naming here, in the meantime, would mind adding a CHANGELOG entry?

Thanks for the PR!

@lgfa29
Copy link
Contributor

lgfa29 commented Jun 1, 2022

@shantanugadgil I spoke with the team and they brought up a good point, which is that these naming are really standardized and the same ISA can have even more than two common representations. For example, 386 can be i386 but also x86 in some places, so it would not be feasible for us to support all the different options.

One suggestion is to instead set node metadata values for the architectures that you need. How does that sound for you?

@shantanugadgil
Copy link
Contributor Author

I agree and disagree. Let me explain...

I agree to the part that "32-bit" was x86 for some operating systems (Windows) and i386 for others (Linux).
Sometimes there was no indication of "32-bit" (example, for SPARC it was just sparc and sparc64 for 64 bit)
BTW Windows "64-bit" is "x64" (non x86_64)

That said, I still somehow want the access to the classic names.

In hindsight, I see my PR was Linux specific and should be updated.
The arch_classic string should be set based on the "GOOS", I think

I do not agree completely with what Golang has done with the names. There was a reference of nomenclature before.

NetBSD's list of platforms was quite exhaustive and should have been factored into the GOXXXXX variables somehow.
(in my opinion)

I don't have big problem with adding node meta variables, though it upsets the workflow as the node metadata is set in userdata and now I would have to go and re-run the userdata on a bunch of existing machines.
This is fine for ASGs but for EC2 it reboots the machines 😞

If this "classic arch" variable was available, I would need to make changes only in the Nomad job file and re-run the job.

@shantanugadgil
Copy link
Contributor Author

after thinking about this a bit more ... what I essentially need/want is (pseudo-code):

if (linux) {
    attr.cpu.machine=$(uname -m)
}

attr.cpu.machine can be attr.os.machine as well.

@lgfa29
Copy link
Contributor

lgfa29 commented Jun 1, 2022

Ah yeah, that's a great point. To make the implementation even more robust and support other operating systems, you can read this value from the gopsutil, which is the library we use to fingerprint other system properties.

More specifically, you can do this in the host.go fingerprinter and read the value of KernelArch from host.InfoStat, just like we do for kernel.version. So this would be a new attribute called kernel.arch.

And then, just one last thing missing in the PR would to document this new attribute in this page:
https://github.com/hashicorp/nomad/blob/main/website/content/docs/runtime/interpolation.mdx#node-variables-interpreted_node_vars-node-variables-

@lgfa29 lgfa29 self-requested a review June 1, 2022 19:38
@shantanugadgil shantanugadgil changed the title fingerprint classic cpu architecture names fingerprint kernel architecture name Jun 2, 2022
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Thanks @shantanugadgil!

I just pushed a commit to keep the attribute table in alphabetical order.

@lgfa29 lgfa29 added the backport/1.3.x backport to 1.3.x release line label Jun 2, 2022
@lgfa29 lgfa29 merged commit f0bc4ce into hashicorp:main Jun 2, 2022
Nomad - Community Issues Triage automation moved this from Needs Triage to Done Jun 2, 2022
@github-actions
Copy link

github-actions bot commented Oct 8, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.3.x backport to 1.3.x release line
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants