orchestra/remote: extend mktemp() to accept data by rishabh-d-dave · Pull Request #1634 · ceph/teuthology

rishabh-d-dave · 2021-03-26T11:00:08Z

No description provided.

teuthology/orchestra/remote.py

batrick · 2021-03-26T17:25:25Z

teuthology/orchestra/remote.py

+            kwargs['path'] = self.mktemp(kwargs.pop('suffix',  None),
+                                         kwargs.pop('parentdir', None))


Suggested change

kwargs['path'] = self.mktemp(kwargs.pop('suffix', None),

kwargs.pop('parentdir', None))

kwargs['path'] = self.mktemp(suffix=suffix, parentdir=parentdir)

teuthology/orchestra/remote.py

kshtsk

I suggest to just use mktemp with data paremeter, then adding another one write_file method.

rishabh-d-dave · 2021-03-30T09:26:23Z

Removed write_temp_file() and upgraded mktemp() instead in last push.

teuthology/orchestra/remote.py

kshtsk · 2021-03-30T10:06:29Z

teuthology/orchestra/remote.py

+            path = self.sh(args).strip()
+
+        if data:
+            # sudo is set to False since root user can't write files in /tmp


this is strange clause, root user can write to any directory in fact

And btw, if the file is created by other user, than current, you must use sudo, but the whole idea of temporary files is to use the same user you operate.

No, it can not anymore. Try writing files using sudo on kernel version post 4.19, I guess. Try ubuntu 20. This is what the entire is about. Please take a look at the commit message/PR description and at the ticket linked.

Ah... I see it is because of the new kernel feature, but anyways, why would you explicitly declare False, if it is already a default value?

As for https://tracker.ceph.com/issues/49466 this looks like a test bug, not the teuthology. The client code should care about tracking users or just always use default and not use sudo_write_file when it is not needed.

Ah... I see it is because of the new kernel feature, but anyways, why would you explicitly declare False, if it is already a default value?

So that the reader know it's intentional and required even if the reader isn't aware of context.

It is self explanatory, if you create a random file and use write_file api, it is with user without sudo.

It is self explanatory, if you create a random file and use write_file api, it is with user without sudo.

If it was, I wouldn't have written this in the first place - #1634 (comment). :)

I am sorry :-) I don't get it, what can be read differently when we have sudo_write_file and write_file? Why would we have sudo_write_file if write_file would have sudo=True?

It's worth mentioning why we need sudo should be set ot False IMHO (the note is more comprehensible if it's immediately visible that sudo is set to False). However, I've gotten rid of the comment and "redundant" parameter from the call.

kshtsk · 2021-03-30T10:26:56Z

The bug referred in the commit message is related to the code from
qa/tasks/cephfs/cephfs_test_case.py:

    def create_keyring_file(self, remote, keyring):
        keyring_path = remote.run(args=['mktemp'], stdout=StringIO()).\
            stdout.getvalue().strip()
        sudo_write_file(remote, keyring_path, keyring)

        # required when triggered using vstart_runner.py.
        remote.run(args=['chmod', '644', keyring_path])

        return keyring_path

And rather be fixed in it instead of trying to fix teuthology api.

kshtsk · 2021-03-30T10:31:18Z

Why just instead of:

        sudo_write_file(remote, keyring_path, keyring)

        # required when triggered using vstart_runner.py.
        remote.run(args=['chmod', '644', keyring_path])

write:

        remote.write_file(keyring_path, keyring, mode='0644')

(wondering if new kernel allows to use sudo for changing mode and ownership in tmp files?)

rishabh-d-dave · 2021-03-30T10:40:53Z

The bug referred in the commit message is related to the code:

    def create_keyring_file(self, remote, keyring):
        keyring_path = remote.run(args=['mktemp'], stdout=StringIO()).\
            stdout.getvalue().strip()
        sudo_write_file(remote, keyring_path, keyring)

        # required when triggered using vstart_runner.py.
        remote.run(args=['chmod', '644', keyring_path])

        return keyring_path

Instead of teuthology api.

I never said the bug is in teuthology API. About the code referred to in the commit message , it's already being fixed. PTAL at ceph/ceph#40431.

kshtsk · 2021-03-30T10:42:25Z

So why this change is required? I don't think we need it.

rishabh-d-dave · 2021-03-30T10:42:34Z

sudo_write_file(remote, keyring_path, keyring)

write:

remote.write_file(keyring_path, keyring)

No, that won't fix the issue. What is need is -

keyring_path = remote.mktemp()
remote.write_file(keyring_path, keyring, sudo=False)

But what is much much better than this is -

keyring_path = remote.mktemp(data=keyring)

This is simpler and more powerful because I get to create and write temporary file in a single call. But, much more importantly, it closes the possibility of having a test failure because someone forgot to set sudo to False in possibly one of many calls to write_file().

