search

hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.31k stars 9.49k forks source link

remote-state locking issue on swift versioned backend #27218

Closed yanndegat closed 1 year ago

yanndegat commented 3 years ago

Terraform Version

Terraform v0.14.0

Terraform Configuration Files

terraform {                                                                                                                                                                                                         
  backend "swift" {}                                                                                                                                                                                                
}

Crash Output

...
Acquiring state lock. This may take a few moments...

Error: Error locking state: Error acquiring the state lock: Couldn't read lock info: Resource not found

Expected Behavior

ok

Actual Behavior

Error: Error locking state: Error acquiring the state lock: Couldn't read lock info: Resource not found

Steps to Reproduce

  1. terraform init 
    terraform init -backend-config=archive_container=archive-test -backend-config=cloud=tfstate -backend-config=container=test -backend-config=region_name=REGION -backend-config=state_name=test/tfstate.tf
  2. terraform plan (the plan has to be longer than the lock periodic renewal)
  3. terraform plan

Additional Context

The issue is the following:

On versioned swift containers, each time you update an object, the previous version is copied in the corresponding archive container. When you delete the object, the last version is restored on the container. Thus, to completely remove an object you have to delete it N times, N = the number of archived versions + 1

This is an issue for the lock object, which is updated periodically, thus versioned N times, N = terraform plan duration / renewal interval, but it is deleted only once at the end of the terraform action.

Proposed Fix

pkolyvas commented 3 years ago

The Swift backend's status is currently unmaintained as per our codeowners file. We rely exclusively on contributions for updates and changes to it.

We would welcome a PR in this case.

iokiwi commented 3 years ago

@yanndegat there's one thing I don't undertstand about that explanation which is that at the top of the stack of error messages, the error is Resource not found

Error: Error locking state: Error acquiring the state lock: Couldn't read lock info: Resource not found

This implies no such lock object exists.

Allow me to offer an alternate explanation.

The short version.

  1. Terraform reports Resource not found when trying to retrieve the lock
  2. ...because no lock exists
  3. ...because the lock failed to create
  4. ...because of a potential bug in swift checking If-None-Match: "*" header precondtion.

Specifically I think swift is creating a tombstone object and when checking If-None-Match: "*", precondition, is reporting the existance of the tombstone object as if it were the lockfile.

Showing my Working

Terraform tries to create the lock, and if one already exists, get the lock info

I created the container manually before hand

terraform init

State of the container: Empty

$ swift stat test-5-terraform-state
            Account: [REDACTED]
          Container: test-5-terraform-state
            Objects: 0
              Bytes: 0
           Read ACL:
          Write ACL:
            Sync To:
           Sync Key:
             Server: nginx/1.16.0
       Content-Type: text/plain; charset=utf-8
      Accept-Ranges: bytes
   X-Storage-Policy: nz-hlz-1--o1--sr-r3
        X-Timestamp: 1609235625.11128
X-Versions-Location: test-5-terraform-state-archive
         X-Trans-Id: txa7df093047f14914bb2a8-005feaff73

$ openstack object list test-5-terraform-state

First run of terraform plan creates and releases a lock successfully.

2020-12-29T22:57:08.017+1300 [DEBUG] Acquired Lock 5ff4897f-
2a16-3cc1-4329-74bb0f81aed0 on tfstate.tf
2020-12-29T22:57:08.017+1300 [DEBUG] Getting object test-5-terraform-state/tfstate.tf
2020-12-29T22:57:08.017+1300 [DEBUG] Renew lock Lock Info:
  ID:        5ff4897f-2a16-3cc1-4329-74bb0f81aed0
  Path:      tfstate.tf.lock
  Operation: OperationTypePlan
  Who:       simon@simon-ubuntu-vm
  Version:   0.15.0
  Created:   2020-12-29 09:57:07.65285366 +0000 UTC
  Info:
2020-12-29T22:57:08.093+1300 [DEBUG] Object doesn't exist to download.
2020-12-29T22:57:13.995+1300 [DEBUG] Releasing Lock 5ff4897f-2a16-3cc1-4329-74bb0f81aed0 on tfstate.tf
2020-12-29T22:57:13.996+1300 [DEBUG] Getting object test-5-terraform-state/tfstate.tf.lock
2020-12-29T22:57:14.172+1300 [DEBUG] Deleting object test-5-terraform-state/tfstate.tf.lock
2020-12-29T22:57:14.172+1300 [DEBUG] Stopping Periodic renew of lock 5ff4897f-2a16-3cc1-4329-74bb0f81aed0
2020-12-29T22:57:14.249+1300 [DEBUG] Getting object test-5-terraform-state/tfstate.tf.lock
2020-12-29T22:57:14.305+1300 [DEBUG] Lock has been deleted.

State after first plan: No lock exists.

$ swift stat test-5-terraform-state
            Account: [REDACTED]
          Container: test-5-terraform-state
            Objects: 0
              Bytes: 0
           Read ACL:
          Write ACL:
            Sync To:
           Sync Key:
             Server: nginx/1.16.0
       Content-Type: text/plain; charset=utf-8
      Accept-Ranges: bytes
   X-Storage-Policy: nz-hlz-1--o1--sr-r3
        X-Timestamp: 1609235625.11128
X-Versions-Location: test-5-terraform-state-archive
         X-Trans-Id: txa7df093047f14914bb2a8-005feaff73

$ openstack object list test-5-terraform-state

Run terraform plan again

This time swift tells terraform that a lock lock already exists.

$ export TF_LOG=DEBUG
$ terraform plan
[...]

2020-12-29T23:35:50.491+1300 [DEBUG] Couldn't write lock 8d4b0e46-a27a-615a-43a5-c5d4fc167226. One already exists.
2020-12-29T23:35:50.492+1300 [DEBUG] Getting object test-5-terraform-state/tfstate.tf.lock

Error: Error locking state: Error acquiring the state lock: Couldn't read lock info: Resource not found

I added some of my own logging to see more about the response from swift.

httpErr: Expected HTTP response code [] when accessing [PUT https://object-storage.nz-hlz-1.catalystcloud.io:443/v1/[REDACTED]/test-5-terraform-state/tfstate.tf.lock], but got 412 instead
<html><h1>Precondition Failed</h1><p>A precondition for this request was not met.</p></html>

So we can see that swift refused to create the lock because swift thinks one already exists - even though we just saw that listing the objects of the container yeilds no results - i.e. the container is empty.

Since terraform thinks the lock exists, it tries to get the lock info - but it fails because no lock actually exists.

remote-state/swift/client.go#L388-L406

    // httpErr: 412 Precondition Failed - swift thinks resource already exists
    if httpErr, ok := err.(gophercloud.ErrUnexpectedResponseCode); ok && httpErr.Actual == 412 {
        log.Printf("[DEBUG] Couldn't write lock %s. One already exists.", info.ID)
        // Get current lock (doesn't actually exist)
        info2, err2 := c.lockInfo()
        if err2 != nil {
            // err2: 'Resource not found'
            return fmt.Errorf("Couldn't read lock info: %v", err2)
        }
        return c.lockError(err, info2)
    }

State of the container at this time

$ swift stat test-5-terraform-state
            Account: [REDACTED]
          Container: test-5-terraform-state
            Objects: 0
              Bytes: 0
           Read ACL:
          Write ACL:
            Sync To:
           Sync Key:
             Server: nginx/1.16.0
       Content-Type: text/plain; charset=utf-8
      Accept-Ranges: bytes
   X-Storage-Policy: nz-hlz-1--o1--sr-r3
        X-Timestamp: 1609235625.11128
X-Versions-Location: test-5-terraform-state-archive
         X-Trans-Id: txa7df093047f14914bb2a8-005feaff73

$ openstack object list test-5-terraform-state

So why is swift telling us that the lock object already exists when we try to create a new one?

Firstly, note that the HTTP 412 response code is not even documented in the openstack swift api reference

I had a discussion with the upstream openstack swift team an they pointed me towards tombstone objects, which are breifly mentioned in the openstack swift architecture docs about the object server.

# freenode: #openstack-swift

22:10 <iokiwi> hey where should I look to figure out why I am getting http 412 when creating object (with X-If-None-Matches; "*") after deleting object with same name.

22:11 <iokiwi> swift api is behaving as if the recently delete object still exists i.e it hasn't been cleaned up fully yet. But I am not sure where I should look to inspect the state of the deleted object

04:01 <DHE> as a user, in some ways it isn't. there is a "tombstone" object left behind so that replication can replicate the delete event

11:02 <iokiwi> okay cheers. I have admin access to the openstack/swift cluster just not sure where to start (other than skimming the source code) Is that behaviour specified or documented anywhere? The API ref for swift does not even specify that 412 is a possible return code

11:07 <+timburke> iokiwi, i'd use swift-get-nodes to find the disks on which we'd expect to find the object, then start poking around and seeing how they respond to curl

11:07 <+timburke> syntax looks like `swift-get-nodes <ring file> <acct>/<cont>/<obj>`, and it'll even include sample curl commands for doing HEADs

11:21 <iokiwi> nice thanks very much timburke.

Proposal

I think terraform should manually and explicitly check if the object exists before trying to create the lock - Since there seems to be an issue utilising If-None-Match header functionality but listing the contents of the container seems to more reliably tell us if the object actually does or does not exist.

let me know what you think of my analysis - I am happy to propose a patch.

iokiwi commented 3 years ago

Except now I think about it, the reason the original author would have used the If-None-Match: "*" is because it averts any race conditions between two users simultaneously checking if a lock exists and creating the lock.

E.g

  1. Person 1 check if lock exists - no lock
  2. Person 2 checks if no lock exists - no lock
  3. Person 1 creates lock
  4. Person 2 creates lock - which I think will just update/overwrite Person 1's lock.

I think its fundamentally impossible to avoid this race condition without the use of the If-None-Match header. Ultimately this bug needs to be fixed upstream in swift (and may have already been, my version of swift is very old).

  1. Identify which versions of swift are affected.
  2. Decide whether to wait for problem to be fixed in swift allowing locking to remain totally non functional in the meantime, or implement a subtly but fundamentally broken (yet ironically more functional) locking.

At least in its completely broken state the user is fully aware that by proceeding with the -lock=falseflag that something could go very wrong. It's probably better the user is aware that they are about to do something dangerous than totally unaware that there's a small chance what they are about to do could blow up.

yanndegat commented 3 years ago

hi @iokiwi

this swift stuff is very tricky due to its eventual consistency nature. i agree with you that removing the "if-none-match" may leed to more bugs.

one other important flag to be aware of is the "x-newest" on get methods. which always query all the replicas to ensure the newest vesion of the object is returned.

i think my first interpretation was wrong as i've digged into my issue with the swift team i rely on, and it appeared that there was an issue with one of the servers holding a replica of my object.

still, something is wrong in the current implemention: lock object shall not be versioned. because as soon as you delete it, its previous version is restored.

so either we store it on a non versioned container, either we track the number of version we have to delete and delete all of them.

what do you think ?

iokiwi commented 3 years ago

Hey @yanndegat,

Firstly, I just wondered what version of swift you are using. I, for one, am on a pretty old version.

$ swift capabilities | grep version
  version: 2.7.0.dev3

still, something is wrong in the current implemention: lock object shall not be versioned. because as soon as you delete it, its previous version is restored.

so either we store it on a non versioned container, either we track the number of version we have to delete and delete all of them.

I think the state files are being versioned but the lock files are not being versioned (or at least multiple versions of the lock file should not be being created) because we are using If-None-Match: * to prevent a lock file from being created if a lock file already exists.

After the first lock file is created - all subsequent attempts to create a new lock file / version will fail so there should never be more than one version of the lock file.

After the lock is released, the one and only lock file version should be deleted. There should be no old versions of the lock file to replace it as no new version should have ever existed in the first place.

So I just don't see where the extra versions of the lock files are coming from?

But if it is the case that lock files exists maybe you could go step by step like I did and show the contents of the terraform-state and terraform-state-archive containers after each step?

it appeared that there was an issue with one of the servers holding a replica of my object.

That seem in line with what I feel is going on. I was reading the swift source to see what goes on there.

https://github.com/openstack/swift/blob/7a327f6285329aac55c02b0a7683c1de570b9328/swift/proxy/controllers/obj.py#L554-L570

    def _check_failure_put_connections(self, putters, req, min_conns):
        """
        Identify any failed connections and check minimum connection count.
        :param putters: a list of Putter instances
        :param req: request
        :param min_conns: minimum number of putter connections required
        """
        if req.if_none_match is not None and '*' in req.if_none_match:
            statuses = [
                putter.resp.status for putter in putters if putter.resp]
            if HTTP_PRECONDITION_FAILED in statuses:
                # If we find any copy of the file, it shouldn't be uploaded
                self.app.logger.debug(
                    _('Object PUT returning 412, %(statuses)r'),
                    {'statuses': statuses})
                raise HTTPPreconditionFailed(request=req)

The swift object server returns 412 if any of its 'putters' return 412. I am not sure exactly but I assume putters are workers which store the object - so if any of them throw a 412, presumably because they found a replica, then we get the 412 back.

So if, as you say you found a stray replica then I think we would expect swift to return 412.

this swift stuff is very tricky due to its eventual consistency nature. i agree with you that removing the "if-none-match" may leed to more bugs.

Yes. And my understanding is that when doing create / delete operations swift requires a quorum of successful writes to disk across its storage nodes to report that operation was successful. E.g. When an object is created and distributed to multiple replica's swift will report success as long as more than half of the writes are sucessful.

Which is interesting because then the conditions under which swift will report 412 appear to be much stricter / assume a much higher level of consistency that the requirements that swift reports that an object was successfully created (and/or deleted)

One other thing I wondered about was if this bug was relevant: https://bugs.launchpad.net/swift/+bug/1455221

As we are setting the Delete-After header

https://github.com/hashicorp/terraform/blob/master/backend/remote-state/swift/client.go#L29

    lockTTL = 60 * time.Second

https://github.com/hashicorp/terraform/blob/master/backend/remote-state/swift/client.go#L148-L150

    if err := c.writeLockInfo(info, lockTTL, "*"); err != nil {
        return "", err
    }

https://github.com/hashicorp/terraform/blob/master/backend/remote-state/swift/client.go#L388-L389

func (c *RemoteClient) writeLockInfo(info *statemgr.LockInfo, deleteAfter time.Duration, ifNoneMatch string) error {
    err := c.put(c.lockFilePath(), info.Marshal(), int(deleteAfter.Seconds()), ifNoneMatch)

I haven't looked very far into it but skimming it, it may seem relevant.

yanndegat commented 3 years ago

hi @iokiwi

I think the state files are being versioned but the lock files are not being versioned (or at least multiple versions of the lock file should not be being created) because we are using If-None-Match: * to prevent a lock file from being created if a lock file already exists.

no the lock file is also versioned:

so the lock file is versioned as soon as your apply is longer than TTL, it's exactly versioned (apply_time)/TTL

currently, the system is working, because there's a loop in the Unlock function to delete the lock file until we hit a 404. but this loop has a timeout. cf: https://github.com/hashicorp/terraform/blob/master/backend/remote-state/swift/client.go#L218

i think we should handle this system in a more appropriate way. either by being sure we remove all versions, or by not versioning the lock.

iokiwi commented 3 years ago

Okay thanks for explaining @yanndegat and thanks for your patience.

I totally missed the the lock renewal mechanism creating new versions of the lock. In my testing I was not waiting long enough to trigger the renew. I will try to reproduce.

In terms of fixing this issues, I think the loop solution is probably much simpler.

Also how does the S3 backend work? Maybe we could check if it handles the same problem

Final word on Resource not found

Sorry to persist on this but the small niggle I can't get past is that the error message in your original report was Error: Error locking state: Error acquiring the state lock: Couldn't read lock info: Resource not found.

Resource not found implies that no lock exists - not that the deletes lock has been replaced by an older version.

In my tests I also wasn't waiting long enough to hit the TTL really - I never saw a versioned lock file and shouldn't have hit your bug. So I think I have run into another separate bug in addition to the bug you reported. I will reassess after your bug has been fixed and if it persists, report my bug separately.

iokiwi commented 3 years ago

I wrote a script (gist) to try reproduce your bug and found an interesting result which in some ways reconciles and answers my question above.

Listing objects in container: test-container-2
test-object-1
test-object-5

Listing objects in container: test-container-2-archive
00dtest-object-5/1610074269.14533

Showing object: test-container-2 | test-object-5
Resource not found
Resource not found

Swift list objects but you cannot download the object.

Same result via CLI

$ openstack object list test-container-2
+---------------+
| Name          |
+---------------+
| test-object-1 |
| test-object-5 |
+---------------+

$ openstack object save test-container-2 test-object-5
Not Found (HTTP 404)

Likely a consistency/replication/timing problem on swift's side? I'm not sure there is any code we could write client side to work around an issue like this.

iokiwi commented 3 years ago

Also how does the S3 backend work? Maybe we could check if it handles the same problem

S3 is using dynamo db for state locking not the s3 bucket itself. https://www.terraform.io/docs/backends/types/s3.html

I run into the same consistency issue demonstrated above when using an un-versioned container as well.

I think the issue here is that swift is fundamentally not fit for purpose for storing the lock files. Locking requires strong consistency in a way that swift is explicitly not designed to provide. https://julien.danjou.info/openstack-swift-consistency-analysis/

If we refer to the CAP theorem, Swift chose availability and partition tolerance and dropped consistency. That means that you'll always get your data, they will be dispersed on many places, but you could get an old version of them (or no data at all) in some odd cases (like some server overload or failure). This compromise is made to allow maximum availability and scalability of the storage platform.

Amazon's S3 is the same. https://www.edureka.co/blog/understanding-amazon-s3/

S3 Data Consistency Model

Dr. Werner Vogels masterminded the Amazon Web Services. According to Gartner Research, Amazon leads in public IWS market with 80% share. The CAP theorem talks about consistency, availability and partition model. Amazon goes with the concept of the CAP theorem without consistency.

And the S3 backend recognises this by storing state in dynamo_db.

So the question is if storing the state lock file in swift is a no go, should the swift backend become a backend without locking or should it become a backend with locking via another strongly consistent service - and if so which service?

apparentlymart commented 1 year ago

Hello! Thanks for reporting this.

We removed the Swift backend in Terraform v1.3 because it hasn't had a dedicated maintainer for a few versions now and so it was becoming problematic to keep working in newer Terraform versions. Since that backend is no longer present in the Terraform codebase, I'm going to close this issue.

If you're currently using this backend with an older version of Terraform, see Removal of Deprecated State Storage Backends in the Terraform v1.3 upgrade guide for some information on the available options for migration.

Thanks again!

github-actions[bot] commented 1 year ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.