[installinator] add support for fetching an installinator document #8733
Conversation
Created using spr 1.3.6-beta.1
| impl ArtifactsToDownload { | ||
| pub(crate) fn host_phase_2_id(&self) -> ArtifactHashId { | ||
| ArtifactHashId { | ||
| kind: ArtifactKind::HOST_PHASE_2, | ||
| hash: self.host_phase_2, | ||
| } | ||
| } | ||
|
|
||
| Ok(InstallinatorImageId { update_id, host_phase_2, control_plane }) | ||
| pub(crate) fn control_plane_id(&self) -> ArtifactHashId { | ||
| ArtifactHashId { | ||
| kind: KnownArtifactKind::ControlPlane.into(), | ||
| hash: self.control_plane, |
There was a problem hiding this comment.
The use of ArtifactKind vs KnownArtifactKind looks weirdly asymmetric
There was a problem hiding this comment.
Yeah, it definitely is, though this just moves existing code.
| if !missing.is_empty() { | ||
| bail!( | ||
| "installinator document missing \ | ||
| required artifacts: {:?}", | ||
| missing | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is there any advantage to trying to return an error over bail!?
There was a problem hiding this comment.
Hmm, not quite sure what you mean here -- bail! does return an error.
There was a problem hiding this comment.
I wasn't sure if there was a nicer way to handle the error than system exit but I've done a little more looking at installinator now and just exiting makes sense to me.
There was a problem hiding this comment.
Ah this is within the context of a single update-engine step, so it's not an immediate system exit -- installinator will report this up to wicketd before exiting.
|
I have not yet read through the changes but I want to make sure that the installinator doc is still being treated as an artifact by Nexus (even if it doesn't have any specific instructions for what to do with it) and is being replicated into the repo depot. I don't remember where we landed on the sled recovery workflow but if it's Nexus-driven it will need to have the artifact around. |
Testing notesTested on berlin. Did an update from main (which doesn't know about installinator documents) to this PR (note that installinator didn't fetch the document): 
Then did an update to the other scrimlet, which does download the installinator document: 
|
|
Also uploaded the repo to Nexus and verified that the update dataset has the corresponding hash. All right, this is ready to review now. |
| @@ -503,7 +509,7 @@ mod tests { | |||
| .await?; | |||
|
|
|||
| // `by_id` should contain one entry for every `KnownArtifactKind` | |||
| // (except `ControlPlane`). | |||
| // (except `Zone`), as well as `installinator_document`. | |||
There was a problem hiding this comment.
this changed from ControlPlane to Zone, guessing outdated comment?
There was a problem hiding this comment.
good catch, thanks!
Created using spr 1.3.6-beta.1
sunshowers
deleted the
sunshowers/spr/installinator-add-support-for-fetching-an-installinator-document
branch
Prepare for future work where we're going to transmit other kinds of artifacts over the wire to installinator -- not just host phase 2 and control plane.
For compatibility, installinator supports repos both with and without this document. In the future we can clean this up.
TODO:
Depends on: