-
Notifications
You must be signed in to change notification settings - Fork 239
fix: always select all available pods when running action #9672
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
base: main
Are you sure you want to change the base?
Conversation
|
Auto Cherry-pick Instructions |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9672 +/- ##
==========================================
+ Coverage 59.67% 59.70% +0.02%
==========================================
Files 551 551
Lines 59786 59810 +24
==========================================
+ Hits 35679 35709 +30
+ Misses 20848 20845 -3
+ Partials 3259 3256 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // during events such as planned maintenance or when performing stop, shutdown, restart, or upgrade operations. | ||
| // In a typical consensus system, this action is used to transfer leader role to another replica. | ||
| // | ||
| // When a pod is about to be updated, a switchover action will be triggered to it. So addon implementation must determine |
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.
A switchover action will be triggered for it, but it doesn't mean the action must be executed on it.
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.
You are right. The real problem I encountered is
| lfa, err := lifecycle.New(its.Namespace, clusterName, its.Name, lifecycleActions, templateVars, pod) |
Here when triggering switchover, only the pod that need to be switched is passed to lifecycle.New. So setting a targetPodSelector will make all switchover action fail.
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 is an implementation issue and can be fixed.
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.
Reconfigure action has the same issue. Does it follow the same logic?
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.
Yep, same logic here.
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment |
4006ead to
34ee557
Compare
06161f4 to
8b0e332
Compare
| Complete(r) | ||
| } | ||
|
|
||
| func (r *InstanceReconciler) listPods(ctx context.Context, req ctrl.Request) ([]*corev1.Pod, error) { |
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.
By design, an Instance should not be able to see resources outside of it.
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.
But an instance needs to see all pods to do switchover. Or I can move switchover action to instanceset2 controller?
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.
It requires coordination between ITS and INST. You can leave it as it is for now (I'll implement them later).
| for _, object := range tree.List(&corev1.Pod{}) { | ||
| pods = append(pods, object.(*corev1.Pod)) | ||
| } | ||
| lfa, err := newLifecycleAction(its, pods, pod) |
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.
Pods can be obtained within the newLifecycleAction.
No description provided.