-
Notifications
You must be signed in to change notification settings - Fork 467
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
feat(deployment): Add shutdown-on-startup and skip-upgrade properties to support k8s init container use (#31003) #31047
feat(deployment): Add shutdown-on-startup and skip-upgrade properties to support k8s init container use (#31003) #31047
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right direction? I think the upgrade tasks can be run independently of setting up the Application Server, e.g. I know in gradle you could run the upgrade tasks from the CLI. My concern is that starting up the app server takes way longer than just running the tasks and when there are no tasks, this could double the container startup time.
Does this run every startup? How will we know when to run it? Maybe there is an init script that calls the DB db_version
table, e.g.
psql -c select max(db_version) from db_version
and then reads the dotcms.jar file, parsing the UpgradeTasks
for the highest numbered task, e.g. regexing for something like UpgradeTask(\d{8}).class
and if the db_version does not match that, force the run?
Also, another thing to think about is when multiple containers in a pod are coming up at the same time.
However, there is a case to be made for the current approach of starting the full container in an init phase:
A separate effort is already underway to audit the startup process more closely. This will help clarify whether some tasks can be moved to run post-startup and determine if any steps need to be repeated on each replica or if concurrency issues could arise when multiple instances perform upgrades simultaneously. For now, running the same image in both the init container and replicas minimizes complexity, ensures a single code path, and reduces the risk of inconsistencies between the upgrade process and normal startup behavior. We may eventually want to factor out the init container especially if we manage to make all the upgrade tasks safe to run in the background with the server started, but for now it solves an immediate problem without adding additional code logic and scripts in the deployment layer, keeping it in the core code. Either way this current change only adds extra configurability and information on startup order and should not alter any other existing behaviour allowing us to make any of these choices going forward. |
0efbbff
to
452a5fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great responses Steve. Point number #2 addresses. most if not all of my concerns.
Proposed Changes
Testing
At this stage a basic test that the server does shut down with exit 0 after all startup tasks when DOT_SHUTDOWN_ON_STARTUP=true is passed, and the fact that with the defaults, it does not impact any existing behavior is required. This will be further tested when implementing the init container that will make use of this flag.
This PR fixes: #31003