kshtsk · 2021-03-30T10:44:51Z

The bug referred in the commit message is related to the code:
    def create_keyring_file(self, remote, keyring):
        keyring_path = remote.run(args=['mktemp'], stdout=StringIO()).\
            stdout.getvalue().strip()
        sudo_write_file(remote, keyring_path, keyring)

        # required when triggered using vstart_runner.py.
        remote.run(args=['chmod', '644', keyring_path])

        return keyring_path
Instead of teuthology api.
I never said the bug is in teuthology API. About the code referred to in the commit message , it's already being fixed. PTAL at ceph/ceph#40431.

You said this "Helps fix https://tracker.ceph.com/issues/49466" which is not true.

kshtsk · 2021-03-30T10:51:18Z

sudo_write_file(remote, keyring_path, keyring)

write:

remote.write_file(keyring_path, keyring)

No, that won't fix the issue. What is need is -

keyring_path = remote.mktemp()

It is the same as: keyring_path = remote.run(args=['mktemp'], stdout=StringIO()).\ stdout.getvalue().strip()

remote.write_file(keyring_path, keyring, sudo=False)

As I said, teuthology api sudo=False is default, you should not point it exclusively, so there is no difference.
The remote.mktemp()

But what is much much better than this is -
keyring_path = remote.mktemp(data=keyring)

It depends on your needs, I agree it is more handy, as I mentioned earlier.

This is simpler and more powerful because I get to create and write temporary file in a single call. But, much more importantly, it closes the possibility of having a test failure because someone forgot to set sudo to False in possibly one of many calls to write_file().

Again, we have standard api write_file, and we change it to sudo=True unlikely, because it is odd.

rishabh-d-dave · 2021-03-30T10:52:08Z

@kshtsk

You said this "Helps fix https://tracker.ceph.com/issues/49466" which is not true.

This code keyring_path = remote.mktemp(data=keyring) cannot work without this PR. So this PR is helping out the fix for the tracker issue.

It's best not to just fix the bug but to also fix the root cause of the bug, so that it can't occur again.

kshtsk · 2021-03-30T10:58:53Z

Summarising:

This PR does not help or resolve the new feature of kernel to protect files in /tmp and it becomes more like extending of mktemp usability, so it is needed only to add data argument and use write_file not sudo_write_file. No need to use 'sudo=False', default operations should be sudo less as much as possible.
The commit message does not correspond the idea of the PR, and have to be changed appropriately.

kshtsk · 2021-03-30T11:02:04Z

@kshtsk

You said this "Helps fix https://tracker.ceph.com/issues/49466" which is not true.

This code keyring_path = remote.mktemp(data=keyring) cannot work without this PR. So this PR is helping out the fix for the tracker issue.

But you still can use standard API to fix the issue, so no need to mention it here, so it is not necessary to have it.

It's best not to just fix the bug but to also fix the root cause of the bug, so that it can't occur again.

You are not fixing the root cause of the bug with this PR. The root cause was incorrect usage of sudo_write_file in test.

rishabh-d-dave · 2021-03-30T12:00:07Z

@batrick I've removed every mention of why sudo must not be used from this PR in last update

teuthology/orchestra/remote.py

kshtsk · 2021-03-30T21:23:53Z

@susebot run deploy

susebot · 2021-03-30T22:43:56Z

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

Extend remote.Remote.mktemp() to accept data as a parameter and write the data to the temporary file after it is created. Signed-off-by: Rishabh Dave <ridave@redhat.com>

kshtsk

Looks good to go now, thanks for the patience, and sorry that it took that much time.

kshtsk · 2021-03-31T10:23:05Z

@batrick I think your change request is outdated.

Dismissing change request because it was outdated with new commits and become irrelevant.

rishabh-d-dave · 2021-04-01T09:58:41Z

@kshtsk @batrick Thanks!

rishabh-d-dave requested review from batrick and kshtsk March 26, 2021 11:00

rishabh-d-dave mentioned this pull request Mar 26, 2021

qa/cephfs: don't use sudo to write files in /tmp ceph/ceph#40431

Merged

1 task

batrick approved these changes Mar 26, 2021

View reviewed changes

batrick previously requested changes Mar 26, 2021

View reviewed changes

kshtsk reviewed Mar 26, 2021

View reviewed changes

teuthology/orchestra/remote.py Outdated Show resolved Hide resolved

kshtsk requested changes Mar 26, 2021

View reviewed changes

rishabh-d-dave force-pushed the no-sudo-for-tmp-files branch from 18caf08 to 69ad856 Compare March 30, 2021 09:25

rishabh-d-dave force-pushed the no-sudo-for-tmp-files branch from 69ad856 to ebf4288 Compare March 30, 2021 09:28

rishabh-d-dave requested review from batrick and kshtsk March 30, 2021 09:30