DLPX-83697 follow up to fix merge commit

[don-brady]

Context:

The merge with upstream kernel job resets to the last upstream merge, does a merge with all the latest upstream commits and then cherry-picks the Delphix specific commits on top.

Problem:

The cherry-pick command assumes each commit has only one parent and when used against a merge commit (multiple parents) it will fail, as in:

21:01:19  error: commit 0e0789ce8fcd7ad38b8b3e39bcaff537c63ac9ee is a merge but no -m option was given.
21:01:19  fatal: cherry-pick failed
21:01:19  [repo-HEAD 1366ec77e617] target: login should wait until tx/rx threads have properly started.
21:01:19   Author: Paul Dagnelie <pcd@delphix.com>
21:01:19   Date: Tue Nov 8 12:03:32 2022 -0800
21:01:19   3 files changed, 6 insertions(+)
21:01:19  Error: failed command 'git cherry-pick d66906cca92c3afe176b06ee2669b72223a75d79^..0e0789ce8fcd7ad38b8b3e39bcaff537c63ac9ee'

Solution:

This commit removes the problematic merge commit and adds it back as a normal commit.

[prakashsurya]

It can be added back in as a normal commit in a follow-on PR.

Why not just add it correctly in this PR?

[prakashsurya]

The "files changed" tab seems to be misleading.. when I git diff I see this:

± g d gcp/6.0/stage..FETCH_HEAD
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index c738c18ccdcd..f606e82a8c6e 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3813,7 +3813,6 @@ int iscsi_target_tx_thread(void *arg)
     * connection recovery / failure event can be triggered externally.
     */
    allow_signal(SIGINT);
-   complete(&conn->kthr_start_comp);
 
    while (!kthread_should_stop()) {
        /*
@@ -4042,7 +4041,6 @@ int iscsi_target_rx_thread(void *arg)
     * connection recovery / failure event can be triggered externally.
     */
    allow_signal(SIGINT);
-   complete(&conn->kthr_start_comp);
    /*
     * Wait for iscsi_post_login_handler() to complete before allowing
     * incoming iscsi/tcp socket I/O, and/or failing the connection.
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 884db3ddf577..72d10271bbe1 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -646,7 +646,6 @@ int iscsit_start_kthreads(struct iscsi_conn *conn)
        ret = PTR_ERR(conn->tx_thread);
        goto out_bitmap;
    }
-   wait_for_completion(&conn->kthr_start_comp);
    conn->tx_thread_active = true;
 
    conn->rx_thread = kthread_run(iscsi_target_rx_thread, conn,
@@ -656,7 +655,6 @@ int iscsit_start_kthreads(struct iscsi_conn *conn)
        ret = PTR_ERR(conn->rx_thread);
        goto out_tx;
    }
-   wait_for_completion(&conn->kthr_start_comp);
    conn->rx_thread_active = true;
 
    return 0;
@@ -1129,7 +1127,6 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
    init_completion(&conn->rx_half_close_comp);
    init_completion(&conn->tx_half_close_comp);
    init_completion(&conn->rx_login_comp);
-   init_completion(&conn->kthr_start_comp);
    spin_lock_init(&conn->cmd_lock);
    spin_lock_init(&conn->conn_usage_lock);
    spin_lock_init(&conn->immed_queue_lock);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index b3441f2e7c7a..591cd9e4692c 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -562,7 +562,6 @@ struct iscsi_conn {
    struct completion   conn_logout_comp;
    struct completion   tx_half_close_comp;
    struct completion   rx_half_close_comp;
-   struct completion   kthr_start_comp;
    /* socket used by this connection */
    struct socket       *sock;
    void            (*orig_data_ready)(struct sock *);

[prakashsurya]

We should probably also modify the repository configuration to disable merge commits via PRs.

[sebroy]

I think the commit comment associated with @pcd1193182 's last change isn't right. It should be a Jira issue ID, but what you have in your branch's commit history is "target: login should wait until tx/rx threads have properly started. …
…(#21)".

[prakashsurya]

It should be a Jira issue ID,

we don't have any requirement that the commit message that lands, matches the "normal" one-line JIRA format that we use elsewhere (e.g. the app-gate).. so, while I understand the sentiment, I'm not sure why it matters..

for example, sometimes I will edit a PR title, to appeaze the PR check, that requires a JIRA.. but not update the commit message.. such that when the commit lands, it does not have a JIRA number in the commit message.. and that's allowed by our current PR requirements..

FWIW.. the commit message that's there now, comes from the fact that Don pulled the message from the "normal" commit.. and not the merge commit.. i.e. the two have different messages:

commit 960cee3840d4fff9b7161eea9ae9cdbfb9765b32
Merge: 6707a38dbb4b 4776baad7a95
Author: Paul Dagnelie <paul.dagnelie@delphix.com>
Date:   Mon Nov 14 13:34:07 2022 -0800

    DLPX-83697 iscsi target login should wait until tx/rx threads have properly started

commit 4776baad7a95f460619c0777dd852e556d28555f
Author: Paul Dagnelie <pcd@delphix.com>
Date:   Tue Nov 8 12:03:32 2022 -0800

    target: login should wait until tx/rx threads have properly started.

With all that said, I'm perfectly OK using the message of:

DLPX-83697 iscsi target login should wait until tx/rx threads have properly started

I just would rather us enforce these sorts of things via a mechanism other than human reviewers.. since otherwise, it becomes inconsistently enforced (if enforced at all)..

[prakashsurya]

Additionally, it looks like the commit Don cherry-picked, is the commit that's on the AWS repository:

commit 596ac2721f4002fe7729db229ad0f4e1903b13d9
Author: Paul Dagnelie <paul.dagnelie@delphix.com>
Date:   Mon Nov 14 13:49:55 2022 -0800

    target: login should wait until tx/rx threads have properly started. (#21)

see here

which does not have the JIRA ID in it's message.. so as is, at least we're consistent across all the different repositories.. since, as I understand, Don isn't planning to change the AWS repository (since it doesn't have a merge commit to be fixed).

[sebroy]

Ok, it's not a huge deal given that the commit does have a reference to the PR, and then someone looking at this can trace that back to some bug that was fixed in the commit. In general, though, I think we need to get back to a uniform best practice (perhaps enforced by PR checks, which do exist on other repos btw) across engineering of referencing a Jira issue when we commit code changes to repos.

[prakashsurya]

perhaps enforced by PR checks, which do exist on other repos btw

the PR check (I assume) you're talking about, is required on this repository too.. but, that works at the PR title level, and has no bearing on the commit message(s).. so, even if the PR title is of the JIRA form, that doesn't mean the commit(s) that land will use the same format.

I think we need to get back to a uniform best practice

as far as I understand, the problem we have now on github, though.. is we can't add a pre-receive hook that verifies the commit message, like we had via our old/current git-hooks.. so, while I agree with you, I don't know how to actually implement and/or enforce this on github.

as mentioned above, the current PR check that is trying to enforce this, doesn't actually enforce anything w.r.t. the commit message(s).

[prakashsurya]

For posterity's sake.. @jtamagnan-delphix has thought a bit about how to standardize commit messages on github, and put some information into this doc.