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

Supporting installation of multiple artifacts for plugin components (related to Notifications) #1950

Closed
qreshi opened this issue Apr 11, 2022 · 14 comments · Fixed by #1957
Closed
Assignees
Labels
enhancement New Enhancement v2.0.0

Comments

@qreshi
Copy link
Contributor

qreshi commented Apr 11, 2022

Is your feature request related to a problem? Please describe

In preparation for 2.0 Preview, we'd like to add the Notifications plugin to the manifest so it begins being bundled in the distribution. The Notifications backend is a bit of an uncommon case in that there are two plugins (opensearch-notifications and opensearch-notifications-core) that are being produced from the same repo. The latter needs to be installed first since the former extends it and both need to be installed to use Notifications. The current build process does not support this in a way that's as simple as just adding the repo to the manifest and ensuring the plugin build scripts produce the artifacts to the output directory.

[More information is in the 'additional context' section below]

Describe the solution you'd like

We'd like for the infra build process to ensure that these two plugins are installed and in the correct order to ensure correct assembly of the distribution.

Describe alternatives you've considered

No response

Additional context

I checked out opensearch-build locally and tried testing the building and assembly process with Notifications. After adding it to the manifest file, I ran:

./build.sh manifests/2.0.0/opensearch-2.0.0.yml 

The resulting output file showed that the Notification component was added with multiple plugin artifacts:

  - name: notifications
    repository: https://github.com/opensearch-project/notifications
    ref: main
    commit_id: 731ee820a36edc25bac07a5b6a624c35d1615ce5
    artifacts:
      plugins:
        - plugins/opensearch-notifications-core-2.0.0.0-alpha1.zip
        - plugins/opensearch-notifications-2.0.0.0-alpha1.zip
    version: 2.0.0.0-alpha1

Question: Is the order of this list of plugins above random? (We'd like it to always be the order that it's in above if the plugins are iterated through in this order)

After building, I attempted to assemble:

./assemble.sh tar/builds/opensearch/manifest.yml

At this point, it only installed the first plugin (opensearch-notifications-core) and completed:

