这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@edigaryev
Copy link
Collaborator

@edigaryev edigaryev requested a review from fkorotkov April 6, 2022 09:52
Copy link
Contributor

@fkorotkov fkorotkov left a comment

Choose a reason for hiding this comment

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

I was mostly wondering about arguments for run command to specify cpu/memory so it's easy experiment with them on demand.

Plus it will play nicely for persistent workers. No need to clone-set-run, can just clone and run right away.

Do you think it makes sense to include these changes here or in a separate PR?


self.memorySize = try container.decode(UInt64.self, forKey: .memorySize)

// Migrate to newer version
Copy link
Contributor

Choose a reason for hiding this comment

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

Before we release it to the public we can probably ignore it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed in a8b46ba.

@edigaryev
Copy link
Collaborator Author

I was mostly wondering about arguments for run command to specify cpu/memory so it's easy experiment with them on demand.

Honestly, I've started working on this PR with this mindset and realized that it's not a good idea in terms of transparency.

Here's why: we already store VM's configuration in a file. Starting a VM with configuration other than the one described in it's configuration file is confusing in the event the user chooses to inspect the configuration file (#1 (comment)) and infer some knowledge from it. It will be misleading.

Plus it will play nicely for persistent workers. No need to clone-set-run, can just clone and run right away.

This process is fully automated, so no issue here at all.

Do you think it makes sense to include these changes here or in a separate PR?

I'd vote for a separate PR.

@edigaryev edigaryev requested a review from fkorotkov April 7, 2022 18:38
@fkorotkov
Copy link
Contributor

I'd vote for a separate PR.

SGTM. There is already #4 for that actually.

@fkorotkov fkorotkov self-requested a review April 8, 2022 21:29
@fkorotkov
Copy link
Contributor

On a second though why do we store cpu and memory for a VM? What about storing only the min values and have flags like --cpu and --memory for run command with some feasible defaults like 2 CPUs and 4G of memory? Don't think it there will be a VM of M1 machine which won't comply with these requirements. Then there won't be a need in set at all and when a persone will run an existing VM it won't be too slow with the defaults.

@edigaryev edigaryev merged commit 466c3c5 into main Apr 11, 2022
@edigaryev edigaryev deleted the set-cpu-memory branch April 11, 2022 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants