Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Implement MountSnapshotJob / UnmountSnapshotJob #2156

Merged
merged 11 commits into from
Aug 18, 2020
Merged

Implement MountSnapshotJob / UnmountSnapshotJob #2156

merged 11 commits into from
Aug 18, 2020

Conversation

mkpankov
Copy link
Contributor

@mkpankov mkpankov commented Aug 13, 2020

Signed-off-by: Michael Pankov work@michaelpankov.com


This change is Reviewable

@mkpankov
Copy link
Contributor Author

It works like this:

[root@mds1 vagrant]# lctl snapshot_list -F zfsmo 

filesystem_name: zfsmo
snapshot_name: test1
snapshot_fsname: 1494052f 
create_time: Mon Jul 27 14:09:53 2020
modify_time: Mon Jul 27 14:09:53 2020
status: not mount
➜  ~ jarg message="Test command" 'jobs[0][class_name]=MountSnapshotJob' 'jobs[0][args][hosts][0]=/api/host/1/' 'jobs[0][args][fsname]=zfsmo' 'jobs[0][args][name]=test1' | http --verbose --json --verify=false https://localhost:8443/api/command/ authorization:'ApiKey api:15ec8aca8b4139ad741e4bbf9157b1e193a07a52'
POST /api/command/ HTTP/1.1
Accept: application/json, */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Content-Length: 146
Content-Type: application/json
Host: localhost:8443
User-Agent: HTTPie/1.0.3
authorization: ApiKey api:15ec8aca8b4139ad741e4bbf9157b1e193a07a52

{
    "jobs": [
        {
            "args": {
                "fsname": "zfsmo",
                "hosts": [
                    "/api/host/1/"
                ],
                "name": "test1"
            },
            "class_name": "MountSnapshotJob"
        }
    ],
    "message": "Test command"
}

HTTP/1.1 201 Created
Connection: keep-alive
Content-Length: 212
Content-Type: application/json
Date: Thu, 13 Aug 2020 10:59:33 GMT
Location: /api/command/30/
Server: nginx/1.16.1
Vary: Accept
X-Frame-Options: SAMEORIGIN

{
    "cancelled": false,
    "complete": false,
    "created_at": "2020-08-13T10:59:33.597354",
    "errored": false,
    "id": 30,
    "jobs": [
        "/api/job/42/"
    ],
    "logs": "",
    "message": "Test command",
    "resource_uri": "/api/command/30/"
}
[root@mds1 vagrant]# lctl snapshot_list -F zfsmo 

filesystem_name: zfsmo
snapshot_name: test1
snapshot_fsname: 1494052f 
create_time: Mon Jul 27 14:09:53 2020
modify_time: Mon Jul 27 14:09:53 2020
status: mounted

@mkpankov mkpankov self-assigned this Aug 13, 2020
@mkpankov mkpankov added this to the IML EX V3 milestone Aug 13, 2020
@mkpankov mkpankov requested review from jgrund and a team August 13, 2020 11:05
@mkpankov
Copy link
Contributor Author

@jgrund dependencies and locks are missing, but I wanted to clarify if the approach taken is correct with respect to "DB calls should be made in Rust" as specified in the issue.

Since the state machine stays in Python and we need to save the Job itself, I guess these don't count as DB calls?

chroma_core/models/host.py Outdated Show resolved Hide resolved
chroma_core/models/host.py Outdated Show resolved Hide resolved
@jgrund
Copy link
Member

jgrund commented Aug 13, 2020

Since the state machine stays in Python and we need to save the Job itself, I guess these don't count as DB calls?

Yes, I'm more talking about associated queries needed to get related objects.

chroma_core/models/host.py Outdated Show resolved Hide resolved
@mkpankov
Copy link
Contributor Author

I've tested this with non-imported pools and no dependencies were necessary besides ldev.conf.

ip1981
ip1981 previously approved these changes Aug 14, 2020
@jgrund
Copy link
Member

jgrund commented Aug 14, 2020

I've tested this with non-imported pools and no dependencies were necessary besides ldev.conf.

At the very least the filesystem needs to be in the available state. Mounting a snapshot should not work if the fs is unavailable I think

@mkpankov mkpankov requested a review from jgrund August 14, 2020 13:53
@mkpankov mkpankov changed the title Implement MountSnapshotJob Implement MountSnapshotJob / UnmountSnapshotJob Aug 14, 2020
@mkpankov mkpankov requested a review from a team August 17, 2020 13:56
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
Signed-off-by: Michael Pankov <work@michaelpankov.com>
@mkpankov mkpankov requested a review from ip1981 August 18, 2020 09:07
ip1981
ip1981 previously approved these changes Aug 18, 2020
@mkpankov
Copy link
Contributor Author

So, I've retested everything.

  1. Mounting existing snapshot doesn't actually require running lustre FS. It works after ZFS pools are imported with import-pools.
  2. With DependOn(ManagedFilesystem.objects.get(name=self.fsname), "available") as in current version, it does require a running lustre FS that is detected by IML.

Signed-off-by: Igor Pashev <pashev.igor@gmail.com>
# To prevent circular imports
from chroma_core.models.filesystem import ManagedFilesystem

return DependOn(ManagedFilesystem.objects.get(name=self.fsname), "available")
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to check if this is needed for ldiskfs since it's apparently not needed with zfs

@jgrund jgrund requested a review from ip1981 August 18, 2020 13:12
@@ -1785,6 +1785,37 @@ class NoNidsPresent(Exception):
pass


class CreateSnapshotJob(Job):
fqdn = models.CharField(max_length=256, help_text="MGS host to create the snapshot on")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine for now but in the future, we will have a local action plugin that we can call to get the fqdn of the MGS node.

@@ -196,4 +196,8 @@
"revoke_ticket": "Revoking Ticket",
"forget_ticket": "Forgetting Ticket",
"create_task": "Creating Task",
"mount_snapshot": "Mounting Snapshot",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really a big deal but the long descriptions are shorter than the standard descriptions. You could store the values that you have under description in this help.py file and then call them:

 @classmethod
    def long_description(cls, stateful_object):
        return help_text["unmount_snapshot"] % stateful_object.fqdn

    def description(self):
        return self.long_description(self)

@jgrund jgrund merged commit 5b4c1d2 into master Aug 18, 2020
@jgrund jgrund deleted the mount-job branch August 18, 2020 16:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants