offline-updates: Allow specification of local ostree repo #439
Conversation
|
@mike-sul please take a look. I can't add reviewers to the PR. |
This option allows a local ostree repo to be specified as replacement for the factory ostree repo when building the offline bundle. This if required when a custom ostree repo was pushed with `fiopush` and a target was was manually added referencing a hash from this custom repo. Signed-off-by: Andre Detsch <[email protected]>
When a target was manually added with `fioctl targets add`, it may reference a ostree hash that is not included the factory's repo. An offline bundle created in such condition will fail installation, so printing a warning is appropriate. Signed-off-by: Andre Detsch <[email protected]>
detsch
force-pushed
the
detsch-add-ostree-repo-source
branch
from
47696c0
to
0ab88cd
Compare
| @@ -87,6 +89,8 @@ func init() { | |||
| "Skip fetching Target Apps") | |||
| offlineUpdateCmd.Flags().BoolVarP(&ouAllowMultipleTargets, "allow-multiple-targets", "", false, | |||
| "Allow multiple targets to be stored in the same <dst> directory") | |||
| offlineUpdateCmd.Flags().StringVarP(&ouOstreeRepoSrc, "ostree-repo-source", "", "", | |||
| "Path to the local ostree repo to be used in the offline bundle") | |||
There was a problem hiding this comment.
Maybe "to be added to the offline bundle" or "to be copied to"?
| } | ||
| defer dstFile.Close() | ||
|
|
||
| _, err = dstFile.ReadFrom(srcFile) |
There was a problem hiding this comment.
I don't have strong opinion, just io.Copy seems like would be more flexible.
There was a problem hiding this comment.
I thought io.Copy could replace the entire function, but unfortunately it does not.
No idea what flexibility you mean... a doc says this:
If src implements [WriterTo](https://pkg.go.dev/io#WriterTo),
the copy is implemented by calling src.WriteTo(dst).
Otherwise, if dst implements [ReaderFrom](https://pkg.go.dev/io#ReaderFrom),
the copy is implemented by calling dst.ReadFrom(src).
So it seems to only add an extra function call.
There was a problem hiding this comment.
By flexibility I meant exactly what the doc extract says - it agnostic to details of src and dst implementation. Their type change won't need to change io.Copy() call.
As I said, I don't have strong opinion, just looks more uniform to use io.Copy.
| } | ||
| defer srcFile.Close() | ||
|
|
||
| dstFile, err := os.Create(dst) |
There was a problem hiding this comment.
I would improve the overall file copying process by:
- Checking if
dstexists, and skip copying if it does. - At first copy to some tmp file (e.g.
dst.tmp ) and then "Sync" & rename to the real name (dst).
It will help to avoid to recopying files if the command is interrupted and then a new attempt is invoked. Also, it makes sure that files are properly persisted on storage (sync).
There was a problem hiding this comment.
(2) probably makes sense... although I'm not a great fan of a trade-off (added complexity).
As for (1), I think it comes with inherent risk of copying wrong files.
Think of it this way: a user might supply a wrong source, interrupt, and then supply a correct source.
If we don't forcibly replace all files, we can end up with the wrong files.
Not sure how accurate are my statements. Maybe, ostree has a directory structure preventing the above.
So, I leave it up to you to weigh on this.
There was a problem hiding this comment.
(2) is important if a user copies files to external thumb drive to guarantee data are synced.
There was a problem hiding this comment.
a user might supply a wrong source, interrupt, and then supply a correct source.
I don't see how (1) would cause an issue here taking into account (2) and the fact that each file in ostree repo has unique name - its hash.
There was a problem hiding this comment.
each file in ostree repo has unique name - its hash
Told you, I can be missing something.
how (1) would cause an issue here
But, I must be backing my claims about safety still.
You see, fsync only makes sure to sync the write to the hardware level, but that data may still be stored in the hardware (firmware) level cache first, and only later written to the persistent storage.
So, if we are talking about e.g. a thumb drive, than I'd say to do one of the following:
- Always rewrite - easy way.
- If file exists - check the file size/hash, and rewrite if it differs.
Trust me, I remember those times when I had to trash 1 in 100 CDs, because all those "flush to disk" things are never as reliable as assumed.
There was a problem hiding this comment.
check&write vs rewrite and "write to tmp" and fsync are two different things.
If we write to tmp then a file does not exist until it's fsynced and renamed to the file correct name. If we assume that "fsync and rename" are 99.9999% guarantee that data are persisted then we don't need to check file hashes. We use this techniques in fioconfig and aklite and composectl and we haven't seen any issues with it so far.
| sha256String := base64.StdEncoding.EncodeToString(ti.sha256) | ||
| expectedCommit := path.Join(dstDir, "ostree_repo", "objects", sha256String[0:2], sha256String[2:]+".commit") | ||
| if _, err := os.Stat(expectedCommit); errors.Is(err, os.ErrNotExist) { | ||
| fmt.Printf("WARNING: ostree repo does not contain target's hash %s, installation will fail. If the target references a custom ostree repo, re-run specifying --ostree-repo-source.\n", sha256String) |
There was a problem hiding this comment.
IMO, this is the critical error, it doesn't make sense to ship bundle to devices if the commit hash is missing, so I would just exit/die at this spot.
| @@ -180,6 +182,15 @@ Notice that multiple targets in the same directory is only supported in LmP >= v | |||
| fmt.Printf("Downloading an ostree repo from the Target's OE build %d...\n", ti.ostreeVersion) | |||
| subcommands.DieNotNil(downloadOstree(factory, ti.ostreeVersion, ti.hardwareID, dstDir), "Failed to download Target's ostree repo:") | |||
| } | |||
|
|
|||
| if ti.sha256 != nil { | |||
There was a problem hiding this comment.
The hash ti.sha256 is mandatory, it is nil I would just die.
Allowing the specification of a local ostree repo during offline bundle creation is required when a custom ostree repo is in use (
fiopush+fioctl targets add).This PR also add a warning message if the ostree repo does not contain the expected hash.