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.

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

Vapp creation fix #387

Merged
merged 6 commits into from
Jun 25, 2021
Merged

Vapp creation fix #387

merged 6 commits into from
Jun 25, 2021

Conversation

dataclouder
Copy link
Contributor

@dataclouder dataclouder commented Jun 21, 2021

Deprecate vdc.ComposeRawVApp (returns task) and replace it with vdc.CreateRawVApp (returns vApp)

Addresses #386

Giuseppe Maxia added 3 commits June 20, 2021 06:54
* deprecate vdc.ComposeRawVApp

Signed-off-by: Giuseppe Maxia <[email protected]>
Signed-off-by: Giuseppe Maxia <[email protected]>
Signed-off-by: Giuseppe Maxia <[email protected]>
@dataclouder dataclouder marked this pull request as ready for review June 21, 2021 06:42
@dataclouder dataclouder requested a review from Didainius June 21, 2021 06:42
Copy link

@Beshman420 Beshman420 left a comment

Choose a reason for hiding this comment

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

Is a name change the correct change? The terraform-provider-vcd will be calling ComposeRawVApp in their resource_vcd_vapp.go

Nvm, I looked in the pull requests :)

@dataclouder
Copy link
Contributor Author

Is a name change the correct change? The terraform-provider-vcd will be calling ComposeRawVApp in their resource_vcd_vapp.go

Nvm, I looked in the pull requests :)

It's more than a name change. I could not simply change the behavior, because vdc.ComposeRawVApp returns a task, and the correct return type is a vApp.
PR #681 takes care of Terraform provider using the right method.

@Beshman420
Copy link

Beshman420 commented Jun 22, 2021

Is a name change the correct change? The terraform-provider-vcd will be calling ComposeRawVApp in their resource_vcd_vapp.go
Nvm, I looked in the pull requests :)

It's more than a name change. I could not simply change the behavior, because vdc.ComposeRawVApp returns a task, and the correct return type is a vApp.
PR #681 takes care of Terraform provider using the right method.

I realised today, that the method vdc.ComposeRawVApp doesn't return a task, just an error. If no error occurs, then nil is returned

Is deprecation still valid?

Signed-off-by: Giuseppe Maxia <[email protected]>
@dataclouder dataclouder merged commit 62fc442 into vmware:master Jun 25, 2021
@Beshman420
Copy link

Beshman420 commented Jun 25, 2021

I'm sorry, I'm very confused by this resolution.

That "fix", does not work

vAppContents.Tasks is always nil, at least from my own testing, so the vApp composing is still skipped and could lead to the same 400 errors that were seen before

Do you see any logs that show the taks being waited on until success?

@dataclouder
Copy link
Contributor Author

I'm sorry, I'm very confused by this resolution.

That "fix", does not work

vAppContents.Tasks is always nil, at least from my own testing, so the vApp composing is still skipped and could lead to the same 400 errors that were seen before

Do you see any logs that show the taks being waited on until success?

If you look at my answer above, you will see that there are tasks. Not always, but often.
The code looks for these tasks. If there are none, all is well: means that the vApp is already composed. If there are tasks, which may happen depending on how fast your hardware and network are, they are processed.
I don't see what can be wrong from your standpoint.
What was before an erroneous call response (expecting a task when we should get a vApp) is now correctly processed as a vApp. Possible tasks are detected and processed. I have a test that makes sure this is happening.

I am satisfied that the code is correct. If you have a test that proves otherwise, we'll gladly have a look.

@Beshman420
Copy link

I'm sorry, I'm very confused by this resolution.
That "fix", does not work
vAppContents.Tasks is always nil, at least from my own testing, so the vApp composing is still skipped and could lead to the same 400 errors that were seen before
Do you see any logs that show the taks being waited on until success?

If you look at my answer above, you will see that there are tasks. Not always, but often.
The code looks for these tasks. If there are none, all is well: means that the vApp is already composed. If there are tasks, which may happen depending on how fast your hardware and network are, they are processed.
I don't see what can be wrong from your standpoint.
What was before an erroneous call response (expecting a task when we should get a vApp) is now correctly processed as a vApp. Possible tasks are detected and processed. I have a test that makes sure this is happening.

I am satisfied that the code is correct. If you have a test that proves otherwise, we'll gladly have a look.

There is always a task, even if the status is instantly set to success, then it should at least log it. If there is no task, then either

  • Creation went wrong, which is caught already
  • The getting of a task for the vApp, isn't working

If the task has succeeded, it doesnt hurt to log it

@dataclouder
Copy link
Contributor Author

The task is logged only when the SDK receives it.
If the task ran so fast that we could not detect it, we can't log it, because the SDK does not see it.
The tasks that appear in the structure of the composed vApp are called TasksInProgress. If they are not in progress, they are not reported, and we see a nil: hence, it is not logged.

@Beshman420
Copy link

The task is logged only when the SDK receives it.

The task is always returned inside the vApp creation response. If it is completed, why not log it? It makes debugging and checking logs for completion of the task a lot simpler.

I dont believe a compose vApp task can be completed in a vCloud director quicker than the code be performed. That doesn't sound correct.

The fix I added has worked for 100s of real vApp creations at this point and has always had a task to wait for.

Am I correct in assuming you're stubbing the requests in test and haven't used it in a real scenario?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants