Azure-sdk-for-go: Change to Future Result from Func to Field

[devigned]

Bug Report

In https://github.com/Azure/azure-sdk-for-go/releases/tag/v50.0.0 the Future structs had a very abrupt breaking change from having a Result func to now having a Result field. This is unfortunate since the changes make it more difficult to deal with futures for a given resource.

A good example of this is how Cluster API Provider Azure interacts with VMSS. We have a reconcile loop that will either, PUT, PATCH or DELETE a VMSS. We transform a future into a generic future so that we can handle them in a uniform way (see below).

These changes are pretty significant breaking changes. I imagine we can work around them, but it's a little painful.

Would the Result field be serializable? Would we be able to unmarshal the Result and execute it?

/cc @jhendrixMSFT, @ArcturusZhang, @cpanato

type genericScaleSetFuture interface {
    DoneWithContext(ctx context.Context, sender autorest.Sender) (done bool, err error)
    Result(client compute.VirtualMachineScaleSetsClient) (vmss compute.VirtualMachineScaleSet, err error)
}

type deleteResultAdapter struct {
    compute.VirtualMachineScaleSetsDeleteFuture
}

// Result wraps the delete result so that we can treat it generically. The only thing we care about is if the delete
// was successful. If it wasn't, an error will be returned.
func (da *deleteResultAdapter) Result(client compute.VirtualMachineScaleSetsClient) (compute.VirtualMachineScaleSet, error) {
    _, err := da.VirtualMachineScaleSetsDeleteFuture.Result(client)
    return compute.VirtualMachineScaleSet{}, err
}

// GetResultIfDone fetches the result of a long running operation future if it is done
func (ac *AzureClient) GetResultIfDone(ctx context.Context, future *infrav1.Future) (compute.VirtualMachineScaleSet, error) {
    var genericFuture genericScaleSetFuture
    futureData, err := base64.URLEncoding.DecodeString(future.FutureData)
    if err != nil {
        return compute.VirtualMachineScaleSet{}, errors.Wrapf(err, "failed to base64 decode future data")
    }

    switch future.Type {
    case PatchFuture:
        var future compute.VirtualMachineScaleSetsUpdateFuture
        if err := json.Unmarshal(futureData, &future); err != nil {
            return compute.VirtualMachineScaleSet{}, errors.Wrap(err, "failed to unmarshal future data")
        }

        genericFuture = &future
    case PutFuture:
        var future compute.VirtualMachineScaleSetsCreateOrUpdateFuture
        if err := json.Unmarshal(futureData, &future); err != nil {
            return compute.VirtualMachineScaleSet{}, errors.Wrap(err, "failed to unmarshal future data")
        }

        genericFuture = &future
    case DeleteFuture:
        var future compute.VirtualMachineScaleSetsDeleteFuture
        if err := json.Unmarshal(futureData, &future); err != nil {
            return compute.VirtualMachineScaleSet{}, errors.Wrap(err, "failed to unmarshal future data")
        }

        genericFuture = &deleteResultAdapter{
            VirtualMachineScaleSetsDeleteFuture: future,
        }
    default:
        return compute.VirtualMachineScaleSet{}, errors.Errorf("unknown furture type %q", future.Type)
    }

    done, err := genericFuture.DoneWithContext(ctx, ac.scalesets)
    if err != nil {
        return compute.VirtualMachineScaleSet{}, errors.Wrapf(err, "failed checking if the operation was complete")
    }

    if !done {
        return compute.VirtualMachineScaleSet{}, azure.WithTransientError(azure.NewOperationNotDoneError(future), 15*time.Second)
    }

    vmss, err := genericFuture.Result(ac.scalesets)
    if err != nil {
        return vmss, errors.Wrapf(err, "failed fetching the result of operation for vmss")
    }

    return vmss, nil
}

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/27f1cf5404583e96e076249e3c5300ac9ce49ffb/azure/services/scalesets/client.go#L263-L314

• SDK version v0.50.0+

• What happened?
• What did you expect or want to happen?
• How can we reproduce it?
• Anything we should know about your environment.

[devigned]

I just dug a bit deeper and found: https://github.com/Azure/azure-sdk-for-go/issues/14010. The interface is an improvement, but I'm still deeply concerned about the Result field and it's func var not being able to be marshaled / unmarshaled.

[devigned]

@jhendrixMSFT, in track 2 are the pollers able to be marshaled / unmarshaled?

I'm guessing this is the mechanism for resuming after a marshal / unmarshal, correct?
https://github.com/Azure/azure-sdk-for-go/blob/268147d542fc0a6101db6f2056fc7a4d43efaca3/sdk/arm/compute/2020-09-30/armcompute/zz_generated_pollers.go#L1140-L1144

[jhendrixMSFT]

Breaking marshallilng/unmarshalling was a (gross) oversight, we will look at how we can get this fixed.

Correct, the ResumeToken() is how pollers are marshalled/unmarshalled (we wanted to make it an explicit part of the contract in track 2).

[jhendrixMSFT]

@ArcturusZhang I have a solution for this. We will need an unmarshaller for the generated future types, and we move the implementation of Result() to an unexported method on the future type then wire it up accordingly (this should be safe as we create separate future types for each operation). I mocked this up for redis to test it out, and it's working as expected. Take a look.

https://github.com/jhendrixMSFT/azure-sdk-for-go/commit/f07a80d33524f49aac36289161d52143f02b1fe9

[ArcturusZhang]

I will take a look at this and make a fix on the code gen.

[CecileRobertMichon]

@ArcturusZhang @jhendrixMSFT is this issue fixed in newer versions of the SDK?

[ArcturusZhang]

@ArcturusZhang @jhendrixMSFT is this issue fixed in newer versions of the SDK?

this will be fixed in the major version this week using the approach in this comment: https://github.com/Azure/azure-sdk-for-go/issues/14351#issuecomment-786097594

[devigned]

@jhendrixMSFT and @ArcturusZhang we've opened the PR to update Cluster API Provider Azure to v53.1.0 (linked above). It was a little clunky. I think it would have been a little easier if the Result field on the future struct was actually a method so that it could be part of an interface described by the consumer of the future. In our PR, we needed to create an adapter struct to implement our generic future interface.

Clearly, without generics we can't have a fully "generic" future, but at least for all of the operations across a given resource type, it is still advantageous to treat the futures in a uniform manor.

Any chance, a function could be added to the future struct for Result so that the struct could fulfill an interface?

[jhendrixMSFT]

I agree it's clunky, it was a trade-off made to keep it (i.e. future mocking fix) compatible with earlier versions.

Track 2 has improved on this by returning interface types directly, see an example here. Once we have generics, the FinalResponse() method can be moved to azcore.Poller and all of the generated, type-specific interfaces go away.

If we were to add a method back, it would have to have a different name than the Result field, so now you have two ways of doing the same thing which seems weird (i.e. do I call future.Result() or future.ResultFn()?). Can you live with this for now? Track 2 is really where we're focusing on cleaning up all of this cruft.

[devigned]

I guess I was just surprised the future wouldn't work like this:

type SomeFuture struct {
    azure.FutureAPI
    resultFn func(...) (..., error)
}

func (f SomeFuture) Result(...) (..., error) {
    return f.resultFn(...)
}

Then it would be easy to mock in your tests, and if you wanted to open it to other folks, just export the resultFn field. wdyt?

[devigned]

@jhendrixMSFT when should we move to track2?