Add remote file operations #1538

kshtsk · 2020-07-16T16:26:04Z

No description provided.

kshtsk · 2020-07-16T16:36:52Z

>>> l=r('localhost')
>>> l.write_file('/tmp/dir/subdir/deep/deep/subdir/file', b'zzz', mode='0600', mkdir=True)
2020-07-16 18:33:34,795.795 INFO:teuthology.orchestra.remote:Trying to reconnect to host
2020-07-16 18:33:34,873.873 INFO:teuthology.orchestra.run.localhost:> true
2020-07-16 18:33:35,073.073 INFO:teuthology.orchestra.run.localhost:> set -ex
2020-07-16 18:33:35,073.073 INFO:teuthology.orchestra.run.localhost:> mkdir -p /tmp/dir/subdir/deep/deep/subdir
2020-07-16 18:33:35,074.074 INFO:teuthology.orchestra.run.localhost:> dd of=/tmp/dir/subdir/deep/deep/subdir/file
2020-07-16 18:33:35,074.074 INFO:teuthology.orchestra.run.localhost:> chmod 0600 /tmp/dir/subdir/deep/deep/subdir/file
2020-07-16 18:33:35,214.214 INFO:teuthology.orchestra.run.localhost.stderr:+ mkdir -p /tmp/dir/subdir/deep/deep/subdir
2020-07-16 18:33:35,217.217 INFO:teuthology.orchestra.run.localhost.stderr:+ dd of=/tmp/dir/subdir/deep/deep/subdir/file
2020-07-16 18:33:35,219.219 INFO:teuthology.orchestra.run.localhost.stderr:0+1 records in
2020-07-16 18:33:35,219.219 INFO:teuthology.orchestra.run.localhost.stderr:0+1 records out
2020-07-16 18:33:35,219.219 INFO:teuthology.orchestra.run.localhost.stderr:3 bytes copied, 8.0701e-05 s, 37.2 kB/s
2020-07-16 18:33:35,219.219 INFO:teuthology.orchestra.run.localhost.stderr:+ chmod 0600 /tmp/dir/subdir/deep/deep/subdir/file

and:

> ls -la /tmp/dir/subdir/deep/deep/subdir/file
-rw------- 1 kyr users 3 Jul 16 18:33 /tmp/dir/subdir/deep/deep/subdir/file

kshtsk · 2020-07-16T16:37:08Z

@susebot run deploy

susebot · 2020-07-16T18:11:57Z

Commit ba1732a is OK.
Check tests results in the Jenkins job: https://ceph-ci.suse.de/job/pr-teuthology-deploy/237/

kshtsk · 2020-07-16T18:33:09Z

@susebot run deploy

susebot · 2020-07-16T20:02:53Z

Commit afb5e5a is OK.
Check tests results in the Jenkins job: https://ceph-ci.suse.de/job/pr-teuthology-deploy/238/

rishabh-d-dave

Looks perfect otherwise!

rishabh-d-dave · 2020-07-17T11:41:44Z

teuthology/orchestra/remote.py

+        args = 'set -ex' + '\n' + args
+        self.run(args=args, stdin=data)
+
+    def sudo_write_file(self, path, data, **kwargs):


IMHO, sudo_write_file() is not super-convenient compared to write_file(path, data, sudo=True). So I won't mind not having sudo_write_file().

But, yes, not having it would break many tests.

I can delete it, I don't it can break anything because it is new function, I added because someone might think it handy to use sudo_write_file because of shorter syntax.

But, yes, not having it would break many tests.

this is why we do not delete teuthology.misc.sudo_write_file but make it use remote.write_file(sudo=True)

i'd like to see PRs on ceph side which use the newly added functions. otherwise these new methods are just unused and untested.

@tchaikov it is already used there, through misc.sudo_write_file, check there logs:
https://pulpito.ceph.com/kyr-2020-07-16_20:09:04-rados:cephadm-master-distro-basic-smithi/
You can find there a 'dd' command.
Even if you don't see it used in upstream ceph, it does not mean we (mostly me) do not plan to use it downstream.
I agree we can add more test cases, however it is "tested" through misc api.

I don't want to refactor and go through the PRs on ceph side now and go through the all cycles of merging and backporting.
I'm more comfortable to do this later, this is why a deprication message.

@tchaikov what do you have on mind, to backport it to 'octopus', 'nautilus', or also py2 depended branches 'luminous' and correspondingly backport this PR to teuthology py2?

@tchaikov is that kind of PR do you expect? ceph/ceph#36211

[OT] also, please let me know if you are going to work on ceph/ceph#35674 (comment) ? if not, i will cherry-pick the fixes.

Do you suggest to finish it before merging this PR?

[OT] also, please let me know if you are going to work on ceph/ceph#35674 (comment) ? if not, i will cherry-pick the fixes.

Do you suggest to finish it before merging this PR?

no, it's not related to this changeset. i marked "[OT]" (short for off-topic) here. as per your reply, i just realized that you are not likely to work on backports. hence the question.

no, it's not related to this changeset. i marked "[OT]" (short for off-topic) here. as per your reply, i just realized that you are not likely to work on backports. hence the question.

thanks for the remark, sorry sometimes I'm bad to decode things like "ot". Regarding backport, sooner or later I might work on it. But not right now. So go ahead and cherry-pick it.

rishabh-d-dave · 2020-07-17T12:05:13Z

teuthology/orchestra/remote.py

+            chown = 'sudo chown' if sudo else 'chown'
+            args += '\n' + chown + ' ' + owner + ' ' + path
+        args = 'set -ex' + '\n' + args
+        self.run(args=args, stdin=data)


Oh, I must also mention that I would prefer returning the return value of self.run() since access to the object representing the process can allow better testing and troubleshooting. Applies to this method as well as to other methods here and in misc.py.

As I said, we don't need trouble shoot write_file (it is a utility function which always should work), we need to stop as fast as possible and analize logs, even more, it is bad to continue execution because it make more troubles to figure out the first source of the failure.

So, if we want more control on error handling I would prefer to specific exception produced by the file operation, so the user don't need to check output of the script of write_file. We need to declare all specific to the function exception in the doc for the function and we must not let user to do this instead of us, because the code of script can be changed and will be very difficult to support. Basically user should not use anything about internals of functions like write_file and treat it as blackbox.

tchaikov · 2020-07-19T15:12:33Z

setup.py

@@ -49,6 +49,7 @@
        'Topic :: System :: Filesystems',
    ],
    install_requires=['apache-libcloud',
+                      'Deprecated >= 1.2.10',


instead of introducing yet another dependency, i'd suggest just update all the callers of the deprecated functions, and then drop them.

what is the problem with introducing new dependency which is useful?

i'd suggest just update all the callers of the deprecated functions, and then drop them.

@tchaikov I've dropped the dependency.

kshtsk · 2020-07-24T19:58:26Z

retest this please

Add advanced write_file and sudo_write_file to remote context for more intuitive and handy coding Signed-off-by: Kyr Shatskyy <kyrylo.shatskyy@suse.com>

Deprecate misc.write_file() and misc.sudo_write_file() in favor of the orchestra.remote package methods. The code of the misc's methods is now calling the remote's ones. Signed-off-by: Kyr Shatskyy <kyrylo.shatskyy@suse.com>

The misc.append_lines_to_file method has been using get remote file to temporary file and then write back to remote host. The orchestra.remote.write_file method has 'append' parameter when using which there is no data downloaded from remote host. Signed-off-by: Kyr Shatskyy <kyrylo.shatskyy@suse.com>

kshtsk · 2020-09-03T21:47:14Z

@susebot run deploy

susebot · 2020-09-03T23:46:08Z

Commit 2790fa9 is OK.
Check tests results in the Jenkins job: https://ceph-ci.suse.de/job/pr-teuthology-deploy/251/

kshtsk · 2020-09-04T12:38:59Z

@susebot run deploy

Signed-off-by: Kyr Shatskyy <kyrylo.shatskyy@suse.com>

susebot · 2020-09-04T13:43:04Z

Commit e13a356 is OK.
Check tests results in the Jenkins job: https://ceph-ci.suse.de/job/pr-teuthology-deploy/252/

tchaikov · 2020-09-04T13:59:02Z

teuthology/orchestra/remote.py

+            chown = 'sudo chown' if sudo else 'chown'
+            args += '\n' + chown + ' ' + owner + ' ' + dst
+        args = 'set -ex' + '\n' + args
+        self.run(args=args)


i'd suggest rewrite this using a helper function for adding optional sudo and f-string, but my suggestion was dropped by accident when posting it. i will add a follow-up PR later on.

thanks for the heads up, I also would like to backport this patch to py2, however I'm not sure that I will do that, hopefully we will not need py2 based branches of teuthology downstream in couple of months.

kshtsk changed the title ~~Wip remote write file~~ remote write file Jul 16, 2020

kshtsk mentioned this pull request Jul 16, 2020

misc: upgrade sudo_write_file() #1537

Closed

kshtsk requested a review from tchaikov July 16, 2020 20:11

kshtsk changed the title ~~remote write file~~ remote file operations Jul 17, 2020

rishabh-d-dave previously approved these changes Jul 17, 2020

View reviewed changes

rishabh-d-dave reviewed Jul 17, 2020

View reviewed changes

tchaikov reviewed Jul 19, 2020

View reviewed changes

kshtsk dismissed rishabh-d-dave’s stale review via 1060c6a July 20, 2020 13:19

kshtsk force-pushed the wip-remote-write-file branch from 1060c6a to b58a76c Compare July 20, 2020 15:45

kshtsk added the DNM label Jul 20, 2020

kshtsk changed the title ~~remote file operations~~ [WIP] remote file operations Jul 20, 2020

kshtsk force-pushed the wip-remote-write-file branch 2 times, most recently from 57c7654 to a10871e Compare July 21, 2020 16:51

kshtsk force-pushed the wip-remote-write-file branch from a10871e to 1f74b1c Compare August 5, 2020 13:26

kshtsk removed the DNM label Aug 13, 2020

orchestra/remote: add (sudo_)write_file method

a58d1cf

Add advanced write_file and sudo_write_file to remote context for more intuitive and handy coding Signed-off-by: Kyr Shatskyy <kyrylo.shatskyy@suse.com>

kshtsk force-pushed the wip-remote-write-file branch from 1f74b1c to 8d001b1 Compare September 3, 2020 21:35

Kyr Shatskyy added 2 commits September 3, 2020 23:43

misc: deprecate (sudo_)write_file methods

45aba9c

Deprecate misc.write_file() and misc.sudo_write_file() in favor of the orchestra.remote package methods. The code of the misc's methods is now calling the remote's ones. Signed-off-by: Kyr Shatskyy <kyrylo.shatskyy@suse.com>

kshtsk force-pushed the wip-remote-write-file branch from 8d001b1 to 2790fa9 Compare September 3, 2020 21:44

kshtsk changed the title ~~[WIP] remote file operations~~ remote file operations Sep 3, 2020

kshtsk changed the title ~~remote file operations~~ Add remote file operations Sep 3, 2020

kshtsk requested review from tchaikov and rishabh-d-dave September 4, 2020 13:00

orchestra.remote: add move_file, copy_file and read_file

ab6fa1d

Signed-off-by: Kyr Shatskyy <kyrylo.shatskyy@suse.com>

kshtsk force-pushed the wip-remote-write-file branch from e13a356 to ab6fa1d Compare September 4, 2020 13:27

kshtsk mentioned this pull request Sep 4, 2020

use remote write file ceph/ceph#36211

Merged

3 tasks

tchaikov approved these changes Sep 4, 2020

View reviewed changes

tchaikov reviewed Sep 4, 2020

View reviewed changes

kshtsk merged commit c6539b8 into ceph:master Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remote file operations #1538

Add remote file operations #1538

kshtsk commented Jul 16, 2020

kshtsk commented Jul 16, 2020 •

edited

kshtsk commented Jul 16, 2020

susebot commented Jul 16, 2020

kshtsk commented Jul 16, 2020

susebot commented Jul 16, 2020

rishabh-d-dave left a comment

rishabh-d-dave Jul 17, 2020

kshtsk Jul 17, 2020

kshtsk Jul 17, 2020

tchaikov Jul 19, 2020

kshtsk Jul 19, 2020

kshtsk Jul 20, 2020

kshtsk Jul 20, 2020

kshtsk Jul 20, 2020

tchaikov Jul 20, 2020 •

edited

kshtsk Jul 20, 2020

rishabh-d-dave Jul 17, 2020

kshtsk Jul 17, 2020 •

edited

kshtsk Jul 17, 2020

tchaikov Jul 19, 2020

kshtsk Jul 19, 2020

tchaikov Jul 20, 2020

kshtsk Sep 3, 2020

kshtsk commented Jul 24, 2020

kshtsk commented Sep 3, 2020

susebot commented Sep 3, 2020

kshtsk commented Sep 4, 2020

susebot commented Sep 4, 2020

tchaikov Sep 4, 2020

kshtsk Sep 4, 2020

Add remote file operations #1538

Add remote file operations #1538

Conversation

kshtsk commented Jul 16, 2020

kshtsk commented Jul 16, 2020 • edited

kshtsk commented Jul 16, 2020

susebot commented Jul 16, 2020

kshtsk commented Jul 16, 2020

susebot commented Jul 16, 2020

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Jul 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshtsk Jul 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshtsk commented Jul 24, 2020

kshtsk commented Sep 3, 2020

susebot commented Sep 3, 2020

kshtsk commented Sep 4, 2020

susebot commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshtsk commented Jul 16, 2020 •

edited

tchaikov Jul 20, 2020 •

edited

kshtsk Jul 17, 2020 •

edited