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

OnDeploy Scripts on Cloud Service #2324

Open
3 tasks
kaushalmall opened this issue Jun 4, 2020 · 20 comments
Open
3 tasks

OnDeploy Scripts on Cloud Service #2324

kaushalmall opened this issue Jun 4, 2020 · 20 comments
Labels

Comments

@kaushalmall
Copy link
Contributor

kaushalmall commented Jun 4, 2020

Required Information

  • Cloud Service
  • 4.7.0
  • Yes

Although the OnDeploy Scripts show up in the Build Image logs as being executed for Cloud Service deployments, most of the use cases that OnDeploy Scripts would be useful for would be considered as CS anti-patterns. Creating this issue to track the work for marking OnDeploy Scripts as incompatible for Cloud Service and discussing providing an alternative solution that in CS compatible in the future.

@HitmanInWis @davidjgonzalez @justinedelson @badvision @joerghoh @adamcin

@HitmanInWis
Copy link
Contributor

Looping in @adamcin

@HitmanInWis
Copy link
Contributor

Would it make sense to briefly summarize what is meant by "most of the use cases that OnDeploy Scripts would be used for fall under CS anti-patterns" in order to facilitate the discussion? Mark Adamcin and I were literally discussing the other day whether On-Deploy Scripts can still be a thing on AEMaaCS, but given the newness of the platform it might make sense to ensure we're all on the same page.

@kaushalmall
Copy link
Contributor Author

Hi Brett, the biggest anti-pattern is that it will try to update content during a code deployment and since every deployment is a "new" image some of things might not work as expected. For example, if I have a script that runs a query to find all nodes of RT1 and wants to change them to RT2.

I'd recommend using Sling Pipes for above use case in CS. Repoinit is another alternative to do the work that OnDeploy Scripts do.

HTH.

@HitmanInWis
Copy link
Contributor

HitmanInWis commented Jun 5, 2020

Yep, sounds like we're on the same page then. One of the most common reasons we've used On-Deploy Scripts is to update node structures to match refactored code - one of the common examples being a refactored component with a new property naming structure. Given that both the new instance being started up (with the new code) and the existing instance not yet spun down (with the old code) are pointing to the same content data source during the blue/green deployment process, we really can't handle this case with a simple JCR content update since the update will break the currently running server, which is still running until the deploy completes, and will remain running (with the updated content that breaks it) if the deployment fails.

@HitmanInWis
Copy link
Contributor

Is there a period of time where both the newly spinning up servers (new code) and the old servers (old code) are serving end users at the same time? Or is it a clean 100% cut from all traffic going to old servers to all traffic going to new servers?

@kaushalmall
Copy link
Contributor Author

AFAIK, it's a clean 100% cutover. @justinedelson @davidjgonzalez can correct if needed.

@kaushalmall
Copy link
Contributor Author

are we ok marking it as incompatible for now and maybe figure out if we want to update it for a future release later?

@HitmanInWis
Copy link
Contributor

probably the safest bet

Throwing a (potentially bad) idea out there. If there is truly a 100% cutover (no period of time where both new/old servers are serving users) could we somehow trigger the jobs to run at the point of cutover? Maybe not a "great" solution, but in an imperfect world it might be fine for many use cases.

@royteeuwen
Copy link
Contributor

royteeuwen commented Jun 5, 2020 via email

@kaushalmall
Copy link
Contributor Author

@royteeuwen we should use Sling Pipes for updating content.

@royteeuwen
Copy link
Contributor

royteeuwen commented Jun 5, 2020 via email

@kaushalmall
Copy link
Contributor Author

The goal is to not change content as part of the code deployment, Sling Pipes are used on the environment by calling the end point via cURL or similar and not as part of the code deployment. There are also timeouts set in the build image step for CS deployments, if your OnDeploy scripts takes a long time, it will break the build.

@shsteimer
Copy link
Contributor

shsteimer commented Jun 5, 2020

I believe the suggested approach would be to use a resource decorator to wrap from an old component model to a new one. This can be in the interim until you can execute a sling pipe to update existing content (or perhaps even use your decorator to trigger your sling pipe asynchronously, you can avoid having to remember to call the sling pipe via curl)

@davidjgonzalez
Copy link
Contributor

As @shsteimer notes, the suggested approach to perform "(otherwise) breaking content changes" is:

0a) [CODE] Code handles "old" content structure
0b) [CONTENT] Content is using "old" structure

  1. [CODE] Update code to handle BOTH the old content and new content structures and deploy.