2022-04-11 02:57:45 INFO     Recording common-utils
2022-04-11 02:57:45 INFO     Installing notifications
2022-04-11 02:57:45 INFO     Executing "/tmp/tmpqueccp_h/opensearch-2.0.0-alpha1/bin/opensearch-plugin install --batch file:/tmp/tmpqueccp_h/opensearch-notifications-core-2.0.0.0-alpha1.zip" in /tmp/tmpqueccp_h/opensearch-2.0.0-alpha1
-> Installing file:/tmp/tmpqueccp_h/opensearch-notifications-core-2.0.0.0-alpha1.zip
-> Downloading file:/tmp/tmpqueccp_h/opensearch-notifications-core-2.0.0.0-alpha1.zip
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@     WARNING: plugin requires additional permissions     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
* java.io.FilePermission /home/qreshi/.aws/*#plus read
* java.lang.RuntimePermission accessDeclaredMembers
* java.lang.RuntimePermission createClassLoader
* java.lang.RuntimePermission getClassLoader
* java.lang.reflect.ReflectPermission suppressAccessChecks
* java.net.NetPermission getProxySelector
* java.net.SocketPermission * connect,resolve
* javax.management.MBeanPermission com.amazonaws.metrics.* *
* javax.management.MBeanServerPermission createMBeanServer,findMBeanServer
* javax.management.MBeanTrustPermission register
See http://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html
for descriptions of what these permissions allow and the associated risks.
-> Installed opensearch-notifications-core with folder name opensearch-notifications-core
2022-04-11 02:57:46 INFO     Executing "bash /local/home/qreshi/workspace/opensearch-build/scripts/default/install.sh -v 2.0.0-alpha1 -p linux -a x64 -f /local/home/qreshi/workspace/opensearch-build/tar/builds/opensearch -o /tmp/tmpqueccp_h/opensearch-2.0.0-alpha1" in /tmp/tmpqueccp_h/opensearch-2.0.0-alpha1
+ getopts :h:v:q:s:o:p:a:f: arg
+ case $arg in
+ VERSION=2.0.0-alpha1
+ getopts :h:v:q:s:o:p:a:f: arg
+ case $arg in
+ PLATFORM=linux
+ getopts :h:v:q:s:o:p:a:f: arg
+ case $arg in
+ ARCHITECTURE=x64
+ getopts :h:v:q:s:o:p:a:f: arg
+ case $arg in
+ ARTIFACTS=
+ getopts :h:v:q:s:o:p:a:f: arg
+ case $arg in
+ OUTPUT=/tmp/tmpqueccp_h/opensearch-2.0.0-alpha1
+ getopts :h:v:q:s:o:p:a:f: arg
+ '[' -z 2.0.0-alpha1 ']'
+ '[' -z '' ']'
+ SNAPSHOT=false
+ '[' -z linux ']'
+ '[' -z x64 ']'
+ exit 0
2022-04-11 02:57:46 INFO     Installed plugins: ['opensearch-notifications-core']
2022-04-11 02:58:31 INFO     Published /local/home/qreshi/workspace/opensearch-build/tar/dist/opensearch/opensearch-2.0.0-alpha1-linux-x64.tar.gz.
2022-04-11 02:58:31 INFO     Done.

It appears this is because the install_plugin() function (which is called for each plugin component) retrieves the path of the plugins artifact but only takes the the first path.

One option could be to change this to _get_real_paths which returns a list of paths. We can then iterate through it in install_plugin() and call the plugin installation for each path.

@qreshi qreshi added enhancement New Enhancement untriaged Issues that have not yet been triaged labels Apr 11, 2022
@bbarani
Copy link
Member

bbarani commented Apr 11, 2022

@qreshi Can we also look in to the option of bundling both the zip's in to one zip similar to PA / RCA?

@qreshi
Copy link
Contributor Author

qreshi commented Apr 11, 2022

@bbarani I'm not too familiar with the PA/RCA plugins, are they two separate plugins that need to be installed in a particular order?

Who would be the point of contact to understand that format?

@peterzhuamazon
Copy link
Member

@qreshi I think you can contact @sruti1312 and get some ideas on how PA did this.

Thanks.

@qreshi
Copy link
Contributor Author

qreshi commented Apr 11, 2022

@sruti1312 Care you to give a description of your use case? I'd like to see if it's applicable here.

Although, in general, installing the plugins in a certain order for producing a distribution is an infra issue, not an individual plugins necessarily. I'm fine with moving fast here but we should think of how to make the infra builds more flexible for handling this use case since I can imagine this can happen again since plugins are all about extensibility.

@dblock
Copy link
Member

dblock commented Apr 11, 2022

Is the problem just that there are two ZIPs and they require to be installed in order? The manifest could very well accommodate a new section called install that lists certain installation paths instead of defaulting to "install the first zip you find".

@bbarani
Copy link
Member

bbarani commented Apr 11, 2022

I dont think its just a matter of installing 2 zips rather maintaining the lifecycle of 2 artifacts as well. We need to also think about the implications on users when we start publishing these artifacts for consumption (via Maven, S3 etc..). I would prefer to encapsulate all the dependencies within a zip to simplify it.

@qreshi
Copy link
Contributor Author

qreshi commented Apr 11, 2022

Hm, what's the concern of maintaining these two exactly? They're in one repo so the maintenance is actually reduced (like all the operational overhead we have from version changes, etc.). As for publishing, as far as I'm aware, we upload to S3 based on the plugins that are in the output directory. The build script in the Notifications repo is outputting both plugins there so shouldn't that be taken care of?

Even if we combined two separate zips into one zip, it's still two plugins and we would still need to ensure they install in a particular order.

@qreshi
Copy link
Contributor Author

qreshi commented Apr 11, 2022

Alternatively, could we not also just have two entries in the manifest. They'd point to the same repo but the working_directory would be different. Assuming the working_directory is used to resolve the build.sh location in the repo, we could build the two with their own scripts. That should also determine the installation order since it looks like the output of the build is created in that order.

DB's suggestion is also an alternative to the same concept and I think I prefer that one since its intention is clearer, whereas my suggestion above is trying to retrofit the use case with minimal code changes.

@sruti1312
Copy link
Contributor

sruti1312 commented Apr 11, 2022

PA and PA-RCA are a bit different. PA-RCA is a dependency of PA plugin and separate process that runs in the node. Both the PA and PA-RCA jar are bundled into the single PA plugin jar and hence only PA is declared in the manifest.

@qreshi They are not two separate plugins. PA is plugin and PA-RCA is separate process and hence not be installed in a particular order.

@qreshi
Copy link
Contributor Author

qreshi commented Apr 11, 2022

Thanks for the explanation @sruti1312

Looks like we do want to support this use case from the manifest then.

@CEHENKLE
Copy link
Member

@qreshi Hey, I ass-u-me you're also looking to consolidate down to a single zip?

@dblock dblock self-assigned this Apr 12, 2022
@qreshi
Copy link
Contributor Author

qreshi commented Apr 12, 2022

I'm still not sure how two plugins in a single zip works. Conventionally we have 1 zip per plugin. So if we combined two plugins into a single zip, would it even work if we tried to install that zip via the OpenSearch cli?

@dblock
Copy link
Member

dblock commented Apr 12, 2022

Both versions of the suggested implementations in #1956 and #1957, please pick one!

@peterzhuamazon
Copy link
Member

Picked #1957.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New Enhancement v2.0.0
Projects
None yet
7 participants