Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate SPT threading support on NonStop. #22807

Closed
wants to merge 1 commit into from

Conversation

rsbeckerca
Copy link
Contributor

This fix removes explicit support for the SPT threading model in configurations. This also reverts commit f63e1b4 that were required for SPT but broke other models.

Checklist
  • documentation is added or updated

@rsbeckerca
Copy link
Contributor Author

This pull request has been tested on all threading and unthreading models on NonStop.

Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

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

Some minor wording quibbles, but otherwise looks good.

NOTES-NONSTOP.md Outdated Show resolved Hide resolved
NOTES-NONSTOP.md Outdated Show resolved Hide resolved
This fix removes explicit support for the SPT threading model in configurations.
This also reverts commit f63e1b4 that were
required for SPT but broke other models.

Fixes: openssl#22798

Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca>
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 labels Nov 23, 2023
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

I'd consider splitting this into two commits/PRs. One that reverts the bio_sock change as that should go to 3.2 as well, perhaps mentioning that QUIC might not fully work on SPT. And the other that should go into master branch that removes it completely?

@rsbeckerca
Copy link
Contributor Author

I'd consider splitting this into two commits/PRs. One that reverts the bio_sock change as that should go to 3.2 as well, perhaps mentioning that QUIC might not fully work on SPT. And the other that should go into master branch that removes it completely?

Both changes need to go to both branches. Do you mean two PRs that do the same thing but for two branches?

@rsbeckerca
Copy link
Contributor Author

I'd consider splitting this into two commits/PRs. One that reverts the bio_sock change as that should go to 3.2 as well, perhaps mentioning that QUIC might not fully work on SPT. And the other that should go into master branch that removes it completely?

We also determined that any use of SPT on NonStop would break OpenSSL because of the BIO changes. That model had to be dropped forever. 3.1 and 3.0 are fine because DGRAM is not used, which is where SPT falls down.

@hlandau hlandau removed the approval: otc review pending This pull request needs review by an OTC member label Nov 23, 2023
@t8m
Copy link
Member

t8m commented Nov 23, 2023

If you build 3.2 with no-quic the new things in BIO_dgram won't be used. It looks fairly strange to remove compilation targets on already released stable version. Perhaps the correct fix would be to disable quic for SPT targets automatically on 3.2?

@rsbeckerca
Copy link
Contributor Author

If you build 3.2 with no-quic the new things in BIO_dgram won't be used. It looks fairly strange to remove compilation targets on already released stable version. Perhaps the correct fix would be to disable quic for SPT targets automatically on 3.2?

Wait... 3.2 is not officially release. We are keeping SPT on 3.0 and 3.1. That does not change. This is for 3.2 and master. The reason it does not work for 3.2 is that the DGRAM code requires a part of SPT that enables new processing on fgets that does not work. spt_fgetsx, which is what is used at 3.2 to get BIO to compile with SPT, causes a missed EOF detection on configuration files regardless of no-quic. SPT cannot be part of any 3.2 build at all, regardless of the options selected otherwise it hangs across the board reading openssl.cnf.

@t8m
Copy link
Member

t8m commented Nov 23, 2023

3.2.0 was released today. I understand that you did not release the binaries for NonStop yet but there are no changes going into 3.2.0. As for the source code base the 3.2 is a stable branch and there shouldn't be added changes like removing build targets. I understand that without reverting f63e1b4 the 3.2 is totally unusable for NonStop but that is a downstream change and something that we certainly want to apply upstream before the 3.2.1 release.

@rsbeckerca
Copy link
Contributor Author

3.2.0 was released today. I understand that you did not release the binaries for NonStop yet but there are no changes going into 3.2.0. As for the source code base the 3.2 is a stable branch and there shouldn't be added changes like removing build targets. I understand that without reverting f63e1b4 the 3.2 is totally unusable for NonStop but that is a downstream change and something that we certainly want to apply upstream before the 3.2.1 release.

That is correct. We cannot and will not release 3.2.0 binaries. I wish that was in the notes for it. I will try to tell the entire community that 3.2.0 is unusable.

@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 23, 2023
@t8m
Copy link
Member

t8m commented Nov 24, 2023

IMO this deserves an OTC discussion.

OTC: Is it acceptable to remove the NonStop SPT build targets in 3.2 branch? QUIC cannot be properly supported on SPT.

@t8m t8m added the hold: need otc decision The OTC needs to make a decision label Nov 24, 2023
@hlandau
Copy link
Member

hlandau commented Nov 24, 2023

@rsbeckerca Can you link any NonStop documentation on the different threading models and their merits/uses?

Do you have any data or rough feeling on usage levels of these different threading models?

@beldmit
Copy link
Member

beldmit commented Nov 24, 2023

Do we have to remove this support? Could we just enforce disable-quic for this option?

@rsbeckerca
Copy link
Contributor Author

@rsbeckerca Can you link any NonStop documentation on the different threading models and their merits/uses?