Now, the running code handles BOTH old and new, but is running against the "old" content structure.

  1. [CONTENT] Update the old content structure to the new structure (how this happens isn't important for this discussion)
  2. [CODE] If needed, remove the old content support from the code, since all content should be transformed into the "new" structure.

... @shsteimer RD being an example of this high-level approach

@kaushalmall
Copy link
Contributor Author

joerghoh added a commit to joerghoh/acs-aem-commons that referenced this issue Jun 5, 2020
@HitmanInWis
Copy link
Contributor

Taking that approach @davidjgonzalez means that On-Deploy Scripts could still be used for Step 2, right? That's assuming it is done in a second release - it needs to be done in a release where the code to support both old and new format is already deployed on the "running" server.

@stale
Copy link

stale bot commented Aug 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 8, 2020
joerghoh added a commit that referenced this issue Aug 8, 2020
* Revert "#2298 - Removed DynamicDeck dependency on deprecated package com.day.cq.dam.api.collection which causes problems w/ AEM CS deployments. (#2319)" (#2322)

This reverts commit d4f62de.

* #2324 On-Deploy-Scripts are not supported on AEMaaCS (#2326)

* Removed accidental changelog statement

* Updated target AEM version

* #2330, Versioned ClientLibs are not supported by Page Exports (#2331)

* Versioned ClientLibs are not supported by Page Exports (cw-wcm-content-sync)

* Added comment explaining where the rr.map(..) is invoked

* Incorrect Injectors ordering with service.ranking #2344 - fix service… (#2345)

* Incorrect Injectors ordering with service.ranking #2344 - fix service ranking according to OSGi specification

Co-authored-by: abrdashevskiy <[email protected]>

* #2350 -  Fixed null check in VanityServiceUrlImpl, Added hook for Van… (#2351)

* #2350 -  Fixed null check in VanityServiceUrlImpl, Added hook for VanityUrlAdjuster in VanityServiceUrlImpl

* Feature/2359 deep prune administrators (#2360)

* #2359 - Deprecated adminOnlyProcessDefinitionFactory and switched Deep Prune to Administrators Only (allowing all of the administrators group to use that tool)

* #2303 - EnsureOakIndexServlet (exposed via the OSGi Console) should b… (#2304)

* #2303 - EnsureOakIndexServlet (exposed via the OSGi Console) should be invokable via an inline HTML form

* Removed DynamicDeck dependency on deprecated package com.day.cq.dam.api.collection which causes problems w/ AEM CS deployments. (#2323)

* #2298 - Removed DynamicDeck dependency on deprecated package com.day.cq.dam.api.collection which causes problems w/ AEM CS deployments.

* #2357 - Added safeguards to SMTPMailServiceHealthCheck to help avoid … (#2358)

* #2357 - Added safeguards to SMTPMailServiceHealthCheck to help avoid run-way email pings

* Changelog for 4.7.2 completed

* [maven-release-plugin] prepare for next development iteration

* Update CHANGELOG.md

* Revert "[maven-release-plugin] prepare for next development iteration"

This reverts commit f051690.

* [maven-release-plugin] prepare release acs-aem-commons-4.7.2

* [maven-release-plugin] prepare for next development iteration

* Updated PGP instructions to clarify some points of confusion

* #2366 CQIncludePropertyNamespaceServlet - UnsupportedOperationException (#2367)

* #2366 CQIncludePropertyNamespaceServlet - UnsupportedOperationException, added test

* Feature/2354 - ACS AEM Commons web console (#2355)

* ACS AEM Commons Console POC

* Feature/ms office asset selector (#2356)

* Added MS Office Addin components and content
Co-authored-by: Chris Workman <[email protected]>
Co-authored-by: Athiers <[email protected]>

* v4.8.0 changelog

* [maven-release-plugin] prepare release acs-aem-commons-4.8.0

* [maven-release-plugin] prepare for next development iteration

* Fix SCR warnings at build time (#2369)

* fixed SCR warnings

* environmentfilter breaks http asset api (fixes #2371) (#2372)

* created testcase for #2371

* Update CHANGELOG.md

* [maven-release-plugin] prepare for next development iteration

* v4.8.2 re-release

* [maven-release-plugin] prepare release acs-aem-commons-4.8.4

* [maven-release-plugin] prepare for next development iteration

* Fix test warnings (#2373)

* no need to catch exceptions in unittests, junit does that for us
* fix warnings on some test executions
* fix some more Mockito warnings
* cleanup compilation warnings in test classes
* removed unused MockPageManager, use the PageManager impl of AEM Mocks instead
* fix more deprecation warnings
* fix codeclimate issue
* added changelog

* [trivial] fix exception message (#2383)

* [trivial] fix exception message

* Sitemap Disable Vanity for URLs (#2378)

* Adding a feature to the sitemap to not use the vanity when generating the URL
* avoid too many negations, makes it easier to reason about it

Co-authored-by: Jörg Hoh <[email protected]>

* Fix compilation warnings (#2381)

* Fix compilation warnings
* suppress deprecation warnings because we cannot fix them right now or with the current implementation
* use the static method valueOf instead of the constructors of the wrapped primitive types (Java9)
* replace type.newInstance() with the recommended version (Java9)

Co-authored-by: Brendan Robert <[email protected]>

* Bugfix for incomplete request wrapper for sling models (#2379)

* - Fixed issue where sling model injection by @ChildResourceFromRequestInjector was not including all sling model bindings, thus resulting in models being injected with incorrect values for thing like currentStyle
Co-authored-by: lokeshvajrala <[email protected]>
Co-authored-by: Lokesh Vajrala <[email protected]>

* Check for missing resource instead of freaking out and causing problems (#2384)

* Check for missing resource instead of freaking out and causing problems

* #2386 - Bulk Asset Import Overwrites Existing Folder Titles (#2387)

* Added option to not overwrite the destination folder title when creating the folder hierarchy.
* Added tests.

* ignore failing test

Co-authored-by: Brendan Robert <[email protected]>
Co-authored-by: david g <[email protected]>
Co-authored-by: Dominik Förderreuther <[email protected]>
Co-authored-by: Andrey B <[email protected]>
Co-authored-by: abrdashevskiy <[email protected]>
Co-authored-by: Brendan Robert <[email protected]>
Co-authored-by: David Gonzalez <[email protected]>
Co-authored-by: Dan Klco <[email protected]>
Co-authored-by: Brett Birschbach <[email protected]>
Co-authored-by: Robert Botha <[email protected]>
@stale stale bot closed this as completed Aug 16, 2020
joerghoh added a commit that referenced this issue Aug 23, 2020
* Revert "#2298 - Removed DynamicDeck dependency on deprecated package com.day.cq.dam.api.collection which causes problems w/ AEM CS deployments. (#2319)" (#2322)

This reverts commit d4f62de.

* #2324 On-Deploy-Scripts are not supported on AEMaaCS (#2326)

* Removed accidental changelog statement

* Updated target AEM version

* #2330, Versioned ClientLibs are not supported by Page Exports (#2331)

* Versioned ClientLibs are not supported by Page Exports (cw-wcm-content-sync)

* Added comment explaining where the rr.map(..) is invoked

* Incorrect Injectors ordering with service.ranking #2344 - fix service… (#2345)

* Incorrect Injectors ordering with service.ranking #2344 - fix service ranking according to OSGi specification

Co-authored-by: abrdashevskiy <[email protected]>

* #2350 -  Fixed null check in VanityServiceUrlImpl, Added hook for Van… (#2351)

* #2350 -  Fixed null check in VanityServiceUrlImpl, Added hook for VanityUrlAdjuster in VanityServiceUrlImpl

* Feature/2359 deep prune administrators (#2360)

* #2359 - Deprecated adminOnlyProcessDefinitionFactory and switched Deep Prune to Administrators Only (allowing all of the administrators group to use that tool)

* #2303 - EnsureOakIndexServlet (exposed via the OSGi Console) should b… (#2304)

* #2303 - EnsureOakIndexServlet (exposed via the OSGi Console) should be invokable via an inline HTML form

* Removed DynamicDeck dependency on deprecated package com.day.cq.dam.api.collection which causes problems w/ AEM CS deployments. (#2323)

* #2298 - Removed DynamicDeck dependency on deprecated package com.day.cq.dam.api.collection which causes problems w/ AEM CS deployments.

* #2357 - Added safeguards to SMTPMailServiceHealthCheck to help avoid … (#2358)

* #2357 - Added safeguards to SMTPMailServiceHealthCheck to help avoid run-way email pings

* Changelog for 4.7.2 completed

* [maven-release-plugin] prepare for next development iteration

* Update CHANGELOG.md

* Revert "[maven-release-plugin] prepare for next development iteration"

This reverts commit f051690.

* [maven-release-plugin] prepare release acs-aem-commons-4.7.2

* [maven-release-plugin] prepare for next development iteration

* Updated PGP instructions to clarify some points of confusion

* #2366 CQIncludePropertyNamespaceServlet - UnsupportedOperationException (#2367)

* #2366 CQIncludePropertyNamespaceServlet - UnsupportedOperationException, added test

* Feature/2354 - ACS AEM Commons web console (#2355)

* ACS AEM Commons Console POC

* Feature/ms office asset selector (#2356)

* Added MS Office Addin components and content
Co-authored-by: Chris Workman <[email protected]>
Co-authored-by: Athiers <[email protected]>

* v4.8.0 changelog

* [maven-release-plugin] prepare release acs-aem-commons-4.8.0

* [maven-release-plugin] prepare for next development iteration

* Fix SCR warnings at build time (#2369)

* fixed SCR warnings

* environmentfilter breaks http asset api (fixes #2371) (#2372)

* created testcase for #2371

* Update CHANGELOG.md

* [maven-release-plugin] prepare for next development iteration

* v4.8.2 re-release

* [maven-release-plugin] prepare release acs-aem-commons-4.8.4

* [maven-release-plugin] prepare for next development iteration

* Fix test warnings (#2373)

* no need to catch exceptions in unittests, junit does that for us
* fix warnings on some test executions
* fix some more Mockito warnings
* cleanup compilation warnings in test classes
* removed unused MockPageManager, use the PageManager impl of AEM Mocks instead
* fix more deprecation warnings
* fix codeclimate issue
* added changelog

* [trivial] fix exception message (#2383)

* [trivial] fix exception message

* Sitemap Disable Vanity for URLs (#2378)

* Adding a feature to the sitemap to not use the vanity when generating the URL
* avoid too many negations, makes it easier to reason about it

Co-authored-by: Jörg Hoh <[email protected]>

* Fix compilation warnings (#2381)

* Fix compilation warnings
* suppress deprecation warnings because we cannot fix them right now or with the current implementation
* use the static method valueOf instead of the constructors of the wrapped primitive types (Java9)
* replace type.newInstance() with the recommended version (Java9)

Co-authored-by: Brendan Robert <[email protected]>

* Bugfix for incomplete request wrapper for sling models (#2379)

* - Fixed issue where sling model injection by @ChildResourceFromRequestInjector was not including all sling model bindings, thus resulting in models being injected with incorrect values for thing like currentStyle
Co-authored-by: lokeshvajrala <[email protected]>
Co-authored-by: Lokesh Vajrala <[email protected]>

* Check for missing resource instead of freaking out and causing problems (#2384)

* Check for missing resource instead of freaking out and causing problems

* #2386 - Bulk Asset Import Overwrites Existing Folder Titles (#2387)

* Added option to not overwrite the destination folder title when creating the folder hierarchy.
* Added tests.

* add build profiles classic and cloudservice

* remove compile time dependencies on packages which are not present in cloudservice

* add missing methods to make it compile with cloudservice

* all existing travis jobs with classic, but one new job with jdk11 and cloudservice

* fix travis

* ignore failing tests (just for the moment)

* switch activation of classic profile to make it work on travis

* fix profile activation

* definition for cloudservice

* also execute the checks against the package versions of AEM CS

* execute the oakpal-verify actions only when building in classic profile

Co-authored-by: Brendan Robert <[email protected]>
Co-authored-by: david g <[email protected]>
Co-authored-by: Dominik Förderreuther <[email protected]>
Co-authored-by: Andrey B <[email protected]>
Co-authored-by: abrdashevskiy <[email protected]>
Co-authored-by: Brendan Robert <[email protected]>
Co-authored-by: David Gonzalez <[email protected]>
Co-authored-by: Dan Klco <[email protected]>
Co-authored-by: Brett Birschbach <[email protected]>
Co-authored-by: Robert Botha <[email protected]>
@HitmanInWis
Copy link
Contributor

HitmanInWis commented Nov 30, 2020

Thought of a new idea today. Could on-deploy scripts continue to work in Cloud Service if they were triggered to run by a content package? My understanding is that content packages are installed at the point of traffic cut-over from blue to green in the deployment, meaning that code has already fully completed deployment and the server is fully ready to go.

Current Impl: On-Deploy Scripts run based on presence in code and absence of a status node under /var.

New, Cloud-compatible Impl: On-Deploy Scripts run based on presence in code and presence of a status node under /var where the status is "ready". A content package is used to install the status node, which occurs at the point of traffic cut-over.

As long as developers follow the principal of both content formats (pre-script and post-script) working until a subsequent release, this should work.

And for those that want to run fast and loose (and dont mind the site having errors for a minute or two) they could avoid that precaution and the site will be back in order once the content package installs the status node and the script runs. Obviously not recommended though :).

@HitmanInWis HitmanInWis reopened this Nov 30, 2020
@stale stale bot removed the wontfix label Nov 30, 2020
@kaushalmall
Copy link
Contributor Author

@HitmanInWis not sure if this is still anti-pattern. I know for a fact that the usage of /var is being frowned upon now, especially if you are going to replicate that path, but adding code execution as part of the mutable content might be a no no as well. @davidjgonzalez thoughts? maybe Dominik would know, but, I don't know his github handle to tag.

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants