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

Fix appup removal order #2882

Merged
merged 2 commits into from
Jan 18, 2021

Conversation

garazdawi
Copy link
Contributor

This PR makes it possible to do release upgrades with an application replacing which applications it depends on.

One problem remains and that is that the new dependency is started after processes are resumed in the upgraded application.

@garazdawi garazdawi added team:VM Assigned to OTP team VM fix labels Nov 24, 2020
@garazdawi garazdawi self-assigned this Nov 24, 2020
@garazdawi garazdawi added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Nov 30, 2020
@garazdawi garazdawi force-pushed the lukas/sasl/fix_appup_removal_order branch from c9207f4 to 6ebc280 Compare January 14, 2021 08:51
@garazdawi
Copy link
Contributor Author

Doing this turned out to be much harder than I thought it would be. Basically, in order to handle this, the systools_relup dependency management engine has to be rewritten as it cannot represent what we want to do. So I'm closing this PR until I or someone else has time to work on it.

@garazdawi garazdawi closed this Jan 14, 2021
@lrascao
Copy link
Contributor

lrascao commented Jan 14, 2021

I'm curious on the nature of the blocker, could you provide a high level explanation of it please?

@garazdawi
Copy link
Contributor Author

The problem is that the primitive instructions used within systools to manage dependencies do not include enough information to do this type of relup. There needs to be a way to make module addition and deletion depend on application start/stop instructions. Also the digraph used to calculate the module purge/load dependencies does a bunch of assumptions that do not work when you try to replace a dependency.

It is not an impossible problem to solve I just don't have the time to solve it right now.

The testcases that I could not make pass are system -> upgrade_SUITE:major+ancient.

@lrascao
Copy link
Contributor

lrascao commented Jan 14, 2021

👍 thanks for the explanation

@zmstone
Copy link

zmstone commented Jan 14, 2021

I'd be happy with only this change:

- { RUs ++ RU0s, W0s}.
+ { RU0s ++ RUs, W0s}.

Does ^ this break some existing tests or some assumptions made somewhere ?

@garazdawi
Copy link
Contributor Author

I'd be happy with only this change:

- { RUs ++ RU0s, W0s}.
+ { RU0s ++ RUs, W0s}.

Does ^ this break some existing tests or some assumptions made somewhere ?

I don't think it breaks anything, but it doesn't fix it for all scenarios either. Only doing this change feels like putting a bandaid on the problem, though I suppose it may be a step in the right direction.

@zmstone
Copy link

zmstone commented Jan 14, 2021

I'd be happy with only this change:

- { RUs ++ RU0s, W0s}.
+ { RU0s ++ RUs, W0s}.

Does ^ this break some existing tests or some assumptions made somewhere ?

I don't think it breaks anything, but it doesn't fix it for all scenarios either. Only doing this change feels like putting a bandaid on the problem, though I suppose it may be a step in the right direction.

Can we at least do this (and only this) in this PR ?.

@garazdawi garazdawi reopened this Jan 14, 2021
@garazdawi garazdawi force-pushed the lukas/sasl/fix_appup_removal_order branch from 6ebc280 to 0b42fd5 Compare January 14, 2021 10:32
@garazdawi
Copy link
Contributor Author

I'll put it in test and see if it breaks anything.

@garazdawi garazdawi force-pushed the lukas/sasl/fix_appup_removal_order branch from 0b42fd5 to 520c450 Compare January 14, 2021 10:35
@garazdawi garazdawi changed the base branch from master to maint January 18, 2021 09:46
@garazdawi garazdawi force-pushed the lukas/sasl/fix_appup_removal_order branch from 520c450 to 4d8702f Compare January 18, 2021 09:46
@CLAassistant
Copy link

CLAassistant commented Jan 18, 2021

CLA assistant check
All committers have signed the CLA.

@garazdawi garazdawi force-pushed the lukas/sasl/fix_appup_removal_order branch from 4d8702f to 4fa4a53 Compare January 18, 2021 09:46
@garazdawi garazdawi merged commit 1269663 into erlang:maint Jan 18, 2021
@garazdawi
Copy link
Contributor Author

All tests passed. Merged for release in OTP-23.3.

Fixes https://bugs.erlang.org/browse/ERL-1410

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants