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

Test replacement of one application with another #891

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Sep 26, 2023

When device configuration is updated using tools like Terraform, it is possible that in one configuration iteration a user application is removed and immediately replaced with another. Because the previous and the new applications could have conflicting configurations, it is important for EVE to execute operations of replacement carefully and in the right order. And in case EVE fails to create the new app because the obsolete one still holds some resources (e.g. allocated IP address), it should make a retry attempt after the obsolete app is fully removed.

Note that these scenarios were not working in EVE properly until the version 10.10, when a patch was submitted, meaning that this is the minimal version for this test to pass.

@milan-zededa
Copy link
Contributor Author

@uncleDecart @giggsoff This new test is now ready for review and it is expected to pass with EVE >10.10.

@milan-zededa
Copy link
Contributor Author

@uncleDecart @giggsoff Please review :)


# Change the second app UUID which is effectively the same as replacing
# one application with another in one config iteration.
exec -t 1m bash change-app-uuid.sh app2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please increase the time up to e.g. 5m. In my experiments with xen, 1m is not enough (I can see 130 seconds on my PC).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you!

eden.escript.test -testdata ../app/testdata/ -test.run TestEdenScripts/2dockers_test
/bin/echo Testing replacement of one application with another (27.2/{{$tests}})
eden.escript.test -testdata ../app/testdata/ -test.run TestEdenScripts/app_replace_test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a blocker, but just to mention: test located here will run only on EdenGCP workflow for now, as we have {{ if or (eq $workflow "large") (eq $workflow "gcp") }} above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

and we need to add this tests to new workflows, because EVE is using them. I suggest to put this in user app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already put this to user-apps.tests.txt

Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad, didn't see it :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giggsoff I didn't realize that the test would be skipped in the old workflow, but since we are going to move to the new workflow soon, I'm going to keep it as it is.

Copy link
Collaborator

@uncleDecart uncleDecart left a comment

Choose a reason for hiding this comment

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

LGTM

When device configuration is updated using tools like Terraform,
it is possible that in one configuration iteration a user application
is removed and immediately replaced with another. Because the previous
and the new applications could have conflicting configurations, it is
important for EVE to execute operations of replacement carefully and
in the right order. And in case EVE fails to create the new app because
the obsolete one still holds some resources (e.g. allocated IP address),
it should make a retry attempt after the obsolete app is fully removed.

Note that these scenarious were not working in EVE properly until the
version 10.10, when a patch was submitted, meaning that this is the
minimal version for this test to pass.

Signed-off-by: Milan Lenco <milan@zededa.com>
@giggsoff giggsoff merged commit 7496dc8 into lf-edge:master Oct 18, 2023
5 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants