-
Notifications
You must be signed in to change notification settings - Fork 102
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
Add Azurestack Support #561
Conversation
/hold |
@@ -0,0 +1,90 @@ | |||
// Copyright 2017 CoreOS, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2021
This came up in discussion in another project as well. For this project at least, we should probably be able to drop all these blocks and just rely on the Cargo.toml
license field. (To be clear, I mean this as a follow-up, not something you need to do here.)
You mean when running Afterburn from the initrd, right? Hmm, not sure offhand. Can you paste the logs? |
Yes, afterburn! I keep mistakenly saying "from ignition".
Looks like I did not save the logs. I remember there was minimal info in the error, with a 400 status code. I will run again in order to capture. Let me know if you have any suggestions for getting a more detailed debug output. |
That's odd, I don't know why it would give a 400 in the initrd, but work in the real root. Hmm, wonder if it wants the node to check in first? If you have interactive access to the console BTW, you can try booting with |
/retest |
b9b0da4
to
e3c7e0f
Compare
src/providers/azurestack/mod.rs
Outdated
let instance_metadata = AzureStack::fetch_identity(); | ||
Ok(Some(instance_metadata.unwrap().vm_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter is indirectly saying to use Err
instead of unwrap()
, e.g.:
let instance_metadata = AzureStack::fetch_identity(); | |
Ok(Some(instance_metadata.unwrap().vm_name)) | |
let instance_metadata = AzureStack::fetch_identity() | |
.context("fetching instance identity")?; | |
Ok(Some(instance_metadata.vm_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way I should change fetch_identity
: https://github.com/coreos/afterburn/pull/561/files#diff-34cbce01c9aa769653cb8f06a598911f28d90de5710e9b602560660669d702d1R172-R183 where I already have context
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetch_identity
function looks good. The context
stuff is like a human-friendly backtrace where you can add more error-prefixing as the error bubbles up the stack to make it easier to tell where the error came from. It's up to you when to add a .context(...)?
vs just ?
. If you think e.g. it's repetitive with the context already added by the callee, you can omit it in the caller. In this case for example, yeah it's probably superfluous given that my suggestion is more or less repeating the context provided by fetch_identity
, and our caller ultimately adds the context that the failure happened while fetching the hostname:
// write hostname if configured to do so
self.hostname_file
.map_or(Ok(()), |x| metadata.write_hostname(x))
.context("writing hostname")?;
So you can probably just swap out .context("fetching instance identity")?
with just ?
in my suggestion above.
5d8f3f2
to
ccaaa2d
Compare
/retest |
@jlebon All work on this is done and tested. Can you give it a more thorough review or suggest how to get more eyes on it? I have logs from testing master and bootstrap nodes if that will help. Not sure why continuous-integration/jenkins/pr-merge test is failing. Perhaps this is a test ifnra problem? I cannot replicate locally. Also can you tip me on how to retrigger this test? I can't figure out a way. Do I have to push a new empty commit? |
ccaaa2d
to
436b818
Compare
The merge test is failing with:
This looks like a problem running |
Yeah, CI was hitting issues yesterday. It should be fine now, so I've restarted another run. |
So in a first pass, there seems to be a lot of duplication between
So then the top-level |
Prep for adding an `azurestack` provider which shares a lot with `azure`.
OK, updated this based on #561 (comment). The only change was that I made the top-level module Still need to test this. |
Adds support and tests for the Azure Stack platform. Sets the hostname based on the vm name. Obtains SSH keys. Checks in to finish the boot process. Unlike Azure, does not offer any attributes. Follows the same pattern as Azure with some notable differences: The hostname is obtained by parsing the vm name from the identity metadata service, similar to the WALinuxAgent. Early attempts were made to obtain this vm name from the compute instance metadata service which was available in a pre-release lab environment, but the endpoint was not reachable pre-check in. Consdering the vm name was available from the identity endpoint, accessing the instance metadata service can be saved for future work. SSH key functionality is the same as Azure. Regarding attributes, obtaining the IP addresses in the same manner as Azure led to incorrect values, so they were removed as well as vmsize for simplicity. Therefore no attributes are available. Check in is the same process as Azure. Co-authored-by: Ben Howard <ben.howard@redhat.com> Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Now with authorship actually preserved! :) |
@jlebon there seems to be still quite a bit of overlap between |
Other than possible overlap of constants/etc.. is there anything else holding this PR up? |
To answer both of you at the same time: this PR is being tested in ASH, which is painful to do. If testing comes back green, my suggestion is to get this in as is and tweak things in follow-ups. |
I tested this in Azure Stack and everything looked good. I did not test ssh key fetching which I am not familiar with. But happy to help further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also finally been able to successfully test this in ASH, including SSH key fetching (which matters more for FCOS):
[root@jlebon-vm-bootstrap ~]# journalctl -u afterburn-sshkeys@core.service
-- Logs begin at Thu 2021-04-08 01:43:58 UTC, end at Thu 2021-04-08 03:27:48 UTC. --
Apr 08 02:00:22 jlebon-vm-bootstrap systemd[1]: Starting Afterburn (SSH Keys)...
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.046 WARN Failed to get fabric address from DHCP: maximum number of retries (60) reached
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.046 INFO Using fallback address
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.055 INFO Fetching http://168.63.129.16/?comp=versions: Attempt #1
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.062 INFO Fetch successful
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.062 INFO Fetching http://168.63.129.16/machine/?comp=goalstate: Attempt #1
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.219 INFO Fetch successful
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.270 INFO Fetching http://168.63.129.16/machine/70f676d8-fb84-4921-9716-96cc629c05a7/...
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: Apr 08 02:00:51.577 INFO Fetch successful
Apr 08 02:00:51 jlebon-vm-bootstrap afterburn[1929]: wrote ssh authorized keys file for user: core
Apr 08 02:00:51 jlebon-vm-bootstrap systemd[1]: afterburn-sshkeys@core.service: Succeeded.
Apr 08 02:00:51 jlebon-vm-bootstrap systemd[1]: Started Afterburn (SSH Keys).
[root@jlebon-vm-bootstrap ~]# cat /home/core/.ssh/authorized_keys.d/afterburn
ssh-rsa AAA...
So my vote is to get this in and we can do more cleanups afterwards. @lucab Would appreciate a stamp to sanity-check there isn't anything majorly wrong.
One thing worth noting is that in the base case, ASH support also requires coreos/fedora-coreos-config#935 so it has networking in the initrd. For OKD/OCP it works without it, because we use pointer Ignition configs and so Ignition itself needs networking and requests it. |
Updated as this is no longer WIP.
This PR:
I tested this and it pulled unique hostnames for a bootstrap node and 3 masters. The SSH keys also appear in the login screen.
Note: because this uses the identity metadata service, this will work in any azure stack environment.
Further todo on this PR:
Regarding the compute endpoint:
From a running VM (in the lab environment) I am able to query it:
But when I hit that same endpoint during afterburn I get an error. Because I was able to get the vm name from the identity endpoint, I prioritized that. as future work we could add more support for the instance metadata service.