Skip to content

Conversation

@dadoonet
Copy link
Contributor

While working on #26751 and doing some manual integration testing I found that this #22858 removed an important line of our code:

AzureRepository overrides default initializeSnapshot method which creates metadata files and do other stuff.

But with PR #22858, I wrote:

    @Override
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
    }

instead of

    @Override
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
        super.initializeSnapshot(snapshotId, indices, clusterMetadata);
    }

As we never call super.initializeSnapshot(...) files are not created and we can't restore what we saved.

Closes #26777.

While working on elastic#26751 and doing some manual integration testing I found that this elastic#22858 removed an important line of our code:

`AzureRepository` overrides default `initializeSnapshot` method which creates metadata files and do other stuff.

But with PR elastic#22858, I wrote:

```java
    @OverRide
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
    }
```

instead of

```java
    @OverRide
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
        super.initializeSnapshot(snapshotId, indices, clusterMetadata);
    }
```

As we never call `super.initializeSnapshot(...)` files are not created and we can't restore what we saved.

Closes elastic#26777.
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

The changes look good, but we need to figure out the test story here. That shouldn't have happened in the first place.

@dadoonet
Copy link
Contributor Author

@imotov Agreed. So I discovered that while running AzureSnapshotRestoreTests#testSimpleWorkflow() with -Dtests.thirdparty=true.

The problem is that we never run automatically those tests. The manual tests I did previously were sadly:

  • install the plugin
  • start elasticsearch
  • create an index and some data
  • create the repository
  • create a snapshot

But I did not test:

  • delete the index
  • restore the snapshot

That's why I did not hit the issue.

And BTW IT are not compatible anymore with 6.0 / 7.0 because we are now using secured settings.

That's why I also did:

Which I'm planning to backport anyway to 6.0 branch.

Do you want me to backport within this PR the IT changes instead of doing it in #26751?

@imotov
Copy link
Contributor

imotov commented Sep 25, 2017

@dadoonet I don't think it really matters if it goes as a part of this PR or another PR. As long as we have a test that would have failed without this change at the end I am fine with either.

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM

@s1monw
Copy link
Contributor

s1monw commented Sep 25, 2017

lets merge this.

@jasontedor jasontedor merged commit 3f71772 into elastic:master Sep 25, 2017
jasontedor pushed a commit that referenced this pull request Sep 25, 2017
While working on #26751 and doing some manual integration testing I found that this #22858 removed an important line of our code:

`AzureRepository` overrides default `initializeSnapshot` method which creates metadata files and do other stuff.

But with PR #22858, I wrote:

```java
    @OverRide
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
    }
```

instead of

```java
    @OverRide
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
        super.initializeSnapshot(snapshotId, indices, clusterMetadata);
    }
```

As we never call `super.initializeSnapshot(...)` files are not created and we can't restore what we saved.

Closes #26777.
jasontedor pushed a commit that referenced this pull request Sep 25, 2017
While working on #26751 and doing some manual integration testing I found that this #22858 removed an important line of our code:

`AzureRepository` overrides default `initializeSnapshot` method which creates metadata files and do other stuff.

But with PR #22858, I wrote:

```java
    @OverRide
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
    }
```

instead of

```java
    @OverRide
    public void initializeSnapshot(SnapshotId snapshotId, List<IndexId> indices, MetaData clusterMetadata) {
        if (blobStore.doesContainerExist(blobStore.container()) == false) {
            throw new IllegalArgumentException("The bucket [" + blobStore.container() + "] does not exist. Please create it before " +
                " creating an azure snapshot repository backed by it.");
        }
        super.initializeSnapshot(snapshotId, indices, clusterMetadata);
    }
```

As we never call `super.initializeSnapshot(...)` files are not created and we can't restore what we saved.

Closes #26777.
@dadoonet dadoonet deleted the fix/26777-azure-meta-missing branch September 26, 2017 01:13
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@clintongormley clintongormley added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository Azure labels Feb 14, 2018
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.

7 participants