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.

Sign up for GitHub

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

Jump to bottom

install: Disable fsync() in repo when pulling && improved pull progress #655

Merged
merged 3 commits into from
Jul 3, 2024
Merged

install: Disable fsync() in repo when pulling && improved pull progress #655

merged 3 commits into from
Jul 3, 2024

Conversation

cgwalters
Copy link
Collaborator

See individual patches. Motivated by #646

@cgwalters
install: Use separate pull stage for progress output
8b30128 
The goal here is to get interactive progress on pulling,
as that can be slow and also I'd like to get more information there.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
install: Disable fsync() in repo when pulling
6c81a40 
This more than doubles the copy phase speed for me. We
rely on a final fsync/flush of the disks when unmounting.

Signed-off-by: Colin Walters <walters@verbum.org>
@github-actions github-actions bot added the area/install Issues related to `bootc install` label Jun 29, 2024
@cgwalters cgwalters mentioned this pull request Jun 29, 2024
Closed
@cgwalters
Copy link
Collaborator Author

@henrywang Hmm,

fatal: [guest]: FAILED! => changed=false 
  assertion: result_booted_cachedupdate_digest.stdout == result_rollback_image_digest.stdout

Have you see this in other PRs? I think it'd be helpful to acquire the full systemd journal here, is that easily doable?

@cgwalters
pull: Improved progress output
d8b5df2 
indicatif has nice support for multiple bars. Instead
of hand-rolling a `[nn/NN]` for the overall layer count,
change things so that we have:

```
Fetching layers [bar...] 8/65
 ostree chunk sha256:29fc11ff03e4b3 [bar] (0 B/s)
```

Signed-off-by: Colin Walters <walters@verbum.org>
@jeckersb
Copy link
Contributor

jeckersb commented Jul 3, 2024

Overall looks great to me. The only thing is that it appears to cut off the byte_bar at 80 characters, which looks weird on a wider terminal and cuts off the majority of the message. Ends up like:

 └ Fetching ██████████████████░░ 127.51 MiB/136.26 MiB (232.34 MiB/s) layer sha2

I'm not familiar with indicatif but I'll see if I can make it use more space when it's available.

@cgwalters
Copy link
Collaborator Author

Yeah, I finally figured out that mutating the {prefix} were what was causing the "eating previous terminal output" behavior I saw in previous changes here, and I switched to setting {msg} which is how things are intended to work. But indeed it seems like the library isn't detecting the terminal width correctly, or something else is going wrong. Could add a dbg! with https://docs.rs/indicatif/latest/indicatif/trait.TermLike.html#tymethod.width ?

cgwalters added a commit to cgwalters/podman-bootc-cli that referenced this pull request Jul 3, 2024
@cgwalters
disk: Also pass stdin to install container
fedcf86 
It's only by doing this that we end up calling into the
code inside `attach()` that tries to ensure the terminal
size matching. Otherwise we just get the default of 80
columns (and no dynamic SIGWINCH resizing).

This fixes the output in containers/bootc#655

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters mentioned this pull request Jul 3, 2024
Merged
@cgwalters
Copy link
Collaborator Author

Figured it out containers/podman-bootc#55

@jeckersb
Copy link
Contributor

jeckersb commented Jul 3, 2024

Figured it out containers/podman-bootc#55

Nice, I had gotten as far as:

https://github.com/console-rs/console/blob/master/src/unix_term.rs#L46-L64

And determined that isatty is correctly returning 1 but the ioctl is leaving the winsize rows and cols as 0 so it falls back to the default width of 80.

jeckersb
jeckersb approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@jeckersb jeckersb left a comment

Choose a reason for hiding this comment

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

Confirmed this works like a charm with the podman-bootc fix 🎉

@cgwalters cgwalters merged commit 2c4ca36 into containers:main Jul 3, 2024
9 of 19 checks passed
@cgwalters cgwalters mentioned this pull request Jul 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeckersb jeckersb jeckersb approved these changes

Assignees
No one assigned
Labels
area/install Issues related to `bootc install`
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cgwalters @jeckersb