Allow more resources to be predeployed #969
Open
+262
−43
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
- 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
|
Hi @c-gerke is this PR ready for a proper review? Thanks for the contribution! |
Hi there @timothysmith0609! I do believe this PR is ready for a proper review. I appreciate having such an awesome and useful project to contribute to and I look forward to learning how I can make it better and hopefully get the changes accepted! |
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.
What are you trying to accomplish with this PR?
This PR aims to allow operators of the Krane gem to designate more types of resources as being able to be "predeployed" than just CRs and CRDs, as was the previous behavior of the "predeployed" logic.
How is this accomplished?
There are a few changes that have been made to facilitate this:
Documentation Updates:
CONTRIBUTING.md: Added instructions for implementing thepredeployed?method in custom Kubernetes resource classes and updated the references for adding new resourcesREADME.md: Expanded the description of thekrane.shopify.io/predeployedannotation to include more resource types and clarified its default behaviorCode Enhancements:
lib/krane/kubernetes_resource.rb: Introduced aPREDEPLOYED_RESOURCE_TYPESconstant to hold all of the resources that were previously statically predeployed, and apredeployed?method to determine if a resource should be predeployed based on its type or annotation, which is inherited by alllib/krane/kubernetes_resourcefileslib/krane/kubernetes_resource/custom_resource.rb: Added apredeployed?method to handle custom resources, defaulting to true, similar to that incustom_resource_definition.rblib/krane/deploy_task.rb: Updated thepredeploy_sequencemethod to include additional resource types in the predeploy sequenceDeployment Process:
lib/krane/resource_deployer.rb: Modified thepredeploy_priority_resourcesmethod to filter resources based on thepredeployed?method (see questions section below for a clarification request)lib/krane/resource_deployer.rb: Added a StatsD measurement for the sync duration in thedeploy_resourcesmethodTests:
test/helpers/mock_resource.rb: Modified to allow the test MockResource to be able to be predeployedtest/integration-serial/serial_deploy_test.rb: Added multiple tests to verify that resources with thekrane.shopify.io/predeployedannotation are correctly predeployedtest/unit/krane/resource_deployer_test.rb: Modified unit tests to ensure thepredeploy_priority_resourcesmethod respects the predeploy list and thepredeployed?annotation, and that the unit tests know how to operate in a universe where thekrane.shopify.io/predeployedannotation can be deployed to almost any resourceWhat could go wrong?
This could allow operators of Krane to deploy resources out of sequence, causing deployments to fail and requiring troubleshooting.
An example of this failure mode would be adding the annotation to a
Service, causing it to be predeployed before aDeploymentthat theServicewould target. In this case where thekrane.shopify.io/predeployed: "true"annotation is set on theServicewithout something like thekrane.shopify.io/skip-endpoint-validation: "true"annotation, theServicewill fail citing there are no endpoints for theServiceand the deployment is likely invalid.In essence, most of the observed "cons" are giving operators of Krane a gun that could be used to shoot themselves in the foot.
Additional comments and questions
lib/krane/resource_deployer.rbandlib/krane/deploy_task.rbIn
lib/krane/resource_deployer.rb, we are getting all of the resources and filtering afterwards for those with thepredeployedannotation. 1 This is a different behavior than that for CRs inlib/krane/deploy_task.rbbecause we don't have access inlib/krane/deploy_task.rbat that point in the process to read the files and determine if they need to be predeployed.This causes us to always hit 'phase 3 - predeploy' of the deployment process (thus the change in line 347 of
test/integration-serial/serial_deploy_test.rb), which seems a little inefficient. Is there a better way that we should do this so we can skip hitting 'phase 3 - predeploy' if there aren't any resources that will need to be predeployed?