-
Notifications
You must be signed in to change notification settings - Fork 0
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
Predeploy more resources #5
base: upstream-clone
Are you sure you want to change the base?
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.
Docs?
I just pushed ca20352 to add some information to the README, I'm looking more to see if it would make sense to document this elsewhere! |
README.md
Outdated
- `krane.shopify.io/predeployed`: Causes a Custom Resource to be deployed in the pre-deploy phase. | ||
- _Compatibility_: Custom Resource Definition | ||
- `krane.shopify.io/predeployed`: Causes a Custom Resource, Deployment, Service, or Job to be deployed in the pre-deploy phase. | ||
- _Compatibility_: Custom Resource Definition, Deployment, Service, Job |
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.
This might be controversial upstream; the label originally got applied to the resource definition (class) not the concrete instance, so this is now conceptually mixed.
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.
I had been thinking about that as well, that this implementation/change was the result of taking a feature that existed with one purpose and kind of stealing or borrowing it to apply to different objects. I don't doubt it would be controversial upstream, and I'm ready to have those conversations on if this should even be merged upstream too!
I think it's a worthwhile feature/functional change though (I do realize I'm a little close to it to see it completely objectively), and there's at least one other person at some point who was looking for this type of functionality, so I think it's worth at least proposing and discussing if this is something that would be desired in its current state.
- Updated `predeploy_sequence` to specify resource groups for better clarity. - Modified `predeployed?` method logic to allow for default behavior based on resource type. - Introduced `default_to_predeployed?` method in multiple resource classes to standardize predeployment behavior. - Removed redundant `predeployed?` methods from `Deployment`, `Job`, and `Service` classes. - Added debug logging in integration tests for better traceability of resource groups during deployment.
This will be fixed later on, but getting a proposed version up in a PR to Shopify's Krane is the priority
- Introduced a constant `PREDEPLOYED_RESOURCE_TYPES` to manage predeployed resource types. - Updated `predeployed?` method to utilize the new constant for Role and RoleBinding checks. - Removed redundant `predeployed?` methods from ConfigMap, NetworkPolicy, PersistentVolumeClaim, ResourceQuota, Role, RoleBinding, Secret, and ServiceAccount classes, simplifying their implementation. - Set `default_to_predeployed?` to always return true in CustomResourceDefinition.
… on this PR Also add CRDs and CRs to their previous functionality, not using the `default_to_predeployed` functionality
… annotation is correctly deployed in phase 3
Multiple test cases in `serial_deploy_test.rb` to verify that various Kubernetes resources (Service, Ingress, Job, DaemonSet, PodDisruptionBudget, PodTemplate, ReplicaSet, StatefulSet) with the `krane.shopify.io/predeployed` annotation are correctly predeployed and logged in the expected phases.
Remove a pedantic documentation change that increases the number of changes in this fork for little benefit Fix link that points to the Power fork instead of Krane fork
What are you trying to accomplish with this PR?
This will allow more resources to be marked as predeployed, including Services, Jobs and Deployments. This will allow invoking Krane fewer times to deploy the same number of resources in the case where multiple Krane invocations would be necessary to deploy resources in a specific order, e.g. making sure a Redis instance is set up before a primary application without invoking Krane twice.
How is this accomplished?
This is accomplished by having the cluster resource discovery respect if the 'predeployed' annotation is set on Services, Jobs and Deployments. This includes checking if the 'predeployed' annotation is set on Services, Deployments and Jobs, and then having the resource deployer check if the predeployed annotation is set. If it is, the deploy task deploys Deployments and Services before the individual pods, and Jobs after the pods.
After all of that is said and done, Deployments, Services and Jobs will be deployed in 'Phase 3', before 'Phase 4' where the main deployment occurs.
What could go wrong?
This could unintentionally create race conditions if the annotations are set incorrectly on resources that shouldn't be predeployed, creating an out of order deployment.