feat:Block volume with autotune config#501
Conversation
Unit tests output:E2e tests output: |
| // * `20`: Represents Higher Performance option. | ||
| // * `30`-`120`: Represents the Ultra High Performance option. | ||
| // For performance autotune enabled volumes, it would be the Default(Minimum) VPUs/GB. | ||
| VpusPerGB *int64 `json:"vpusPerGB,omitempty"` |
There was a problem hiding this comment.
I'm sure I'll find out later in the code, but do we validate these values?
| RPCConnectionId *string `json:"rpcConnectionId,omitempty"` | ||
| } | ||
|
|
||
| type BlockVolumeSpec struct { |
There was a problem hiding this comment.
I know we aren't perfect with this, but should we add // +optional for any param that is optional?
or call out defaults we set on behalf of the user
|
|
||
| // The type of volume attachment used to attach this block volume to the instance | ||
| VolumeType string `json:"volumeType,omitempty"` | ||
| } |
There was a problem hiding this comment.
There are a lot of new values here that can be set. Have we tested them all? should we make a new e2e test?
api/v1beta1/types.go
Outdated
|
|
||
| // The availability domain of the block volume replica. | ||
| // Example: `Uocm:PHX-AD-1` | ||
| AvailabilityDomain *string `json:"availabilityDomain,omitempty"` |
There was a problem hiding this comment.
I don't know enough about this. I see there is a cross region and cross AD replication support. Does this support both of those? Have we tested this works? Is there a way we can e2e test this works? or something?
|
|
||
| type AutotunePolicy struct { | ||
| // This field can be of type DETACHED_VOLUME or PERFORMANCE_BASED | ||
| AutotuneType string `json:"autotuneType,omitempty"` |
There was a problem hiding this comment.
I'm sure I'll find out later in the code, but do we validate these values?
cloud/scope/machine.go
Outdated
| }, | ||
| }...) | ||
| } else { | ||
| m.Logger.Info("Unknown attachment type not supported") |
| } | ||
|
|
||
| m.Logger.Info("Instance state, waiting...", "state", getInstanceResp.Instance.LifecycleState) | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
we probably also want to need some exit here as well. I'm not sure what that might be though 🤔
cloud/scope/machine.go
Outdated
| getInstanceResp, err := m.ComputeClient.GetInstance(ctx, getInstanceReq) | ||
| if err != nil { | ||
| if serviceErr, ok := err.(common.ServiceError); ok { | ||
| if serviceErr.GetHTTPStatusCode() == 404 { |
| break | ||
| } | ||
| } | ||
| m.Logger.Info("Error checking instance state", "error", err.Error()) |
There was a problem hiding this comment.
Also if there is an error here we will still try to delete the DeleteBlockVolume below? Will that work?
| if instance != nil { | ||
| m.Logger.Info("Found an existing instance") | ||
| m.Logger.Info("Reconcile block volume") | ||
| m.ReconcileBlockVolume(ctx) |
There was a problem hiding this comment.
Also what will happen here if the instance already exists, will a new blockvolume be created? will that leak resources?
it looks like ReconcileBlockVolume gets the BV by name, if found it doesn't do anything what happens if the user wants to update the display name? Do we handle that case? Reading the code it seems it might create a new BV.
What this PR does:
Why we need it:
At instance creation you cannot specify autotune enabled configuration for your attached block volume: https://github.com/oracle/oci-go-sdk/blob/master/core/launch_instance_details.go -> https://github.com/oracle/oci-go-sdk/blob/master/core/launch_create_volume_from_attributes.go
In order to do that you should create a block volume with that configuration and after that at instance creation specify that block volume for attachments.
The changes from this PR give the capability to run that logic behind the scenes just by specifying in OCIMachineTemplate the blockvolumeSpec block with its fields. There is an example under templates/cluster-template-with-bv-autotuned.yaml
Which issue(s) this PR fixes :
Fixes #462
Checklist