Skip to content

guestagent refactoring #3504

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
Merged
merged 3 commits into from
May 9, 2025
Merged

guestagent refactoring #3504

merged 3 commits into from
May 9, 2025

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented May 7, 2025 •
edited
Loading

Commit 1: guestagent: prioritize BIN.gz over BIN

When both lima-guestagent.Linux-<ARCH> and lima-guestagent.Linux-<ARCH>.gz are present, the compressed one is chosen with a warning.

Preparation for:

Commit 2: limactl info: show the detected guest agent paths

Commit 3: hostagent: verify existence of guestagent earlier

@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone May 7, 2025
@AkihiroSuda AkihiroSuda added the kind/refactoring Refactoring label May 7, 2025
@AkihiroSuda AkihiroSuda mentioned this pull request May 7, 2025
Closed
@AkihiroSuda AkihiroSuda force-pushed the refactor-guestagent branch 2 times, most recently from 80cd346 to 7c208a5 Compare May 7, 2025 09:18
AkihiroSuda
AkihiroSuda commented May 7, 2025
View reviewed changes
pkg/usrlocalsharelima/usrlocalsharelima.go
res, err := newest([]string{comp, uncomp})
if err != nil {
logrus.Debug(err)
return "", fmt.Errorf("guest agent binary could not be found for %s-%s (Hint: try installing `lima-additional-guestagents` package)", ostype, arch)
Copy link
Member Author

Choose a reason for hiding this comment

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

lima-additional-guestagents is being packaged in:

@jandubois
Copy link
Member

I'm not a fan of using file modification time to determine which file is newer. Modification time represents the time when a file was build / packaged / downloaded, so a newer file could very well be an older version.

I'm not sure which scenario this is supposed to address; why would somebody have both a compressed and an uncompressed version? Does this only happen for Lima developers / maintainers, or can regular users run into this situation.

I would prefer to treat this as an error, and make the user delete one of the files to resolve the ambiguity.

@AkihiroSuda
Copy link
Member Author

I'm not sure which scenario this is supposed to address; why would somebody have both a compressed and an uncompressed version? Does this only happen for Lima developers / maintainers, or can regular users run into this situation.

Regular users may hit this when unpacking v1.1.x.tar.gz (tentative) after unpacking v1.0.x.tar.gz

@jandubois
Copy link
Member

Regular users may hit this when unpacking v1.1.x.tar.gz (tentative) after unpacking v1.0.x.tar.gz

In that case I would still prefer that we will always pick the compressed version, if available, and show a warning when both exist, with a suggestion to delete the uncompressed one.

And with 1.1.0 going forward we should only be releasing the compressed version.

@AkihiroSuda AkihiroSuda force-pushed the refactor-guestagent branch from 7c208a5 to 50a3eb8 Compare May 8, 2025 06:56
@AkihiroSuda
Copy link
Member Author

In that case I would still prefer that we will always pick the compressed version, if available, and show a warning when both exist, with a suggestion to delete the uncompressed one.

Updated

@AkihiroSuda AkihiroSuda requested a review from jandubois May 9, 2025 01:24
jandubois
jandubois previously approved these changes May 9, 2025
View reviewed changes
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

Only one comment looks off

pkg/infoutil/infoutil.go Outdated
)

type Info struct {
Version string `json:"version"`
Templates []templatestore.Template `json:"templates"`
DefaultTemplate *limayaml.LimaYAML `json:"defaultTemplate"`
LimaHome string `json:"limaHome"`
VMTypes []string `json:"vmTypes"` // since Lima v0.14.2
VMTypes []string `json:"vmTypes"` // since Lima v0.14.2
GuestAgents map[limayaml.Arch]string `json:"guestAgents"` // since Lima v1.1.10
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GuestAgents map[limayaml.Arch]string `json:"guestAgents"` // since Lima v1.1.10
GuestAgents map[limayaml.Arch]string `json:"guestAgents"` // since Lima v1.1.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Also changed string to GuestAgent struct for extensibility

AkihiroSuda added 3 commits May 9, 2025 22:34
@AkihiroSuda
guestagent: prioritize BIN.gz over BIN
3d36eb1 
When both `lima-guestagent.Linux-<ARCH>` and `lima-guestagent.Linux-<ARCH>.gz`
are present, the compressed one is chosen with a warning.

Preparation for issue 3325

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
limactl info: show the detected guest agent paths
a047e65 
Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
hostagent: verify existence of guestagent earlier
008aca9 
Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda AkihiroSuda force-pushed the refactor-guestagent branch from 50a3eb8 to 008aca9 Compare May 9, 2025 13:38
jandubois
jandubois approved these changes May 9, 2025
View reviewed changes
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@jandubois jandubois merged commit f2100aa into lima-vm:master May 9, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jandubois jandubois jandubois approved these changes

Assignees
No one assigned
Labels
kind/refactoring Refactoring
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@AkihiroSuda @jandubois