search

dell / terraform-provider-redfish

Terraform provider for Redfish REST APIs
https://registry.terraform.io/providers/dell/redfish/latest
Mozilla Public License 2.0
89 stars 21 forks source link

BIOS update is not idempotent #19

Closed brutus333 closed 3 years ago

brutus333 commented 3 years ago

Community Note

Terraform CLI and Terraform Redfish Provider Version

terraform -v
Terraform v0.14.7

Server(s) details and firmware version

PowerEdge R640 iDRAC 9 version 4.20.20.20

Affected Resource(s)

redfish_bios

Terraform Configuration Files

# Copy-paste your Terraform configurations here - for large Terraform configs,
# please use a service like Dropbox and share a link to the ZIP file.
resource "redfish_bios" "bios" {

  redfish_server {
    endpoint = "https://192.168.8.103:6443"
    ssl_insecure = true
  }
  attributes = {
    "SysProfile" = "PerfOptimized"
  }
  settings_apply_time = "OnReset"
  action_after_apply = "ForceRestart"
}

Debug Output

https://gist.github.com/brutus333/950a67e89c0a099356060af3a4ea4f2e

Panic Output

Expected Behavior

Once the configuration is updated TF should not try to change the resource again

Actual Behavior

Terraform tried to update the resource again (even if it was compliant with the definition), so the change is not idempotent as it should be.

Steps to Reproduce

  1. terraform apply

Important Factoids

References

brutus333 commented 3 years ago

I have actually found a way to deal with this but not sure if this is the intended way to use the provider:

data "redfish_bios" "bios" {

  redfish_server {
    endpoint = "https://192.168.8.103:6443"
    ssl_insecure = true
  }
}

locals {
  changed_attrs = {
     SysProfile = "PerfOptimized"
  }
  attributes = merge(data.redfish_bios.bios.attributes, local.changed_attrs)
}

resource "redfish_bios" "bios" {

  redfish_server {
    endpoint = "https://192.168.8.103:6443"
    ssl_insecure = true
  }
  attributes = local.attributes
  settings_apply_time = "OnReset"
  action_after_apply = "ForceRestart"
}
mikeletux commented 3 years ago

Hi @brutus333 ,

Thank you very much for your feedback, really appreciate it. I had that conversation with @anupamaloke some weeks ago. Also we escalated the way things are handle to Hashicorp to understand that better.

Currently Terraform has a limitation in terms of handling big maps on the state file. Not sure if you're familiar with Terraform development, but either way I'll explain.

When you create a BIOS resource, in the state file (if I don't remember wrong) we actually save the whole BIOS attributes. In your .TF file there are only defined the ones you wish to modify. Turns out that Terraform does some internal validations when applying, and if the contents of the .TF file and state file does not match, it triggers an update. And that's what is happening. Since you only have one BIOS attribute to change (SysProfile), but in the state file are all of them (that update that you are seeing does not change anything, but I agree it is confusing).

We could change the behavior to make it moreless idempotent following this. Rather than saving the whole list of BIOS attributes, just save the ones you want to modify. That would make Terraform not to trigger an update every time. The thing is, as soon as you add or remove any attribute, an update will be triggered, since .TF file resource and state file one won't match.

What do you think? Thanks! /Miguel

When you create a resource, an entry for that resource is created in the state file.

brutus333 commented 3 years ago

I looked a bit through the documentation and other providers code and it seems that customdiffs is the way to go:

https://www.terraform.io/docs/extend/resources/customizing-differences.html https://github.com/terraform-provider-openstack/terraform-provider-openstack/blob/53f886ad12a1abcce778d535b101a91d06911a7c/openstack/networking_subnet_v2.go#L125

I'm sure that other providers are able to deal with partial configuration (I can't recall an example now) so it should be a way to fix this.

On the other hand, I have to say that I don't have experience with writing TF providers, but I can try to contribute to this one.

mikeletux commented 3 years ago

Hey @brutus333 ,

Let me take a look into that and I'll see what can be done there :). I totally agree with you this needs to be re-thought.

I'll keep this thread open to update when a fix has been implemented. We are also going to reach out Hashicorp to see if we can overcome that with the info you provided.

About contributing, we are open to that, so you'll be more than welcome 😄 Thanks! /Miguel

anupamaloke commented 3 years ago

I have actually found a way to deal with this but not sure if this is the intended way to use the provider:

data "redfish_bios" "bios" {

  redfish_server {
    endpoint = "https://192.168.8.103:6443"
    ssl_insecure = true
  }
}

locals {
  changed_attrs = {
     SysProfile = "PerfOptimized"
  }
  attributes = merge(data.redfish_bios.bios.attributes, local.changed_attrs)
}

resource "redfish_bios" "bios" {

  redfish_server {
    endpoint = "https://192.168.8.103:6443"
    ssl_insecure = true
  }
  attributes = local.attributes
  settings_apply_time = "OnReset"
  action_after_apply = "ForceRestart"
}

@brutus333, this is an excellent workaround. I am taking a look to see how the custom diff could be implemented for the Bios attributes. In the meantime, I will add your workaround to the example plans.

grantcurell commented 3 years ago

@anupamaloke heads up - I'm working on some optimizations for it. My in progress branch is here: https://github.com/grantcurell/terraform-provider-redfish/tree/18-wait-for-bios-update-job-to-finish

mikeletux commented 3 years ago

@grantcurell don't spend much time on this as I think Anupam had sorted it out already using a custom diff function. Let's sync up tomorrow!

grantcurell commented 3 years ago

@mikeletux it took me a bit to catch up but I just realized what @brutus333 was talking about and why it's happening. Agreed, yes, using a custom diff makes sense to me.