Merged
Conversation
Member
|
@Pearl1594 see https://github.com/apache/cloudstack/blob/main/ui/src/views/compute/DeployVM.vue#L2167-L2171 |
Member
|
@Pearl1594 you probably don't want to do this as a default, we need to check if mgmt server logs API requests when things are POST-ed. You can probably add conditionals for new user-data related APIs. @hrak did you face issues when using the deploy VM API or other user-data related APIs? |
Contributor
Author
|
@weizhouapache @rohityadavcloud I was just testing to see if there is any issue with using POST for all commands. Hence raised this draft PR. |
|
cloudstack-go also uses POST by default for both DeployVirtualMachine and UpdateVirtualMachine, but i wouldn't use POST for everything as this PR seems to do. |
Yes, i can't submit userdata > 4K using Cloudmonkey. |
Member
|
@Pearl1594 isn't deploy VM using POST in cmk? |
Contributor
Author
|
@rohityadavcloud No, deployVM is a GET request. I'll identify the commands that use POST and update the PR, that said, all operations seem to work fine using post |
Member
|
@Pearl1594 I suppose a simple fix could be something like if the request size is say > 2048 or > 4096 then use POST, otherwise use GET. One thing we must check if when we use POST APIs, does cloudstack log them (for debugging/support purposes). |
Member
or use POST if |
4133f08 to
9f81008
Compare
Member
|
@hrak |
Contributor
|
Can we get a config item for the user to choose POST or not e.g. |
Contributor
|
FYI - I tested this out and am getting auth errors via keys if I try to call I did not run in debug mode so I didn't capture that output, but retrying without the userdata worked. Then trying to |
Member
|
@mlsorensen can you share a test userdata string/content for us to test/reproduce it? Could you also check mgmt server logs (share the same)? It could be a limitation/issue in cmk or backend mgmt server. |
DaanHoogland
reviewed
Nov 2, 2023
Member
|
@DaanHoogland @Pearl1594 is there a way you could test against large userdata input string to see how it behaves wrt post vs get request? |
Contributor
|
after a short discuss off(this)line with @Pearl1594 , the performance is not very relevant as with only 2kb of url length userdata is going to be too long easily. If we wish we can implement a flag to try get first or a flag to use get (or post) for deploymachine. I think this is out of scope for this PR, though. |
DaanHoogland
approved these changes
Nov 6, 2023
Contributor
|
liberally stealing from my chat with @Pearl1594; after: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #139