feat: add instanceset controller codes#83
Conversation
veophi
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new InstanceSet controller for managing Instance resources with features including scaling, in-place updates, lifecycle hooks, and revision management. The implementation follows a similar pattern to Kubernetes StatefulSet but adapted for Instance custom resources.
Key Changes:
- Added InstanceSet CRD with scaling and update strategies, lifecycle hooks, and revision management
- Implemented reconciler logic for instance scaling, updating, and status management
- Added utility functions for instance sorting, revision management, and reference management
- Integrated lifecycle hooks for pre-delete and in-place update operations
Reviewed Changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| api/workloads/v1alpha1/instanceset_types.go | Added lifecycle hooks, constants, and fixed resource path to plural form |
| api/workloads/v1alpha1/constant.go | Added SetInstanceOwnerLabelKey for InstanceSet ownership |
| pkg/reconciler/instanceset/* | Core reconciler implementation with sync, scale, update, and revision control logic |
| pkg/inplace/instance/lifecycle/lifecycle_utils.go | Lifecycle management utilities for instance state transitions |
| deploy/helm/rbgs/crds/* | CRD definitions with lifecycle hook specifications |
| config/crd/bases/* | Fixed CRD name from singular to plural form |
| examples/instanceset/* | Example YAML files for InstanceSet and Instance resources |
| cmd/rbgs/main.go | Registered InstanceSet controller and added scheme |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| description: InPlaceUpdate is the hook before Pod to update and | ||
| after Pod has been updated. |
There was a problem hiding this comment.
The description incorrectly refers to 'Pod' but should refer to 'Instance' since this CRD is for InstanceSet managing Instance resources, not Pods.
| description: InPlaceUpdate is the hook before Pod to update and | |
| after Pod has been updated. | |
| description: InPlaceUpdate is the hook before Instance to update and | |
| after Instance has been updated. |
| markPodNotReady: | ||
| description: |- | ||
| MarkNotReady = true means: | ||
| - Pod will be set to 'NotReady' at preparingDelete/preparingUpdate state. |
There was a problem hiding this comment.
The description incorrectly refers to 'Pod' but should refer to 'Instance' since this is managing Instance readiness state.
| type: boolean | ||
| type: object | ||
| preDelete: | ||
| description: PreDelete is the hook before Pod to be deleted. |
There was a problem hiding this comment.
The description incorrectly refers to 'Pod' but should refer to 'Instance'.
| markPodNotReady: | ||
| description: |- | ||
| MarkNotReady = true means: | ||
| - Pod will be set to 'NotReady' at preparingDelete/preparingUpdate state. |
There was a problem hiding this comment.
The description incorrectly refers to 'Pod' but should refer to 'Instance'.
| @@ -0,0 +1,260 @@ | |||
| /* | |||
| Copyright 2019 The Beirut Authors. | |||
There was a problem hiding this comment.
Corrected spelling of 'Beirut' to 'Kruise' to match the standard copyright header pattern used in other files.
| Copyright 2019 The Beirut Authors. | |
| Copyright 2019 The Kruise Authors. |
There was a problem hiding this comment.
Please fix the standard copyright header pattern.
Signed-off-by: veophi <[email protected]>
veophi
force-pushed
the
feat/instanceset-controller
branch
from
0b19c3e to
ca6341f
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
config/crd/bases/workloads.x-k8s.io_instancesets.yaml:95
- The field name 'markPodNotReady' should be 'markNotReady' to be consistent with the struct field and avoid confusion. This field applies to Instances, not Pods.
deploy/helm/rbgs/crds/workloads.x-k8s.io_instancesets.yaml:95 - The field name 'markPodNotReady' should be 'markNotReady' to be consistent with the struct field and avoid confusion. This field applies to Instances, not Pods.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
| ) | ||
|
|
||
| // RefManager provides the method to |
There was a problem hiding this comment.
Incomplete comment. Should be 'RefManager provides the methods to manage owner references for resources.'
| // RefManager provides the method to | |
| // RefManager provides the methods to manage owner references for resources. |
| } | ||
|
|
||
| // NewRefManager returns a RefManager that exposes | ||
| // methods to manage the controllerRef of pods. |
There was a problem hiding this comment.
Comment refers to 'pods' but should refer to 'objects' or 'resources' since RefManager is generic and manages any metav1.Object, not specifically pods.
| // methods to manage the controllerRef of pods. | |
| // methods to manage the controllerRef of objects. |
| ) | ||
|
|
||
| const ( | ||
| // LengthOfInstanceID is the length of instanceutil-id |
There was a problem hiding this comment.
Corrected 'instanceutil-id' to 'instance-id' in comment for clarity.
| // LengthOfInstanceID is the length of instanceutil-id | |
| // LengthOfInstanceID is the length of instance-id |
| klog.V(3).Infof("InstanceSet %s begin to scale out %d instances including %d (current rev)", | ||
| controllerKey, expectedCreations, expectedCurrentCreations) | ||
|
|
||
| // available instanceutil-id come from free pvc |
There was a problem hiding this comment.
Corrected 'instanceutil-id' to 'instance-id' and changed 'come' to 'comes' for grammatical correctness.
| // available instanceutil-id come from free pvc | |
| // available instance-id comes from free pvc |
|
/lgtm |
Ⅰ. Motivation
Support in-place update and multi-node deployment using a new workload -- InstanceSet.
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #45
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.