fix: link launch dir with existing nucleus package if does not exist #1675
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Binary incompatibility detected for commit a2ecf14. com.aws.greengrass.lifecyclemanager.KernelAlternatives is binary incompatible and is source incompatible because of CONSTRUCTOR_REMOVED Produced by binaryCompatability.py |
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against a2ecf14 |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against a2ecf14 |
junfuchen99
reviewed
Dec 4, 2024
| Path currentDir = getCurrentDir(); | ||
| if (!Files.isSymbolicLink(currentDir)) { | ||
| throw new DirectoryValidationException("Missing symlink to current nucleus launch directory"); | ||
| if (!Files.isSymbolicLink(getCurrentDir())) { |
There was a problem hiding this comment.
what if current exists but loader path does not exist?
There was a problem hiding this comment.
I am finding it hard to imagine a scenario where current points to a valid Nucleus package but loader does not exist in that package
There was a problem hiding this comment.
current exist does not mean that current points to a valid package though
junfuchen99
reviewed
Dec 5, 2024
| } | ||
|
|
||
| List<Path> localNucleusExecutablePaths = componentManager.findValidLocalNucleusPackage(); | ||
| if (!localNucleusExecutablePaths.isEmpty()) { |
There was a problem hiding this comment.
aren't you calling isEmpty on null object?
There was a problem hiding this comment.
Updated. This will still be an empty check in case artifacts dir is empty (initial installation) but this code path should only hit if a customer purposefully deletes the loader script from the unarchived dir. Imo, this is low risk.
alter-mage
force-pushed
the
ldc_exp
branch
2 times, most recently
from
1f77429 to
4b3ae05
Compare
saranyailla
reviewed
Dec 8, 2024
src/main/java/com/aws/greengrass/lifecyclemanager/KernelAlternatives.java
Show resolved
Hide resolved
alter-mage
force-pushed
the
ldc_exp
branch
2 times, most recently
from
760825c to
0f4b820
Compare
saranyailla
reviewed
Dec 9, 2024
| try { | ||
| Path unarchivePath = | ||
| nucleusPaths.unarchiveArtifactPath(nucleusComponentIdentifier, getFileName(file)); | ||
| unarchiver.unarchive(Unarchive.ZIP, file, unarchivePath); |
There was a problem hiding this comment.
Should we check if the archive file exists on the disk before unarchiving?
There was a problem hiding this comment.
That is not necessary, if the file does not exist, unarchiver will throw an IOException which ComponentManager will catch and just return null. The stream will filter out nonNull objects later on. If no artifact file existed then the list returned by ComponentManager will be empty and KernelAlternatives will handle that appropriately.
saranyailla
reviewed
Dec 9, 2024
| .map(file -> { | ||
| try { | ||
| Path unarchivePath = | ||
| nucleusPaths.unarchiveArtifactPath(nucleusComponentIdentifier, getFileName(file)); |
There was a problem hiding this comment.
I understand that we have zip artifacts only in the nucleus component versions, but this line would always copy over the artifacts to unarchived-artifacts folder irrespective of the Unarchive type of the artifact in the recipe.
There was a problem hiding this comment.
Yep, since this code path is used only for Nucleus artifacts in the near future, I do not see a harm in hard coding the unarchive as ZIP. But I can modify to fetch unarchive from the recipe if you have a strong opinion on it.
saranyailla
reviewed
Dec 9, 2024
src/main/java/com/aws/greengrass/lifecyclemanager/KernelAlternatives.java
Show resolved
Hide resolved
saranyailla
approved these changes
Dec 9, 2024
yitingb
approved these changes
Dec 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
Re create (if not present) and re link the current launch directory symbolic link with the Nucleus package which is currently being executed. This will only be executed if the current launch directory symbolic link does not exist.
If the current Nucleus package in execution does not have a loader file in its un-archived path, un-archive the the current Nucleus package artifact to re-populate the loader paths.
Why is this change necessary:
This change is necessary to mitigate deployment failures due to Launch Dir Corrupted.
How was this change tested:
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.