Do you have any data or rough feeling on usage levels of these different threading models?

I have no evidence that anyone is using 3.2 in SPT. There was only one download in our history of SPT for 1.1.1. We know of one company - my personal contacts - who used OpenSSL it, but have since moved to PUT.

@rsbeckerca
Copy link
Contributor Author

rsbeckerca commented Nov 24, 2023

Do we have to remove this support? Could we just enforce disable-quic for this option?

Simply building with SPT will trigger the problem because fgets() is replaced with spt_fgetsx(), which hangs reading openssl.cnf regardless of whether QUIC is used. We are highly unlikely to get fixes to SPT at this stage for the problem for the specific problem from the platform vendor because of the limited support state of this threading model. With no evidence of anyone using OpenSSL with SPT on NonStop - or at least support of 3.0 and 3.1 with SPT through those series end of life, customers have options if needed.

@t8m
Copy link
Member

t8m commented Nov 24, 2023

Simply building with SPT will trigger the problem because fgets() is replaced with spt_fgetsx(), which hangs reading openssl.cnf regardless of whether QUIC is used.

That can be fixed by reverting f63e1b4 and faa4351

@rsbeckerca
Copy link
Contributor Author

rsbeckerca commented Nov 24, 2023

Simply building with SPT will trigger the problem because fgets() is replaced with spt_fgetsx(), which hangs reading openssl.cnf regardless of whether QUIC is used.

That can be fixed by reverting f63e1b4 and faa4351

I agree. I can revise the PR if OpenSSL wants to keep the SPT configurations although I will no longer be building or maintaining those going forward.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@nhorman
Copy link
Contributor

nhorman commented Nov 28, 2023

@rsbeckerca Just fyi, the OTC met today and held a vote on this issue, decision was:

  1. Remove config target from master immediately (no process question there)
  2. Remove config target from 3.2 after announcing its removal on our various communication channels, just to give current users of a stable branch to voice concerns prior to said removal.

Thanks for your work on this!

@hlandau
Copy link
Member

hlandau commented Nov 28, 2023

From the minutes:

    topic: Support for the NonStop SPT threading model and SPT build configs
           can be removed from 3.2 branch subject to no objections being
           received from the community within two weeks after this vote passing
    accepted:  yes  (for: 9, against: 0, abstained: 0, not voted: 1)

openssl/technical-policies#86

@rsbeckerca
Copy link
Contributor Author

From the minutes:

    topic: Support for the NonStop SPT threading model and SPT build configs
           can be removed from 3.2 branch subject to no objections being
           received from the community within two weeks after this vote passing
    accepted:  yes  (for: 9, against: 0, abstained: 0, not voted: 1)

openssl/technical-policies#86

Thank you for your support in this matter.

@nhorman
Copy link
Contributor

nhorman commented Dec 12, 2023

@rsbeckerca the two week timer on public comments has expired on this, feel free to merge it, or let me know and I can take care of it

@rsbeckerca
Copy link
Contributor Author

@rsbeckerca the two week timer on public comments has expired on this, feel free to merge it, or let me know and I can take care of it

I would love to have the authority to merge on this project, but do not. Would you be able to handle it?

@nhorman nhorman removed the hold: need otc decision The OTC needs to make a decision label Dec 12, 2023
@nhorman
Copy link
Contributor

nhorman commented Dec 12, 2023

ack

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 12, 2023
openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
This fix removes explicit support for the SPT threading model in configurations.
This also reverts commit f63e1b4 that were
required for SPT but broke other models.

Fixes: #22798

Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca>

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #22807)
openssl-machine pushed a commit that referenced this pull request Dec 12, 2023
This fix removes explicit support for the SPT threading model in configurations.
This also reverts commit f63e1b4 that were
required for SPT but broke other models.

Fixes: #22798

Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca>

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #22807)

(cherry picked from commit 5cd1792)
@nhorman
Copy link
Contributor

nhorman commented Dec 12, 2023

merged to master and 3.2

@nhorman nhorman closed this Dec 12, 2023
@rsbeckerca
Copy link
Contributor Author

@nhorman Thank you

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
This fix removes explicit support for the SPT threading model in configurations.
This also reverts commit f63e1b48ac893dd6110452e70ed08f191547cd89 that were
required for SPT but broke other models.

Fixes: #22798

Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca>

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#22807)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
This fix removes explicit support for the SPT threading model in configurations.
This also reverts commit f63e1b48ac893dd6110452e70ed08f191547cd89 that were
required for SPT but broke other models.

Fixes: #22798

Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca>

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#22807)

(cherry picked from commit 5cd17920167a8b4f7a81722a1ed3b514115702de)
Signed-off-by: fly2x <fly2x@hitls.org>
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
This fix removes explicit support for the SPT threading model in configurations.
This also reverts commit f63e1b4 that were
required for SPT but broke other models.

Fixes: openssl#22798

Signed-off-by: Randall S. Becker <randall.becker@nexbridge.ca>

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#22807)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants